Fixed some Shader Graph shaders fail to compile for Android XR #4806

thomas-zeng · 2021-06-04T20:28:50Z

Purpose of this PR

Fixed fogbugz case https://fogbugz.unity3d.com/f/cases/1340356/

Testing status

QA
ABV

Comments to reviewers

Added special handling for fields within UNITY_STEREO_INSTANCING_ENABLED define so that they could be placed at the end of PackedVaryings struct. This is required for SV_RenderTargetArrayIndex to avoid error: Non system-generated input signature parameter () cannot appear after a system generated value
To QA: I have tested the repro project on Quest device and was able to see objects with shader graph materials render as expected.

…on system generated semantics.

… into universal/xr/fix-sg-packedvaryings

fabien-unity · 2021-06-04T21:10:27Z

com.unity.shadergraph/Editor/Generation/Processors/GenerationUtils.cs

                    //
                    if (subscript.HasPreprocessor() && (subscript.preprocessor.Contains("SHADER_STAGE_FRAGMENT")))
                        postUnpackedSubscripts.Add(subscript);
+                    // special case, "UNITY_STEREO_INSTANCING_ENABLED" fields must be packed at the end of the struct because they are system generated semantics


This will not conflict with the previous one (SHADER_STAGE_FRAGMENT) which also needs to be at the end ?

SHADER_STAGE_FRAGMENT field is also a system generated semantic(FRONT_FACE_SEMANTIC) so it won't conflict in our XR use case. However the comment mentioned semantic ordering in fragment shader I am not sure about that use case. Will dig in more.

Graphics/com.unity.shadergraph/Editor/Generation/TargetResources/StructFields.cs

Line 60 in c6ae759

public static FieldDescriptor cullFace = new FieldDescriptor(Varyings.name, "cullFace", "VARYINGS_NEED_CULLFACE", "FRONT_FACE_TYPE",

The SHADER_STAGE_FRAGMENT case is there for situations where frag only SVs coming in wouldn't've been part of the vertex output but still needs to be in the interpolator structure. Since the custom interpolator feature allows arbitrarily adding more interpolators, we had to make sure frag only fields were pushed to the bottom.

A similar issue shows up with instancing, but the instance ids are optionally established in the vertex stage. So-- I think what we want here is for the UNITY_STEREO_INSTANCING_ENABLED case to come before the SHADER_STAGE_FRAGMENT case. They both need to be pushed to the end, but fragment should come after instancing.

I think it's safe (and preferred) to generalize this for any type of instancing, not just stereo.

Hi @esmelusina , thanks for the feedback!
I have moved the UNITY_STEREO_INSTANCING_ENABLED case before the SHADER_STAGE_FRAGMENT case in 3774019. I also checked the usage of SV_InstanceID in shader graph. It is only used as vertex shader input and not being used in Varyings. So I think we don't need to handle it in the PackedVaryings logic here.

It can be optionally pulled in if the "Instance ID" node is used in the fragment stage. I'd prefer if you went ahead and generalized it.

Thanks!

Thanks @esmelusina ,
I have made another commit that generalize the instance case here: 2a2afbe

esmelusina

Changes requested in the comment thread there (and mentioned on slack).

Let me know if that's unclear.

DennisDeRykeUnity · 2021-06-09T05:06:35Z

Dev branch (universal/xr/fix-sg-packedvaryings bb08c6a) is green on Yamato for Android XR OpenGL URP Foundation, Lighting, PostProc, and Terrain per links below. For Vulkan these builders fail, but per discussion with XR Graphics devs this is a separate bug(s). I see that PR 4806 is still under dev review, so there will probably be further commits before it can be reviewed by QA.

https://yamato.cds.internal.unity3d.com/jobs/902-Graphics/tree/universal%252Fxr%252Ffix-sg-packedvaryings/.yamato%252Furp_foundation-android-opengles3.yml%2523Build_URP_Foundation_Android_OpenGLES3_Standalone_XR_il2cpp_Linear_trunk/7176785/job

https://yamato.cds.internal.unity3d.com/jobs/902-Graphics/tree/universal%252Fxr%252Ffix-sg-packedvaryings/.yamato%252Furp_lighting-android-opengles3.yml%2523URP_Lighting_Android_OpenGLES3_Standalone_XR_il2cpp_Linear_trunk/7176794/job

https://yamato.cds.internal.unity3d.com/jobs/902-Graphics/tree/universal%252Fxr%252Ffix-sg-packedvaryings/.yamato%252Furp_postpro-android-opengles3.yml%2523URP_PostPro_Android_OpenGLES3_Standalone_XR_il2cpp_Linear_trunk/7176801/job

https://yamato.cds.internal.unity3d.com/jobs/902-Graphics/tree/universal%252Fxr%252Ffix-sg-packedvaryings/.yamato%252Furp_terrain-android-opengles3.yml%2523URP_Terrain_Android_OpenGLES3_Standalone_XR_il2cpp_Linear_trunk/7176803/job

…ce semantic handling.

- Generalized the case for all preprocessor cases that contains INSTANCING as keyword.

esmelusina

Thanks for the changes. I'm approving for shadergraph, I'm going to @tian-unity to see if they want to check that this interacts correctly with speed tree.

DennisDeRykeUnity

This PR’s dev branch last inherited from master around 7/4/2021 so I am comparing Nightly from back then on master against today’s Nightly on the dev branch

Graphics (master 45a5ae2)
https://yamato.cds.internal.unity3d.com/jobs/902-Graphics/tree/master/.yamato%252Fall-vfx.yml%2523Nightly_VFX_trunk/7130646/job/pipeline
Graphics (universal/xr/fix-sg-packedvaryings 2a2afbe)
https://yamato.cds.internal.unity3d.com/jobs/902-Graphics/tree/universal%252Fxr%252Ffix-sg-packedvaryings/.yamato%252F_abv.yml%2523all_project_ci_nightly_trunk/7365566/job

The dev branch is green wherever master was green except for several jobs. These became green upon re-run except for these two:

URP_Performance_BoatAttack on Win_DX12_performance_build_editmode_mono_Linear
URP_Performance_BoatAttack on Win_Vulkan_performance_build_editmode_mono_Linear

Both of these jobs had the following error and crashed. I do not believe this is due to this PR because I see the same error and crash in recent jobs on Graphics master for these two pipelines.

Error retrieving packages from source 'https://artifactory-slo.bf.unity3d.com/artifactory/api/nuget/slough-nuget':
Could not connect to the feed specified at 'https://artifactory-slo.bf.unity3d.com/artifactory/api/nuget/slough-nuget'. Please verify that
[12:17:30.831 Information] the package source (located in the Package Manager Settings) is valid and ensure your network connectivity.

esmelusina

Re-Approving to clear lock.

thomas-zeng added 2 commits June 4, 2021 13:20

* Fixed an issue where system generated semantics are placed before n…

43b0324

…on system generated semantics.

Merge branch 'master' of https://github.com/Unity-Technologies/Graphics…

a19fc17

… into universal/xr/fix-sg-packedvaryings

thomas-zeng requested a review from fabien-unity June 4, 2021 20:29

github-actions bot added the Shader Graph label Jun 4, 2021

thomas-zeng requested a review from sandy-carter-unity June 4, 2021 20:29

thomas-zeng added XR universal labels Jun 4, 2021

thomas-zeng requested a review from DennisDeRykeUnity June 4, 2021 20:31

fabien-unity reviewed Jun 4, 2021

View reviewed changes

thomas-zeng requested a review from esmelusina June 4, 2021 21:38

* Formatted the file.

bb08c6a

esmelusina requested changes Jun 8, 2021

View reviewed changes

* Addressed some feedback - moving stereo output logic before cull fa…

3774019

…ce semantic handling.

fabien-unity approved these changes Jun 10, 2021

View reviewed changes

thomas-zeng requested a review from esmelusina June 10, 2021 17:05

* Addressed second round feedback.

2a2afbe

- Generalized the case for all preprocessor cases that contains INSTANCING as keyword.

esmelusina approved these changes Jun 14, 2021

View reviewed changes

thomas-zeng marked this pull request as ready for review June 15, 2021 20:54

thomas-zeng requested a review from a team as a code owner June 15, 2021 20:54

DennisDeRykeUnity approved these changes Jun 23, 2021

View reviewed changes

thomas-zeng removed the request for review from sandy-carter-unity June 24, 2021 16:33

esmelusina self-requested a review July 6, 2021 17:38

esmelusina approved these changes Jul 6, 2021

View reviewed changes

thomas-zeng merged commit b2f7eb7 into master Jul 6, 2021

thomas-zeng deleted the universal/xr/fix-sg-packedvaryings branch July 6, 2021 17:56

jessebarker added needs-backport-7.x needs-backport-10.x labels Sep 28, 2021

jessebarker mentioned this pull request Sep 29, 2021

[ShaderGraph][2019.4][Backport] Ensure varying ordering by making instance ID SystemGenerated #5846

Closed

thomas-zeng removed the needs-backport-7.x label Nov 18, 2021

thomas-zeng removed the needs-backport-10.x label Nov 18, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fixed some Shader Graph shaders fail to compile for Android XR #4806

Fixed some Shader Graph shaders fail to compile for Android XR #4806

Uh oh!

thomas-zeng commented Jun 4, 2021 •

edited

Loading

Uh oh!

fabien-unity Jun 4, 2021

Uh oh!

thomas-zeng Jun 4, 2021 •

edited

Loading

Uh oh!

esmelusina Jun 8, 2021

Uh oh!

thomas-zeng Jun 9, 2021

Uh oh!

esmelusina Jun 14, 2021

Uh oh!

thomas-zeng Jun 14, 2021

Uh oh!

esmelusina left a comment

Uh oh!

DennisDeRykeUnity commented Jun 9, 2021 •

edited

Loading

Uh oh!

esmelusina left a comment

Uh oh!

DennisDeRykeUnity left a comment

Uh oh!

esmelusina left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Fixed some Shader Graph shaders fail to compile for Android XR #4806

Fixed some Shader Graph shaders fail to compile for Android XR #4806

Uh oh!

Conversation

thomas-zeng commented Jun 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Purpose of this PR

Testing status

Comments to reviewers

Uh oh!

fabien-unity Jun 4, 2021

Choose a reason for hiding this comment

Uh oh!

thomas-zeng Jun 4, 2021 • 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!

esmelusina Jun 8, 2021

Choose a reason for hiding this comment

Uh oh!

thomas-zeng Jun 9, 2021

Choose a reason for hiding this comment

Uh oh!

esmelusina Jun 14, 2021

Choose a reason for hiding this comment

Uh oh!

thomas-zeng Jun 14, 2021

Choose a reason for hiding this comment

Uh oh!

esmelusina left a comment

Choose a reason for hiding this comment

Uh oh!

DennisDeRykeUnity commented Jun 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

esmelusina left a comment

Choose a reason for hiding this comment

Uh oh!

DennisDeRykeUnity left a comment

Choose a reason for hiding this comment

Uh oh!

esmelusina left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

thomas-zeng commented Jun 4, 2021 •

edited

Loading

thomas-zeng Jun 4, 2021 •

edited

Loading

DennisDeRykeUnity commented Jun 9, 2021 •

edited

Loading