[VFX] Crash with subgraph

TestProjects/VisualEffectGraph_HDRP/Assets/AllTests/Editor/Tests/VFXControllerTests.cs

@@ -11,6 +11,7 @@
 using System.IO;
 using UnityEditor.VFX.Block.Test;
 using UnityEngine.UIElements;
+using UnityEngine.TestTools;
 
 namespace UnityEditor.VFX.Test
 {
@@ -20,9 +21,13 @@ public class VFXControllersTests
         VFXViewController m_ViewController;
 
         const string testAssetName = "Assets/TmpTests/VFXGraph1.vfx";
+
         const string testSubgraphAssetName = "Assets/TmpTests/VFXGraphSub.vfx";
         const string testSubgraphSubAssetName = "Assets/TmpTests/VFXGraphSub_Subgraph.vfx";
 
+        const string testAssetMainSubgraph = "Assets/TmpTests/VFXGraphSubGraph_Main.vfx";
+        const string testSubgraphSubOperatorAssetName = "Assets/TmpTests/VFXGraphSub_Subgraph.vfxoperator";
+
         private int m_StartUndoGroupId;
 
         [SetUp]
@@ -53,7 +58,10 @@ public void DestroyTestAsset()
             m_ViewController.useCount--;
             Undo.RevertAllDownToGroup(m_StartUndoGroupId);
             AssetDatabase.DeleteAsset(testAssetName);
-            AssetDatabase.DeleteAsset(testAssetName);
+            AssetDatabase.DeleteAsset(testAssetMainSubgraph);
+            AssetDatabase.DeleteAsset(testSubgraphAssetName);
+            AssetDatabase.DeleteAsset(testSubgraphSubAssetName);
+            AssetDatabase.DeleteAsset(testSubgraphSubOperatorAssetName);
         }
 
         #pragma warning disable 0414
@@ -979,6 +987,175 @@ public void Subgraph_Event_Link_To_Spawn()
 
             window.graphView.controller = null;
         }
+
+
+        //Regression test for case 1345426
+        [UnityTest]
+        public IEnumerator ConvertToSubGraphOperator()
+        {
+            var window = VFXViewWindow.GetWindow<VFXViewWindow>();
+
+            VisualEffectAsset asset = VisualEffectAssetEditorUtility.CreateNewAsset(testAssetMainSubgraph);
+            VisualEffectResource resource = asset.GetResource();
+            window.LoadAsset(AssetDatabase.LoadAssetAtPath<VisualEffectAsset>(testAssetMainSubgraph), null);
+
+            var viewController = VFXViewController.GetController(VisualEffectResource.GetResourceAtPath(testAssetMainSubgraph));
+
+            var graph = viewController.graph;
+            var add_A = ScriptableObject.CreateInstance<Operator.Add>();
+            graph.AddChild(add_A);
+            var add_B = ScriptableObject.CreateInstance<Operator.Add>();
+            graph.AddChild(add_B);
+            Assert.IsTrue(add_A.outputSlots[0].Link(add_B.inputSlots[1]));
+
+            //Simple compilable system
+            {
+                var spawnerContext = ScriptableObject.CreateInstance<VFXBasicSpawner>();
+
+                var spawnerInit = ScriptableObject.CreateInstance<VFXBasicInitialize>();
+                var spawnerOutput = ScriptableObject.CreateInstance<VFXPlanarPrimitiveOutput>();
+
+                var blockAttributeDesc = VFXLibrary.GetBlocks().FirstOrDefault(o => o.modelType == typeof(Block.SetAttribute));
+                var blockAttribute = blockAttributeDesc.CreateInstance();
+                blockAttribute.SetSettingValue("attribute", "position");
+                spawnerInit.AddChild(blockAttribute);
+
+                graph.AddChild(spawnerContext);
+                graph.AddChild(spawnerInit);
+                graph.AddChild(spawnerOutput);
+
+                spawnerInit.LinkFrom(spawnerContext);
+                spawnerOutput.LinkFrom(spawnerInit);
+            }
+
+            var initialize = viewController.graph.children.OfType<VFXBasicInitialize>().FirstOrDefault();
+            Assert.IsNotNull(initialize);
+            Assert.IsTrue(initialize.children.Any());
+
+            var allSlot = initialize.children.SelectMany(o => o.inputSlots);
+            var firstVector3 = allSlot.FirstOrDefault(o => o.property.type == typeof(Position));
+            Assert.IsNotNull(firstVector3);
+            Assert.IsTrue(add_B.outputSlots[0].Link(firstVector3[0][0]));
+            Assert.IsTrue(firstVector3.HasLink(true));
+            viewController.LightApplyChanges();
+
+            yield return null;
+
+            var controller = window.graphView.Query<VFXOperatorUI>().ToList().Where(t => t.controller.model is Operator.Add).Select(o => o.controller).Cast<Controller>();
+            Assert.AreEqual(2, controller.Count());
+            VFXConvertSubgraph.ConvertToSubgraphOperator(window.graphView, controller, Rect.zero, testSubgraphSubOperatorAssetName);
+
+            for (int i = 0; i < 32; ++i)
+                yield return null;
+
+            //Check the status of the newly integrated subgraph, expecting one output
+            var subgraph = viewController.graph.children.OfType<VFXSubgraphOperator>().FirstOrDefault();
+            Assert.IsNotNull(subgraph);
+            Assert.AreEqual(1, subgraph.outputSlots.Count);
+            Assert.IsFalse(subgraph.outputSlots.Any(s => s == null));
+
+            //If we reach here without any error or crash, the bug has been fixed
+            window.graphView.controller = null;
+        }
+
+        //Extension of previous test (related to 1345426) : create two outputs in subgraph (instead of one), revert and restore
+        [UnityTest]
+        public IEnumerator ConvertToSubGraphOperator_And_ModifySubgraph()
+        {
+            var previousTest = ConvertToSubGraphOperator();
+            while (previousTest.MoveNext())
+                yield return previousTest.Current;
+
+            var resource = VisualEffectResource.GetResourceAtPath(testSubgraphSubOperatorAssetName);
+            resource.WriteAsset();
+
+            var oneOutputState = File.ReadAllText(testSubgraphSubOperatorAssetName);
+            Assert.IsFalse(string.IsNullOrEmpty(oneOutputState));
+
+            resource = VisualEffectResource.GetResourceAtPath(testSubgraphSubOperatorAssetName);
+            Assert.IsNotNull(resource);
+            var window = VFXViewWindow.GetWindow<VFXViewWindow>();
+            window.LoadResource(resource, null);
+
+            var viewController = window.graphView.controller;
+            var graph = viewController.graph;
+            Assert.IsNotNull(graph);
+
+            var parameter = VFXLibrary.GetParameters().FirstOrDefault(o => o.model.type == typeof(Sphere));
+            Assert.IsNotNull(parameter);
+            var newParam = viewController.AddVFXParameter(Vector2.zero, (VFXModelDescriptorParameters)parameter);
+            newParam.isOutput = true;
+            var otherParamName = "programatically_new_name_test";
+            newParam.SetSettingValue("m_ExposedName", otherParamName);
+            viewController.ApplyChanges();
+
+            resource.WriteAsset();
+
+            yield return null;
+            window.Close();
+
+            var twoOutputState = File.ReadAllText(testSubgraphSubOperatorAssetName);
+            Assert.IsFalse(string.IsNullOrEmpty(twoOutputState));
+            Assert.AreNotEqual(oneOutputState, twoOutputState);
+            Assert.IsTrue(twoOutputState.Contains(otherParamName));
+
+            window.graphView.controller = null;
+            for (int i = 0; i < 16; ++i)
+                yield return null;
+
+            //Check the actual status, should have now two slots
+            {
+                AssetDatabase.ImportAsset(testAssetMainSubgraph);
+                var mainAsset = AssetDatabase.LoadAssetAtPath<VisualEffectAsset>(testAssetMainSubgraph);
+                var mainGraph = mainAsset.GetOrCreateResource().GetOrCreateGraph();
+
+                var subGraph = mainGraph.children.OfType<VFXSubgraphOperator>().FirstOrDefault();
+                Assert.IsNotNull(subGraph);
+                Assert.AreEqual(2, subGraph.outputSlots.Count);
+                Assert.IsFalse(subGraph.outputSlots.Any(o => o == null));
+            }
+            yield return null;
+
+            //Removing old slots, shouldn't get unexpected removed 
+            {
+                File.WriteAllText(testSubgraphSubOperatorAssetName, oneOutputState);
+                AssetDatabase.Refresh();
+
+                AssetDatabase.ImportAsset(testAssetMainSubgraph);
+                var mainAsset = AssetDatabase.LoadAssetAtPath<VisualEffectAsset>(testAssetMainSubgraph);
+                var mainGraph = mainAsset.GetOrCreateResource().GetOrCreateGraph();
+
+                var subGraph = mainGraph.children.OfType<VFXSubgraphOperator>().FirstOrDefault();
+                Assert.IsNotNull(subGraph);
+                Assert.AreEqual(1, subGraph.outputSlots.Count);
+                Assert.IsFalse(subGraph.outputSlots.Any(o => o == null));
+            }
+            yield return null;
+
+            //Here, restore to the previous state, if one outputSlot is null, then we did a resync slot during compilation
+            {
+                File.WriteAllText(testSubgraphSubOperatorAssetName, twoOutputState);
+                AssetDatabase.Refresh();
+
+                AssetDatabase.ImportAsset(testAssetMainSubgraph);
+                var mainAsset = AssetDatabase.LoadAssetAtPath<VisualEffectAsset>(testAssetMainSubgraph);
+                var mainGraph = mainAsset.GetOrCreateResource().GetOrCreateGraph();
+
+                var subGraph = mainGraph.children.OfType<VFXSubgraphOperator>().FirstOrDefault();
+                Assert.IsNotNull(subGraph);
+                Assert.AreEqual(2, subGraph.outputSlots.Count);
+                Assert.IsFalse(subGraph.outputSlots.Any(o => o == null));
+            }
+            yield return null;
+
+            //Finally, open the source asset it will crash if the vfxasset is wrongly formed
+            resource = VisualEffectResource.GetResourceAtPath(testAssetMainSubgraph);
+            window = VFXViewWindow.GetWindow<VFXViewWindow>();
+            window.LoadResource(resource, null);
+
+            for (int i = 0; i < 16; ++i)
+                yield return null;
+        }
     }
 }
 #endif

com.unity.visualeffectgraph/CHANGELOG.md

@@ -75,6 +75,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 - Unexpected operator and block removal during migration [Case 1344645](https://issuetracker.unity3d.com/product/unity/issues/guid/1344645/)
 - Inspector group headers now have a better indentation and alignment
 - 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/)
 
 ## [11.0.0] - 2020-10-21
 ### Added

com.unity.visualeffectgraph/Editor/GraphView/Views/VFXConvertSubgraph.cs

@@ -19,10 +19,10 @@ public static void ConvertToSubgraphContext(VFXView sourceView, IEnumerable<Cont
             ctx.ConvertToSubgraphContext(sourceView, controllers, rect, path);
         }
 
-        public static void ConvertToSubgraphOperator(VFXView sourceView, IEnumerable<Controller> controllers, Rect rect)
+        public static void ConvertToSubgraphOperator(VFXView sourceView, IEnumerable<Controller> controllers, Rect rect, string path = null)
         {
             var ctx = new Context();
-            ctx.ConvertToSubgraphOperator(sourceView, controllers, rect);
+            ctx.ConvertToSubgraphOperator(sourceView, controllers, rect, path);
         }
 
         public static void ConvertToSubgraphBlock(VFXView sourceView, IEnumerable<Controller> controllers, Rect rect)
@@ -251,19 +251,36 @@ public void ConvertToSubgraphContext(VFXView sourceView, IEnumerable<Controller>
                 UninitSmart();
             }
 
-            public void ConvertToSubgraphOperator(VFXView sourceView, IEnumerable<Controller> controllers, Rect rect)
+            public void ConvertToSubgraphOperator(VFXView sourceView, IEnumerable<Controller> controllers, Rect rect, string path)
             {
                 this.m_Rect = rect;
                 Init(sourceView, controllers);
-                if (!CreateUniqueSubgraph("SubgraphOperator", VisualEffectSubgraphOperator.Extension, VisualEffectAssetEditorUtility.CreateNew<VisualEffectSubgraphOperator>))
-                    return;
+                if (path == null)
+                {
+                    if (!CreateUniqueSubgraph("SubgraphOperator", VisualEffectSubgraphOperator.Extension, VisualEffectAssetEditorUtility.CreateNew<VisualEffectSubgraphOperator>))
+                        return;
+                }
+                else
+                {
+                    m_TargetSubgraph = VisualEffectAssetEditorUtility.CreateNew<VisualEffectSubgraphOperator>(path);
+                    m_TargetController = VFXViewController.GetController(m_TargetSubgraph.GetResource());
+                    m_TargetController.useCount++;
+                    m_TargetControllers = new List<VFXNodeController>();
+                }
                 CopyPasteNodes();
                 m_SourceNode = ScriptableObject.CreateInstance<VFXSubgraphOperator>();
                 PostSetupNode();
                 m_SourceControllersWithBlocks = m_SourceControllers.Concat(m_SourceControllers.OfType<VFXContextController>().SelectMany(t => t.blockControllers));
                 TransferEdges();
                 TransfertOperatorOutputEdges();
                 Uninit();
+
+                //The PrepareSubgraphs was initially in compilation
+                //This change has been canceled to prevent creation of model in the wrong place
+                //Be sure the newly created operator has expected slot
+                var subGraphOperator = m_SourceNode as VFXSubgraphOperator;
+                subGraphOperator.RecreateCopy();
+                subGraphOperator.ResyncSlots(true);
             }
 
             List<VFXBlockController> m_SourceBlockControllers;

com.unity.visualeffectgraph/Editor/Models/VFXGraph.cs

@@ -105,7 +105,20 @@ static void OnCompileResource(VisualEffectResource resource)
                     }
                     else
                     {
-                        resource.GetOrCreateGraph().CompileForImport();
+                        //Workaround, use backup system to prevent any modification of the graph during compilation
+                        //The responsible of this unexpected change is PrepareSubgraphs => RecurseSubgraphRecreateCopy => ResyncSlots
+                        //It will let the VFXGraph in a really bad state after compilation.
+                        graph = resource.GetOrCreateGraph();
+                        var dependencies = new HashSet<ScriptableObject>();
+                        dependencies.Add(graph);
+                        graph.CollectDependencies(dependencies);
+                        var backup = VFXMemorySerializer.StoreObjectsToByteArray(dependencies.ToArray(), CompressionLevel.None);
+
+                        graph.CompileForImport();
+
+                        VFXMemorySerializer.ExtractObjects(backup, false);
+                        //The backup during undo/redo is actually calling UnknownChange after ExtractObjects
+                        //You have to avoid because it will call ResyncSlot
                     }
                 }
                 else