[2021.2] [Bugfix 1296776] Enforce SRP Batcher and Hybrid renderer compatibility by always using a consolidated property list (#3625)

Chris Tchou · web-flow · commit ea27a6a979ec · 2021-02-24T16:46:06.000-08:00
* Cherry pick of 10.x.x/sg/fix/1296776

# Conflicts:
#	com.unity.shadergraph/CHANGELOG.md
#	com.unity.shadergraph/Editor/Generation/Processors/Generator.cs
#	com.unity.shadergraph/Editor/Generation/Processors/PropertyCollector.cs
#	com.unity.shadergraph/Editor/Importers/ShaderSubGraphImporter.cs

# Conflicts:
#	com.unity.shadergraph/CHANGELOG.md
#	com.unity.shadergraph/Editor/Generation/Processors/Generator.cs
#	com.unity.shadergraph/Editor/Importers/ShaderGraphImporter.cs
#	com.unity.shadergraph/Editor/Importers/ShaderSubGraphImporter.cs

* Adding comment to address feedback
diff --git a/com.unity.shadergraph/CHANGELOG.md b/com.unity.shadergraph/CHANGELOG.md
@@ -26,6 +26,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 - Fixed treatment of node precision in subgraphs, now allows subgraphs to switch precisions based on the subgraph node [1304050] (https://issuetracker.unity3d.com/issues/precision-errors-when-theres-a-precision-discrepancy-between-subgraphs-and-parent-graphs)
 - Fixed an issue where the Rectangle Node could lose detail at a distance.  New control offers additional method that preserves detail better [1156801]
 - Fixed virtual texture layer reference names allowing invalid characters [1304146]
+- Fixed issue with SRP Batcher compatibility [1310624]
+- Fixed issue with Hybrid renderer compatibility [1296776]
 
 
 ## [11.0.0] - 2020-10-21
diff --git a/com.unity.shadergraph/Editor/Data/Implementation/NodeUtils.cs b/com.unity.shadergraph/Editor/Data/Implementation/NodeUtils.cs
@@ -102,7 +102,7 @@ public static SlotReference DepthFirstCollectRedirectNodeFromNode(RedirectNodeDa
         }
 
         public static void DepthFirstCollectNodesFromNode(List<AbstractMaterialNode> nodeList, AbstractMaterialNode node,
-            IncludeSelf includeSelf = IncludeSelf.Include, List<KeyValuePair<ShaderKeyword, int>> keywordPermutation = null)
+            IncludeSelf includeSelf = IncludeSelf.Include, List<KeyValuePair<ShaderKeyword, int>> keywordPermutation = null, bool ignoreActiveState = false)
         {
             // no where to start
             if (node == null)
@@ -132,11 +132,11 @@ public static void DepthFirstCollectNodesFromNode(List<AbstractMaterialNode> nod
                 {
                     var outputNode = edge.outputSlot.node;
                     if (outputNode != null)
-                        DepthFirstCollectNodesFromNode(nodeList, outputNode, keywordPermutation: keywordPermutation);
+                        DepthFirstCollectNodesFromNode(nodeList, outputNode, keywordPermutation: keywordPermutation, ignoreActiveState: ignoreActiveState);
                 }
             }
 
-            if (includeSelf == IncludeSelf.Include && node.isActive)
+            if (includeSelf == IncludeSelf.Include && (node.isActive || ignoreActiveState))
                 nodeList.Add(node);
         }
 
diff --git a/com.unity.shadergraph/Editor/Drawing/PreviewManager.cs b/com.unity.shadergraph/Editor/Drawing/PreviewManager.cs
@@ -217,7 +217,7 @@ void OnNodeModified(AbstractMaterialNode node, ModificationScope scope)
         static HashSet<AbstractMaterialNode> m_TempAddedToNodeWave = new HashSet<AbstractMaterialNode>();
 
         // cache the Action to avoid GC
-        Action<AbstractMaterialNode> AddNextLevelNodesToWave =
+        static Action<AbstractMaterialNode> AddNextLevelNodesToWave =
             nextLevelNode =>
         {
             if (!m_TempAddedToNodeWave.Contains(nextLevelNode))
@@ -227,7 +227,7 @@ void OnNodeModified(AbstractMaterialNode node, ModificationScope scope)
             }
         };
 
-        enum PropagationDirection
+        internal enum PropagationDirection
         {
             Upstream,
             Downstream
@@ -236,7 +236,7 @@ enum PropagationDirection
         // ADDs all nodes in sources, and all nodes in the given direction relative to them, into result
         // sources and result can be the same HashSet
         private static readonly ProfilerMarker PropagateNodesMarker = new ProfilerMarker("PropagateNodes");
-        void PropagateNodes(HashSet<AbstractMaterialNode> sources, PropagationDirection dir, HashSet<AbstractMaterialNode> result)
+        internal static void PropagateNodes(HashSet<AbstractMaterialNode> sources, PropagationDirection dir, HashSet<AbstractMaterialNode> result)
         {
             using (PropagateNodesMarker.Auto())
                 if (sources.Count > 0)
diff --git a/com.unity.shadergraph/Editor/Generation/Processors/Generator.cs b/com.unity.shadergraph/Editor/Generation/Processors/Generator.cs
@@ -88,6 +88,7 @@ public ActiveFields GatherActiveFieldsFromNode(AbstractMaterialNode outputNode,
         void BuildShader()
         {
             var activeNodeList = Pool.ListPool<AbstractMaterialNode>.Get();
+            bool ignoreActiveState = (m_Mode == GenerationMode.Preview);  // for previews, we ignore node active state
             if (m_OutputNode == null)
             {
                 foreach (var block in m_Blocks)
@@ -97,12 +98,12 @@ void BuildShader()
                     if (!block.isActive)
                         continue;
 
-                    NodeUtils.DepthFirstCollectNodesFromNode(activeNodeList, block, NodeUtils.IncludeSelf.Include);
+                    NodeUtils.DepthFirstCollectNodesFromNode(activeNodeList, block, NodeUtils.IncludeSelf.Include, ignoreActiveState: ignoreActiveState);
                 }
             }
             else
             {
-                NodeUtils.DepthFirstCollectNodesFromNode(activeNodeList, m_OutputNode);
+                NodeUtils.DepthFirstCollectNodesFromNode(activeNodeList, m_OutputNode, ignoreActiveState: ignoreActiveState);
             }
 
             var shaderProperties = new PropertyCollector();
@@ -132,6 +133,10 @@ void BuildShader()
                 target.CollectShaderProperties(shaderProperties, m_Mode);
             }
 
+            // set the property collector to read only
+            // (to ensure no rogue target or pass starts adding more properties later..)
+            shaderProperties.SetReadOnly();
+
             m_Builder.AppendLine(@"Shader ""{0}""", m_Name);
             using (m_Builder.BlockScope())
             {
@@ -144,9 +149,11 @@ void BuildShader()
                     // Instead of setup target, we can also just do get context
                     m_Targets[i].Setup(ref context);
 
+                    var subShaderProperties = GetSubShaderPropertiesForTarget(m_Targets[i], m_GraphData, m_Mode, m_OutputNode);
+
                     foreach (var subShader in context.subShaders)
                     {
-                        GenerateSubShader(i, subShader);
+                        GenerateSubShader(i, subShader, subShaderProperties);
                     }
 
                     var customEditor = context.defaultShaderGUI;
@@ -167,7 +174,7 @@ void BuildShader()
             m_ConfiguredTextures = shaderProperties.GetConfiguredTexutres();
         }
 
-        void GenerateSubShader(int targetIndex, SubShaderDescriptor descriptor)
+        void GenerateSubShader(int targetIndex, SubShaderDescriptor descriptor, PropertyCollector subShaderProperties)
         {
             if (descriptor.passes == null)
                 return;
@@ -195,12 +202,70 @@ void GenerateSubShader(int targetIndex, SubShaderDescriptor descriptor)
 
                     // Check masternode fields for valid passes
                     if (pass.TestActive(activeFields))
-                        GenerateShaderPass(targetIndex, pass.descriptor, activeFields, currentBlockDescriptors.Select(x => x.descriptor).ToList());
+                        GenerateShaderPass(targetIndex, pass.descriptor, activeFields, currentBlockDescriptors.Select(x => x.descriptor).ToList(), subShaderProperties);
+                }
+            }
+        }
+
+        // this builds the list of properties for a Target / Graph combination
+        static PropertyCollector GetSubShaderPropertiesForTarget(Target target, GraphData graph, GenerationMode generationMode, AbstractMaterialNode outputNode)
+        {
+            PropertyCollector subshaderProperties = new PropertyCollector();
+
+            // Collect shader properties declared by active nodes
+            using (var activeNodes = PooledHashSet<AbstractMaterialNode>.Get())
+            {
+                if (outputNode == null)
+                {
+                    // shader graph builds active nodes starting from the set of active blocks
+                    var currentBlocks = graph.GetNodes<BlockNode>();
+                    var activeBlockContext = new TargetActiveBlockContext(currentBlocks.Select(x => x.descriptor).ToList(), null);
+                    target.GetActiveBlocks(ref activeBlockContext);
+
+                    foreach (var blockFieldDesc in activeBlockContext.activeBlocks)
+                    {
+                        // attempt to get BlockNode(s) from the stack
+                        var vertBlockNode = graph.vertexContext.blocks.FirstOrDefault(x => x.value.descriptor == blockFieldDesc).value;
+                        if (vertBlockNode != null)
+                            activeNodes.Add(vertBlockNode);
+
+                        var fragBlockNode = graph.fragmentContext.blocks.FirstOrDefault(x => x.value.descriptor == blockFieldDesc).value;
+                        if (fragBlockNode != null)
+                            activeNodes.Add(fragBlockNode);
+                    }
+                }
+                else
+                {
+                    // preview and/or subgraphs build their active node set based on the single output node
+                    activeNodes.Add(outputNode);
                 }
+
+                PreviewManager.PropagateNodes(activeNodes, PreviewManager.PropagationDirection.Upstream, activeNodes);
+
+                // NOTE: this is NOT a deterministic ordering
+                foreach (var node in activeNodes)
+                    node.CollectShaderProperties(subshaderProperties, generationMode);
+
+                // So we sort the properties after
+                subshaderProperties.Sort();
+            }
+
+            // Collect graph properties
+            {
+                graph.CollectShaderProperties(subshaderProperties, generationMode);
+            }
+
+            // Collect shader properties declared by the Target
+            {
+                target.CollectShaderProperties(subshaderProperties, generationMode);
             }
+
+            subshaderProperties.SetReadOnly();
+
+            return subshaderProperties;
         }
 
-        void GenerateShaderPass(int targetIndex, PassDescriptor pass, ActiveFields activeFields, List<BlockFieldDescriptor> currentBlockDescriptors)
+        void GenerateShaderPass(int targetIndex, PassDescriptor pass, ActiveFields activeFields, List<BlockFieldDescriptor> currentBlockDescriptors, PropertyCollector subShaderProperties)
         {
             // Early exit if pass is not used in preview
             if (m_Mode == GenerationMode.Preview && !pass.useInPreview)
@@ -227,6 +292,7 @@ void GenerateShaderPass(int targetIndex, PassDescriptor pass, ActiveFields activ
             CustomInterpSubGen customInterpSubGen = new CustomInterpSubGen(m_OutputNode != null);
 
             // Initiailize Collectors
+            // NOTE: propertyCollector is not really used anymore -- we use the subshader PropertyCollector instead
             var propertyCollector = new PropertyCollector();
             var keywordCollector = new KeywordCollector();
             m_GraphData.CollectShaderKeywords(keywordCollector, m_Mode);
@@ -663,7 +729,7 @@ void ProcessStackForPass(ContextData contextData, BlockFieldDescriptor[] passBlo
 
             using (var propertyBuilder = new ShaderStringBuilder())
             {
-                propertyCollector.GetPropertiesDeclaration(propertyBuilder, m_Mode, m_GraphData.graphDefaultConcretePrecision);
+                subShaderProperties.GetPropertiesDeclaration(propertyBuilder, m_Mode, m_GraphData.graphDefaultConcretePrecision);
                 if (propertyBuilder.length == 0)
                     propertyBuilder.AppendLine("// GraphProperties: <None>");
                 spliceCommands.Add("GraphProperties", propertyBuilder.ToCodeBlock());
@@ -672,17 +738,12 @@ void ProcessStackForPass(ContextData contextData, BlockFieldDescriptor[] passBlo
             // --------------------------------------------------
             // Dots Instanced Graph Properties
 
-            bool hasDotsProperties = false;
-            m_GraphData.ForeachHLSLProperty(h =>
-            {
-                if (h.declaration == HLSLDeclaration.HybridPerInstance)
-                    hasDotsProperties = true;
-            });
+            bool hasDotsProperties = subShaderProperties.HasDotsProperties();
 
             using (var dotsInstancedPropertyBuilder = new ShaderStringBuilder())
             {
                 if (hasDotsProperties)
-                    dotsInstancedPropertyBuilder.AppendLines(propertyCollector.GetDotsInstancingPropertiesDeclaration(m_Mode));
+                    dotsInstancedPropertyBuilder.AppendLines(subShaderProperties.GetDotsInstancingPropertiesDeclaration(m_Mode));
                 else
                     dotsInstancedPropertyBuilder.AppendLine("// HybridV1InjectedBuiltinProperties: <None>");
                 spliceCommands.Add("HybridV1InjectedBuiltinProperties", dotsInstancedPropertyBuilder.ToCodeBlock());
diff --git a/com.unity.shadergraph/Editor/Generation/Processors/PropertyCollector.cs b/com.unity.shadergraph/Editor/Generation/Processors/PropertyCollector.cs
@@ -1,7 +1,9 @@
+using System;
 using System.Collections.Generic;
 using System.Linq;
 using System.Text;
 using UnityEditor.ShaderGraph.Internal;
+using UnityEngine;
 
 namespace UnityEditor.ShaderGraph
 {
@@ -14,13 +16,117 @@ public struct TextureInfo
             public bool modifiable;
         }
 
-        public readonly List<AbstractShaderProperty> properties = new List<AbstractShaderProperty>();
+        bool m_ReadOnly;
+        List<HLSLProperty> m_HLSLProperties = null;
 
-        public void AddShaderProperty(AbstractShaderProperty chunk)
+        // reference name ==> property index in list
+        Dictionary<string, int> m_ReferenceNames = new Dictionary<string, int>();
+
+        // list of properties (kept in a list to maintain deterministic declaration order)
+        List<AbstractShaderProperty> m_Properties = new List<AbstractShaderProperty>();
+
+        public int propertyCount => m_Properties.Count;
+        public IEnumerable<AbstractShaderProperty> properties => m_Properties;
+        public AbstractShaderProperty GetProperty(int index) { return m_Properties[index]; }
+
+        public void Sort()
         {
-            if (properties.Any(x => x.referenceName == chunk.referenceName))
+            if (m_ReadOnly)
+            {
+                Debug.LogError("Cannot sort the properties when the PropertyCollector is already marked ReadOnly");
                 return;
-            properties.Add(chunk);
+            }
+
+            m_Properties.Sort((a, b) => String.CompareOrdinal(a.referenceName, b.referenceName));
+        }
+
+        public void SetReadOnly()
+        {
+            m_ReadOnly = true;
+        }
+
+        private static bool EquivalentHLSLProperties(AbstractShaderProperty a, AbstractShaderProperty b)
+        {
+            bool equivalent = true;
+            var bHLSLProps = new List<HLSLProperty>();
+            b.ForeachHLSLProperty(bh => bHLSLProps.Add(bh));
+            a.ForeachHLSLProperty(ah =>
+            {
+                var i = bHLSLProps.FindIndex(bh => bh.name == ah.name);
+                if (i < 0)
+                    equivalent = false;
+                else
+                {
+                    var bh = bHLSLProps[i];
+                    if ((ah.name != bh.name) ||
+                        (ah.type != bh.type) ||
+                        (ah.precision != bh.precision) ||
+                        (ah.declaration != bh.declaration) ||
+                        ((ah.customDeclaration == null) != (bh.customDeclaration == null)))
+                    {
+                        equivalent = false;
+                    }
+                    else if (ah.customDeclaration != null)
+                    {
+                        var ssba = new ShaderStringBuilder();
+                        var ssbb = new ShaderStringBuilder();
+                        ah.customDeclaration(ssba);
+                        bh.customDeclaration(ssbb);
+                        if (ssba.ToCodeBlock() != ssbb.ToCodeBlock())
+                            equivalent = false;
+                    }
+                    bHLSLProps.RemoveAt(i);
+                }
+            });
+            return equivalent && (bHLSLProps.Count == 0);
+        }
+
+        public void AddShaderProperty(AbstractShaderProperty prop)
+        {
+            if (m_ReadOnly)
+            {
+                Debug.LogError("ERROR attempting to add property to readonly collection");
+                return;
+            }
+
+            int propIndex = -1;
+            if (m_ReferenceNames.TryGetValue(prop.referenceName, out propIndex))
+            {
+                // existing referenceName
+                var existingProp = m_Properties[propIndex];
+                if (existingProp != prop)
+                {
+                    // duplicate reference name, but different property instances
+                    if (existingProp.GetType() != prop.GetType())
+                    {
+                        Debug.LogError("Two properties with the same reference name (" + prop.referenceName + ") using different types");
+                    }
+                    else
+                    {
+                        if (!EquivalentHLSLProperties(existingProp, prop))
+                            Debug.LogError("Two properties with the same reference name (" + prop.referenceName + ") produce different HLSL properties");
+                    }
+                }
+            }
+            else
+            {
+                // new referenceName, new property
+                propIndex = m_Properties.Count;
+                m_Properties.Add(prop);
+                m_ReferenceNames.Add(prop.referenceName, propIndex);
+            }
+        }
+
+        private List<HLSLProperty> BuildHLSLPropertyList()
+        {
+            SetReadOnly();
+            if (m_HLSLProperties == null)
+            {
+                m_HLSLProperties = new List<HLSLProperty>();
+                foreach (var p in m_Properties)
+                    p.ForeachHLSLProperty(h => m_HLSLProperties.Add(h));
+            }
+            return m_HLSLProperties;
         }
 
         public void GetPropertiesDeclaration(ShaderStringBuilder builder, GenerationMode mode, ConcretePrecision defaultPrecision)
@@ -32,8 +138,7 @@ public void GetPropertiesDeclaration(ShaderStringBuilder builder, GenerationMode
             }
 
             // build a list of all HLSL properties
-            var hlslProps = new List<HLSLProperty>();
-            properties.ForEach(p => p.ForeachHLSLProperty(h => hlslProps.Add(h)));
+            var hlslProps = BuildHLSLPropertyList();
 
             if (mode == GenerationMode.Preview)
             {
@@ -151,6 +256,18 @@ public void GetPropertiesDeclaration(ShaderStringBuilder builder, GenerationMode
                     h.AppendTo(builder);
         }
 
+        public bool HasDotsProperties()
+        {
+            var hlslProps = BuildHLSLPropertyList();
+            bool hasDotsProperties = false;
+            foreach (var h in hlslProps)
+            {
+                if (h.declaration == HLSLDeclaration.HybridPerInstance)
+                    hasDotsProperties = true;
+            }
+            return hasDotsProperties;
+        }
+
         public string GetDotsInstancingPropertiesDeclaration(GenerationMode mode)
         {
             // Hybrid V1 needs to declare a special macro to that is injected into
@@ -161,12 +278,7 @@ public string GetDotsInstancingPropertiesDeclaration(GenerationMode mode)
             var batchAll = (mode == GenerationMode.Preview);
 
             // build a list of all HLSL properties
-            var hybridHLSLProps = new List<HLSLProperty>();
-            properties.ForEach(p => p.ForeachHLSLProperty(h =>
-            {
-                if (h.declaration == HLSLDeclaration.HybridPerInstance)
-                    hybridHLSLProps.Add(h);
-            }));
+            var hybridHLSLProps = BuildHLSLPropertyList().Where(h => h.declaration == HLSLDeclaration.HybridPerInstance);
 
             if (hybridHLSLProps.Any())
             {
diff --git a/com.unity.shadergraph/Editor/Importers/ShaderGraphImporter.cs b/com.unity.shadergraph/Editor/Importers/ShaderGraphImporter.cs
diff --git a/com.unity.shadergraph/Editor/Importers/ShaderSubGraphImporter.cs b/com.unity.shadergraph/Editor/Importers/ShaderSubGraphImporter.cs

Original file line number	Diff line number	Diff line change
`@@ -102,7 +102,7 @@ public static SlotReference DepthFirstCollectRedirectNodeFromNode(RedirectNodeDa`
`102`	`102`	`}`
`103`	`103`
`104`	`104`	`public static void DepthFirstCollectNodesFromNode(List<AbstractMaterialNode> nodeList, AbstractMaterialNode node,`
`105`		`- IncludeSelf includeSelf = IncludeSelf.Include, List<KeyValuePair<ShaderKeyword, int>> keywordPermutation = null)`
	`105`	`+ IncludeSelf includeSelf = IncludeSelf.Include, List<KeyValuePair<ShaderKeyword, int>> keywordPermutation = null, bool ignoreActiveState = false)`
`106`	`106`	`{`
`107`	`107`	`// no where to start`
`108`	`108`	`if (node == null)`
`@@ -132,11 +132,11 @@ public static void DepthFirstCollectNodesFromNode(List<AbstractMaterialNode> nod`
`132`	`132`	`{`
`133`	`133`	`var outputNode = edge.outputSlot.node;`
`134`	`134`	`if (outputNode != null)`
`135`		`- DepthFirstCollectNodesFromNode(nodeList, outputNode, keywordPermutation: keywordPermutation);`
	`135`	`+ DepthFirstCollectNodesFromNode(nodeList, outputNode, keywordPermutation: keywordPermutation, ignoreActiveState: ignoreActiveState);`
`136`	`136`	`}`
`137`	`137`	`}`
`138`	`138`
`139`		`- if (includeSelf == IncludeSelf.Include && node.isActive)`
	`139`	`+ if (includeSelf == IncludeSelf.Include && (node.isActive \|\| ignoreActiveState))`
`140`	`140`	`nodeList.Add(node);`
`141`	`141`	`}`
`142`	`142`
Original file line number	Diff line number	Diff line change
`@@ -217,7 +217,7 @@ void OnNodeModified(AbstractMaterialNode node, ModificationScope scope)`
`217`	`217`	`static HashSet<AbstractMaterialNode> m_TempAddedToNodeWave = new HashSet<AbstractMaterialNode>();`
`218`	`218`
`219`	`219`	`// cache the Action to avoid GC`
`220`		`- Action<AbstractMaterialNode> AddNextLevelNodesToWave =`
	`220`	`+ static Action<AbstractMaterialNode> AddNextLevelNodesToWave =`
`221`	`221`	`nextLevelNode =>`
`222`	`222`	`{`
`223`	`223`	`if (!m_TempAddedToNodeWave.Contains(nextLevelNode))`
`@@ -227,7 +227,7 @@ void OnNodeModified(AbstractMaterialNode node, ModificationScope scope)`
`227`	`227`	`}`
`228`	`228`	`};`
`229`	`229`
`230`		`- enum PropagationDirection`
	`230`	`+ internal enum PropagationDirection`
`231`	`231`	`{`
`232`	`232`	`Upstream,`
`233`	`233`	`Downstream`
`@@ -236,7 +236,7 @@ enum PropagationDirection`
`236`	`236`	`// ADDs all nodes in sources, and all nodes in the given direction relative to them, into result`
`237`	`237`	`// sources and result can be the same HashSet`
`238`	`238`	`private static readonly ProfilerMarker PropagateNodesMarker = new ProfilerMarker("PropagateNodes");`
`239`		`- void PropagateNodes(HashSet<AbstractMaterialNode> sources, PropagationDirection dir, HashSet<AbstractMaterialNode> result)`
	`239`	`+ internal static void PropagateNodes(HashSet<AbstractMaterialNode> sources, PropagationDirection dir, HashSet<AbstractMaterialNode> result)`
`240`	`240`	`{`
`241`	`241`	`using (PropagateNodesMarker.Auto())`
`242`	`242`	`if (sources.Count > 0)`