[VFX][Fix] Bounds helper feature polish (#263)

* Fix Error refresh not happening: set parent-child relation on data when pasting

* Use VFXEnumField for unified appearance of enums

* Added null check (silent but throw exception when debugging)

* Changes cull mode when system is Automatic

* Display warning when switching to Auto

* Set Bounds Mode of templates to Recorded + Pre-recorded bounds

* missing template

* ComputedBounds are always in local space

* Padding does not get accumulated

* fix tooltip + changelog

* More correct way of setting the graph as parent to the data

The previous fix made targetData become a child of the VFXGraph, whereas currently it is an assymetrical relationship between VFXData and VFXGraph

* Fix NullRefException when deleting VFX asset when attached

* remove unnecessary null check

* Add note about Automatic systems and culling flag

* Avoids having bounds converted to infinity

* Restore grey out when Automatic

First() so it enters the catch statement if "bounds" doesn't exist

* != operator failed, causing infinite recompile

* fix effect after merge

* Fix wrong old way of changing setting value

* Attributes --> VFXAttributes

* Remove try/catch + HasLink()

Author: ludovic-theobald

com.unity.render-pipelines.high-definition/Editor/VFXGraph/Outputs/VFXLitMeshOutput.cs

@@ -130,7 +130,7 @@ protected override void GenerateErrors(VFXInvalidateErrorReporter manager)
         {
             base.GenerateErrors(manager);
             var dataParticle = GetData() as VFXDataParticle;
-            if (dataParticle != null && dataParticle.boundsSettingMode != BoundsSettingMode.Manual)
+            if (dataParticle != null && dataParticle.boundsMode != BoundsSettingMode.Manual)
                 manager.RegisterError("WarningBoundsComputation", VFXErrorType.Warning, $"Bounds computation have no sense of what the scale of the output mesh is," +
                     $" so the resulted computed bounds can be too small or big" +
                     $" Please use padding to mitigate this discrepancy.");

com.unity.visualeffectgraph/CHANGELOG.md

@@ -124,6 +124,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 - Fix culling of point output [Case 1225764](https://issuetracker.unity3d.com/product/unity/issues/guid/1225764/)
 - Compilation issue when normal is used in shadergraph for opacity with unlit output
 - Fix Exception on trying to invert a degenerate TRS matrix [Case 1307068](https://issuetracker.unity3d.com/product/unity/issues/guid/1307068/)
+- Fix bounds helper tool (automatic systems culling, world bounds computation, ...)
 - Fix IsFrontFace shader graph node for VFX.
 - Fix crash when loading SDF Baker settings holding a mesh prefab [Case 1343898](https://issuetracker.unity3d.com/product/unity/issues/guid/1343898/)
 - Exception using gizmo on exposed properties [Case 1340818](https://issuetracker.unity3d.com/product/unity/issues/guid/1340818/)

com.unity.visualeffectgraph/Documentation~/visual-effect-bounds.md

@@ -8,7 +8,7 @@ Each System in a visual effect defines its bounds in the [Initialize Context](Co
 
 - **Manual**: You set the bounds directly in the Initialize Context. You can calculate the bounds dynamically using Operators and send the output to the Initialize Context's **Bounds** input ports.
 - **Recorded**: Allows you to record the System from the Target visual effect GameObject panel. For information on how to do this, see [Bounds Recording](#bounds-recording). In this mode, you can also calculate the bounds using Operators and pass them to the Initialize Context, like in **Manual**. This overrides any recorded bounds.
-- **Automatic**: Unity calculates the bounds automatically.
+- **Automatic**: Unity calculates the bounds automatically. Note: This will force the culling flags of the VFX asset to "Always recompute bounds and simulate". 
 
 The Initialize Context also contains a **Bounds Padding** input port. This is a Vector3 that enlarges the per-axis bounds of the System. If a System uses **Recorded** or **Automatic** bounds, Unity calculates the bounds of the System during the Update Context. This means that any changes to the size, position, scale, or pivot of particles that occur in the Output Context don't affect the bounds during that frame. Adding padding to the bounds helps to mitigate this effect.
 

com.unity.visualeffectgraph/Editor/Compiler/VFXGraphCompiledData.cs

@@ -43,7 +43,8 @@ class VFXDependentBuffersData
     class VFXGraphCompiledData
     {
         // 3: Serialize material
-        public const uint compiledVersion = 3;
+        // 4: Bounds helper change
+        public const uint compiledVersion = 4;
 
         public VFXGraphCompiledData(VFXGraph graph)
         {

com.unity.visualeffectgraph/Editor/Data/VFXDataParticle.cs

@@ -223,10 +223,11 @@ internal enum DataType
 
         public bool NeedsComputeBounds() => needsComputeBounds;
 
+        [FormerlySerializedAs("boundsSettingMode")]
         [VFXSetting(VFXSettingAttribute.VisibleFlags.All),
          Tooltip("Specifies how the bounds are set. They can be set manually, recorded in the Target GameObject window, or computed automatically at a small performance cost."),
          SerializeField]
-        public BoundsSettingMode boundsSettingMode = BoundsSettingMode.Recorded;
+        public BoundsSettingMode boundsMode = BoundsSettingMode.Recorded;
 
         public bool hasStrip { get { return dataType == DataType.ParticleStrip; } }
 
@@ -240,19 +241,22 @@ public override void OnSettingModified(VFXSetting setting)
                 stripCapacity = 1;
             else if (setting.name == "particlePerStripCount" && particlePerStripCount == 0)
                 particlePerStripCount = 1;
-            else if (setting.name == "boundsSettingMode")
+            else if (setting.name == "boundsMode")
             {
                 //Refresh errors on Output contexts
                 var allSystemOutputContexts = owners.Where(ctx => ctx is VFXAbstractParticleOutput);
                 foreach (var ctx in allSystemOutputContexts)
                 {
                     ctx.RefreshErrors(GetGraph());
                 }
-                if (boundsSettingMode == BoundsSettingMode.Automatic)
+
+                if (boundsMode == BoundsSettingMode.Automatic)
+                {
                     needsComputeBounds = true;
+                    var graph = GetGraph();
+                    graph.visualEffectResource.cullingFlags = VFXCullingFlags.CullNone;
+                }
             }
-
-
             if (hasStrip)
             {
                 if (setting.name == "dataType") // strip has just been set
@@ -674,15 +678,15 @@ public override void FillDescs(
                 systemFlag |= VFXSystemFlag.SystemHasDirectLink;
             }
 
-            if (needsComputeBounds || boundsSettingMode == BoundsSettingMode.Automatic)
+            if (needsComputeBounds || boundsMode == BoundsSettingMode.Automatic)
             {
                 systemFlag |= VFXSystemFlag.SystemNeedsComputeBounds;
 
                 boundsBufferIndex = dependentBuffers.boundsBuffers[this];
                 systemBufferMappings.Add(new VFXMapping("boundsBuffer", boundsBufferIndex));
             }
 
-            if (boundsSettingMode == BoundsSettingMode.Automatic)
+            if (boundsMode == BoundsSettingMode.Automatic)
             {
                 systemFlag |= VFXSystemFlag.SystemAutomaticBounds;
             }
@@ -1020,7 +1024,7 @@ public override void Sanitize(int version)
         {
             if (version < 8)
             {
-                SetSettingValue("boundsSettingMode", BoundsSettingMode.Manual);
+                SetSettingValue("boundsMode", BoundsSettingMode.Manual);
             }
             base.Sanitize(version);
         }

com.unity.visualeffectgraph/Editor/GraphView/BoundsRecorder/VFXBoundsRecorder.cs

@@ -100,7 +100,7 @@ VFXDataParticle GetSystem(string systemName)
 
         public BoundsSettingMode GetSystemBoundsSettingMode(string systemName)
         {
-            return GetSystem(systemName).boundsSettingMode;
+            return GetSystem(systemName).boundsMode;
         }
 
         public VFXBoundsRecorder(VisualEffect effect, VFXView view)
@@ -176,7 +176,7 @@ bool NeedsToBeRecorded(VFXDataParticle system)
                 return false;
             }
             var boundsSlot = initContext.inputSlots.FirstOrDefault(s => s.name == "bounds");
-            return system.boundsSettingMode == BoundsSettingMode.Recorded && !boundsSlot.AllChildrenWithLink().Any();
+            return system.boundsMode == BoundsSettingMode.Recorded && !boundsSlot.AllChildrenWithLink().Any();
         }
 
         bool NeedsToBeRecorded(VFXDataParticle system, out ExclusionCause cause)
@@ -195,16 +195,16 @@ bool NeedsToBeRecorded(VFXDataParticle system, out ExclusionCause cause)
             VFXSlot boundsSlot;
             try
             {
-                boundsSlot = initContext.inputSlots.FirstOrDefault(s => s.name == "bounds");
-                if (boundsSlot.AllChildrenWithLink().Any())
+                boundsSlot = initContext.inputSlots.First(s => s.name == "bounds");
+                if (boundsSlot != null && boundsSlot.HasLink(true))
                 {
                     cause = ExclusionCause.kGraphComputed;
                     return false;
                 }
             }
             catch
             {
-                if (system.boundsSettingMode == BoundsSettingMode.Automatic)
+                if (system.boundsMode == BoundsSettingMode.Automatic)
                 {
                     cause = ExclusionCause.kAutomatic;
                     return false;
@@ -213,7 +213,7 @@ bool NeedsToBeRecorded(VFXDataParticle system, out ExclusionCause cause)
                 return false;
             }
 
-            if (system.boundsSettingMode == BoundsSettingMode.Manual)
+            if (system.boundsMode == BoundsSettingMode.Manual)
             {
                 cause = ExclusionCause.kManual;
                 return false;
@@ -233,7 +233,7 @@ void OnParamSystemModified(VFXModel model, VFXModel.InvalidationCause cause)
                     if (model is VFXSlot)
                     {
                         var slot = model as VFXSlot;
-                        if (slot.name == "bounds")
+                        if (slot.name == "bounds" || slot.name == "boundsPadding")
                             return;
                         foreach (var data in GetAffectedData(slot))
                         {
@@ -309,11 +309,8 @@ void UpdateBounds()
                         Bounds currentBounds = m_Effect.GetComputedBounds(systemName);
                         if (currentBounds.size == Vector3.zero)
                             continue;
-                        if (system.space == VFXCoordinateSpace.World)
-                        {
-                            Matrix4x4 worldToLocal = m_Effect.transform.worldToLocalMatrix;
-                            currentBounds = TransformAABBSlow(currentBounds, worldToLocal);
-                        }
+                        var padding = m_Effect.GetCurrentPadding(systemName);
+                        currentBounds.extents -= padding;
                         if (m_FirstBound[systemName])
                         {
                             m_Bounds[systemName] = currentBounds;
@@ -332,7 +329,6 @@ void UpdateBounds()
 
         void RenderBounds(SceneView sv)
         {
-            //TODO : Render all bounds when the selected system is not "Recorded"
             if (m_IsRecording && m_Effect.gameObject.activeSelf)
             {
                 bool renderAllRecordedBounds = false;
@@ -376,8 +372,13 @@ void RenderBounds(SceneView sv)
                         continue;
                     }
 
-                    if ((selectedSystems.Contains(systemName) || renderAllRecordedBounds) && m_Bounds.ContainsKey(systemName) && NeedsToBeRecorded(system))
-                        RenderBoundsSystem(m_Bounds[systemName]);
+                    if ((selectedSystems.Contains(systemName) || renderAllRecordedBounds) &&
+                        m_Bounds.ContainsKey(systemName) && NeedsToBeRecorded(system))
+                    {
+                        var padding = m_Effect.GetCurrentPadding(systemName);
+                        var paddedBounds = new Bounds(m_Bounds[systemName].center, 2 * (m_Bounds[systemName].extents + padding));
+                        RenderBoundsSystem(paddedBounds);
+                    }
                 }
             }
         }
@@ -431,24 +432,10 @@ private Vector3[] ExtractVerticesFromBounds(Bounds bounds)
             return points;
         }
 
-        private Bounds TransformAABBSlow(Bounds bounds, Matrix4x4 transform)
-        {
-            Vector3[] v = ExtractVerticesFromBounds(bounds);
-            Bounds transformed = new Bounds(transform.MultiplyPoint(v[0]), Vector3.zero);
-
-            for (int i = 1; i < 8; i++)
-            {
-                Vector3 point = transform.MultiplyPoint(v[i]);
-                transformed.Encapsulate(point);
-            }
-            return transformed;
-        }
-
         public void ModifyMode(string syst, BoundsSettingMode mode)
         {
             VFXDataParticle system = GetSystem(syst);
-            system.boundsSettingMode = mode;
-            system.SetSettingValue("boundsSettingMode", mode);
+            system.SetSettingValue("boundsMode", mode);
         }
 
         public void ToggleRecording()
@@ -496,13 +483,16 @@ public void ApplyCurrentBounds()
                         break;
                     }
                     var boundsSlot = initContext.inputSlots.FirstOrDefault(s => s.name == "bounds");
-                    if (initContext.GetOutputSpaceFromSlot(boundsSlot) == VFXCoordinateSpace.Local) //This should always be the case
-                        boundsSlot.value = new AABox() { center = m_Bounds[systemName].center, size = m_Bounds[systemName].size };
+                    // if (initContext.GetOutputSpaceFromSlot(boundsSlot) == VFXCoordinateSpace.Local) //TODO: Investigate why use space instead of GetOutputSpaceFromSlot
+                    var padding = m_Effect.GetCurrentPadding(systemName);
+                    var paddedBounds = new Bounds(m_Bounds[systemName].center, 2 * (m_Bounds[systemName].extents + padding));
+                    if (boundsSlot.space == VFXCoordinateSpace.Local)
+                        boundsSlot.value = new AABox() { center = paddedBounds.center, size = paddedBounds.size };
                     else
                     {
-                        var localToWorld = m_Effect.transform.localToWorldMatrix;
-                        var transformedBounds = TransformAABBSlow(m_Bounds[systemName], localToWorld);
-                        boundsSlot.value = new AABox() { center = transformedBounds.center, size = transformedBounds.size };
+                        //Subject to change depending on the future behavior of AABox w.r.t. to Spaceable
+                        var positionWorld = m_Effect.transform.TransformPoint(paddedBounds.center);
+                        boundsSlot.value = new AABox() { center = positionWorld, size = paddedBounds.size };
                     }
                 }
             }

com.unity.visualeffectgraph/Editor/GraphView/BoundsRecorder/VFXBoundsRecorderField.cs

@@ -153,7 +153,7 @@ public void Select(VisualElement selectionContainer, bool additive)
                         selection.ClearSelection();
                         selection.AddToSelection(this);
                     }
-                    else // prevent heterogenous selections between stack child nodes and other nodes
+                    else
                     {
                         selection.AddToSelection(this);
                     }

com.unity.visualeffectgraph/Editor/GraphView/VFXComponentBoard.cs

@@ -6,7 +6,7 @@
 using UnityEditor.Experimental.GraphView;
 using UnityEditor.SceneManagement;
 using UnityEditor.UIElements;
-
+using UnityEditor.VFX.UIElements;
 using UnityEngine;
 using UnityEngine.VFX;
 using UnityEngine.UIElements;
@@ -361,7 +361,7 @@ void DeleteBoundsRecorder()
 
         void UpdateBoundsRecorder()
         {
-            if (m_AttachedComponent != null)
+            if (m_AttachedComponent != null && m_View.controller.graph != null)
             {
                 controller.RecompileExpressionGraphIfNeeded();
                 bool wasRecording = false;
@@ -932,10 +932,7 @@ public void Setup(string systemName, VFXBoundsRecorder boundsRecorder)
             var initContextUI = m_BoundsRecorder.GetInitContextController(m_SystemName);
             m_SystemNameButton.Setup(initContextUI, m_BoundsRecorder.view);
             m_SystemNameButton.text = m_SystemName;
-            m_BoundsMode = this.Query<Button>("bounds-mode");
-            m_BoundsMode.AddStyleSheetPathWithSkinVariant("VFXControls");
-            m_BoundsMode.clickable.clicked += OnBoundsModeMenu;
-            m_BoundsMode.text = m_BoundsRecorder.GetSystemBoundsSettingMode(systemName).ToString();
+            InitBoundsModeElement();
             m_Colors = new Dictionary<string, StyleColor>()
             {
                 {"included", m_SystemNameButton.style.color},
@@ -952,23 +949,22 @@ public void Setup(string systemName, VFXBoundsRecorder boundsRecorder)
             }
         }
 
-        private List<string> m_BoundsModes = new List<string> { "Manual", "Recorded", "Automatic" };
-
-        void OnBoundsModeMenu()
+        void InitBoundsModeElement()
         {
-            GenericMenu menu = new GenericMenu();
-            foreach (var mode in Enum.GetValues(typeof(BoundsSettingMode)))
-            {
-                bool IsOn = (BoundsSettingMode)mode == m_CurrentMode;
-                menu.AddItem(BoundsSystemContents.modesContent[(BoundsSettingMode)mode], IsOn, SetSystemBoundMode, mode);
-            }
-            menu.DropDown(m_BoundsMode.worldBound);
+            m_BoundsMode = new VFXEnumField(s_EmptyEnumLabel, typeof(BoundsSettingMode));
+            m_BoundsMode.OnValueChanged += OnValueChanged;
+            m_BoundsMode.SetValue((int)m_CurrentMode);
+            m_BoundsMode.AddToClassList("bounds-mode");
+            Add(m_BoundsMode);
         }
 
+        private List<string> m_BoundsModes = new List<string> {"Manual", "Recorded", "Automatic"};
+
         public void UpdateLabel()
         {
             m_CurrentMode = m_BoundsRecorder.GetSystemBoundsSettingMode(m_SystemName);
-            m_BoundsMode.text = m_CurrentMode.ToString();
+            m_BoundsMode.SetValue((int)m_CurrentMode);
+            OnValueChanged();
             if (!m_BoundsRecorder.NeedsToBeRecorded(m_SystemName, out VFXBoundsRecorder.ExclusionCause cause))
             {
                 m_SystemNameButton.text = $"{m_SystemName} {VFXBoundsRecorder.exclusionCauseString[cause]}";
@@ -994,10 +990,19 @@ public bool HasSystemBeenRenamed()
         void SetSystemBoundMode(object mode)
         {
             m_CurrentMode = (BoundsSettingMode)mode;
-            m_BoundsMode.text = mode.ToString();
+            m_BoundsMode.SetValue((int)mode);
             m_BoundsRecorder.ModifyMode(m_SystemName, (BoundsSettingMode)mode);
         }
 
+        void OnValueChanged()
+        {
+            if (m_CurrentMode != (BoundsSettingMode)m_BoundsMode.value)
+            {
+                m_CurrentMode = (BoundsSettingMode)m_BoundsMode.value;
+                m_BoundsRecorder.ModifyMode(m_SystemName, m_CurrentMode);
+            }
+        }
+
         public void ReleaseBoundsRecorder()
         {
             m_BoundsRecorder.isRecording = false;
@@ -1011,10 +1016,11 @@ public bool Unselect()
 
         string m_SystemName;
         VFXBoundsRecorderField m_SystemNameButton;
-        Button m_BoundsMode;
+        VFXEnumField m_BoundsMode;
         BoundsSettingMode m_CurrentMode;
         VFXBoundsRecorder m_BoundsRecorder;
         Dictionary<string, StyleColor> m_Colors;
+        private static Label s_EmptyEnumLabel = new Label();
 
         static class BoundsSystemContents
         {

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

@@ -694,6 +694,10 @@ private void PasteDatas(ref SerializableGraph serializableGraph)
                         serializableGraph.contexts[i].dataIndex >= 0)
                     {
                         var data = serializableGraph.datas[serializableGraph.contexts[i].dataIndex];
+
+                        //At this stage, the context has the VFXGraph as its parent, so it can create a properly parented VFXData
+                        contextController.model.SetDefaultData(false);
+
                         VFXData targetData = contextController.model.GetData();
                         if (targetData != null)
                         {