Remove BLEND keywords and replace with constant branch #1884

FrancescoC-unity · 2020-09-15T14:28:55Z

Mostly all in the title. This should reduce build times when a mix of blend modes are used in the project.

There were some gotchas with VFX and shadergraph, but all are sorted in this PR.
Tests pass locally, but launching yamato anyway to be sure.

alelievr

LGTM

com.unity.render-pipelines.high-definition/Runtime/Material/Material.hlsl

PaulDemeulenaere

We discussed about this : https://github.com/Unity-Technologies/Graphics/pull/1884/files#diff-0f3209dd5b092701c9a479cd175f56c9R434

It seems fine, no real impact on the VFX where it's still a #define

Specific to HDRP.

com.unity.render-pipelines.high-definition/Runtime/Material/Lit/LitTessellation.shader

com.unity.render-pipelines.high-definition/Runtime/Material/Material.hlsl

sebastienlagarde · 2020-10-01T19:27:13Z

...render-pipelines.high-definition/Editor/VFXGraph/Outputs/VFXAbstractParticleHDRPLitOutput.cs

@@ -431,13 +431,13 @@ public override void OnEnable()
                switch (blendMode)
                {
                    case BlendMode.Alpha:
-                        forwardDefines.WriteLine("#define _BLENDMODE_ALPHA");
+                        forwardDefines.WriteLine("#define _BlendMode 0");


hmm... wondering if we can define based on

#define BLENDMODE_ALPHA (0)
#define BLENDMODE_PREMULTIPLY (4)
#define BLENDMODE_ADDITIVE (1)

here

No won't work as we don't have the include in hlsl all the time for vfx last I checked. I can see if I can make sure that is included or if I didn't look well enough :-)

That said, while def it would look better, it is still hardcoded in a string, so won't save our ass in case of changes.

FYI, this defines lands here
Unlit (not concerned) :

Lit :

Indeed, there isn't any include before these define, we can potential feed these #define BLENDMODE_ALPHA (0) within the same function, before the switch, not sure it's worth it.
Isn't there a enum which fits these value ? I could be a generated string.

#define BLENDMODE_ALPHA (0) #define BLENDMODE_PREMULTIPLY (4) #define BLENDMODE_ADDITIVE (1)

@FrancescoC-unity Can you confirm these defines are only needed for lit output ? The output which are including :

Packages/com.unity.render-pipelines.high-definition/Runtime/Material/Material.hlsl

Packages/com.unity.render-pipelines.high-definition/Runtime/Material/Lit/Lit.hlsl

Packages/com.unity.render-pipelines.high-definition/Runtime/Material/BuiltinUtilities.hlsl

There is an enum on C# that generate an hlsl file with the defines, that hlsl file is included in Material/Material.hlsl
, however we cannot do our define after the include of Material.hlsl, otherwise it won't compile. So we need an additional include for that specific file.

Whether is worth it or not I guess I'll leave the decision to you guys, feels a bit overkill to me, but indeed it'd make it a tad more readable.

This would be needed for whatever includes Material.hlsl, so yes, I assume only lit output.

no, it is required for both lit and unlit transparent.
ok nevermind for now, this is something that can't change anyway to not break the universe, let's just add a comment on which enum it map for now.

No, unlit vfx doesn't use the blend mode as in HDRP, so does not need this as it does not include Material.hlsl

This was verified btw running the test, if it was needed for Unlit too the test wouldn't have compiled

no, it is required for both lit and unlit transparent.

No, unlit vfx doesn't use the blend mode as in HDRP,

I confirm @FrancescoC-unity point, only needed for Lit HDR. For instance, it's needed using internal EvaluateAtmosphericScattering
For the fog evaluation, we are manually applying falloff using other defines : https://github.cds.internal.unity3d.com/unity/vfx-graphics/blob/vfx/staging/com.unity.render-pipelines.high-definition/Runtime/VFXGraph/Shaders/VFXCommon.hlsl#L154

If there is a compilation error in case of mistake, that's fine !

sebastienlagarde · 2020-10-01T19:28:08Z

com.unity.render-pipelines.high-definition/Runtime/Material/AxF/AxF.shader

@@ -120,7 +120,8 @@ Shader "HDRP/AxF"


 //      [ToggleUI] _EnableFogOnTransparent("Enable Fog", Float) = 1.0
-//      [ToggleUI] _EnableBlendModePreserveSpecularLighting("Enable Blend Mode Preserve Specular Lighting", Float) = 1.0
+        // We never set this as it is always 1, but its definition is needed when performing blending in Material.hlsl
+        [HideInInspector][ToggleUI] _EnableBlendModePreserveSpecularLighting("Enable Blend Mode Preserve Specular Lighting", Float) = 1.0


it is not true (at least for now as _BLENDMODE_PRESERVE_SPECULAR_LIGHTING is still a keyword)

I am referencing the property not keyword here, beforehand the property was not used, here I instead just define it (the keyword is still used as it should)

In any case it is actually not true anymore (is not needed anymore after some subsequent changes I made) so I will revert this bit tomorrow.

This reverts commit e3183f0.

* Fix warning in ShaderPassDecal.hlsl * Tooltip for microshadows (#2100) * Update Alembic version to stop Alembic complaining * Disable quad overdraw on ps4 (#2107) * disable quad overdraw on ps4 * update doc Co-authored-by: sebastienlagarde <sebastien@unity3d.com> * Enable the DXR Vertex Format test for RG (#2022) * Add option to enable pure specular planar reflection (optimization) (#2105) * - Added a rough refraction option on planar reflections. - Added a rough distortion frame setting and and info box on distortion materials. - Added tests to cover the new features. * review changes * removed PerceptualRoughnessToMipmapLevel_LODFlag which isn't required. * clean more code (remove OutputSmoothPlanarReflection) * missing one update * removed rough distortion * remove distortion test scene * Update DistortionUIBlock.cs * clean distortion * update documentation * address doc feedback * Update Upgrading-from-2020.1-to-2020.2.md * Update Upgrading-from-2020.1-to-2020.2.md * update test 2220 * update smoothness test * Create 2220_SmoothPlanarReflection.png * update reference screenshots Co-authored-by: Anis <anis@unity3d.com> * Implementing scalabity settings for planar resolutions (#2059) * - Added a rough refraction option on planar reflections. - Added a rough distortion frame setting and and info box on distortion materials. - Added tests to cover the new features. * review changes * Added scalability settings for the planar reflection resolution. * support migration. * renamed the resolution parameters for the planar reflections * removed PerceptualRoughnessToMipmapLevel_LODFlag which isn't required. * clean more code (remove OutputSmoothPlanarReflection) * missing one update * removed rough distortion * remove distortion test scene * Update DistortionUIBlock.cs * clean distortion * update documentation * address doc feedback * Update Upgrading-from-2020.1-to-2020.2.md * Update Upgrading-from-2020.1-to-2020.2.md * update test 2220 * Revert "renamed the resolution parameters for the planar reflections" This reverts commit 753af88. * cleanup migration code * update smoothness test * Create 2220_SmoothPlanarReflection.png * some cleanup * Revert "Revert "renamed the resolution parameters for the planar reflections"" This reverts commit fbf3572. * update doc Co-authored-by: sebastienlagarde <sebastien@unity3d.com> * Remove useless SampleSkyTexture function * Fix errors when resizing the compositor's output and when removing/adding the compositor (#2118) # Conflicts: # com.unity.render-pipelines.high-definition/CHANGELOG.md * Remove BLEND keywords and replace with constant branch (#1884) * Removing blend keyword. * Changelog * Update DecalProperties.hlsl * revert change in AxF shader * fix compile issue with ordering when using VT * update comment * Update Upgrading-from-2020.1-to-2020.2.md * update comment + optimized code version (in case of bad compiler) * update comment revert (wrong) optimization * remove unused multicopmile in test * candidate fix for VFX * Revert "candidate fix for VFX" This reverts commit e3183f0. * Another tentative fix * Try differently * Fix compile issue. * Another vfx fix attempt. * Latest flavour of fix. * Remove old defines * Fix hybrid * Update includes in ray tracing * Update Upgrading-from-2020.1-to-2020.2.md Co-authored-by: Sebastien Lagarde <sebastien@unity3d.com> * Mixed cached shadow maps (Cached static + dynamic rendered every frame) (#1559) * UI for mixed cached shadows * Baby steps (# 1) * Baby steps (# 2) * Extrapolate the update request data into a function * Commit before blit code * Need to switch branch * Need to switch branch * Working as long the light does not move. * Fixed area light case * Make it render graph ready * Make sure we correctly update the freshly updated cached shadows * Raw update upon movement option (Missing API for threshold) * Threshold API * Add unity binary tag * New docs * Shader name change * Update docs * Fix bad merge * Update Shadows-in-HDRP.md * For now disable the directional light case. * Disable directional mixed. * Make it disabled under define * Remove docs * Implement rendergraph version * Remove superflous blit * Tentative fix for yamato. Co-authored-by: JordanL8 <lewis.jordan@hotmail.co.uk> * update meta file with wrong information Co-authored-by: Adrien de Tocqueville <adrien.tocqueville@unity3d.com> Co-authored-by: anisunity <42026998+anisunity@users.noreply.github.com> Co-authored-by: Anis <anis@unity3d.com> Co-authored-by: Pavlos Mavridis <pavlos.mavridis@unity3d.com> Co-authored-by: FrancescoC-unity <43168857+FrancescoC-unity@users.noreply.github.com> Co-authored-by: JordanL8 <lewis.jordan@hotmail.co.uk>

FrancescoC-unity added 2 commits September 15, 2020 15:58

Removing blend keyword.

eeb426c

Changelog

a8c6b12

FrancescoC-unity added the HDRP label Sep 15, 2020

FrancescoC-unity requested review from alelievr, sebastienlagarde and PaulDemeulenaere September 15, 2020 14:28

FrancescoC-unity self-assigned this Sep 15, 2020

alelievr approved these changes Sep 15, 2020

View reviewed changes

sebastienlagarde reviewed Sep 15, 2020

View reviewed changes

com.unity.render-pipelines.high-definition/Runtime/Material/Material.hlsl Show resolved Hide resolved

PaulDemeulenaere approved these changes Sep 30, 2020

View reviewed changes

sebastienlagarde added 2 commits October 1, 2020 19:37

Merge branch 'HDRP/staging' into HDRP/remove-blend-keyword

b45cf64

Update DecalProperties.hlsl

9565a43

sebastienlagarde reviewed Oct 1, 2020

View reviewed changes

com.unity.render-pipelines.high-definition/Runtime/Material/Lit/LitTessellation.shader Show resolved Hide resolved

sebastienlagarde reviewed Oct 1, 2020

View reviewed changes

com.unity.render-pipelines.high-definition/Runtime/Material/Material.hlsl Show resolved Hide resolved

sebastienlagarde reviewed Oct 1, 2020

View reviewed changes

FrancescoC-unity and others added 14 commits October 2, 2020 08:46

revert change in AxF shader

d6a14a1

Merge branch 'HDRP/staging' into HDRP/remove-blend-keyword

e96956e

fix compile issue with ordering when using VT

e243743

update comment

82db84e

Update Upgrading-from-2020.1-to-2020.2.md

d4c0cea

update comment + optimized code version (in case of bad compiler)

2e945e7

update comment revert (wrong) optimization

a9d8502

Merge branch 'HDRP/staging' into HDRP/remove-blend-keyword

7ad64a0

Merge branch 'HDRP/staging' into HDRP/remove-blend-keyword

a96f083

remove unused multicopmile in test

c29c2c2

candidate fix for VFX

e3183f0

Revert "candidate fix for VFX"

4f9ee94

This reverts commit e3183f0.

Another tentative fix

4d7bc8c

Try differently

8307108

FrancescoC-unity added 4 commits October 5, 2020 11:58

Fix compile issue.

6dd8c6d

Another vfx fix attempt.

0d41c06

Latest flavour of fix.

b07cec0

Remove old defines

dbb63b4

sebastienlagarde approved these changes Oct 5, 2020

View reviewed changes

sebastienlagarde and others added 5 commits October 5, 2020 20:48

Merge branch 'HDRP/staging' into HDRP/remove-blend-keyword

8acf5ff

Fix hybrid

bb5730a

Update includes in ray tracing

c26d612

Merge branch 'HDRP/staging' into HDRP/remove-blend-keyword

112a16d

Update Upgrading-from-2020.1-to-2020.2.md

f207165

sebastienlagarde approved these changes Oct 7, 2020

View reviewed changes

sebastienlagarde merged commit d9549ad into HDRP/staging Oct 7, 2020

sebastienlagarde deleted the HDRP/remove-blend-keyword branch October 7, 2020 22:29

sebastienlagarde mentioned this pull request Oct 7, 2020

Merge Hdrp/staging #2109

Merged

Remove BLEND keywords and replace with constant branch #1884

Remove BLEND keywords and replace with constant branch #1884

Uh oh!

Conversation

FrancescoC-unity commented Sep 15, 2020

Uh oh!

alelievr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

PaulDemeulenaere left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FrancescoC-unity Oct 1, 2020 • 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!

FrancescoC-unity Oct 1, 2020 • 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!

PaulDemeulenaere Oct 2, 2020 • 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!

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!

FrancescoC-unity Oct 1, 2020 •

edited

Loading

FrancescoC-unity Oct 1, 2020 •

edited

Loading

PaulDemeulenaere Oct 2, 2020 •

edited

Loading