Skip to content

Commit ea27a6a

Browse files
author
Chris Tchou
authored
[2021.2] [Bugfix 1296776] Enforce SRP Batcher and Hybrid renderer compatibility by always using a consolidated property list (#3625)
* 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
1 parent 8b4b37b commit ea27a6a

File tree

7 files changed

+213
-38
lines changed

7 files changed

+213
-38
lines changed

com.unity.shadergraph/CHANGELOG.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
2626
- 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)
2727
- Fixed an issue where the Rectangle Node could lose detail at a distance. New control offers additional method that preserves detail better [1156801]
2828
- Fixed virtual texture layer reference names allowing invalid characters [1304146]
29+
- Fixed issue with SRP Batcher compatibility [1310624]
30+
- Fixed issue with Hybrid renderer compatibility [1296776]
2931

3032

3133
## [11.0.0] - 2020-10-21

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

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -102,7 +102,7 @@ public static SlotReference DepthFirstCollectRedirectNodeFromNode(RedirectNodeDa
102102
}
103103

104104
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)
106106
{
107107
// no where to start
108108
if (node == null)
@@ -132,11 +132,11 @@ public static void DepthFirstCollectNodesFromNode(List<AbstractMaterialNode> nod
132132
{
133133
var outputNode = edge.outputSlot.node;
134134
if (outputNode != null)
135-
DepthFirstCollectNodesFromNode(nodeList, outputNode, keywordPermutation: keywordPermutation);
135+
DepthFirstCollectNodesFromNode(nodeList, outputNode, keywordPermutation: keywordPermutation, ignoreActiveState: ignoreActiveState);
136136
}
137137
}
138138

139-
if (includeSelf == IncludeSelf.Include && node.isActive)
139+
if (includeSelf == IncludeSelf.Include && (node.isActive || ignoreActiveState))
140140
nodeList.Add(node);
141141
}
142142

com.unity.shadergraph/Editor/Drawing/PreviewManager.cs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -217,7 +217,7 @@ void OnNodeModified(AbstractMaterialNode node, ModificationScope scope)
217217
static HashSet<AbstractMaterialNode> m_TempAddedToNodeWave = new HashSet<AbstractMaterialNode>();
218218

219219
// cache the Action to avoid GC
220-
Action<AbstractMaterialNode> AddNextLevelNodesToWave =
220+
static Action<AbstractMaterialNode> AddNextLevelNodesToWave =
221221
nextLevelNode =>
222222
{
223223
if (!m_TempAddedToNodeWave.Contains(nextLevelNode))
@@ -227,7 +227,7 @@ void OnNodeModified(AbstractMaterialNode node, ModificationScope scope)
227227
}
228228
};
229229

230-
enum PropagationDirection
230+
internal enum PropagationDirection
231231
{
232232
Upstream,
233233
Downstream
@@ -236,7 +236,7 @@ enum PropagationDirection
236236
// ADDs all nodes in sources, and all nodes in the given direction relative to them, into result
237237
// sources and result can be the same HashSet
238238
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)
240240
{
241241
using (PropagateNodesMarker.Auto())
242242
if (sources.Count > 0)

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

Lines changed: 75 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -88,6 +88,7 @@ public ActiveFields GatherActiveFieldsFromNode(AbstractMaterialNode outputNode,
8888
void BuildShader()
8989
{
9090
var activeNodeList = Pool.ListPool<AbstractMaterialNode>.Get();
91+
bool ignoreActiveState = (m_Mode == GenerationMode.Preview); // for previews, we ignore node active state
9192
if (m_OutputNode == null)
9293
{
9394
foreach (var block in m_Blocks)
@@ -97,12 +98,12 @@ void BuildShader()
9798
if (!block.isActive)
9899
continue;
99100

100-
NodeUtils.DepthFirstCollectNodesFromNode(activeNodeList, block, NodeUtils.IncludeSelf.Include);
101+
NodeUtils.DepthFirstCollectNodesFromNode(activeNodeList, block, NodeUtils.IncludeSelf.Include, ignoreActiveState: ignoreActiveState);
101102
}
102103
}
103104
else
104105
{
105-
NodeUtils.DepthFirstCollectNodesFromNode(activeNodeList, m_OutputNode);
106+
NodeUtils.DepthFirstCollectNodesFromNode(activeNodeList, m_OutputNode, ignoreActiveState: ignoreActiveState);
106107
}
107108

108109
var shaderProperties = new PropertyCollector();
@@ -132,6 +133,10 @@ void BuildShader()
132133
target.CollectShaderProperties(shaderProperties, m_Mode);
133134
}
134135

136+
// set the property collector to read only
137+
// (to ensure no rogue target or pass starts adding more properties later..)
138+
shaderProperties.SetReadOnly();
139+
135140
m_Builder.AppendLine(@"Shader ""{0}""", m_Name);
136141
using (m_Builder.BlockScope())
137142
{
@@ -144,9 +149,11 @@ void BuildShader()
144149
// Instead of setup target, we can also just do get context
145150
m_Targets[i].Setup(ref context);
146151

152+
var subShaderProperties = GetSubShaderPropertiesForTarget(m_Targets[i], m_GraphData, m_Mode, m_OutputNode);
153+
147154
foreach (var subShader in context.subShaders)
148155
{
149-
GenerateSubShader(i, subShader);
156+
GenerateSubShader(i, subShader, subShaderProperties);
150157
}
151158

152159
var customEditor = context.defaultShaderGUI;
@@ -167,7 +174,7 @@ void BuildShader()
167174
m_ConfiguredTextures = shaderProperties.GetConfiguredTexutres();
168175
}
169176

170-
void GenerateSubShader(int targetIndex, SubShaderDescriptor descriptor)
177+
void GenerateSubShader(int targetIndex, SubShaderDescriptor descriptor, PropertyCollector subShaderProperties)
171178
{
172179
if (descriptor.passes == null)
173180
return;
@@ -195,12 +202,70 @@ void GenerateSubShader(int targetIndex, SubShaderDescriptor descriptor)
195202

196203
// Check masternode fields for valid passes
197204
if (pass.TestActive(activeFields))
198-
GenerateShaderPass(targetIndex, pass.descriptor, activeFields, currentBlockDescriptors.Select(x => x.descriptor).ToList());
205+
GenerateShaderPass(targetIndex, pass.descriptor, activeFields, currentBlockDescriptors.Select(x => x.descriptor).ToList(), subShaderProperties);
206+
}
207+
}
208+
}
209+
210+
// this builds the list of properties for a Target / Graph combination
211+
static PropertyCollector GetSubShaderPropertiesForTarget(Target target, GraphData graph, GenerationMode generationMode, AbstractMaterialNode outputNode)
212+
{
213+
PropertyCollector subshaderProperties = new PropertyCollector();
214+
215+
// Collect shader properties declared by active nodes
216+
using (var activeNodes = PooledHashSet<AbstractMaterialNode>.Get())
217+
{
218+
if (outputNode == null)
219+
{
220+
// shader graph builds active nodes starting from the set of active blocks
221+
var currentBlocks = graph.GetNodes<BlockNode>();
222+
var activeBlockContext = new TargetActiveBlockContext(currentBlocks.Select(x => x.descriptor).ToList(), null);
223+
target.GetActiveBlocks(ref activeBlockContext);
224+
225+
foreach (var blockFieldDesc in activeBlockContext.activeBlocks)
226+
{
227+
// attempt to get BlockNode(s) from the stack
228+
var vertBlockNode = graph.vertexContext.blocks.FirstOrDefault(x => x.value.descriptor == blockFieldDesc).value;
229+
if (vertBlockNode != null)
230+
activeNodes.Add(vertBlockNode);
231+
232+
var fragBlockNode = graph.fragmentContext.blocks.FirstOrDefault(x => x.value.descriptor == blockFieldDesc).value;
233+
if (fragBlockNode != null)
234+
activeNodes.Add(fragBlockNode);
235+
}
236+
}
237+
else
238+
{
239+
// preview and/or subgraphs build their active node set based on the single output node
240+
activeNodes.Add(outputNode);
199241
}
242+
243+
PreviewManager.PropagateNodes(activeNodes, PreviewManager.PropagationDirection.Upstream, activeNodes);
244+
245+
// NOTE: this is NOT a deterministic ordering
246+
foreach (var node in activeNodes)
247+
node.CollectShaderProperties(subshaderProperties, generationMode);
248+
249+
// So we sort the properties after
250+
subshaderProperties.Sort();
251+
}
252+
253+
// Collect graph properties
254+
{
255+
graph.CollectShaderProperties(subshaderProperties, generationMode);
256+
}
257+
258+
// Collect shader properties declared by the Target
259+
{
260+
target.CollectShaderProperties(subshaderProperties, generationMode);
200261
}
262+
263+
subshaderProperties.SetReadOnly();
264+
265+
return subshaderProperties;
201266
}
202267

203-
void GenerateShaderPass(int targetIndex, PassDescriptor pass, ActiveFields activeFields, List<BlockFieldDescriptor> currentBlockDescriptors)
268+
void GenerateShaderPass(int targetIndex, PassDescriptor pass, ActiveFields activeFields, List<BlockFieldDescriptor> currentBlockDescriptors, PropertyCollector subShaderProperties)
204269
{
205270
// Early exit if pass is not used in preview
206271
if (m_Mode == GenerationMode.Preview && !pass.useInPreview)
@@ -227,6 +292,7 @@ void GenerateShaderPass(int targetIndex, PassDescriptor pass, ActiveFields activ
227292
CustomInterpSubGen customInterpSubGen = new CustomInterpSubGen(m_OutputNode != null);
228293

229294
// Initiailize Collectors
295+
// NOTE: propertyCollector is not really used anymore -- we use the subshader PropertyCollector instead
230296
var propertyCollector = new PropertyCollector();
231297
var keywordCollector = new KeywordCollector();
232298
m_GraphData.CollectShaderKeywords(keywordCollector, m_Mode);
@@ -663,7 +729,7 @@ void ProcessStackForPass(ContextData contextData, BlockFieldDescriptor[] passBlo
663729

664730
using (var propertyBuilder = new ShaderStringBuilder())
665731
{
666-
propertyCollector.GetPropertiesDeclaration(propertyBuilder, m_Mode, m_GraphData.graphDefaultConcretePrecision);
732+
subShaderProperties.GetPropertiesDeclaration(propertyBuilder, m_Mode, m_GraphData.graphDefaultConcretePrecision);
667733
if (propertyBuilder.length == 0)
668734
propertyBuilder.AppendLine("// GraphProperties: <None>");
669735
spliceCommands.Add("GraphProperties", propertyBuilder.ToCodeBlock());
@@ -672,17 +738,12 @@ void ProcessStackForPass(ContextData contextData, BlockFieldDescriptor[] passBlo
672738
// --------------------------------------------------
673739
// Dots Instanced Graph Properties
674740

675-
bool hasDotsProperties = false;
676-
m_GraphData.ForeachHLSLProperty(h =>
677-
{
678-
if (h.declaration == HLSLDeclaration.HybridPerInstance)
679-
hasDotsProperties = true;
680-
});
741+
bool hasDotsProperties = subShaderProperties.HasDotsProperties();
681742

682743
using (var dotsInstancedPropertyBuilder = new ShaderStringBuilder())
683744
{
684745
if (hasDotsProperties)
685-
dotsInstancedPropertyBuilder.AppendLines(propertyCollector.GetDotsInstancingPropertiesDeclaration(m_Mode));
746+
dotsInstancedPropertyBuilder.AppendLines(subShaderProperties.GetDotsInstancingPropertiesDeclaration(m_Mode));
686747
else
687748
dotsInstancedPropertyBuilder.AppendLine("// HybridV1InjectedBuiltinProperties: <None>");
688749
spliceCommands.Add("HybridV1InjectedBuiltinProperties", dotsInstancedPropertyBuilder.ToCodeBlock());

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

Lines changed: 124 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,9 @@
1+
using System;
12
using System.Collections.Generic;
23
using System.Linq;
34
using System.Text;
45
using UnityEditor.ShaderGraph.Internal;
6+
using UnityEngine;
57

68
namespace UnityEditor.ShaderGraph
79
{
@@ -14,13 +16,117 @@ public struct TextureInfo
1416
public bool modifiable;
1517
}
1618

17-
public readonly List<AbstractShaderProperty> properties = new List<AbstractShaderProperty>();
19+
bool m_ReadOnly;
20+
List<HLSLProperty> m_HLSLProperties = null;
1821

19-
public void AddShaderProperty(AbstractShaderProperty chunk)
22+
// reference name ==> property index in list
23+
Dictionary<string, int> m_ReferenceNames = new Dictionary<string, int>();
24+
25+
// list of properties (kept in a list to maintain deterministic declaration order)
26+
List<AbstractShaderProperty> m_Properties = new List<AbstractShaderProperty>();
27+
28+
public int propertyCount => m_Properties.Count;
29+
public IEnumerable<AbstractShaderProperty> properties => m_Properties;
30+
public AbstractShaderProperty GetProperty(int index) { return m_Properties[index]; }
31+
32+
public void Sort()
2033
{
21-
if (properties.Any(x => x.referenceName == chunk.referenceName))
34+
if (m_ReadOnly)
35+
{
36+
Debug.LogError("Cannot sort the properties when the PropertyCollector is already marked ReadOnly");
2237
return;
23-
properties.Add(chunk);
38+
}
39+
40+
m_Properties.Sort((a, b) => String.CompareOrdinal(a.referenceName, b.referenceName));
41+
}
42+
43+
public void SetReadOnly()
44+
{
45+
m_ReadOnly = true;
46+
}
47+
48+
private static bool EquivalentHLSLProperties(AbstractShaderProperty a, AbstractShaderProperty b)
49+
{
50+
bool equivalent = true;
51+
var bHLSLProps = new List<HLSLProperty>();
52+
b.ForeachHLSLProperty(bh => bHLSLProps.Add(bh));
53+
a.ForeachHLSLProperty(ah =>
54+
{
55+
var i = bHLSLProps.FindIndex(bh => bh.name == ah.name);
56+
if (i < 0)
57+
equivalent = false;
58+
else
59+
{
60+
var bh = bHLSLProps[i];
61+
if ((ah.name != bh.name) ||
62+
(ah.type != bh.type) ||
63+
(ah.precision != bh.precision) ||
64+
(ah.declaration != bh.declaration) ||
65+
((ah.customDeclaration == null) != (bh.customDeclaration == null)))
66+
{
67+
equivalent = false;
68+
}
69+
else if (ah.customDeclaration != null)
70+
{
71+
var ssba = new ShaderStringBuilder();
72+
var ssbb = new ShaderStringBuilder();
73+
ah.customDeclaration(ssba);
74+
bh.customDeclaration(ssbb);
75+
if (ssba.ToCodeBlock() != ssbb.ToCodeBlock())
76+
equivalent = false;
77+
}
78+
bHLSLProps.RemoveAt(i);
79+
}
80+
});
81+
return equivalent && (bHLSLProps.Count == 0);
82+
}
83+
84+
public void AddShaderProperty(AbstractShaderProperty prop)
85+
{
86+
if (m_ReadOnly)
87+
{
88+
Debug.LogError("ERROR attempting to add property to readonly collection");
89+
return;
90+
}
91+
92+
int propIndex = -1;
93+
if (m_ReferenceNames.TryGetValue(prop.referenceName, out propIndex))
94+
{
95+
// existing referenceName
96+
var existingProp = m_Properties[propIndex];
97+
if (existingProp != prop)
98+
{
99+
// duplicate reference name, but different property instances
100+
if (existingProp.GetType() != prop.GetType())
101+
{
102+
Debug.LogError("Two properties with the same reference name (" + prop.referenceName + ") using different types");
103+
}
104+
else
105+
{
106+
if (!EquivalentHLSLProperties(existingProp, prop))
107+
Debug.LogError("Two properties with the same reference name (" + prop.referenceName + ") produce different HLSL properties");
108+
}
109+
}
110+
}
111+
else
112+
{
113+
// new referenceName, new property
114+
propIndex = m_Properties.Count;
115+
m_Properties.Add(prop);
116+
m_ReferenceNames.Add(prop.referenceName, propIndex);
117+
}
118+
}
119+
120+
private List<HLSLProperty> BuildHLSLPropertyList()
121+
{
122+
SetReadOnly();
123+
if (m_HLSLProperties == null)
124+
{
125+
m_HLSLProperties = new List<HLSLProperty>();
126+
foreach (var p in m_Properties)
127+
p.ForeachHLSLProperty(h => m_HLSLProperties.Add(h));
128+
}
129+
return m_HLSLProperties;
24130
}
25131

26132
public void GetPropertiesDeclaration(ShaderStringBuilder builder, GenerationMode mode, ConcretePrecision defaultPrecision)
@@ -32,8 +138,7 @@ public void GetPropertiesDeclaration(ShaderStringBuilder builder, GenerationMode
32138
}
33139

34140
// build a list of all HLSL properties
35-
var hlslProps = new List<HLSLProperty>();
36-
properties.ForEach(p => p.ForeachHLSLProperty(h => hlslProps.Add(h)));
141+
var hlslProps = BuildHLSLPropertyList();
37142

38143
if (mode == GenerationMode.Preview)
39144
{
@@ -151,6 +256,18 @@ public void GetPropertiesDeclaration(ShaderStringBuilder builder, GenerationMode
151256
h.AppendTo(builder);
152257
}
153258

259+
public bool HasDotsProperties()
260+
{
261+
var hlslProps = BuildHLSLPropertyList();
262+
bool hasDotsProperties = false;
263+
foreach (var h in hlslProps)
264+
{
265+
if (h.declaration == HLSLDeclaration.HybridPerInstance)
266+
hasDotsProperties = true;
267+
}
268+
return hasDotsProperties;
269+
}
270+
154271
public string GetDotsInstancingPropertiesDeclaration(GenerationMode mode)
155272
{
156273
// Hybrid V1 needs to declare a special macro to that is injected into
@@ -161,12 +278,7 @@ public string GetDotsInstancingPropertiesDeclaration(GenerationMode mode)
161278
var batchAll = (mode == GenerationMode.Preview);
162279

163280
// build a list of all HLSL properties
164-
var hybridHLSLProps = new List<HLSLProperty>();
165-
properties.ForEach(p => p.ForeachHLSLProperty(h =>
166-
{
167-
if (h.declaration == HLSLDeclaration.HybridPerInstance)
168-
hybridHLSLProps.Add(h);
169-
}));
281+
var hybridHLSLProps = BuildHLSLPropertyList().Where(h => h.declaration == HLSLDeclaration.HybridPerInstance);
170282

171283
if (hybridHLSLProps.Any())
172284
{

0 commit comments

Comments
 (0)