Case 1357902: Optimize Mesh Data setting will corrupt UV mesh channels

[KEngelstoft]

Purpose of this PR

Fix case 1357902: 'Optimize Mesh Data' strip away variants without the keyword set on the vertex stage.

Testing status

How to reproduce:

1. Open project "TEST 2.zip" from https://fogbugz.unity3d.com/f/cases/1357902
2. In the menu bar, click on File>Build And Run
3. When the application launches, observe materials of the GameObjects around

Expected result: Materials of the GameObjects are rendered the same as in the Editor
Actual result: Materials of the GameObjects are rendered with the lightmap on it

Reproducible with: 2021.2.0b6, 2021.2.0b8, 2022.1.0a6
Not reproducible with: 2020.3.16f1, 2021.1.17f1
Can't reproduce with: 2019.4.30f1 - Can't build because of errors in the Console

Tested locally using Windows10 and a Threadripper 3990 ES and a Radeon 6800XT.

Before, the shader graph generated:
#pragma multi_compile_fragment _ LIGHTMAP_ON
#pragma multi_compile_raytracing _ LIGHTMAP_ON

After, the shader graph generates:
#pragma multi_compile_vertex _ LIGHTMAP_ON
#pragma multi_compile_fragment _ LIGHTMAP_ON
#pragma multi_compile_raytracing _ LIGHTMAP_ON

Comments to reviewers

Case https://fogbugz.unity3d.com/f/cases/1357902
Jira https://jira.unity3d.com/browse/GFXGI-642

The same bug appears to affect all the KeywordDescriptors that was using fragment + raytracing in HDTarget.cs, so I fixed all of them.
We could go even wider and make it target all stages, instead of targeting vertex, fragment and raytracing like this:
#pragma multi_compile _ LIGHTMAP_ON

[github-actions]

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://yamato.cds.internal.unity3d.com/jobs/902-Graphics
Search for your PR branch using the sidebar on the left, then add the following segment(s) to the end of the URL (you may need multiple tabs depending on how many packages you change)

HDRP
/.yamato%252Fall-hdrp.yml%2523PR_HDRP_trunk
With changes to HDRP packages, you should also run
/.yamato%252Fall-lightmapper.yml%2523PR_LightMapper_trunk

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

[sebastienlagarde]

Hey thanks for the investigation on this issue. I will take the ownership of the follow up on it as in current state it simply revert all shader variant optimization that we have done with stage frequency. And the issue will be the same with URP. Which mean stage frequency is basically useless (cc @FrancescoC-unity and @aleks01010101 ). Will open a discusison on slack about it.

regarding the PR itself we will need to apply it to regualr lit, axf, layered lit etc... shader as well if we chose to do this. Will inform you when I know more, thanks for tracking this

[sebastienlagarde]

Currently let's first start the discussion here (For Aleksander and other) - cc @phi-lira as maybe URP have done the same agressive optimization on lightmap?

This PR fix an issue with our usage of stage frequency to reduce shader variant. If I understand correctly when we use 'Optimize Mesh Data', stage frequency basically become useless.

I guess that in the PR this is a bit agressive as I think what we are trying to solve is just to be sure that UV1 and 2 (i.E for lighting data) aren't stripped. Other thing like - disable SSR, surface gradient, light layers, probe volume etc... shouldn't be impacted as they aren't related to attribute - correct me if I am wrong.

I am correct? And then the true problem is that 'Optimize Mesh Data' is using vertex shader information to know what can be strip, but as we dont generate variant for ligthmap for vertex shader it incorrectly strip it right? Guess we should rather try to understand how to fix this behavior

[KEngelstoft]

Currently let's first start the discussion here (For Aleksander and other) - cc @phi-lira as maybe URP have done the same agressive optimization on lightmap?

This PR fix an issue with our usage of stage frequency to reduce shader variant. If I understand correctly when we use 'Optimize Mesh Data', stage frequency basically become useless.

I guess that in the PR this is a bit agressive as I think what we are trying to solve is just to be sure that UV1 and 2 (i.E for lighting data) aren't stripped. Other thing like - disable SSR, surface gradient, light layers, probe volume etc... shouldn't be impacted as they aren't related to attribute - correct me if I am wrong.

I am correct? And then the true problem is that 'Optimize Mesh Data' is using vertex shader information to know what can be strip, but as we dont generate variant for ligthmap for vertex shader it incorrectly strip it right? Guess we should rather try to understand how to fix this behavior

The vertex shader is queried for the used VertexComponents and the unused components are discarded in Player builds when using 'Optimize Mesh Data'. See CalculateRequiredVertexComponentsForRenderer in Editor\Src\BuildPipeline\ComputeBuildUsageTagOnObjects.cpp on the C++ side as reference.

For instance, if light layers or another feature is used which relies on non-standard vertex components (position, normal, UV0), it will not work in the player either as tangents etc. will be stripped. The only reason we get any vertex data is this old fall back in Shader::CalculateUsedVertexComponents used for fixed function style shaders (the DXR shader passes don't have a fragment shader and causes the fallback to be hit):

if (!pass.HasProgram(kShaderVertex))
{
// No vertex shader? Should not happen, but let's play safe and bind some channels
mask |= kShaderChannelMaskVertex | kShaderChannelMaskNormal | kShaderChannelMaskTexCoord0;
continue;
}

For this reason, I don't think this issue is specific to the lightmap related vertex streams (UV1 and UV2).

Furthermore we might have an orthogonal issue if tessellation is used - it is not included in these multicompiles (could crash in D3D12 because VS and PS semantics don't match) cc @IonutNedelcuUnity.

[aleks01010101]

LIGHTMAP_ON is a special case here - there's a hardcoded check on the C++ side.
Optimise mesh data shouldn't affect other stage-frequency keywords, as you cannot have a varying that is conditionally excluded based on a FS stage keyword, for example.

[sebastienlagarde]

LIGHTMAP_ON is a special case here - there's a hardcoded check on the C++ side.
Optimise mesh data shouldn't affect other stage-frequency keywords, as you cannot have a varying that is conditionally excluded based on a FS stage keyword, for example.

Ok so based on Kasper and Aleksandr comment and if I get it correctly we should only change the frequency for Lightmap.
i.e all lightmap related keyword (LIGHTMAP_ON, DIRLIGHTMAP_COMBINED, DYNAMICLIGHTMAP_ON) should be at both vertex and fragment stage. No need to change other one.
(and from Aleksandr comment, it looks like only LIGHTMAP_ON is sufficient, all other can stay unchanged, right?)

@KEngelstoft I can take the PR and update it if you want. Need to be done for shader graph but also for Axf, lit, layeredlit, and tessellation version.

[KEngelstoft]

LIGHTMAP_ON is a special case here - there's a hardcoded check on the C++ side.
Optimise mesh data shouldn't affect other stage-frequency keywords, as you cannot have a varying that is conditionally excluded based on a FS stage keyword, for example.

Ok so based on Kasper and Aleksandr comment and if I get it correctly we should only change the frequency for Lightmap.
i.e all lightmap related keyword (LIGHTMAP_ON, DIRLIGHTMAP_COMBINED, DYNAMICLIGHTMAP_ON) should be at both vertex and fragment stage. No need to change other one.
(and from Aleksandr comment, it looks like only LIGHTMAP_ON is sufficient, all other can stay unchanged, right?)

@KEngelstoft I can take the PR and update it if you want. Need to be done for shader graph but also for Axf, lit, layeredlit, and tessellation version.

Please go ahead if none of the other ones use the non-standard vertex data (only kShaderChannelMaskVertex | kShaderChannelMaskNormal | kShaderChannelMaskTexCoord0).

As for the whole shader variant explosion, stripping problems and shader variant compilation times now measured in hours, perhaps the time is right for HDRP to go towards an uber shader approach with coherent branching based on uniform bools? Branching would be coherent for the warp and this model would save hours of compilation and stripping build time and potentially lots of disk space.

[aleks01010101]

only LIGHTMAP_ON is sufficient
Yes, this should be sufficient. This is the only one that has special treatment for vertex data optimisation.

uber shader approach with coherent branching based on uniform bools
Juho is preparing a feature for this right now :)

[sebastienlagarde]

As for the whole shader variant explosion, stripping problems and shader variant compilation times now measured in hours, perhaps the time is right for HDRP to go towards an uber shader approach with coherent branching based on uniform bools?

There is devils in the detail on shader variant with shader feature and multi compule. We have perform various change and optimization without saying any gain sometimes. Most efficient is mainly to focus on multi-compile (the lod fade is a killer for example but we can't remove it :( ).

Speaking about this we would love to remove all those multicompile for lightmaps and have the opportunity to do dynamic branching instead. But I am not sure about what will be the consequence for the C++ side. For HDRP it will save a lot of variant and dynamic branching is efficient in such a case. however for the platform supported by urp it will not be an option. Let's have a chat about it if you are agree.

[sebastienlagarde]

@qa: tested the repro scene and it is working now with this PR

[jessebarker]

LGTM

[sebastienlagarde]

Error in Yamato come from master as we don't have merge master. Yamato is green otherwise. For Qa: There is no need for further testing than Yamato and the scene repro, so merging