[URP] Remove unnecessary CopyDepth pass before post processing #6097
Conversation
…y for the post processing pass
|
Hi! This comment will help you figure out which jobs to run before merging your PR. The suggestions are dynamic based on what files you have changed. URP Depending on the scope of your PR, you may need to run more jobs than what has been suggested. Please speak to your lead or a Graphics SDET (#devs-graphics-automation) if you are unsure. |
| // m_Depth is the main geometry pass' depth buffer. If condition is true, we bind this depth buffer to _CameraDepthTexture shader binding resource | ||
| if (m_UseDepthTextureFromMainPass) | ||
| cmd.SetGlobalTexture("_CameraDepthTexture", m_Depth.Identifier()); | ||
|
|
||
| Render(cmd, ref renderingData); | ||
|
|
||
| // Restore the _CameraDepthTexture binding to its original bound resource | ||
| if (m_UseDepthTextureFromMainPass) | ||
| cmd.SetGlobalTexture("_CameraDepthTexture", m_CameraDepthTexture.Identifier()); |
There was a problem hiding this comment.
Is there a way to improve depth binding in URP so we would unify the depth buffer and the shader resource with a single ID, similar to how we handle color? I feel otherwise this is quite confusing for user that would like to do the same thing. I guess this is because the depth resolve issues and the fact that we don't have secondary id?
| && createDepthTexture; | ||
| // Post process passes that require depth texture (Motion Blur / DoF) as shader read resource will need to access depth information from main geometry pass if there is no depth copy | ||
| // and depth pre-pass enqueued | ||
| bool postProcessingUseDepthTextureMainPass = !requiresDepthCopyPass && !requiresDepthPrepass && cameraHasPostProcessingWithDepth; |
There was a problem hiding this comment.
| bool postProcessingUseDepthTextureMainPass = !requiresDepthCopyPass && !requiresDepthPrepass && cameraHasPostProcessingWithDepth; | |
| bool postProcessingUseDepthTextureMainPass = !requiresDepthCopyPass && cameraHasPostProcessingWithDepth; |
requiresDepthPrepass is reduntant logic here. If you expand requiresDepthCopyPass the expression you get:
bool postProcessingUseDepthTExtureMainPass = (!requiresDepthPrepass && requiresDepthTexture && createDepthTexture) && !requiresDepthPrepass && cameraHasPostProcessingWithDepth;
| // we could StoreAndResolve when the depth copy is after opaque, but performance wise doing StoreAndResolve of depth targets is more expensive than a simple Store + following depth copy pass on Apple GPUs, | ||
| // because of the extra resolve step. So, unless we are copying the depth after the transparent pass, just Store the depth target. | ||
| if (isCopyDepthAfterTransparent && !copyColorPass) | ||
| if ((isCopyDepthAfterTransparent || postProcessingUseDepthTextureMainPass) && !copyColorPass) |
There was a problem hiding this comment.
Nitpicking: you have in multiple places isCopyDEpthAfterTransparent || postProcessingUseDepthTextureMainPass maybe you could give have a variable with some meaning for this to improve readability. The logic of store actions is a little bit convoluted, but I guess the plan is to improve it with RenderGraph anyway, so no big deal.
|
this is WIP and not currentlly being worked on. Closing the PR temporarily to avoid confusion |
2 participants
…y for the post processing pass
Please read the Contributing guide before making a PR.
Checklist for PR maker
need-backport-*label. After you backport the PR, the label changes tobackported-*.CHANGELOG.mdfile.Purpose of this PR
Why is this PR needed, what hard problem is it solving/fixing?
Testing status
Describe what manual/automated tests were performed for this PR
Comments to reviewers
Notes for the reviewers you have assigned.