[Universal] RenderPass merging #4099
Conversation
… the color target's resolve surface
…he RenderPass attachment list. Merging subpasses and attachments which are the same or compatible
…ndle coorrectly edge cases)
…nity-Technologies/Graphics into universal/render-pass-conversion
…nity-Technologies/Graphics into universal/render-pass-conversion
…preparation step to move into different files (potentially)
… to NativeRenderPassUtils
…nity-Technologies/Graphics into universal/render-pass-conversion
…enderPass data to the NativeRenderPass class
… moved RenderPass data to the NativeRenderPass class" This reverts commit ef3e215.
| // index to track the position in the current frame | ||
| internal int renderPassQueueIndex { get; set; } | ||
|
|
||
| internal NativeArray<int> m_InputAttachmentIndices; |
There was a problem hiding this comment.
I dont see where it is used
There was a problem hiding this comment.
@lukaschod - yeah, it's not used yet. we can remove it for now, but it will still be added in next PR that will introduce input attachments/fb fetch and etc.
com.unity.render-pipelines.universal/Runtime/NativeRenderPass.cs
Outdated
Show resolved
Hide resolved
com.unity.render-pipelines.universal/Runtime/Passes/ScriptableRenderPass.cs
Outdated
Show resolved
Hide resolved
ernestasKupciunas
requested review from
ernestasKupciunas
and removed request for
a team
There was a problem hiding this comment.
PR looks good. I was running projects using Deferred and Forward renderers. Tested Metal, Vulkan and Gles3 Graphics APIs. Visual correctness looks good. No suspicious activity in the logs. No bugs related to this PR. Good job!
Winter:
BoatAttack:
PolyFarm:
Projects used for testing:
Devices under testing:
VLNQA00217, Razer Phone 2, 9.0.0, Snapdragon 845 SDM845, Adreno 630
VLNQA00268, Samsung Galaxy S10+, 10.0.0, Exynos 9 9820, Mali-G76
OnePlus Nord, 10, Snapdragon 765G SM7250-AB, Adreno 620
Samsung Galaxy Z Fold2 5G, 10, Snapdragon 865 SM8250, Adreno 650
iPad Air4, SoC: A14, iOS: 14.0
iPhone 12Pro, SoC: A14, iOS: 14.1
Unity used for testing:
Version: 2021.2.0a16.2217
Revision: trunk b5c81da9dae0
Built: Sun, 25 Apr 2021 10:56:38 GMT
com.unity.render-pipelines.universal/Runtime/NativeRenderPass.cs
Outdated
Show resolved
Hide resolved
com.unity.render-pipelines.universal/Runtime/NativeRenderPass.cs
Outdated
Show resolved
Hide resolved
| w = width; | ||
| h = height; | ||
| samples = sampleCount; | ||
| depthID = rtID; |
There was a problem hiding this comment.
Do we support stencil only formats for descriptors and/or separate stencil usage flags? Did we resolve this discussion in our last meeting? So VK_FORMAT_S8_UINT format, and also https://www.khronos.org/registry/vulkan/specs/1.2-extensions/man/html/VkImageStencilUsageCreateInfo.html
There was a problem hiding this comment.
as discussed in the call: this will require Vulkan work to be added to the backlog. Currently Unity doesn't support this
com.unity.render-pipelines.universal/Runtime/ScriptableRendererFeature.cs
Outdated
Show resolved
Hide resolved
| // keep track if this is the current camera's last pass and the RT is the backbuffer (BuiltinRenderTextureType.CameraTarget) | ||
| // knowing isLastPassToBB can help decide the optimal store action as it gives us additional information about the current frame | ||
| bool isLastPassToBB = pass.isLastPass && (colorAttachmentTarget == BuiltinRenderTextureType.CameraTarget); | ||
| currentAttachmentDescriptor.ConfigureTarget(colorAttachmentTarget, ((uint)finalClearFlag & (uint)ClearFlag.Color) == 0, !(samples > 1 && isLastPassToBB)); |
There was a problem hiding this comment.
I know this proceeded this change, but is this actually the right logic? !(samples > 1 && isLastPassToBB)
Can you clarify it for me, so we want to store when no msaa or when this is not the last pass for the back buffer?
There was a problem hiding this comment.
this is old logic from before the merge PR. For now we always store but the next PR should optimize that
There was a problem hiding this comment.
in general though yes, if the pass wouldn't be merged (which was the case before this PR) this would try to discard if MSAA > 1 and is the last pass.
Now with the merging all the store/discard logic will have to take into account the mergeable pass list, so that logic is outdated and harmless, but will need to be reworked in the next bandwidth optimization PR I mentioned in the call
There was a problem hiding this comment.
just to clarify: "why when MSAA > 1 we requested to store?"
this require an explanation of how Unity implicitly converts to resolve actions in the device implementations:
when MSAA is on, internally the device code converts DontCare actions in Resolve, and Store actions in StoreAndResolve. If the pass is the last pass to BB then we want to NOT store, so the action will become Resolve. Resolve still stores the resolved surface but discards the MSAA'd surface, while StoreAndResolve would also store the MSAA'd surface which wastes a lot of bandwidth.
If MSAA is OFF: then you want to store the non-AA RT, otherwise the final result of the pass wouldn't make it to the BB memory as it would be discarded
Hope it makes sense now
Purpose of this PR
This is a follow up PR for the initial RenderPass PR: #3380.
This PR introduces the RenderPass merging mechanism, that should allow for eligible passes to be merged.
This also includes:
TDD and more in-depth explanation: https://docs.google.com/document/d/1ddJS70UnbyUT21FxvSgeVy6XFoZXScrMWn5u7J4l790
Entry point for the whole RenderPass Conversion:
https://docs.google.com/document/d/174wpA7AvycbY65wg_Nd3FHt2zJK7sAnOx3qiDUynSro
Testing status
Manually ran the URP editor tests, will run them on iOS and compare non-RenderPass and RenderPass rendering paths.
Comments to reviewers
NOTE: This PR won't enable RenderPasses. The setting will still be disabled, and the selector hidden to users. This is an incremental step to enable further work and optimizations.