Adapt URP to use RTHandles - P1 #5346
Conversation
|
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 SRP Core 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. |
sandy-carter-unity
requested review from
jonuuukas,
manuele-bonanno,
thomas-zeng,
fabien-unity and
phi-lira
sandy-carter-unity
force-pushed
the
universal/xr/rt-handles-p1
branch
2 times, most recently
from
b2ca2fe
to
818a331
Compare
sandy-carter-unity
force-pushed
the
universal/xr/rt-handles-p1
branch
6 times, most recently
from
f697dbb
to
973e68e
Compare
sandy-carter-unity
force-pushed
the
universal/xr/rt-handles-p1
branch
3 times, most recently
from
f51a0fb
to
cdde383
Compare
sandy-carter-unity
force-pushed
the
universal/xr/rt-handles-p1
branch
5 times, most recently
from
aed13a9
to
9f09647
Compare
sandy-carter-unity
requested review from
a team
and removed request for
phi-lira
…al/xr/rt-handles-p1
|
|
||
| The `ScriptableRendererFeature` has a new virtual function called `SetupRenderPasses` which is called when render targets are allocated and ready to be used. | ||
|
|
||
| If your code uses `renderer.cameraColorTarget` or `renderer.cameraDepthTarget` inside of the `AddRenderPasses` override, then that use needs to be moved to `SetupRenderPasses`. |
There was a problem hiding this comment.
How does this affect passes added from e.g. components rather than renderer features?
Is there a point in the ScriptableRenderPass where the user can assume that render targets are allocated?
There was a problem hiding this comment.
The issue with features was that universal renderer was calling AddRenderPasses before allocation, by design.
The targets are allocated right after in the Setup of the ScriptableRenderer.
If components are trying to access targets before the setup, then it's too early. Anytime after this is fine.
| * `RenderingUtils.ReAllocateIfNeeded` | ||
| * `ShadowUtils.ShadowRTReAllocateIfNeeded` |
There was a problem hiding this comment.
These don't really feel like helper functions, but more like something you need to use. Can we move them into a more official sounding class?
| /// <param name="mipMapBias">Bias applied to mipmaps during filtering.</param> | ||
| /// <param name="name">Name of the RTHandle.</param> | ||
| /// <returns></returns> | ||
| internal static bool ReAllocateIfNeeded( |
There was a problem hiding this comment.
If these are in our upgrade guide, they should be public
…t is in the upgrade guide
…nto universal/xr/rt-handles-p1
sandy-carter-unity
force-pushed
the
universal/xr/rt-handles-p1
branch
from
fe6a062
to
10d0ea8
Compare
|
Yamato was green on previous changeset. Merged master to solve conflicts with changelog. |
Purpose of this PR
This is a refactor of #4018 made in a way which focuses on backwards compatibility, what-you-use-is-what-you get allocation of targets and without the dynamic scaling changes which will be moved to a p2 PR. The reason for doing it in such a modular way is to have a testing friendly state from which other features dependent on RTHandles can start their work before dynamic scaling lands.
This first part contains a lot high level of refactoring in the way that resources are passed around and created. The second part and other features leveraging RTHandles should not require so many changes.
This PR aims to replace usage of RenderTargetHandle to use RTHandles instead in order to support setting
UnityEngine.XR.XRSettings.renderViewportScalein URP.RTHandle is a SRP core utility class that helps manage unity RenderTexture resources and has DynamicResolutionHandler support. It is currently being used by HDRP and RenderGraph modules. The class also has built-in support for render target dynamic scaling. This will add dynamic resolution support to URP and unifies URP’s render texture management. It also opens the way for URP to integrate with RenderGraph in the future.
Testing status
The current failing tests are also failing on the master branch.
I've looked closely at the test pipeline and this PR does not add new fails.
Additionally, any of the commits behind
HEADpass all tests locally and on Yamato so bisecting is possible.I am also working with QA and XR QA to validate in depth.
Comments to reviewers
See Left to do.
FAQ
Why add
[Obsolete]and then ignore them in places?The switch to
RTHandlesis backwards compatible with uses ofRenderTargetIdentifiers. This is so users creating their own passes can still have their projects working in the future release. However, we want this compatibility layer to be gone in future versions so thatRTHandledynamic scaling in XR works across the board. This probably will impact RenderGraph also.Disabling the warnings happens for the cases where we still use the obsolete API for backwards compatibility purposes.
That being said, there might be ways to reduce the disables and still be backwards compatible, please let me know if there are.
Color and Depth in the same RenderTarget
The nature of RTHandles prevents the combination of color and depth/stencil in the same RTHandle.
Therefore, and RTHandle is equivalent to one GPU render texture
Converting this to RTHandles requires removing all GetTemporaryRTs and replacing them with RTHandles which are allocated to last longer than one frame.
When they are (re)allocated, the reference changes, this target creation is now moved to before setup and in a lot of cases moved to within the Universal Renderer.
In terms of allocated GPU resourced, it should be the same due to the above use of
depthBufferBitson the color attachment.Left to do
Y-Flip of GameView in sample depthCommandBuffer: temporary render texture _GBuffer2 not found while executing (SetGlobalTexture)VRTextureUsagetoRTHandles.AllocCHANGELOG.mdfile.