[Universal] RenderPass conversion #3380

jonuuukas · 2021-02-04T08:14:07Z

Purpose of this PR

Initial PR for the URP RenderPass conversion. This adds the yet limited feature in a hidden manner as it still needs work. This converts current Forward+Deferred in a 1:1 manner - ScriptableRenderPass corresponds to RenderPass. This is not feasible performance-wise on TBDRs, but is a good starting point, as we will continue implementing RenderPass lazy merging. MSAA implementation is not here as well, though we are including some of the related methods (such as ConfigureResolveTarget and etc.)

All of the API and variables are internal, one can enable the checkbox to enable native RenderPass by adding "ENABLE_RENDER_PASS_UI" to the Scripting define symbols. This will create a new checkbox in the renderer assets which can be used to enable RenderPass during runtime or before.

This is work in progress, so documentation and QA passes will be done in the future when we will officially be exposing this API from internal to public

Testing status

This is still very experimental and didn't have a QA round as a lot of work still has to be done. However, on the editor, ~124 tests are green, with few known failures that require changes in the engine-side.

Comments to reviewers

Notes for the reviewers you have assigned.

…ts out because of incompatible blit operations

…RenderingUtils

…r RP-related reasons

…g up deferred MRT formats for that

…lack screen

…nity-Technologies/Graphics into universal/render-pass-conversion

kaychang-unity · 2021-02-08T19:39:01Z

Is ScriptableRenderPass.srpDescriptor a mechanism to get information on render-targets that are not available through RenderTargetIdentifier, such as graphicsFormats, width, height, ... ? It is a shame RenderTargetidentifier is so limited.

Anyway, I don't think I am the most qualified to comment on the design of of RenderPass and how it integrates into URP code.

jonuuukas · 2021-02-09T07:25:35Z

@kaychang-unity - srpDescriptor currently serves that purpose, yeah. However, when the transition to RTHandles happens, we will be removing this class, as the RTHandles carry all the required information for the RenderPass execution.

phi-lira

Made some comments, but have not finished review yet. Let's catch up tomorrow to go through comments? It might be easier to discuss and hear your thoughts over a call.

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

phi-lira · 2021-02-11T16:02:43Z

com.unity.render-pipelines.universal/Runtime/Passes/DepthOnlyPass.cs

+            srpDescriptor.ConfigureDescriptor(GraphicsFormat.DepthAuto, descriptor.width, descriptor.height, 1, true);
            ConfigureTarget(new RenderTargetIdentifier(depthAttachmentHandle.Identifier(), 0, CubemapFace.Unknown, -1));


API wise I have some concerns with this. ScriptableRenderPassDescriptor has a mix of render pass data and attachment data (format).

We are configuring attachment data in two APIs (ConfigureDescriptor (format) and ConfigureTarget (surface handle))

So now you have overloads of ConfigureDescriptor and ConfigureTarget taking arrays of attachment data.
My suggestion is to let ConfigureTarget configure all the per-attachment information, maybe when we support RTHandle it can just take an array of that as that provides the GraphicsFormat you need, but meanwhile an internal overload to take GraphicsFormat and RenderTargetIdentifiers could work?

The other fields you have in ScriptableRenderPassDescriptor (width, height, samples) why do we need a descriptor for that instead of storing in the ScriptableRenderPass itself?

So if user wants to query width of a render pass API would be simpler myPass.width instead of myPass.srpDescriptor.width?

SrpDescriptor changes were reverted and now the API is using ConfigureTarget overloads to get this data

com.unity.render-pipelines.universal/Runtime/Passes/GBufferPass.cs

phi-lira · 2021-02-11T16:32:02Z

com.unity.render-pipelines.universal/Runtime/Passes/ScriptableRenderPassDescriptor.cs

+        internal int width;
+        internal int height;
+        internal int sampleCount;
+        internal GraphicsFormat[] formats;
+        internal bool isLastPass;
+        internal bool isDepthOnly;


Considering GraphicsFormat comes from RTHandle. Why not having the other settings in the ScriptableRenderPass itself?

SrpDescriptor changes were reverted and now the API is using ConfigureTarget overloads to get this data

phi-lira · 2021-02-11T16:38:54Z

com.unity.render-pipelines.universal/Runtime/Passes/ScriptableRenderPass.cs

            overrideCameraTarget = false;
            isBlitRenderPass = false;
            profilingSampler = new ProfilingSampler(nameof(ScriptableRenderPass));
+            useNativeRenderPass = true;


This API is internal, what does it mean for custom passes that can't change this userNativeRenderPass?

TODO: add a comment

At the end, we expect to have a preprocessing mechanism that would determine whether pass is eligible or not to be inside a RenderPass, so this is temporary. Also, Native RenderPass has to be enabled in the renderer asset and having this value as true won't really affect anything unless it's explicitly enabled

phi-lira · 2021-02-11T16:45:06Z

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

+                var attachments =
+                    new NativeArray<AttachmentDescriptor>(useDepth && !renderPass.srpDescriptor.isDepthOnly ? validColorBuffersCount + 1 : 1, Allocator.Temp);
+
+                for (int i = 0; i < validColorBuffersCount; ++i)
+                    attachments[i] = m_ActiveColorAttachmentDescriptors[i];
+
+                if (useDepth && !renderPass.srpDescriptor.isDepthOnly)
+                    attachments[validColorBuffersCount] = m_ActiveDepthAttachmentDescriptor;


This could be encapsulated in the ScriptableRenderPass when configuring attachments we could already cache attachment descriptors?

This is temporary code as it will be changing in the future with lazy merging being introduced. We technically could cache something per-pass, but that will change as soon as we start grouping the render passes

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

… attachment issue

…echnologies/Graphics into universal/render-pass-conversion # Conflicts: # com.unity.render-pipelines.universal/Runtime/ScriptableRenderer.cs

jonuuukas · 2021-02-12T13:05:08Z

@Unity-Technologies/mobile-qa - please do a sanity check on this PR. Checking BoatAttack and UniversalRenderingExamples should be enough. The feature itself shouldn't be tested now, we are only interested whether default behaviour has regressed or got broken with this PR. So basically opening and running these projects on few platforms should be enough

…get path

ernestasKupciunas

Looks good on the mobile side. There were no issues related to these changes, and the default behaviour seems to work as expected.

Testing doc: https://docs.google.com/document/d/1fM3mptWJuI4hvk78xSXi4nOwoswl1ZUWCriNsGOlMMk/edit?usp=sharing

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
VLNQA00108, LGE LG Leon 4G LTE, 5.0.1, Snapdragon 410 MSM8916, Adreno 306
Samsung Galaxy Z Fold2 5G, 10, Snapdragon 865 SM8250, Adreno 650
VLNQA00292, iPad Air3, SoC: A12, iOS: 13.2.3
iPad Air4, SoC: A14, iOS: 14.0
iPhone 12Pro, SoC: A14, iOS: 14.1

…use the same logic

ellioman · 2021-02-26T09:03:48Z

Failures on Yamato are known and not caused by this PR.

jonuuukas and others added 30 commits January 13, 2021 14:28

initial commit and prototype of 1:1 RenderPass conversion

f3b8ce6

enabling blit as renderpass

39aaaad

adding shadows and depthonly renderpass support

2326e65

fix load/store actions for depth

dfb0c68

added opt-out flag at the RenderPass level. The CopyColor pass now op…

dc9c43f

…ts out because of incompatible blit operations

depthonly and depthcopy renderpass-ed

36cd0d1

disable native render pass on postprocess and colorgrading passes

78aa012

some changes for msaa ( not working just yet)

ef791ee

adding clear for targets and reverting blit stuff

bee37cc

fix for intermediate RT for non-msaa

7561217

fix msaa on RenderPass platforms. revert blit as well

03cb8aa

fixing PP to respect the MSAA resolve target

2bf178d

fixing depthOnly clear (temp fix, will need to rework this a bit)

abe0bfb

blit to use RP

7b6aaf3

making m_ActiveColorAttachmentDescriptor to an array and adding some …

20915b4

…RenderingUtils

changing rendertextureformat to graphicsformat and making it array fo…

6f170c0

…r RP-related reasons

moving ConfigureTarget overload call around for the format and hookin…

6e53c52

…g up deferred MRT formats for that

force store single RT attachments for the time being to fix android b…

1ed5d03

…lack screen

moving some code and camera stacking code fixes

19bf12f

Added simple scene information and replaced the hard coded isBlit checks

0b24504

temporary fix for CopyColor to use our ResolveTexture

78ddd02

add renderpass width/height calc depending on cameraRect

34839dd

Merge branch 'universal/render-pass-conversion' of ssh://github.com/U…

0d628a5

…nity-Technologies/Graphics into universal/render-pass-conversion

respect qualitysettings msaa sample count in the last pass to BB

30a7775

fix isLastPassToBB to respect previous logic for setting colorattachment

7cc83d7

load/store actions improvements

8fffce0

added missing isLastPassToBB fix

88535e5

excluding other cameras for NativeRenderPass besides CameraType.Game

6f90801

Merge branch 'universal/render-pass-conversion' of ssh://github.com/U…

855abf4

…nity-Technologies/Graphics into universal/render-pass-conversion

using cameraDepth target if cameraData.targetTexture is not null

00885db

jonuuukas requested review from kaychang-unity and phi-lira February 4, 2021 08:17

formatting test fixes

c70d38e

kaychang-unity approved these changes Feb 8, 2021

View reviewed changes

Merge branch 'master' into universal/render-pass-conversion

3611339

phi-lira suggested changes Feb 11, 2021

View reviewed changes

jonuuukas and others added 4 commits February 12, 2021 11:08

reverting ScriptableRenderPassDescriptor changes

3e2f945

moving RenderPass-related ConfigureTarget overloads to internal scope

9cb631e

simplified some unneeded checks and fixed an MSAA to backbuffer depth…

7c2f4fd

… attachment issue

Merge branch 'universal/render-pass-conversion' of github.com:Unity-T…

1ad3fb0

…echnologies/Graphics into universal/render-pass-conversion # Conflicts: # com.unity.render-pipelines.universal/Runtime/ScriptableRenderer.cs

jonuuukas requested a review from a team February 12, 2021 13:03

jonuuukas and others added 2 commits February 12, 2021 17:36

cleaning up Clear mechanism to respect the same flags as SetRenderTar…

ee852a5

…get path

Added some extra comments regarding the isLastPassToBB flag

39be2b1

jonuuukas marked this pull request as ready for review February 15, 2021 13:12

jonuuukas requested a review from a team as a code owner February 15, 2021 13:12

jonuuukas requested a review from phi-lira February 15, 2021 13:13

ernestasKupciunas requested review from ernestasKupciunas and removed request for a team February 17, 2021 10:10

ernestasKupciunas approved these changes Feb 19, 2021

View reviewed changes

manuele-bonanno approved these changes Feb 19, 2021

View reviewed changes

refactoring color/depthAttachmentTarget to include renderbuffers and …

1460457

…use the same logic

phi-lira approved these changes Feb 25, 2021

View reviewed changes

Merge branch 'master' into universal/render-pass-conversion

b82de55

ellioman merged commit 82e9320 into master Feb 26, 2021

ellioman deleted the universal/render-pass-conversion branch February 26, 2021 09:04

jonuuukas mentioned this pull request Apr 7, 2021

[Universal] RenderPass merging #4099

Merged

		srpDescriptor.ConfigureDescriptor(GraphicsFormat.DepthAuto, descriptor.width, descriptor.height, 1, true);
		ConfigureTarget(new RenderTargetIdentifier(depthAttachmentHandle.Identifier(), 0, CubemapFace.Unknown, -1));

[Universal] RenderPass conversion #3380

[Universal] RenderPass conversion #3380

Uh oh!

Conversation

jonuuukas commented Feb 4, 2021

Purpose of this PR

This is work in progress, so documentation and QA passes will be done in the future when we will officially be exposing this API from internal to public

Testing status

This is still very experimental and didn't have a QA round as a lot of work still has to be done. However, on the editor, ~124 tests are green, with few known failures that require changes in the engine-side.

Comments to reviewers

Uh oh!

kaychang-unity commented Feb 8, 2021

Uh oh!

jonuuukas commented Feb 9, 2021

Uh oh!

phi-lira left a comment

Choose a reason for hiding this comment

Uh oh!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jonuuukas commented Feb 12, 2021

Uh oh!

ernestasKupciunas left a comment

Choose a reason for hiding this comment

Uh oh!

ellioman commented Feb 26, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants