[2021.2] [ShaderGraph] [bugfix 1301915] Node Include files are now tracked explicitly BY GUID #3290

cdxntchou · 2021-01-29T03:43:38Z

Purpose of this PR

Fix for https://fogbugz.unity3d.com/f/cases/1301915/

Include files are now tracked by GUID (so they can find a file even after it moves), the includes are propagated out of subgraphs, and merged uniquely by the generator.

Some care was taken to keep the HDRP "replace placeholder" functionality working properly.

Testing status

Added new Editor test for moving directories of ShaderGraph/SubGraphs containing CFN nodes using include files, similar to the repro case.

verified that the test failed before this PR
verified that it now works, after this PR

ShaderGraph PR job: 🟢
https://yamato.cds.internal.unity3d.com/jobs/902-Graphics/tree/sg%252Ffix%252F1301915/.yamato%252Fall-shadergraph.yml%2523PR_ShaderGraph_trunk/5141106/job/pipeline

ShaderGraph Stereo PR job: 🟢
https://yamato.cds.internal.unity3d.com/jobs/902-Graphics/tree/sg%252Ffix%252F1301915/.yamato%252Fall-shadergraph_stereo.yml%2523PR_ShaderGraph_Stereo_trunk/5141105/job/pipeline

HDRP PR job: failures match master failures
https://yamato.cds.internal.unity3d.com/jobs/902-Graphics/tree/sg%252Ffix%252F1301915/.yamato%252Fall-hdrp.yml%2523PR_HDRP_trunk/5141136/job/pipeline

Master equivalent job:
https://yamato.cds.internal.unity3d.com/jobs/902-Graphics/tree/master/.yamato%252Fall-hdrp.yml%2523PR_HDRP_trunk/5068229/job/pipeline

Failures:
HDRP on Linux_Vulkan_playmode_cache_mono_Linear on version trunk -- same 4 tests failed same way
HDRP on OSX_Metal_playmode_cache_mono_Linear on version trunk -- master failed 8 tests, this branch failed 6, all matching master failures
master additionally failed: HDRP_GraphicTestRunner.2106_GI_EmissionSG, HDRP_GraphicTestRunner.2108_GI_IESCookie_B
seems unrelated to my changes, I assume non-determinism
HDRP on Win_Vulkan_playmode_cache_mono_Linear on version trunk -- same 4 tests failed same way
HDRP on Win_DX12_playmode_cache_mono_Linear on version trunk -- same 5 tests failed same way
slight difference on error amount for one of the same tests that failed in master OSX_Metal above -- assume non-determinism
HDRP on Win_DX11_playmode_XR_cache_mono_Linear on version trunk -- both had script compile failures
HDRP on Win_DX11_playmode_cache_mono_Linear on version trunk -- same 4 tests failed same way

HDRP_DXR PR Job: 🟢
https://yamato.cds.internal.unity3d.com/jobs/902-Graphics/tree/sg%252Ffix%252F1301915/.yamato%252Fall-hdrp_dxr.yml%2523PR_HDRP_DXR_trunk/5141127/job/pipeline

HDRP_Hybrid PR Job: 🟢
https://yamato.cds.internal.unity3d.com/jobs/902-Graphics/tree/sg%252Ffix%252F1301915/.yamato%252Fall-hdrp_hybrid.yml%2523PR_HDRP_Hybrid_trunk/5141129/job/pipeline

HDRP VFX PR job: 🟢
https://yamato.cds.internal.unity3d.com/jobs/902-Graphics/tree/sg%252Ffix%252F1301915/.yamato%252Fall-vfx_hdrp.yml%2523PR_VFX_HDRP_trunk/5141135/job/pipeline

Universal PR job: failures match master failures
https://yamato.cds.internal.unity3d.com/jobs/902-Graphics/tree/sg%252Ffix%252F1301915/.yamato%252Fall-universal.yml%2523PR_Universal_trunk/5174976/job/pipeline

Master equivalent job:
https://yamato.cds.internal.unity3d.com/jobs/902-Graphics/tree/master/.yamato%252Fall-universal.yml%2523PR_Universal_trunk/5068179/job/pipeline

failures:
Universal on iPhone_Metal_Standalone_cache_il2cpp_Linear on version trunk -- same 1 test failed same way
Universal on Android_Vulkan_Standalone_cache_il2cpp_Linear on version trunk -- did not complete, re-running
Universal on Android_OpenGLES3_Standalone_cache_il2cpp_Linear on version trunk -- same 3 tests failed same way
Universal on Win_DX11_playmode_XR_cache_mono_Linear on version trunk -- both had script compile errors

Comments to reviewers

Notes for the reviewers you have assigned.

… of subgraphs, and uniquely merged Some care was taken to keep the HDRP "replace placeholder" functionality working properly..

cdxntchou · 2021-02-01T18:52:39Z

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

-
-                    if (!String.IsNullOrEmpty(include.descriptor.value))
-                        finalIncludes.Add(include.descriptor.value, include.descriptor.location, include.fieldConditions);
+                    var path = include.path;


This seems like an error in that it used to be modifying the values stored in the passDescriptor (which may be static global).. with this PR it no longer modifies the existing structure...

cdxntchou · 2021-02-01T18:54:26Z

com.unity.shadergraph/Editor/Data/Nodes/Utility/CustomFunctionNode.cs

        }

-        public bool Reload(HashSet<string> changedFileDependencies)
+        public bool Reload(HashSet<string> changedFileDependencyGUIDs)


this is just changing the name of the variable to make it clear these are GUIDs not paths

cdxntchou · 2021-02-01T18:55:39Z

com.unity.shadergraph/Editor/Data/Nodes/Utility/SubGraphNode.cs

            if (asset == null || hasError)
                return;

+            registry.RequiresIncludes(asset.includes);


This forwards the includes from the subgraph asset to any graph using them..

cdxntchou · 2021-02-01T18:57:21Z

com.unity.shadergraph/Editor/Data/Util/FunctionRegistry.cs

        Dictionary<string, FunctionSource> m_Sources = new Dictionary<string, FunctionSource>();
        bool m_Validate = false;
        ShaderStringBuilder m_Builder;
+        IncludeCollection m_Includes;


Easiest was to make the FunctionRegistry effectively a "FunctionAndNodeInclude" registry...

cdxntchou · 2021-02-01T18:57:48Z

com.unity.shadergraph/Editor/Generation/Collections/DefineCollection.cs

    [GenerationAPI]
    internal class DefineCollection : IEnumerable<DefineCollection.Item>
    {
-        public class Item : IConditional, IShaderString


IShaderString interface is not used anywhere... I removed it.

cdxntchou · 2021-02-01T18:58:26Z

com.unity.shadergraph/Editor/Generation/Collections/IncludeCollection.cs

 {
    [GenerationAPI]
-    internal class IncludeCollection : IEnumerable<IncludeCollection.Item>
+    [Serializable]


The changes here are to make the IncludeCollection serializable, so we can serialize it as part of the subgraph asset. We also switch it to being GUID-based instead of path-based. We still record the path, but the GUID is treated as the authoritative link to the file.

Note that I removed "Item" and just combined it with the IncludeDescriptor. No one else was using either of them, so there didn't seem to be any reason to have separate classes for the two.

cdxntchou · 2021-02-01T19:02:18Z

com.unity.shadergraph/Editor/Generation/Enumerations/IncludeLocation.cs

 namespace UnityEditor.ShaderGraph
 {
    [GenerationAPI]
+    [Serializable]


Not strictly necessary, enums are serializable by default, but serves as a good reminder that this is a serialized enumeration, if anyone is looking to change it in the future...

cdxntchou · 2021-02-01T19:03:06Z

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

                string command = GenerationUtils.GetSpliceCommand(passPragmaBuilder.ToCodeBlock(), "PassPragmas");
                spliceCommands.Add("PassPragmas", command);
            }



These move further down, so we can combine the Target/pass includes with the node includes before generating them.

ghost

Did just a bit of extra testing (setting up a use case scenario and making sure the bug is fixed). Other than that, developer testing is sufficient.

Node Include files are now tracked explicitly BY GUID, propagated out…

bbdeb9d

… of subgraphs, and uniquely merged Some care was taken to keep the HDRP "replace placeholder" functionality working properly..

github-actions bot added HDRP Shader Graph labels Jan 29, 2021

cdxntchou marked this pull request as ready for review January 30, 2021 00:07

cdxntchou requested a review from a team as a code owner January 30, 2021 00:07

Chris Tchou added 2 commits February 1, 2021 10:10

Adding file move tests

48f0df4

Update Changelog

e90c6fc

cdxntchou added the needs-backport-2021.1 label Feb 1, 2021

cdxntchou requested a review from a user February 1, 2021 18:48

cdxntchou commented Feb 1, 2021

View reviewed changes

cdxntchou requested a review from elizabeth-legros February 1, 2021 19:05

Fix for VFX when using node-includes

438fbb9

elizabeth-legros approved these changes Feb 1, 2021

View reviewed changes

cdxntchou changed the title ~~[master] [ShaderGraph] [bugfix 1301915] Node Include files are now tracked explicitly BY GUID~~ [2021.2] [ShaderGraph] [bugfix 1301915] Node Include files are now tracked explicitly BY GUID Feb 2, 2021

ghost approved these changes Feb 4, 2021

View reviewed changes

Merge branch 'master' into sg/fix/1301915

3f36e4a

cdxntchou merged commit 88058ae into master Feb 4, 2021

cdxntchou deleted the sg/fix/1301915 branch February 4, 2021 20:36

cdxntchou removed the needs-backport-2021.1 label Mar 2, 2021

cdxntchou mentioned this pull request Mar 18, 2021

[ShaderGraph] [2021.2] Fix include ordering #3924

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[2021.2] [ShaderGraph] [bugfix 1301915] Node Include files are now tracked explicitly BY GUID #3290

[2021.2] [ShaderGraph] [bugfix 1301915] Node Include files are now tracked explicitly BY GUID #3290

Uh oh!

cdxntchou commented Jan 29, 2021 •

edited

Loading

Uh oh!

cdxntchou Feb 1, 2021 •

edited

Loading

Uh oh!

cdxntchou Feb 1, 2021

Uh oh!

cdxntchou Feb 1, 2021

Uh oh!

cdxntchou Feb 1, 2021

Uh oh!

cdxntchou Feb 1, 2021

Uh oh!

cdxntchou Feb 1, 2021 •

edited

Loading

Uh oh!

cdxntchou Feb 1, 2021 •

edited

Loading

Uh oh!

cdxntchou Feb 1, 2021

Uh oh!

cdxntchou Feb 1, 2021

Uh oh!

ghost left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

[2021.2] [ShaderGraph] [bugfix 1301915] Node Include files are now tracked explicitly BY GUID #3290

[2021.2] [ShaderGraph] [bugfix 1301915] Node Include files are now tracked explicitly BY GUID #3290

Uh oh!

Conversation

cdxntchou commented Jan 29, 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!

cdxntchou Feb 1, 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!

cdxntchou Feb 1, 2021

Choose a reason for hiding this comment

Uh oh!

cdxntchou Feb 1, 2021

Choose a reason for hiding this comment

Uh oh!

cdxntchou Feb 1, 2021

Choose a reason for hiding this comment

Uh oh!

cdxntchou Feb 1, 2021

Choose a reason for hiding this comment

Uh oh!

cdxntchou Feb 1, 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!

cdxntchou Feb 1, 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!

cdxntchou Feb 1, 2021

Choose a reason for hiding this comment

Uh oh!

cdxntchou Feb 1, 2021

Choose a reason for hiding this comment

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

cdxntchou commented Jan 29, 2021 •

edited

Loading

cdxntchou Feb 1, 2021 •

edited

Loading

cdxntchou Feb 1, 2021 •

edited

Loading

cdxntchou Feb 1, 2021 •

edited

Loading