[VFX] Importer and compilation various fixes #5371
Conversation
|
Hi! This comment will help you figure out which jobs to run before merging your PR. The suggestions are dynamic based on what files you have changed. VFX Depending on the scope of your PR, you may need to run more jobs than what has been suggested. Please speak to your lead or a Graphics SDET (#devs-graphics-automation) if you are unsure. |
|
It appears that you made a non-draft PR! |
| - Zoom and warning icons were blurry in the "Play Controls" and "Visual Effect Model" scene overlays | ||
| - Random crash using subgraph [Case 1345426](https://issuetracker.unity3d.com/product/unity/issues/guid/1345426/) | ||
| - Fix potential infinite compilation when using subgraphs [Case 1346576](https://issuetracker.unity3d.com/product/unity/issues/guid/1346576/) | ||
| - Prevent out of sync serialization of VFX assets that could cause the asset to be dirtied without reason [Case 1327805](https://issuetracker.unity3d.com/product/unity/issues/guid/1327805/) |
There was a problem hiding this comment.
This URL does not work as bug 1327805 is private (since it has the customer's info and discussions on it)
There was a problem hiding this comment.
You're right! Thanks, I removed the link from the changelog
| { | ||
| VFXGraph.compileReporter = reporter; | ||
| AssetDatabase.ImportAsset(AssetDatabase.GetAssetPath(graphView.controller.model)); | ||
| graph.SetExpressionGraphDirty(false); // As are implemented subgraph now, compiling dependents chain can reset dirty flag on used subgraphs, which will make an infinite loop, this is bad! |
There was a problem hiding this comment.
I realized we assumed the previous AssetDatabase.ImportAsset was synchronous here. I don't think it's possible to have an asynchronous import of the vfx anyway.
| { | ||
| if (EditorUtility.IsDirty(graph) || UnityEngine.Object.ReferenceEquals(graph, controller.graph)) | ||
| { | ||
| graph.UpdateSubAssets(); |
There was a problem hiding this comment.
The missing UpdateSubAssets reminds me an old debug, see this conversation
At that time, I experimented a C++ workaround
For sure, even this bug : https://fogbugz.unity3d.com/f/cases/1322919/ isn't reproducible anymore, this change will avoid unexpected orphan references saved in VisualEffectAsset.
| case BuiltInFlag.WorldToLocal: | ||
| return VFXCoordinateSpace.World; | ||
| default: | ||
| return (VFXCoordinateSpace)int.MaxValue; |
There was a problem hiding this comment.
Optionally, if outputSlot.spaceable and you reach the default state, you could throw an invalid operation exception.
| m_reentrant = true; | ||
| ExpressionGraphDirty = true; | ||
| model.GetOrCreateGraph().UpdateSubAssets(); | ||
| EditorUtility.SetDirty(graph); |
There was a problem hiding this comment.
Interesting, I observed this behavior locally (without your change) :
Was there another case where it causes an issue ?
|
Same as with the 10.x backport, one outstanding change in behavior item that this PR introduces in the FTP project which needs to be looked at. Dropping test doc here as well for visibility |
There was a problem hiding this comment.
Looking good, no outstanding issues pertaining to this PR (test doc). Thanks for the fix!
* Fix sanitize of exposed Camera parameters * Fix SDF Baker on PS4/5 failing due to explicit binding * Fixed NRE when starting play mode (#279) * Fixed NRE when starting play mode * Save auto-attach lock state and attached VFX in the editor prefs to avoid loosing them when going in play mode * Better restore attached VFX when leaving play mode (even if it has been removed during play) * Fix compil error when cubemap array is used in compute (even on platform supporting it) (#5582) * Update HDRP assets * Fix failing test * Fix failing test * Revert "Fix failing test" This reverts commit c6fe054. * [VFX] Importer and compilation various fixes #5371 Co-authored-by: Gabriel de la Cruz <gabriel.delacruz@unity3d.com> Co-authored-by: Ludovic Theobald <ludovic.theobald@unity3d.com> Co-authored-by: Julien Amsellem <julien.amsellem@unity3d.com>
4 participants
Purpose of this PR
Various fixes:
Testing status
Tested locally
Comments to reviewers
This PR is meant to be backported to 20.3 and 21.1 and then to partner branch
PR to 20.3 was created now because it's high priority. PR to 21.1 will be made as a cherry pick once merged (hence the label)