[2020.2] [10.x.x] [Bugfix 1296776] Enforce SRP Batcher and Hybrid renderer compatibility by always using a consolidated property list (#3577)

* Make all passes in a subshader use the same HLSL proeprty declarations

* Adding changelog

* Better fix for Targets accessing the current blocks

Author: Chris Tchou

com.unity.shadergraph/CHANGELOG.md

@@ -4,6 +4,12 @@ All notable changes to this package are documented in this file.
 The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/)
 and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.html).
 
+## [10.5.0] - 2021-02-20
+
+### Fixed
+- Fixed an issue where the exposed checkbox was removed from keyword inspectors [1312779] (https://issuetracker.unity3d.com/issues/shader-graph-exposed-parameter-for-keywords-removed-by-accident)
+- Fixed issue with SRP Batcher compatibility [1310624]
+- Fixed issue with Hybrid renderer compatibility [1296776]
 
 ## [10.4.0] - 2021-01-27
 

com.unity.shadergraph/Editor/Data/Implementation/NodeUtils.cs

@@ -101,7 +101,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)
@@ -131,11 +131,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);
         }
 

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)

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

@@ -87,7 +87,8 @@ public ActiveFields GatherActiveFieldsFromNode(AbstractMaterialNode outputNode,
         void BuildShader()
         {
             var activeNodeList = Graphing.ListPool<AbstractMaterialNode>.Get();
-            if(m_OutputNode == null)
+            bool ignoreActiveState = (m_Mode == GenerationMode.Preview);  // for previews, we ignore node active state
+            if (m_OutputNode == null)
             {
                 foreach(var block in m_Blocks)
                 {
@@ -96,12 +97,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();
@@ -131,6 +132,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())
             {
@@ -143,9 +148,11 @@ void BuildShader()
                     // Instead of setup target, we can also just do get context
                     m_Targets[i].Setup(ref context);
 
-                    foreach(var subShader in context.subShaders)
+                    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;
@@ -161,7 +168,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;
@@ -189,12 +196,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);
                 }
             }
         }
 
-        void GenerateShaderPass(int targetIndex, PassDescriptor pass, ActiveFields activeFields, List<BlockFieldDescriptor> currentBlockDescriptors)
+        // 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, PropertyCollector subShaderProperties)
         {
             // Early exit if pass is not used in preview
             if(m_Mode == GenerationMode.Preview && !pass.useInPreview)
@@ -647,7 +712,7 @@ void ProcessStackForPass(ContextData contextData, BlockFieldDescriptor[] passBlo
 
             using (var propertyBuilder = new ShaderStringBuilder())
             {
-                propertyCollector.GetPropertiesDeclaration(propertyBuilder, m_Mode, m_GraphData.concretePrecision);
+                subShaderProperties.GetPropertiesDeclaration(propertyBuilder, m_Mode, m_GraphData.concretePrecision);
                 if(propertyBuilder.length == 0)
                     propertyBuilder.AppendLine("// GraphProperties: <None>");
                 spliceCommands.Add("GraphProperties", propertyBuilder.ToCodeBlock());
@@ -656,17 +721,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());

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 (m_ReadOnly)
+            {
+                Debug.LogError("Cannot sort the properties when the PropertyCollector is already marked ReadOnly");
+                return;
+            }
+
+            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 (properties.Any(x => x.referenceName == chunk.referenceName))
+            if (m_ReadOnly)
+            {
+                Debug.LogError("ERROR attempting to add property to readonly collection");
                 return;
-            properties.Add(chunk);
+            }
+
+            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 inheritedPrecision)
@@ -31,8 +137,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)
             {
@@ -150,6 +255,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
@@ -160,12 +277,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())
             {