[URP] Automatic Alpha-To-Coverage Support #6312

gmitrano-unity · 2021-11-16T17:16:29Z

Purpose of this PR

This change attempts to automatically enable alpha-to-coverage
functionality whenever alpha clipping is in use on opaque materials
in URP. Since this functionality relies on MSAA, it is only active for
forward passes. With this change, alpha clipped geometry such as
foliage should see improved visual quality when MSAA is enabled.

No MSAA:

4x MSAA:

No MSAA (5x Zoom):

4x MSAA (5x Zoom):

Testing status

Manual testing on PC on Viking Village with MSAA on and off with both forward and deferred rendering modes.
Yamato with new AlphaToCoverage Graphics Tests
TODO: Mobile testing

Notes for Reviewers

I believe I've handled all of the edge cases at this point, but I'd appreciate extra review scrutiny on the material override handling and possible consequences of rendering out non 1.0 alpha values in cases where we weren't previously. This should only ever happen during opaque rendering though.

Notes for QA

This change is intended to improve the edge quality of alpha-clipped (ex: foliage) geometry only when MSAA is enabled.
It should have no visual effect when MSAA is off. The visual quality is also expected to change/improve with the number of MSAA samples (ex: 2x, 4x, 8x)

github-actions · 2021-11-16T17:16:44Z

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

Shader Graph
/jobDefinition/.yamato%252Fall-shadergraph.yml%2523PR_ShaderGraph_trunk
Depending on your PR, you may also want
/jobDefinition/.yamato%252Fall-shadergraph_builtin_foundation.yml%2523PR_ShaderGraph_BuiltIn_Foundation_trunk
/jobDefinition/.yamato%252Fall-shadergraph_builtin_lighting.yml%2523PR_ShaderGraph_BuiltIn_Lighting_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.

LaserYGD · 2021-12-02T21:41:36Z

Does this already exist for HDRP's MSAA?
edit: seems like it does not

sebastienlagarde · 2021-12-16T12:18:47Z

FYI, following the proposal in this PR to have alpha to coverage always enabled when MSAA is on, we have done the change in HDRP: #6599 (before this changed , alpha to coverage was manually enabled in HDRP)

The old images had some odd lighting baked into them that didn't show up in the normal render on the master branch. The difference wasn't large enough to actually fail the test, but it still should be corrected.

This change attempts to automatically enable alpha-to-coverage functionality whenever alpha clipping is in use in URP. Since this functionality relies on MSAA, it is only active for forward passes. With this change, alpha clipped geometry such as foliage should see improved visual quality when MSAA is enabled.

This change adds some logic to ensure that Shader Graph shaders always output either 1.0 or 0.0 when alpha clipping is enabled since any other values will have unintended consequences on the MSAA coverage mask.

This change replaces all usage of the AlphaDiscard() function with the new AlphaClip() function to ensure that all of the alpha clipping logic routes through the alpha-to-coverage logic.

This commit modifies the alpha to coverage logic to make sure it's only ever enabled on opaque materials with alpha clipping enabled. This fixes several visual differences for transparent materials that were caused by the previous implementation.

gmitrano-unity · 2021-12-17T15:14:36Z

FYI, following the proposal in this PR to have alpha to coverage always enabled when MSAA is on, we have done the change in HDRP: #6599 (before this changed , alpha to coverage was manually enabled in HDRP)

Took me a while to understand why the change is so much simpler in HDRP, but it's because of the special alpha testing logic in the depth prepass. Due to that, HDRP only needs to mess with a2c state for the prepass which means the normal lit shader pass doesn't have to deal with any edge cases of having a2c on or off. Nice!

Unfortunately, I don't think URP can do the same because it doesn't guarantee a depth prepass. That leaves us with something a bit more complex, but at least it'll be automated for both pipelines soon! 🙂

This change adds two new graphics tests that verify AlphaToCoverage functionality on both standard URP shaders and shader graph shaders.

This change simplifies the alpha-to-coverage logic now that the actual a2c HW feature is specifically only enabled for opaque materials that enable alpha clipping.

The previous commit accidentally broke the logic that ensures opaque geometry outputs 1 as the alpha value when MSAA is set to 1x. This was causing incorrect rendering in some scenes.

This change updates the Alpha-To-Coverage logic to handle the case where material override functionality is enabled. In this case, we aren't able to set the AlphaToMask render state based off of the shader graph target alone since it's possible for it to be changed later via overrides. To solve this, we fall back to using the hidden _AlphaToMask material property similar to the non-graph shaders.

This reverts commit 93396ac.

eh-unity

Some review comments about API breakage and URP shader style inconstencies.
Missing changelog.

eh-unity · 2022-02-07T11:40:59Z

com.unity.render-pipelines.universal/Editor/ShaderGraph/Includes/PBRForwardPass.hlsl

-        half alpha = 1;
-    #endif
+#if defined(_SURFACE_TYPE_TRANSPARENT)
+    half surfaceType = 1.0;


If this is a type and not just a value, let's define a proper constant/macro for the types. No one will remember what 1.0 is without looking a code after a while.

I replaced this type of logic with a simpler isTransparent bool, but I also added constants for surface types as well.

com.unity.render-pipelines.universal/Editor/ShaderGraph/Includes/UnlitPass.hlsl

eh-unity · 2022-02-07T11:50:56Z

com.unity.render-pipelines.universal/ShaderLibrary/ShaderVariablesFunctions.hlsl

+    return (_AlphaToMaskEnabled != 0.0);
+}
+
+half AlphaClip(half alpha, half cutoff)


Renaming shader functions will break user code, needs a backwards compatibility function.

What happened to the offset param? I don't mind not having it, but it must have been there for some reason (also API breakage).

I added the old function back and piped it through into this new logic and it cuts down the diff a bit too which is a nice side effect.

The offset parameter wasn't used anywhere and all we did with it before was add it into the cutoff parameter so it didn't seem very useful to me. It's still available in the AlphaDiscard() function in my latest change.

That should be good enough.

I agree, it doesn't seem very useful, but I would've thought something used it. Anyway it's in the public API.

eh-unity · 2022-02-07T11:52:32Z

com.unity.render-pipelines.universal/ShaderLibrary/ShaderVariablesFunctions.hlsl

-void AlphaDiscard(real alpha, real cutoff, real offset = real(0.0))
+// Only define the alpha clipping helpers when the alpha test define is present.
+// This should help identify usage errors early.
+#if defined(_ALPHATEST_ON)


Personally I like this, and I've tried to use similar thing for including files, but so far the argument has been that users might want to use this functions in their code and therefore we should omit useful functions based on our keywords.
That's why we don't do this in URP shaders currently.

Restoring the AlphaDiscard() reduces the impact of this issue a bit, but overall I'm not sure why we would want to expose these functions to users in cases where they cannot function as intended? The previous implementation just turns into a no-op when the alpha test macro isn't defined. This seems like a case of silent failure versus loud error and I usually prefer loud errors myself. :)

If we were to expose these functions, I think their implementation would just always return false for IsAlphaToMaskAvailable(), and always return input alpha for AlphaClip(). Not sure if that would be useful?

I agree with you as I mentioned. I like to fail fast too.

But in the past I've been told to not do this, so that we don't couple our users to our keywords (perhaps due to variant explosion).
Users can set the constants from code, so it is functional code, but of course they can set keywords too. Or if they fork the shader or SRP they can just add a define.
Let me get another opinion on it and maybe we can agree to change it.

We could also put the Alpha stuff in it's own file and you just include that if you need Alpha ops.

We discussed this internally. It should be ok to land like the way you have it now.

com.unity.render-pipelines.universal/ShaderLibrary/ShaderVariablesFunctions.hlsl

eh-unity · 2022-02-07T11:59:28Z

com.unity.render-pipelines.universal/ShaderLibrary/UnityGBuffer.hlsl

@@ -134,7 +134,7 @@ FragmentOutput SurfaceDataToGbuffer(SurfaceData surfaceData, InputData inputData
    output.GBuffer0 = half4(surfaceData.albedo.rgb, PackMaterialFlags(materialFlags));   // albedo          albedo          albedo          materialFlags   (sRGB rendertarget)
    output.GBuffer1 = half4(surfaceData.specular.rgb, surfaceData.occlusion);            // specular        specular        specular        occlusion
    output.GBuffer2 = half4(packedNormalWS, surfaceData.smoothness);                     // encoded-normal  encoded-normal  encoded-normal  smoothness
-    output.GBuffer3 = half4(globalIllumination, 1);                                      // GI              GI              GI              [optional: see OutputAlpha()] (lighting buffer)
+    output.GBuffer3 = half4(globalIllumination, 1);                                      // GI              GI              GI              unused          (lighting buffer)


Is there a potential breakage here? What if someone (a user) was relying on having alpha in that channel?

I actually removed this because I did look at OutputAlpha() based on the comment and I didn't find anything about how it interacted with the GBuffer. Not sure exactly what the intention was there.

My changes do actually affect the output alpha in some cases, but none of those cases should involve GBuffers. AlphaToMask requires MSAA so my changes are supposed to be limited to forward rendering only.

eh-unity · 2022-02-07T12:00:57Z

com.unity.render-pipelines.universal/ShaderLibrary/UnityInput.hlsl

@@ -63,6 +63,9 @@ float4 _ScreenParams;
 // y = 2.0 ^ [Mip Bias]
 float2 _GlobalMipBias;

+// 1.0 if AlphaToMask is enabled and 0.0 otherwise
+float _AlphaToMaskEnabled;


Is this matching with built-in? I think the UnityInput was for values from the Engine, or values that we setup but match built-in/engine.

Hmmm, no it's not matching built-in. I was just looking for a place I could put a shader parameter where it'd be globally accessible. Is there a better place for this type of thing in URP?

Looking at the UnityInput.hlsl, it looks like the "rule" has been broken already.
UnityInput.hlsl was supposed to match
https://docs.unity3d.com/2022.1/Documentation/Manual/SL-UnityShaderVariables.html
as far as I know, but there's other stuff too like _GlobalMipBias.
Perhaps, it's not an issue then.

I think the correct place would be Input.hlsl where all the URP constants are.

eh-unity · 2022-02-07T12:01:44Z

com.unity.render-pipelines.universal/Shaders/BakedLit.shader

@@ -41,6 +42,8 @@ Shader "Universal Render Pipeline/Baked Lit"
            Name "BakedLit"
            Tags{ "LightMode" = "UniversalForwardOnly" }

+            AlphaToMask[_AlphaToMask]


I wonder if this is actually useful for BakedLit?

com.unity.render-pipelines.universal/Shaders/Nature/SpeedTree8Passes.hlsl

eh-unity · 2022-02-07T12:08:24Z

com.unity.render-pipelines.universal/Shaders/Terrain/WavingGrass.shader

@@ -16,6 +16,8 @@ Shader "Hidden/TerrainEngine/Details/UniversalPipeline/WavingDoublePass"

        Pass
        {
+            AlphaToMask On


Should this be AlphaToMask[_AlphaToMask]?

This change restores the AlphaDiscard() function and replaces its internals with the new alpha-to-coverage logic. This makes several small shader changes no longer necessary and makes the diff a bit smaller (in addition to not breaking existing external code). This change also attempts to rename the AlphaToMaskEnabled shader variables to better reflect their true meaning.

Add an extra note about the behavior of the AlphaDiscard() function when the "_ALPHATEST_ON" define is not present and also updated the changelog.

aleksandrasdzikia

LGTM. Found issue but not related to this PR. Other than that results are as expected.

sebastienlagarde · 2022-05-30T12:43:48Z

@gmitrano-unity guess this PR have landed in private repo? can you close it then? thnaks

gmitrano-unity · 2022-05-31T13:33:39Z

Closing since it's already landed

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

gmitrano-unity force-pushed the urp/a2c branch 3 times, most recently from 51cdba4 to ca37583 Compare November 24, 2021 17:09

gmitrano-unity force-pushed the urp/a2c branch from ca37583 to 6cfd867 Compare December 2, 2021 19:33

adrien-de-tocqueville mentioned this pull request Dec 14, 2021

Automatically enable Alpha-To-Coverage when alpha clipping is on #6599

Merged

gmitrano-unity force-pushed the urp/a2c branch from e1573d3 to fd2d831 Compare December 15, 2021 17:45

gmitrano-unity added 5 commits December 16, 2021 13:47

Updated Deferred Shader Graph Reference Images

93396ac

The old images had some odd lighting baked into them that didn't show up in the normal render on the master branch. The difference wasn't large enough to actually fail the test, but it still should be corrected.

Fix Output Alpha for Shader Graph

7f14183

This change adds some logic to ensure that Shader Graph shaders always output either 1.0 or 0.0 when alpha clipping is enabled since any other values will have unintended consequences on the MSAA coverage mask.

Replace AlphaDiscard() with AlphaClip

9d08a7f

This change replaces all usage of the AlphaDiscard() function with the new AlphaClip() function to ensure that all of the alpha clipping logic routes through the alpha-to-coverage logic.

gmitrano-unity force-pushed the urp/a2c branch from fd2d831 to 1ff77b0 Compare December 16, 2021 23:24

gmitrano-unity added 12 commits December 17, 2021 15:18

Graphics Test For AlphaToCoverage Functionality

4f8d9f5

This change adds two new graphics tests that verify AlphaToCoverage functionality on both standard URP shaders and shader graph shaders.

Small Fixes for A2C Upgrade Logic & Misc Comments

c2b062b

Simplify Alpha-To-Coverage Shader Logic

f0f1cdf

This change simplifies the alpha-to-coverage logic now that the actual a2c HW feature is specifically only enabled for opaque materials that enable alpha clipping.

Fix Issues Introduced By Logic Simplification

b194737

The previous commit accidentally broke the logic that ensures opaque geometry outputs 1 as the alpha value when MSAA is set to 1x. This was causing incorrect rendering in some scenes.

Small Code Clarity Tweak and Note

3a9876e

Formatting Fix for Test Readme File

8a11d99

Add Reference Images for AlphaToCoverage Tests

dafa720

Revert Deferred Shader Graph Reference Images

8521054

This reverts commit 93396ac.

Merge branch 'master' into urp/a2c

276331e

Added Missing .meta Files for A2C Tests

be81705

Merge branch 'master' into urp/a2c

46d48cb

gmitrano-unity marked this pull request as ready for review January 14, 2022 20:50

gmitrano-unity requested review from a team as code owners January 14, 2022 20:50

github-actions bot added Shader Graph testing labels Jan 14, 2022

alekseiunity mentioned this pull request Jan 14, 2022

[14.x.x] LOD Cross Fade #6754

Open

Merge branch 'master' into urp/a2c

f60d8bf

eh-unity reviewed Feb 7, 2022

View reviewed changes

gmitrano-unity added 3 commits February 15, 2022 13:09

Merge branch 'master' into urp/a2c

1c7d770

Comments & Changelog

0395725

Add an extra note about the behavior of the AlphaDiscard() function when the "_ALPHATEST_ON" define is not present and also updated the changelog.

phi-lira requested a review from a team February 21, 2022 15:28

aleksandrasdzikia approved these changes Mar 1, 2022

View reviewed changes

gmitrano-unity closed this May 31, 2022

[URP] Automatic Alpha-To-Coverage Support #6312

[URP] Automatic Alpha-To-Coverage Support #6312

Uh oh!

Conversation

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

Uh oh!

Purpose of this PR

Testing status

Notes for Reviewers

Notes for QA

Uh oh!

github-actions bot commented Nov 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

gmitrano-unity commented Dec 17, 2021

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

aleksandrasdzikia left a comment

Choose a reason for hiding this comment

Uh oh!

sebastienlagarde commented May 30, 2022

Uh oh!

gmitrano-unity commented May 31, 2022

Uh oh!

Uh oh!

gmitrano-unity commented Nov 16, 2021 •

edited

Loading

github-actions bot commented Nov 16, 2021 •

edited

Loading

LaserYGD commented Dec 2, 2021 •

edited

Loading

sebastienlagarde commented Dec 16, 2021 •

edited

Loading