Skip to content

[9.x.x] Fix allocation caused when sorting cameras in URP. #268

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 4 commits into from
Apr 27, 2020
Merged

[9.x.x] Fix allocation caused when sorting cameras in URP. #268

merged 4 commits into from
Apr 27, 2020

Conversation

phi-lira
Copy link
Contributor

Purpose of this PR

Fix allocation caused when sorting cameras in URP.
case 1226448

Testing status

Manual Tests: Validated that the fix works by profiling allocations in the Unity Profiler.
Automated Tests: We already have GC alloc automated tests for URP, however the test framework only tests the main camera in isolation by using camera.Render(). We need to improve the test framework to support catching GC alloc in a the SRP Render callback.

Comments to reviewers

IComparer was causing allocation in ArraySortHelper. Replaced with a cached IComparison delegate.

@phi-lira phi-lira requested a review from a team as a code owner April 25, 2020 08:52
@phi-lira phi-lira requested a review from ellioman April 25, 2020 08:53
ellioman
ellioman approved these changes Apr 25, 2020
View reviewed changes
Copy link
Contributor

@ellioman ellioman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@phi-lira phi-lira merged commit 2a26f63 into master Apr 27, 2020
@phi-lira phi-lira deleted the universal/bugfix/fix-alloc-camera-sort branch April 27, 2020 12:48
phi-lira added a commit that referenced this pull request May 12, 2020
@phi-lira
[9.x.x] Fix allocation caused when sorting cameras in URP. (#268)
761c2c9 
* Fixed allocation caused by camera sort.

* Added changelog.
# Conflicts:
#	com.unity.render-pipelines.universal/CHANGELOG.md
@phi-lira phi-lira mentioned this pull request May 12, 2020
Merged
phi-lira added a commit that referenced this pull request May 14, 2020
@phi-lira
[9.x.x] Fix allocation caused when sorting cameras in URP. (#268)
5506234 
* Fixed allocation caused by camera sort.

* Added changelog.
# Conflicts:
#	com.unity.render-pipelines.universal/CHANGELOG.md

# Conflicts:
#	com.unity.render-pipelines.universal/CHANGELOG.md
#	com.unity.render-pipelines.universal/Runtime/UniversalRenderPipelineCore.cs
@phi-lira phi-lira mentioned this pull request May 14, 2020
Merged
phi-lira added a commit that referenced this pull request May 15, 2020
@phi-lira @Chman
[7.x.x] Backports (#455)
70feea5 
* Fixed quality settings v2. (#181)

* Universal Bugfixes (#6109)

* Fixed shader pass name issue (case 1201696)

* Fixed typo in encodedIrradiance.

* Fixed viewport rect not working with render textures.

* Added changelog

* Enabled Allocations tests when rendering.

* Forward Renderer is not sealed anymore. Now developers can subclass it.

* Added Test Scene

* Excluded post-processing tests from build.

* Enabled playmode tests for all assemblies

* Removed unnecessary new shader. UnlitTexture is used instead.

* Updated reference images.

* Updated scene to fix tests.
# Conflicts:
#	TestProjects/UniversalGraphicsTest/Assets/ReferenceImages/Linear/Android/OpenGLES3/127_ClearRenderTexture.png
#	TestProjects/UniversalGraphicsTest/Assets/ReferenceImages/Linear/Android/OpenGLES3/127_ClearRenderTexture.png.meta
#	TestProjects/UniversalGraphicsTest/Assets/ReferenceImages/Linear/Android/Vulkan/127_ClearRenderTexture.png
#	TestProjects/UniversalGraphicsTest/Assets/ReferenceImages/Linear/Android/Vulkan/127_ClearRenderTexture.png.meta
#	TestProjects/UniversalGraphicsTest/Assets/ReferenceImages/Linear/LinuxEditor/OpenGLCore/127_ClearRenderTexture.png
#	TestProjects/UniversalGraphicsTest/Assets/ReferenceImages/Linear/LinuxEditor/OpenGLCore/127_ClearRenderTexture.png.meta
#	TestProjects/UniversalGraphicsTest/Assets/ReferenceImages/Linear/LinuxEditor/Vulkan/127_ClearRenderTexture.png
#	TestProjects/UniversalGraphicsTest/Assets/ReferenceImages/Linear/LinuxEditor/Vulkan/127_ClearRenderTexture.png.meta
#	TestProjects/UniversalGraphicsTest/Assets/ReferenceImages/Linear/OSXEditor/Metal/127_ClearRenderTexture.png
#	TestProjects/UniversalGraphicsTest/Assets/ReferenceImages/Linear/OSXEditor/Metal/127_ClearRenderTexture.png.meta
#	TestProjects/UniversalGraphicsTest/Assets/ReferenceImages/Linear/OSXEditor/OpenGLCore/127_ClearRenderTexture.png
#	TestProjects/UniversalGraphicsTest/Assets/ReferenceImages/Linear/OSXEditor/OpenGLCore/127_ClearRenderTexture.png.meta
#	TestProjects/UniversalGraphicsTest/Assets/ReferenceImages/Linear/OSXPlayer/Metal/127_ClearRenderTexture.png
#	TestProjects/UniversalGraphicsTest/Assets/ReferenceImages/Linear/OSXPlayer/Metal/127_ClearRenderTexture.png.meta
#	TestProjects/UniversalGraphicsTest/Assets/ReferenceImages/Linear/OSXPlayer/OpenGLCore/127_ClearRenderTexture.png
#	TestProjects/UniversalGraphicsTest/Assets/ReferenceImages/Linear/OSXPlayer/OpenGLCore/127_ClearRenderTexture.png.meta
#	TestProjects/UniversalGraphicsTest/Assets/ReferenceImages/Linear/WindowsEditor/Direct3D11/127_ClearRenderTexture.png
#	TestProjects/UniversalGraphicsTest/Assets/ReferenceImages/Linear/WindowsEditor/Direct3D11/127_ClearRenderTexture.png.meta
#	TestProjects/UniversalGraphicsTest/Assets/ReferenceImages/Linear/WindowsEditor/Vulkan/127_ClearRenderTexture.png
#	TestProjects/UniversalGraphicsTest/Assets/ReferenceImages/Linear/WindowsEditor/Vulkan/127_ClearRenderTexture.png.meta
#	TestProjects/UniversalGraphicsTest/Assets/ReferenceImages/Linear/WindowsPlayer/Direct3D11/127_ClearRenderTexture.png
#	TestProjects/UniversalGraphicsTest/Assets/ReferenceImages/Linear/WindowsPlayer/Direct3D11/127_ClearRenderTexture.png.meta
#	TestProjects/UniversalGraphicsTest/Assets/ReferenceImages/Linear/WindowsPlayer/Vulkan/127_ClearRenderTexture.png
#	TestProjects/UniversalGraphicsTest/Assets/ReferenceImages/Linear/WindowsPlayer/Vulkan/127_ClearRenderTexture.png.meta
#	com.unity.render-pipelines.universal/CHANGELOG.md

# Conflicts:
#	TestProjects/UniversalGraphicsTest/ProjectSettings/ProjectSettings.asset
#	com.unity.render-pipelines.universal/CHANGELOG.md
#	com.unity.render-pipelines.universal/Runtime/ForwardRenderer.cs

# Conflicts:
#	com.unity.render-pipelines.universal/CHANGELOG.md

* Post-processing is not causing GC alloc anymore. (#192)

* [9.x.x] Fix allocation caused when sorting cameras in URP. (#268)

* Fixed allocation caused by camera sort.

* Added changelog.
# Conflicts:
#	com.unity.render-pipelines.universal/CHANGELOG.md

# Conflicts:
#	com.unity.render-pipelines.universal/CHANGELOG.md
#	com.unity.render-pipelines.universal/Runtime/UniversalRenderPipelineCore.cs

* [9.x.x] Fix computation of inverseVP matrix. (#374)

* Fixed issue that caused wrong computation of unity inverse view and projection matrix.

* Added graphics tests.

* filtered test 130 from android due to trunk issues.
# Conflicts:
#	TestProjects/UniversalGraphicsTest/ProjectSettings/EditorBuildSettings.asset
#	com.unity.render-pipelines.universal/CHANGELOG.md

# Conflicts:
#	TestProjects/UniversalGraphicsTest/Assets/ReferenceImages/Linear/Android/OpenGLES3/130_UnityMatrixIVP.png
#	TestProjects/UniversalGraphicsTest/Assets/ReferenceImages/Linear/Android/OpenGLES3/130_UnityMatrixIVP.png.meta
#	TestProjects/UniversalGraphicsTest/Assets/ReferenceImages/Linear/OSXEditor/Metal/130_UnityMatrixIVP.png
#	TestProjects/UniversalGraphicsTest/Assets/ReferenceImages/Linear/OSXEditor/Metal/130_UnityMatrixIVP.png.meta
#	TestProjects/UniversalGraphicsTest/Assets/ReferenceImages/Linear/OSXEditor/OpenGLCore/130_UnityMatrixIVP.png
#	TestProjects/UniversalGraphicsTest/Assets/ReferenceImages/Linear/OSXEditor/OpenGLCore/130_UnityMatrixIVP.png.meta
#	TestProjects/UniversalGraphicsTest/Assets/ReferenceImages/Linear/OSXPlayer/Metal/130_UnityMatrixIVP.png
#	TestProjects/UniversalGraphicsTest/Assets/ReferenceImages/Linear/OSXPlayer/Metal/130_UnityMatrixIVP.png.meta
#	TestProjects/UniversalGraphicsTest/Assets/ReferenceImages/Linear/OSXPlayer/OpenGLCore/130_UnityMatrixIVP.png
#	TestProjects/UniversalGraphicsTest/Assets/ReferenceImages/Linear/OSXPlayer/OpenGLCore/130_UnityMatrixIVP.png.meta
#	TestProjects/UniversalGraphicsTest/Assets/ReferenceImages/Linear/WindowsEditor/Direct3D11/130_UnityMatrixIVP.png
#	TestProjects/UniversalGraphicsTest/Assets/ReferenceImages/Linear/WindowsEditor/Direct3D11/130_UnityMatrixIVP.png.meta
#	TestProjects/UniversalGraphicsTest/Assets/ReferenceImages/Linear/WindowsPlayer/Direct3D11/130_UnityMatrixIVP.png
#	TestProjects/UniversalGraphicsTest/Assets/ReferenceImages/Linear/WindowsPlayer/Direct3D11/130_UnityMatrixIVP.png.meta
#	TestProjects/UniversalGraphicsTest/Assets/ReferenceImages/Linear/WindowsPlayer/Vulkan/130_UnityMatrixIVP.png
#	TestProjects/UniversalGraphicsTest/Assets/ReferenceImages/Linear/WindowsPlayer/Vulkan/130_UnityMatrixIVP.png.meta
#	TestProjects/UniversalGraphicsTest/Assets/Test/TestFilters/TestCaseFilters.asset
#	com.unity.render-pipelines.universal/CHANGELOG.md

* fixed merge conflict

* GC fix (#1227490) (#6228)

* Fixed GC pressure in volume component's GetHashCode

* Changelog update

* Update com.unity.render-pipelines.core/CHANGELOG.md

Co-Authored-By: Felipe Lira <felipedrl@gmail.com>

Co-authored-by: Felipe Lira <felipedrl@gmail.com>

* Added CopyImagesToReferenceFolder file.

* merged and updated reference images

Co-authored-by: Thomas <Chman@users.noreply.github.com>
phi-lira added a commit that referenced this pull request May 27, 2020
@phi-lira @Chman
[8.x.x] Backports (#420)
26be20f 
* Fixed quality settings v2. (#181)

* Universal Bugfixes (#6109)

* Fixed shader pass name issue (case 1201696)

* Fixed typo in encodedIrradiance.

* Fixed viewport rect not working with render textures.

* Added changelog

* Enabled Allocations tests when rendering.

* Forward Renderer is not sealed anymore. Now developers can subclass it.

* Added Test Scene

* Excluded post-processing tests from build.

* Enabled playmode tests for all assemblies

* Removed unnecessary new shader. UnlitTexture is used instead.

* Updated reference images.

* Updated scene to fix tests.
# Conflicts:
#	TestProjects/UniversalGraphicsTest/Assets/ReferenceImages/Linear/Android/OpenGLES3/127_ClearRenderTexture.png
#	TestProjects/UniversalGraphicsTest/Assets/ReferenceImages/Linear/Android/OpenGLES3/127_ClearRenderTexture.png.meta
#	TestProjects/UniversalGraphicsTest/Assets/ReferenceImages/Linear/Android/Vulkan/127_ClearRenderTexture.png
#	TestProjects/UniversalGraphicsTest/Assets/ReferenceImages/Linear/Android/Vulkan/127_ClearRenderTexture.png.meta
#	TestProjects/UniversalGraphicsTest/Assets/ReferenceImages/Linear/LinuxEditor/OpenGLCore/127_ClearRenderTexture.png
#	TestProjects/UniversalGraphicsTest/Assets/ReferenceImages/Linear/LinuxEditor/OpenGLCore/127_ClearRenderTexture.png.meta
#	TestProjects/UniversalGraphicsTest/Assets/ReferenceImages/Linear/LinuxEditor/Vulkan/127_ClearRenderTexture.png
#	TestProjects/UniversalGraphicsTest/Assets/ReferenceImages/Linear/LinuxEditor/Vulkan/127_ClearRenderTexture.png.meta
#	TestProjects/UniversalGraphicsTest/Assets/ReferenceImages/Linear/OSXEditor/Metal/127_ClearRenderTexture.png
#	TestProjects/UniversalGraphicsTest/Assets/ReferenceImages/Linear/OSXEditor/Metal/127_ClearRenderTexture.png.meta
#	TestProjects/UniversalGraphicsTest/Assets/ReferenceImages/Linear/OSXEditor/OpenGLCore/127_ClearRenderTexture.png
#	TestProjects/UniversalGraphicsTest/Assets/ReferenceImages/Linear/OSXEditor/OpenGLCore/127_ClearRenderTexture.png.meta
#	TestProjects/UniversalGraphicsTest/Assets/ReferenceImages/Linear/OSXPlayer/Metal/127_ClearRenderTexture.png
#	TestProjects/UniversalGraphicsTest/Assets/ReferenceImages/Linear/OSXPlayer/Metal/127_ClearRenderTexture.png.meta
#	TestProjects/UniversalGraphicsTest/Assets/ReferenceImages/Linear/OSXPlayer/OpenGLCore/127_ClearRenderTexture.png
#	TestProjects/UniversalGraphicsTest/Assets/ReferenceImages/Linear/OSXPlayer/OpenGLCore/127_ClearRenderTexture.png.meta
#	TestProjects/UniversalGraphicsTest/Assets/ReferenceImages/Linear/WindowsEditor/Direct3D11/127_ClearRenderTexture.png
#	TestProjects/UniversalGraphicsTest/Assets/ReferenceImages/Linear/WindowsEditor/Direct3D11/127_ClearRenderTexture.png.meta
#	TestProjects/UniversalGraphicsTest/Assets/ReferenceImages/Linear/WindowsEditor/Vulkan/127_ClearRenderTexture.png
#	TestProjects/UniversalGraphicsTest/Assets/ReferenceImages/Linear/WindowsEditor/Vulkan/127_ClearRenderTexture.png.meta
#	TestProjects/UniversalGraphicsTest/Assets/ReferenceImages/Linear/WindowsPlayer/Direct3D11/127_ClearRenderTexture.png
#	TestProjects/UniversalGraphicsTest/Assets/ReferenceImages/Linear/WindowsPlayer/Direct3D11/127_ClearRenderTexture.png.meta
#	TestProjects/UniversalGraphicsTest/Assets/ReferenceImages/Linear/WindowsPlayer/Vulkan/127_ClearRenderTexture.png
#	TestProjects/UniversalGraphicsTest/Assets/ReferenceImages/Linear/WindowsPlayer/Vulkan/127_ClearRenderTexture.png.meta
#	com.unity.render-pipelines.universal/CHANGELOG.md

# Conflicts:
#	TestProjects/UniversalGraphicsTest/ProjectSettings/ProjectSettings.asset
#	com.unity.render-pipelines.universal/CHANGELOG.md
#	com.unity.render-pipelines.universal/Runtime/ForwardRenderer.cs

* Post-processing is not causing GC alloc anymore. (#192)

* [9.x.x] Fix allocation caused when sorting cameras in URP. (#268)

* Fixed allocation caused by camera sort.

* Added changelog.
# Conflicts:
#	com.unity.render-pipelines.universal/CHANGELOG.md

* [9.x.x] Fix computation of inverseVP matrix. (#374)

* Fixed issue that caused wrong computation of unity inverse view and projection matrix.

* Added graphics tests.

* filtered test 130 from android due to trunk issues.
# Conflicts:
#	TestProjects/UniversalGraphicsTest/ProjectSettings/EditorBuildSettings.asset
#	com.unity.render-pipelines.universal/CHANGELOG.md

* fixed merge conflict

* added missing reference images

* GC fix (#1227490) (#6228)

* Fixed GC pressure in volume component's GetHashCode

* Changelog update

* Update com.unity.render-pipelines.core/CHANGELOG.md

Co-Authored-By: Felipe Lira <felipedrl@gmail.com>

Co-authored-by: Felipe Lira <felipedrl@gmail.com>

* [9.x.x] updated terrain scene and reference images. (#460)

* updated terrain scene and reference images.

* Reverted non OSX Metal images

This reverts commit 1cba18b.
# Conflicts:
#	TestProjects/UniversalGraphicsTest/Assets/ReferenceImages/Linear/OSXEditor/Metal/None/035_Shader_TerrainShaders.png.meta

Co-authored-by: Thomas <Chman@users.noreply.github.com>
PaulDemeulenaere added a commit that referenced this pull request Jul 27, 2021
@PaulDemeulenaere
[Fix] VFXParameter placement issue (#268)
ddea81c 
* Add editor test to cover case 1352832

* Minor improve test checking connection

* Fix VFXParameter relink

However, this behavior is still pretty hacky but if we are relying on this code, it has to be smarter link closest operator to vfxnode

* Clarify search of closest position

* Fix closest block ignoring block position

* Rename UpdateLinkSlotManualConversion => ConvertExpressionsFromLink

Fix conversation https://github.cds.internal.unity3d.com/unity/vfx-graphics/pull/258#discussion_r162849

* *Update changelog.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ellioman ellioman ellioman approved these changes

Assignees
No one assigned
Labels
backported-7.x backported-8.x universal
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@phi-lira @ellioman