[VFX] Fix compilation error when shaderGraph is modified #5475
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
+ Cleaner exception than a null ref exception
The regression has been probably introduced by #4971 What happens, 1st VFX import: - Not Sanitized - Skip first compilation - PostProcess => Sanitize & CheckGraphBeforeImport - Reimport Later, the ShaderGraph changes - Because the VFX is sanitized - Doesn't skip the first compilation => Fail - Post Process => CheckGraphBeforeImport is applied - ... Later VFX import will be ok until the SG isn't modified.
|
Thanks for the fix Paul! Quick summary - specifically Here's the work in progress test doc, logged all the scenarios I managed to find about this main issue, also logged some not caused by the PR bugs, maybe something will draw your attention and could be fixed. |
Equivalent change from e6d34d3 The following ContainsKey would have thrown an exception anyway
2 tasks
julienf-unity
approved these changes
Sep 7, 2021
|
Thanks for the fix! Error is no longer persistent. Tried:
Other issues raised in test doc are unrelated to PR. |
VitaVFX
approved these changes
Sep 9, 2021
|
I double checked the changelog.md, expected job are green 🟢 at this revision f6d7f03 |
PaulDemeulenaere
deleted the
vfx/fix/1361601-missing-check-graph-before-compile
branch
PaulDemeulenaere
added a commit
that referenced
this pull request
Sep 10, 2021
* Add Editor Test to cover issue 1361601 + Cleaner exception than a null ref exception * Actual fix from https://fogbugz.unity3d.com/f/cases/1361601/ The regression has been probably introduced by #4971 What happens, 1st VFX import: - Not Sanitized - Skip first compilation - PostProcess => Sanitize & CheckGraphBeforeImport - Reimport Later, the ShaderGraph changes - Because the VFX is sanitized - Doesn't skip the first compilation => Fail - Post Process => CheckGraphBeforeImport is applied - ... Later VFX import will be ok until the SG isn't modified. * *Update changelog.md * More readable exception when failing to find expression Equivalent change from e6d34d3 The following ContainsKey would have thrown an exception anyway * Move changelog entry to 13.x.x # Conflicts: # com.unity.visualeffectgraph/CHANGELOG.md
PaulDemeulenaere
added a commit
that referenced
this pull request
Sep 15, 2021
* Add Editor Test to cover issue 1361601 + Cleaner exception than a null ref exception * Actual fix from https://fogbugz.unity3d.com/f/cases/1361601/ The regression has been probably introduced by #4971 What happens, 1st VFX import: - Not Sanitized - Skip first compilation - PostProcess => Sanitize & CheckGraphBeforeImport - Reimport Later, the ShaderGraph changes - Because the VFX is sanitized - Doesn't skip the first compilation => Fail - Post Process => CheckGraphBeforeImport is applied - ... Later VFX import will be ok until the SG isn't modified. * *Update changelog.md * More readable exception when failing to find expression Equivalent change from e6d34d3 The following ContainsKey would have thrown an exception anyway * Move changelog entry to 13.x.x # Conflicts: # com.unity.visualeffectgraph/CHANGELOG.md
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Purpose of this PR
Fix case https://fogbugz.unity3d.com/f/cases/1361601/
The regression has been introduced by #4971 & #5104 but it can't be tested prior this change, the last C++ producing the correct old behavior is 21.2a21.
0) What was happening before "Fix Sanitize" #4971 :
1st VFX import
The dependent shaderGraph is modified
1) After "Fix Sanitize" #4971
1st VFX import
The dependent shaderGraph is modified
2) After "Fix Subgraph" #5104
Same than 1) but the PrepareSubgraph changes are now safely handled preventing any deleted references serialized in graph.
-...
The problem !
After first import the graph is marked as sanitized but the CheckGraphBeforeImport is occurring too late:
(...)
The dependent shaderGraph is modified
3) After this fix (and more precisely f62f1b6) adding CheckGraphBeforeImport process before compilation
N.B.: As PrepareSubgraph, the CheckGraphBeforeImport is safe during preprocess only because we are backuping/restoring the graph, otherwise, slot created during preprocess will be destroyed after the import.
Testing status
Tested locally with described repro step.
Tested manually at this revision (other integration branch) : 61ebca1
Added an editor regression test
Modify_ShaderGraph_Property_Check_VFX_Compilation_Doesnt_FailIt simulates the repro step modifying the shaderGraph content.
Before
After
Run all test locally
Yamato trunk 🟢
Yamato 2021.2 at cba0eba12ed723121f116ca85995475593fe3963 🟢
Comments to reviewers
We are planning a live code review on this change with @julienf-unity
N.B: We could also benefit from the rework of OnPostprocessAllAssets thanks to this PR but these changes hasn't landed in 21.2 (see this conversation)