DecalRendererFeature: Convert to using RTHandles #6401

sandy-carter-unity · 2021-11-25T18:33:14Z

Pass: Remove RTI alises of universal renderer targets, these do not work
with RTHandles, you must get the actual references.
Pass: Convert dBufferColorHandles[0-2] and m_DBufferDepth to RTHandles
Pass: Set global textures in execute to use the same cmd.
Feature: Call pass dispose
Feature: Move setup of targets to Pass' Setup Function called from
SetupRenderPasses

Please read the Contributing guide before making a PR.

Read the Graphics repository & Yamato FAQ.

Checklist for PR maker

Have you updated the changelog? Each package has a CHANGELOG.md file.

Purpose of this PR

The intention of this PR is fix breaking test 230 in Foundation.
It also fixes targets that weren't yet converted to RTHandles.

Testing status

Test 230_Decal_Projector from UniversalGraphicsTest_Foundation was run with the -xr-reuse-tests which reliably produced a red test.

The test seemed to also fail more often if run after 120 to 124. This was also tested.

Comments to reviewers

Notes for the reviewers you have assigned.

github-actions · 2021-11-25T18:33:28Z

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.
Link to Yamato: https://unity-ci.cds.internal.unity3d.com/project/902/
Search for your PR branch using the search bar at the top, then add the following segment(s) to the end of the URL (you may need multiple tabs depending on how many packages you change)

URP
/jobDefinition/.yamato%252Fall-urp.yml%2523PR_URP_trunk
With changes to URP packages, you should also run
/jobDefinition/.yamato%2Fall-lightmapping.yml%23PR_Lightmapping_trunk

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.

Pass: Remove RTI alises of universal renderer targets, these do not work with RTHandles, you must get the actual references. Pass: Convert dBufferColorHandles[0-2] and m_DBufferDepth to RTHandles Pass: Set global textures in execute to use the same cmd. Feature: Call pass dispose Feature: Move setup of targets to Pass' Setup Function called from SetupRenderPasses

sandy-carter-unity · 2021-11-30T22:26:37Z

The following red nightly tests are fixed by this PR for 230 Decal Projector:

https://unity-ci.cds.internal.unity3d.com/job/10214336 (Win DX11 Standalone XR mono Linear Foundation)
https://unity-ci.cds.internal.unity3d.com/job/10214337 (Win DX11 Playmode XR Mono Linear Foundation)
https://unity-ci.cds.internal.unity3d.com/job/10214340 (Win DX12 Standalone XR Mono Linear Foundation)
https://unity-ci.cds.internal.unity3d.com/job/10214344 (Win Vulkan Standalone mono Linear Foundation)
https://unity-ci.cds.internal.unity3d.com/job/10214388 (Win Vulkan Standalone mono Linear Terrain)
https://unity-ci.cds.internal.unity3d.com/job/10214390 (Linux Vulkan Standalone mono Linear Terrain)
https://unity-ci.cds.internal.unity3d.com/job/10214394 (Android Vulkan Standalone il2cpp Linear Terrain)
https://unity-ci.cds.internal.unity3d.com/job/10214460 (Win Vulkan playmode mono Linear Terrain)
https://unity-ci.cds.internal.unity3d.com/job/10214461 (Linux Vulkan playmode mono Linear Terrain)

lukaschod · 2021-12-01T08:31:00Z

com.unity.render-pipelines.universal/Runtime/Decal/DBuffer/DBufferRenderPass.cs

-                depthHandle = m_CameraDepthAttachment;
-            }

            ConfigureTarget(dBufferColorHandles, depthHandle);


I noticed we usually call ConfigureTarget on OnCameraSetup/Configure, wouldn't be issue calling it outside it?

I'm not sure. So far, I don't think so.
The bigger issues I had was getting targets from universal renderer too early.

K so if it works it is okay. I think I will split it in my https://github.com/Unity-Technologies/Graphics/pull/6442/files.

…ctual

phi-lira · 2021-12-07T08:48:59Z

Merged master to rerun tests as there were some failures that were already fixed in master and that was quite hard to compare.

lukaschod · 2021-12-07T10:22:35Z

com.unity.render-pipelines.universal/Editor/ShaderPreprocessor.cs

                {
                    UniversalRenderer universalRenderer = (UniversalRenderer)renderer;
-                    if (universalRenderer.renderingMode == RenderingMode.Deferred)
+                    if (universalRenderer.renderingModeRequested == RenderingMode.Deferred)


Could you add info why this needs to be changed?

This was to make it more clear to understand. RenderingMode is the rendering mode settings and then there was another RenderingModeActual which was the one that is the actual one used and considered when falling back. The renderinMode was renamed to renderingModeRequested to be more clear this is the requested mode that comes from settings but not necessary the mode that is used.

Should I add it to the changelog, maybe?

These are internal / private. No reason to mention in changelog.

eh-unity

Please clarify request vs. actual rendering mode usage in the renderer. Otherwise lgtm.

eh-unity · 2021-12-07T10:52:52Z

com.unity.render-pipelines.universal/Runtime/UniversalRenderer.cs

            }

-            if (this.renderingMode == RenderingMode.Deferred)
+            if (this.renderingModeRequested == RenderingMode.Deferred)


Why these use renderingModeRequested? Shouldn't these depend on the renderingModeActual?

@sandy-carter-unity this is a good point. It seems sensible that the actual rendering mode is figured out very early before all of these settings.

Actually, thinking about it a little bit now, we should expose a property to get the rendering mode that always return the actual one and the requested / setting is private setting, this makes it less confusing.

Is this what you mean?

private RenderingMode renderingModeRequested => m_RenderingMode; internal RenderingMode renderingModeActual => ...

At that point, we could remove the renderingModeRequested property entirely

Yeah, ok I see the renderingModeRequested is private now, but that feels useless tbh as you can just have m_RenderingMode?

phi-lira · 2021-12-09T08:34:11Z

This PR introduces some new failures in Android, but given that it fixes several other failures I will take it to improve the overall coverage situation.

sandy-carter-unity requested review from eh-unity, lukaschod and phi-lira November 25, 2021 18:33

github-actions bot added the universal label Nov 25, 2021

sandy-carter-unity force-pushed the universal/dcal-rthandles branch from 0e6e405 to ae16a85 Compare November 25, 2021 20:27

sandy-carter-unity marked this pull request as ready for review November 30, 2021 22:25

sandy-carter-unity requested review from a team as code owners November 30, 2021 22:25

lukaschod reviewed Dec 1, 2021

View reviewed changes

sandy-carter-unity and others added 3 commits December 2, 2021 10:14

URP: Rename rendering mode to avoid confusion between requested and a…

7506703

…ctual

Decals: Check actual rendering mode to see if deferred

9becec1

Merge branch 'master' into universal/dcal-rthandles

fdd35b7

lukaschod approved these changes Dec 7, 2021

View reviewed changes

lukaschod reviewed Dec 7, 2021

View reviewed changes

eh-unity approved these changes Dec 7, 2021

View reviewed changes

phi-lira changed the base branch from master to universal/staging December 9, 2021 08:33

phi-lira approved these changes Dec 9, 2021

View reviewed changes

phi-lira merged commit 0578ace into universal/staging Dec 9, 2021

phi-lira deleted the universal/dcal-rthandles branch December 9, 2021 08:35

phi-lira mentioned this pull request Dec 9, 2021

Universal Staging Branch #6546

Merged

sandy-carter-unity added the needs-backport-2022.1 label Jan 13, 2022

sandy-carter-unity mentioned this pull request Jan 13, 2022

[2022.1 backport] DecalRendererFeature: Convert to using RTHandles #6746

Merged

1 task

DecalRendererFeature: Convert to using RTHandles #6401

DecalRendererFeature: Convert to using RTHandles #6401

Uh oh!

Conversation

sandy-carter-unity commented Nov 25, 2021

Please read the Contributing guide before making a PR.

Checklist for PR maker

Purpose of this PR

Testing status

Comments to reviewers

Uh oh!

github-actions bot commented Nov 25, 2021

Uh oh!

sandy-carter-unity commented Nov 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lukaschod Dec 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phi-lira commented Dec 7, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eh-unity left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phi-lira commented Dec 9, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

sandy-carter-unity commented Nov 30, 2021 •

edited

Loading

lukaschod Dec 1, 2021 •

edited

Loading