Skip to content

Commit 4a4aa11

Browse files
PaulDemeulenaereEvergreen
PaulDemeulenaere
authored and
Evergreen
committed
[VFX] Fix Custom HLSL collection with diverging branches
Issue introduced at https://github.cds.internal.unity3d.com/unity/unity/pull/48953 (landed in 6000.0.6f1) To illustrate the problem, here a case which doesn't cause any problem: <img width="1088" alt="image" src="https://media.github.cds.internal.unity3d.com/user/42/files/364fc89f-9e77-4795-b82e-bc25f5718a72"> Failing case: <img width="1085" alt="image" src="https://media.github.cds.internal.unity3d.com/user/42/files/7cfdad6d-ff4b-42b3-aafe-a4fb6c62854d"> Initially, I changed the workflow to collect IHLSLCodeHolder & GraphicsBufferUsage per context but the resolution was incomplete. If two end expressions were sharing the same ancestor, only the first evaluation is able to collected needed data. It's fine when we are catching this globally but this is wrong when the information must be computed per context. After several back and forth, I finally decided to extract the expression collection from compilation and introduce a preprocess. In the end, even if it forces to browse the expression graph twice, it separates concern and prevent side issues. Additionally, it removes the former `collectedData` in all compile functions. 🎁 I added `CollectPerContextData` flag to reserve this graph browsing to a single case.
1 parent cdd6063 commit 4a4aa11

File tree

3 files changed

+224
-70
lines changed

3 files changed

+224
-70
lines changed

‎Packages/com.unity.visualeffectgraph/Editor/Compiler/VFXExpressionGraph.cs‎

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -82,14 +82,13 @@ private void CompileExpressionContext(IEnumerable<VFXContext> contexts,
8282
foreach (var exp in expressionsToReduced.Values)
8383
AddExpressionDataRecursively(m_ExpressionsData, exp);
8484

85-
foreach (var bufferTypeUsage in expressionContext.GraphicsBufferTypeUsagePerContext)
85+
if (options.HasFlag(VFXExpressionContextOption.CollectPerContextData))
8686
{
87-
m_BufferTypeUsagePerContext.TryAdd(bufferTypeUsage.Key, bufferTypeUsage.Value);
88-
}
89-
87+
foreach (var bufferTypeUsage in expressionContext.GraphicsBufferTypeUsagePerContext)
88+
{
89+
m_BufferTypeUsagePerContext.TryAdd(bufferTypeUsage.Key, bufferTypeUsage.Value);
90+
}
9091

91-
if (target == VFXDeviceTarget.GPU)
92-
{
9392
foreach (var hlslCodeHolder in expressionContext.hlslCodeHoldersPerContext)
9493
{
9594
m_CustomHLSLExpressionsPerContext.Add(hlslCodeHolder.Key, hlslCodeHolder.Value);
@@ -177,7 +176,7 @@ public void CompileExpressions(IEnumerable<VFXContext> contexts, VFXExpressionCo
177176
var otherContexts = contexts.Where(o => o.contextType != VFXContextType.Spawner);
178177
CompileExpressionContext(spawnerContexts, options | VFXExpressionContextOption.PatchReadToEventAttribute, VFXDeviceTarget.CPU);
179178
CompileExpressionContext(otherContexts, options, VFXDeviceTarget.CPU);
180-
CompileExpressionContext(contexts, options | VFXExpressionContextOption.GPUDataTransformation, VFXDeviceTarget.GPU);
179+
CompileExpressionContext(contexts, options | VFXExpressionContextOption.GPUDataTransformation | VFXExpressionContextOption.CollectPerContextData, VFXDeviceTarget.GPU);
181180

182181
var sortedList = m_ExpressionsData.Where(kvp =>
183182
{

‎Packages/com.unity.visualeffectgraph/Editor/Expressions/VFXExpressionContext.cs‎

Lines changed: 141 additions & 63 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,8 @@ enum VFXExpressionContextOption
1414
CPUEvaluation = 1 << 1,
1515
ConstantFolding = 1 << 2,
1616
GPUDataTransformation = 1 << 3,
17-
PatchReadToEventAttribute = 1 << 4
17+
PatchReadToEventAttribute = 1 << 4,
18+
CollectPerContextData = 1 << 5
1819
}
1920

2021
abstract partial class VFXExpression
@@ -61,62 +62,162 @@ public void UnregisterExpression(VFXExpression expression)
6162
m_EndExpressions.Remove(expression);
6263
}
6364

64-
static readonly ProfilerMarker s_CompileExpressionContext = new ProfilerMarker("VFXEditor.CompileExpressionContext");
65+
class CollectedData
66+
{
67+
public readonly HashSet<VFXExpression> processedExpressions = new();
68+
public readonly HashSet<VFXExpression> markedExpressions = new();
69+
public readonly Dictionary<IHLSLCodeHolder, HashSet<VFXExpression>> childrenExpressionHLSLCodeHolder = new();
70+
public readonly Dictionary<VFXExpressionBufferWithType, HashSet<VFXExpression>> childrenExpressionBufferWithType = new();
71+
}
6572

66-
public void Compile()
73+
private void CollectPerContextDataRecursive(VFXExpression node, Stack<VFXExpression> currentChildren, CollectedData data)
6774
{
68-
using (s_CompileExpressionContext.Auto())
75+
if (data.processedExpressions.Contains(node))
6976
{
70-
bool needToPatch = HasAny(VFXExpressionContextOption.GPUDataTransformation | VFXExpressionContextOption.PatchReadToEventAttribute);
71-
var gpuTransformation = needToPatch && Has(VFXExpressionContextOption.GPUDataTransformation);
72-
var spawnEventPath = needToPatch && Has(VFXExpressionContextOption.PatchReadToEventAttribute);
73-
74-
var collectedData = new CompileCollectedData()
75-
{
76-
bufferTypeUsages = new(),
77-
hlslCodeHolders = new()
78-
};
79-
80-
foreach (var exp in m_EndExpressions)
77+
if (data.markedExpressions.Contains(node))
8178
{
82-
Compile(exp.Key, collectedData);
83-
if (needToPatch)
84-
m_ReducedCache[exp.Key] = PatchVFXExpression(GetReduced(exp.Key), null /* no source in end expression */, gpuTransformation, spawnEventPath, m_GlobalEventAttribute, collectedData);
79+
foreach (var hlslCodeHolderCollection in data.childrenExpressionHLSLCodeHolder)
80+
{
81+
if (hlslCodeHolderCollection.Value.Contains(node))
82+
{
83+
foreach (var child in currentChildren)
84+
{
85+
data.markedExpressions.Add(child);
86+
hlslCodeHolderCollection.Value.Add(child);
87+
}
88+
}
89+
}
8590

86-
if (collectedData.bufferTypeUsages.Count > 0)
91+
foreach (var expressionBufferWithTypeCollection in data.childrenExpressionBufferWithType)
8792
{
88-
foreach (var context in exp.Value)
93+
if (expressionBufferWithTypeCollection.Value.Contains(node))
8994
{
90-
if (!m_GraphicsBufferTypeUsagePerContext.TryGetValue(context, out var usages))
95+
foreach (var child in currentChildren)
9196
{
92-
usages = new Dictionary<VFXExpression, BufferUsage>();
93-
m_GraphicsBufferTypeUsagePerContext.Add(context, usages);
97+
data.markedExpressions.Add(child);
98+
expressionBufferWithTypeCollection.Value.Add(child);
9499
}
100+
}
101+
}
102+
}
103+
return;
104+
}
105+
106+
currentChildren.Push(node);
107+
if (node is IHLSLCodeHolder hlslCodeHolder)
108+
{
109+
if (!data.childrenExpressionHLSLCodeHolder.TryGetValue(hlslCodeHolder, out var childCollection))
110+
{
111+
childCollection = new();
112+
data.childrenExpressionHLSLCodeHolder.Add(hlslCodeHolder, childCollection);
113+
}
114+
115+
foreach (var child in currentChildren)
116+
{
117+
data.markedExpressions.Add(child);
118+
childCollection.Add(child);
119+
}
120+
}
121+
122+
if (node is VFXExpressionBufferWithType expressionWithType)
123+
{
124+
if (!data.childrenExpressionBufferWithType.TryGetValue(expressionWithType, out var childCollection))
125+
{
126+
childCollection = new();
127+
data.childrenExpressionBufferWithType.Add(expressionWithType, childCollection);
128+
}
129+
130+
foreach (var child in currentChildren)
131+
{
132+
data.markedExpressions.Add(child);
133+
childCollection.Add(child);
134+
}
135+
}
136+
137+
foreach (var parent in node.parents)
138+
CollectPerContextDataRecursive(parent, currentChildren, data);
139+
140+
data.processedExpressions.Add(node);
141+
currentChildren.Pop();
142+
}
143+
144+
private void CollectPerContextData()
145+
{
146+
var collectedDataCache = new CollectedData();
147+
var childrenStackCache = new Stack<VFXExpression>();
148+
foreach (var exp in m_EndExpressions)
149+
{
150+
if (childrenStackCache.Count > 0)
151+
throw new InvalidOperationException("Unexpected Children Stack after dependency collection.");
152+
CollectPerContextDataRecursive(exp.Key, childrenStackCache, collectedDataCache);
95153

96-
foreach (var expressionTypeUsage in collectedData.bufferTypeUsages)
154+
if (collectedDataCache.markedExpressions.Contains(exp.Key))
155+
{
156+
foreach (var hlslCodeHolderCollection in collectedDataCache.childrenExpressionHLSLCodeHolder)
157+
{
158+
if (hlslCodeHolderCollection.Value.Contains(exp.Key))
159+
{
160+
foreach (var context in exp.Value)
97161
{
98-
if (!usages.TryAdd(expressionTypeUsage.Key, expressionTypeUsage.Value) && usages[expressionTypeUsage.Key] != expressionTypeUsage.Value)
162+
if (!m_HLSLCollectionPerContext.TryGetValue(context, out var codeHolders))
99163
{
100-
throw new InvalidOperationException($"Diverging type usage for GraphicsBuffer : {usages[expressionTypeUsage.Key]}, {expressionTypeUsage.Value}");
164+
codeHolders = new();
165+
m_HLSLCollectionPerContext.Add(context, codeHolders);
101166
}
167+
codeHolders.Add(hlslCodeHolderCollection.Key);
102168
}
103169
}
104170
}
105-
collectedData.bufferTypeUsages.Clear();
106171

107-
if (collectedData.hlslCodeHolders.Count > 0)
172+
foreach (var expressionBufferWithTypeCollection in collectedDataCache.childrenExpressionBufferWithType)
108173
{
109-
foreach (var context in exp.Value)
174+
if (expressionBufferWithTypeCollection.Value.Contains(exp.Key))
110175
{
111-
if (!m_HLSLCollectionPerContext.TryGetValue(context, out var codeHolders))
176+
foreach (var context in exp.Value)
112177
{
113-
codeHolders = new List<IHLSLCodeHolder>();
114-
m_HLSLCollectionPerContext.Add(context, codeHolders);
178+
if (!m_GraphicsBufferTypeUsagePerContext.TryGetValue(context, out var usages))
179+
{
180+
usages = new();
181+
m_GraphicsBufferTypeUsagePerContext.Add(context, usages);
182+
}
183+
184+
var usage = expressionBufferWithTypeCollection.Key.usage;
185+
var buffer = expressionBufferWithTypeCollection.Key.parents[0];
186+
if (!usages.TryAdd(buffer, usage) && usages[buffer] != usage)
187+
{
188+
throw new InvalidOperationException($"Diverging type usage for GraphicsBuffer : {buffer}, {usage}");
189+
}
115190
}
116-
codeHolders.AddRange(collectedData.hlslCodeHolders);
117191
}
118192
}
119-
collectedData.hlslCodeHolders.Clear();
193+
}
194+
}
195+
}
196+
197+
static readonly ProfilerMarker s_CollectPerContextData = new ProfilerMarker("VFXEditor.CollectPerContextData");
198+
static readonly ProfilerMarker s_CompileExpressionContext = new ProfilerMarker("VFXEditor.CompileExpressionContext");
199+
200+
public void Compile()
201+
{
202+
if (Has(VFXExpressionContextOption.CollectPerContextData))
203+
{
204+
using (s_CollectPerContextData.Auto())
205+
{
206+
CollectPerContextData();
207+
}
208+
}
209+
210+
using (s_CompileExpressionContext.Auto())
211+
{
212+
bool needToPatch = HasAny(VFXExpressionContextOption.GPUDataTransformation | VFXExpressionContextOption.PatchReadToEventAttribute);
213+
var gpuTransformation = needToPatch && Has(VFXExpressionContextOption.GPUDataTransformation);
214+
var spawnEventPath = needToPatch && Has(VFXExpressionContextOption.PatchReadToEventAttribute);
215+
216+
foreach (var exp in m_EndExpressions)
217+
{
218+
Compile(exp.Key);
219+
if (needToPatch)
220+
m_ReducedCache[exp.Key] = PatchVFXExpression(GetReduced(exp.Key), null /* no source in end expression */, gpuTransformation, spawnEventPath, m_GlobalEventAttribute);
120221
}
121222
}
122223
}
@@ -157,7 +258,7 @@ private bool ShouldEvaluate(VFXExpression exp, VFXExpression[] reducedParents)
157258
return true;
158259
}
159260

160-
private VFXExpression PatchVFXExpression(VFXExpression input, VFXExpression targetExpression, bool insertGPUTransformation, bool patchReadAttributeForSpawn, IEnumerable<VFXLayoutElementDesc> globalEventAttribute, CompileCollectedData collectedData)
261+
private VFXExpression PatchVFXExpression(VFXExpression input, VFXExpression targetExpression, bool insertGPUTransformation, bool patchReadAttributeForSpawn, IEnumerable<VFXLayoutElementDesc> globalEventAttribute)
161262
{
162263
if (insertGPUTransformation)
163264
{
@@ -184,7 +285,7 @@ private VFXExpression PatchVFXExpression(VFXExpression input, VFXExpression targ
184285
case VFXExpressionOperation.SampleMeshVertexFloat4:
185286
case VFXExpressionOperation.SampleMeshVertexColor:
186287
var channelFormatAndDimensionAndStream = targetExpression.parents[2];
187-
channelFormatAndDimensionAndStream = Compile(channelFormatAndDimensionAndStream, collectedData);
288+
channelFormatAndDimensionAndStream = Compile(channelFormatAndDimensionAndStream);
188289
if (!(channelFormatAndDimensionAndStream is VFXExpressionMeshChannelInfos))
189290
throw new InvalidOperationException("Unexpected type of expression in mesh sampling : " + channelFormatAndDimensionAndStream);
190291
input = new VFXExpressionVertexBufferFromMesh(input, channelFormatAndDimensionAndStream);
@@ -201,7 +302,7 @@ private VFXExpression PatchVFXExpression(VFXExpression input, VFXExpression targ
201302
if (targetExpression is IVFXExpressionSampleSkinnedMesh skinnedMeshExpression)
202303
{
203304
var channelFormatAndDimensionAndStream = targetExpression.parents[2];
204-
channelFormatAndDimensionAndStream = Compile(channelFormatAndDimensionAndStream, collectedData);
305+
channelFormatAndDimensionAndStream = Compile(channelFormatAndDimensionAndStream);
205306
if (!(channelFormatAndDimensionAndStream is VFXExpressionMeshChannelInfos))
206307
throw new InvalidOperationException("Unexpected type of expression in skinned mesh sampling : " + channelFormatAndDimensionAndStream);
207308
input = new VFXExpressionVertexBufferFromSkinnedMeshRenderer(input, channelFormatAndDimensionAndStream, skinnedMeshExpression.frame);
@@ -223,18 +324,6 @@ private VFXExpression PatchVFXExpression(VFXExpression input, VFXExpression targ
223324
if (input.valueType == VFXValueType.Buffer && input is VFXExpressionBufferWithType bufferWithType)
224325
{
225326
input = input.parents[0]; //Explicitly skip NoOp expression
226-
if (collectedData.bufferTypeUsages != null)
227-
{
228-
var usageType = bufferWithType.usage;
229-
if (!collectedData.bufferTypeUsages.TryGetValue(input, out var registeredType))
230-
{
231-
collectedData.bufferTypeUsages.Add(input, usageType);
232-
}
233-
else if (registeredType != usageType)
234-
{
235-
throw new InvalidOperationException($"Diverging type usage for GraphicsBuffer : {registeredType}, {usageType}");
236-
}
237-
}
238327
}
239328

240329
if (patchReadAttributeForSpawn && input is VFXAttributeExpression attribute)
@@ -261,13 +350,7 @@ private VFXExpression PatchVFXExpression(VFXExpression input, VFXExpression targ
261350
return input;
262351
}
263352

264-
public struct CompileCollectedData
265-
{
266-
public Dictionary<VFXExpression, BufferUsage> bufferTypeUsages;
267-
public List<IHLSLCodeHolder> hlslCodeHolders;
268-
}
269-
270-
public VFXExpression Compile(VFXExpression expression, CompileCollectedData collectedData = default(CompileCollectedData))
353+
public VFXExpression Compile(VFXExpression expression)
271354
{
272355
var gpuTransformation = Has(VFXExpressionContextOption.GPUDataTransformation);
273356
var patchReadAttributeForSpawn = Has(VFXExpressionContextOption.PatchReadToEventAttribute);
@@ -278,11 +361,11 @@ public struct CompileCollectedData
278361
var parents = new VFXExpression[expression.parents.Length];
279362
for (var i = 0; i < expression.parents.Length; i++)
280363
{
281-
var parent = Compile(expression.parents[i], collectedData);
364+
var parent = Compile(expression.parents[i]);
282365
bool currentGPUTransformation = gpuTransformation
283366
&& expression.IsAny(VFXExpression.Flags.NotCompilableOnCPU)
284367
&& !parent.IsAny(VFXExpression.Flags.NotCompilableOnCPU);
285-
parent = PatchVFXExpression(parent, expression, currentGPUTransformation, patchReadAttributeForSpawn, m_GlobalEventAttribute, collectedData);
368+
parent = PatchVFXExpression(parent, expression, currentGPUTransformation, patchReadAttributeForSpawn, m_GlobalEventAttribute);
286369
parents[i] = parent;
287370
}
288371

@@ -299,11 +382,6 @@ public struct CompileCollectedData
299382
reduced = expression;
300383
}
301384

302-
if (expression is IHLSLCodeHolder hlslCodeHolder && collectedData.hlslCodeHolders != null)
303-
{
304-
if (!collectedData.hlslCodeHolders.Contains(hlslCodeHolder))
305-
collectedData.hlslCodeHolders.Add(hlslCodeHolder);
306-
}
307385
m_ReducedCache[expression] = reduced;
308386
}
309387
return reduced;

0 commit comments

Comments
 (0)