Added an emissive as forward pass when the lit is in deferred mode. #2527

anisunity · 2020-11-04T17:30:48Z

In order to properly support APV and SSGI(RTGI). We need to be able to not impact the emissive when in deferred mode. To do so, we added the option to have the emissive rendered in a seperate forward pass only for object that have non null (default value SG or shader) or with a branch plugged in (SG).
This is only possible when the lit model is in deferred and if the matching frame setting is enabled.
The proper migration code has been implemented for both SG and regular shaders.
The pass is not generated for shader graph if the emissive value is black or nothing is plugged in the master node on the emission port.
The pass is disabled on materials that have null emissive values.
The code has not been backported to the non rendergraph path.

Testing status
Ran the tests with and without the feature locally they are all green (had to fix some tests that had issues in the scenes).

QA plan test:
This PR introduce a new pass to render Emissive Color in a new pass but only for Lit Material with a Surface Type Opaque.
There is a new FrameSettings: LitEmissiveAsForward which allow to control if the Emissive contribution of a Material will be rendering into the Lighting Buffer during the GBuffer pass or in an extra Emissive Color pass which is after all the other lighting.
This change of rendering order only apply for objcet render in the deferred pass; Transparent Material or other Material (hair, stacklit, fabric, eye, axf, unlit) aren't affected.

Plan test:

Yamato test already validated that by default nothing change compare to before this PR. Executed code should be exactly the same with same result and no Forward Emissive Pass executed.
When enabling LitEmissiveAsForward in a scene with Lit emissive objects, we should get the same visual result. However there is a difference, now the SSAO algorithm will not affect the emissive and in addition, if two emissive object are on top of each other, they will contribute twice (against one without the change). The new pass can be check in a RenderDoc capture or the FrameDebugger. If Lit Material don't have a Emissive contribution, then nothing should be render in this extra pass.
It can be validated that it work correctly with the SSGI effect. SSGI effect by default remove emissive contribution of object. With the FrameSetting on, it will work as expected.

TomasKiniulis · 2020-11-05T07:58:29Z

Hey @anisunity not quite clear from the description what needs testing here. Notes to QA what are the things that need attention would be appreciated.

sebastienlagarde · 2020-11-05T08:43:18Z

com.unity.render-pipelines.high-definition/Runtime/RenderPipeline/Settings/FrameSettings.cs

@@ -105,6 +105,9 @@ public enum FrameSettingsField
        /// <summary>When enabled, Cameras using these Frame Settings use Alpha To Mask. Activate MSAA to access this option.</summary>
        [FrameSettingsField(0, displayedName: "Alpha To Mask", positiveDependencies: new[] { MSAA }, customOrderInGroup: 3, tooltip: "When enabled, Cameras using these Frame Settings use Alpha To Mask. Activate MSAA to access this option.")]
        AlphaToMask = 56,
+        /// <summary>When enabled, The emissive part of the materials is rendered as a forward pass for the deferred pipeline.</summary>
+        [FrameSettingsField(0, displayedName: "Emissive as Forward", positiveDependencies: new[] { LitShaderMode }, customOrderInGroup: 3, tooltip: "When enabled, The emissive part of the materials is rendered as a forward pass for the deferred pipeline.")]
+        EmissiveAsForward = 57,


Can you name it "LitEmissiveAsForward" so we understand it is only for the Lit shader. Thanks

anisunity · 2020-11-05T08:44:56Z

The pr is still not done, missing some comments and the tesselation versions of some shaders

alelievr · 2020-11-06T10:08:56Z

com.unity.render-pipelines.high-definition/Editor/Material/Lit/BaseLitGUI.cs

+            }
+
+            if (emissionPassCanBeDisabled)
+                material.SetShaderPassEnabled(HDShaderPassNames.s_ForwardEmissiveStr, emissiveIsActive);


Did you test what happens when you create a material with a Lit shader (emission is back and the pass is disabled) and then assign a shader graph set up in a way that the emission pass is enabled?
This is a pretty common use case because when we create a material it has the lit shader by default.

alelievr · 2020-11-06T10:14:48Z

com.unity.render-pipelines.high-definition/Runtime/RenderPipeline/Settings/FrameSettings.cs

@@ -756,6 +759,7 @@ internal static void Sanitize(ref FrameSettings sanitizedFrameSettings, Camera c
            // TODO: The work will be implemented piecemeal to support all passes
            bool msaa = sanitizedFrameSettings.bitDatas[(int)FrameSettingsField.MSAA] &= renderPipelineSettings.supportMSAA && sanitizedFrameSettings.litShaderMode == LitShaderMode.Forward && !pipelineSupportsRayTracing;
            sanitizedFrameSettings.bitDatas[(int)FrameSettingsField.AlphaToMask] &= msaa;
+            sanitizedFrameSettings.bitDatas[(int)FrameSettingsField.LitEmissiveAsForward] &= sanitizedFrameSettings.litShaderMode == LitShaderMode.Deferred;


I see above that the emissive pass feature doesn't work if the frame settings OpaqueObjects is disabled. Maybe it's worth to add it here in the sanitize?

remi-chapelain

First, is there a reason why we disable this by default ? It looks like it's an improvement that users would want since the previous way of dealing with it was making a compromise by sharing a buffer for emissive and GI. Is it because of performance ? From what I've checked profiling the forward emissive pass it looks like the cost is negligible (it only adds 0.03ms on the simple scene I've checked.

What I've tested :

Emissive contribution of the object is restored with the new pass for SSGI/RTGI ✔️
Duplicating emissive object in place now contribute twice as much ✔️
Tested that the pass is automatically disabled in forward ✔️
Tested that the pass appear/disappear in RenderGraph ✔️
Tested that the materials not supported does not throw any errors (other shaders, transparents) ✔️

What went wrong :

Tested Antoine's method (desribed above) to create a material and got a failure, material stays black and emissive is not taken into account regardless of the color 🔴
Also, animating emissive color from a material that starts black does not work (via script), it only works if the material emissive color is not null at start.
https://unity.gyazo.com/7f7094e5456b3ebe37aab13fd9852e66 🔴

sebastienlagarde · 2020-11-10T23:06:17Z

First, is there a reason why we disable this by default ? It looks like it's an improvement that users would want since the previous >way of dealing with it was making a compromise by sharing a buffer for emissive and GI. Is it because of performance ? From >what I've checked profiling the forward emissive pass it looks like the cost is negligible (it only adds 0.03ms on the simple scene >I've checked.

Yes. You pay a CPU cost (re-rendering mesh) + the GPU cost (rasterization of triangle). If you have a lot of Emissive heavy tessellated mesh it can become costly. Now this is a good remark and if we can simplify our codebase, then maybe we should consider to have it all the time. Let me think about it.

sebastienlagarde · 2020-11-25T11:39:59Z

com.unity.render-pipelines.high-definition/Editor/Material/ShaderGraph/LightingSubTarget.cs

+
+            // We need to grab the emission block to check if it is connected, or the default value is non null.
+            bool emissionPassEnabled = false;
+            if (!context.connectedBlocks.Contains(BlockFields.SurfaceDescription.Emission))


I haven't take care up to know, but is this code done for all shader or just lit shader? We must generate EmissiveForward pass only for lit shader.

sebastienlagarde · 2020-11-25T11:41:40Z

...nity.render-pipelines.high-definition/Editor/Material/ShaderGraph/TargetData/LightingData.cs

@@ -88,5 +88,13 @@ public bool overrideBakedGI
            get => m_OverrideBakedGI;
            set => m_OverrideBakedGI = value;
        }
+
+        [SerializeField]
+        bool m_EmissionOverriden;


so it is not in lighting but should be in lit data itself (I don't remind if we have such a struct)

sebastienlagarde · 2020-11-25T11:51:12Z

...r-pipelines.high-definition/Runtime/RenderPipeline/ShaderPass/ShaderPassForwardEmissive.hlsl

+            #endif
+)
+{
+#ifdef _WRITE_TRANSPARENT_MOTION_VECTOR


There is many thing in this file that aren't appropriate as far as I can tell. Like many call to debug mode or motion vector.

sebastienlagarde · 2020-11-25T13:01:23Z

This should be replace by #2748

anisunity added the HDRP label Nov 4, 2020

anisunity requested review from sebastienlagarde and a team November 4, 2020 17:30

anisunity self-assigned this Nov 4, 2020

github-actions bot added the testing label Nov 4, 2020

anisunity requested a review from alelievr November 5, 2020 08:37

sebastienlagarde reviewed Nov 5, 2020

View reviewed changes

alelievr reviewed Nov 6, 2020

View reviewed changes

remi-chapelain reviewed Nov 10, 2020

View reviewed changes

anisunity added 2 commits November 20, 2020 16:37

Added an emissive as forward pass when the lit is in deferred mode.

4694b0c

Review fixes

f6cca55

anisunity force-pushed the forwardEmissiveOpaque branch from a253ee2 to f6cca55 Compare November 20, 2020 16:04

sebastienlagarde reviewed Nov 25, 2020

View reviewed changes

sebastienlagarde mentioned this pull request Nov 25, 2020

Added an emissive as forward pass when the lit is in deferred mode v2 #2748

Closed

anisunity closed this Dec 1, 2020

sebastienlagarde mentioned this pull request Feb 9, 2021

Added an emissive as forward pass when the lit is in deferred mode + Material update #3430

Merged

sebastienlagarde deleted the forwardEmissiveOpaque branch September 1, 2021 10:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added an emissive as forward pass when the lit is in deferred mode. #2527

Added an emissive as forward pass when the lit is in deferred mode. #2527

anisunity commented Nov 4, 2020 •

edited by sebastienlagarde

TomasKiniulis commented Nov 5, 2020

sebastienlagarde Nov 5, 2020

anisunity commented Nov 5, 2020

alelievr Nov 6, 2020

alelievr Nov 6, 2020

remi-chapelain left a comment

sebastienlagarde commented Nov 10, 2020

sebastienlagarde Nov 25, 2020

sebastienlagarde Nov 25, 2020

sebastienlagarde Nov 25, 2020

sebastienlagarde commented Nov 25, 2020

Added an emissive as forward pass when the lit is in deferred mode. #2527

Added an emissive as forward pass when the lit is in deferred mode. #2527

Conversation

anisunity commented Nov 4, 2020 • edited by sebastienlagarde

TomasKiniulis commented Nov 5, 2020

sebastienlagarde Nov 5, 2020

Choose a reason for hiding this comment

anisunity commented Nov 5, 2020

alelievr Nov 6, 2020

Choose a reason for hiding this comment

alelievr Nov 6, 2020

Choose a reason for hiding this comment

remi-chapelain left a comment

Choose a reason for hiding this comment

sebastienlagarde commented Nov 10, 2020

sebastienlagarde Nov 25, 2020

Choose a reason for hiding this comment

sebastienlagarde Nov 25, 2020

Choose a reason for hiding this comment

sebastienlagarde Nov 25, 2020

Choose a reason for hiding this comment

sebastienlagarde commented Nov 25, 2020

anisunity commented Nov 4, 2020 •

edited by sebastienlagarde