fix: Attachables destroy order of operations (#3931)

* fix

Fixing some issues based on varying destroy order of operations (more recent change where instantiation order does not reflect the order in which they will be destroyed via SceneManager during an in-session scene load (single mode).

* test

Adding the test for this fix.

* fix

The actual fix for the issue with additional handling when an attachable's NetworkObject is destroyed while it  is still attached to something else.

* update & style

Adding change log entry.
Fixing some typos.

* style

removing white space.

* style

fixing spelling typo.

* style-pvp

Adding inheritdoc

* refactor

Going ahead and adding an (currently) internal "IsDestroying" flag that is set only when the NetworkObject.OnDestroy method is invoked.

* update

Applying suggested adjustment to logic within OnNetworkPreDespawn.

* style

adding a whitespace

* refactor and fix

This includes some additional edge case fixes... specifically for the scenario where we know a scene is being unloaded that will result in either an `AttachableNode` or `AttachableBehaviour` being destroyed and they are attached.

* style

Removing comment that was just mistakenly left in.

* update + fix

Fixing an edge case that requires manually testing due to how integration tests additively load everything to avoid unloading the test runner test scene when loading a scene.
Two parts:
- When a server is destroying spawned scene objects due to a single mode scene load, it should also include a check for this and handle marking the NetworkObject and associated NetworkBehaviours as being destroyed and then destroy the object.
- Just prior to handling a scene loading event, we need to mark anything that will be destroyed (i.e. destroy with scene) as being destroyed when migrating spawned objects into the DDOL.

* style

Updating some comments based on AI suggestions.

* style

UnityEngine namespace included and no longer needed to prefix.

Author: NoelStephensUnity

com.unity.netcode.gameobjects/CHANGELOG.md

@@ -29,6 +29,7 @@ Additional documentation and release notes are available at [Multiplayer Documen
 
 ### Fixed
 
+- Fixed issue where either an `AttachableBehaviour` or an `AttachableNode` can throw an exception if they are attached during a scene unload where one of the two persists the scene unload event and the other does not. (#3931)
 - Fixed issue where attempts to use `NetworkLog` when there is no `NetworkManager` instance would result in an exception. (#3917)
 
 ### Security

com.unity.netcode.gameobjects/Runtime/Components/Helpers/AttachableBehaviour.cs

@@ -261,23 +261,30 @@ internal void ForceDetach()
             ForceComponentChange(false, true);
 
             InternalDetach();
-            // Notify of the changed attached state
-            NotifyAttachedStateChanged(m_AttachState, m_AttachableNode);
-
-            m_AttachedNodeReference = new NetworkBehaviourReference(null);
 
-            // When detaching, we want to make our final action
-            // the invocation of the AttachableNode's Detach method.
-            if (m_AttachableNode)
+            if (m_AttachableNode != null && !m_AttachableNode.IsDestroying)
             {
+                // Notify of the changed attached state
+                NotifyAttachedStateChanged(m_AttachState, m_AttachableNode);
+                // Only notify of the detach if the node is still valid.
                 m_AttachableNode.Detach(this);
-                m_AttachableNode = null;
             }
+
+            m_AttachedNodeReference = new NetworkBehaviourReference(null);
+            m_AttachableNode = null;
         }
 
         /// <inheritdoc/>
         public override void OnNetworkPreDespawn()
         {
+            // If the NetworkObject is being destroyed and not completely detached, then destroy the GameObject for
+            // this attachable since the associated default parent is being destroyed.
+            if (IsDestroying && m_AttachState != AttachState.Detached)
+            {
+                Destroy(gameObject);
+                return;
+            }
+
             if (NetworkManager.ShutdownInProgress || AutoDetach.HasFlag(AutoDetachTypes.OnDespawn))
             {
                 ForceDetach();
@@ -286,7 +293,7 @@ public override void OnNetworkPreDespawn()
         }
 
         /// <summary>
-        /// This will apply the final attach or detatch state based on the current value of <see cref="m_AttachedNodeReference"/>.
+        /// This will apply the final attach or detach state based on the current value of <see cref="m_AttachedNodeReference"/>.
         /// </summary>
         [MethodImpl(MethodImplOptions.AggressiveInlining)]
         private void UpdateAttachedState()
@@ -304,16 +311,18 @@ private void UpdateAttachedState()
                 return;
             }
 
-            // If we are attached to some other AttachableNode, then detach from that before attaching to a new one.
+            // If we are attaching and already attached to some other AttachableNode,
+            // then detach from that before attaching to a new one.
             if (isAttaching && m_AttachableNode != null && m_AttachState == AttachState.Attached)
             {
-                // Run through the same process without being triggerd by a NetVar update.
+                // Detach the current attachable
                 NotifyAttachedStateChanged(AttachState.Detaching, m_AttachableNode);
                 InternalDetach();
                 NotifyAttachedStateChanged(AttachState.Detached, m_AttachableNode);
-
                 m_AttachableNode.Detach(this);
                 m_AttachableNode = null;
+
+                // Now attach the new attachable
             }
 
             // Change the state to attaching or detaching
@@ -392,7 +401,8 @@ internal void ForceComponentChange(bool isAttaching, bool forcedChange)
 
             foreach (var componentControllerEntry in ComponentControllers)
             {
-                if (componentControllerEntry.AutoTrigger.HasFlag(triggerType))
+                // Only if the component controller still exists and has the appropriate flag.
+                if (componentControllerEntry.ComponentController && componentControllerEntry.AutoTrigger.HasFlag(triggerType))
                 {
                     componentControllerEntry.ComponentController.ForceChangeEnabled(componentControllerEntry.EnableOnAttach ? isAttaching : !isAttaching, forcedChange);
                 }
@@ -457,7 +467,9 @@ public void Attach(AttachableNode attachableNode)
         /// </summary>
         internal void InternalDetach()
         {
-            if (m_AttachableNode)
+            // If this instance is not in the middle of being destroyed, the attachable node is not null, and the node is not destroying
+            // =or= the scene it is located in is in the middle of being unloaded, then re-parent under the default parent.
+            if (!IsDestroying && m_AttachableNode && (!m_AttachableNode.IsDestroying || m_AttachableNode.gameObject.scene.isLoaded))
             {
                 if (m_DefaultParent)
                 {
@@ -553,12 +565,33 @@ private void UpdateAttachStateRpc(NetworkBehaviourReference attachedNodeReferenc
         /// </summary>
         internal void OnAttachNodeDestroy()
         {
-            // If this instance should force a detach on destroy
-            if (AutoDetach.HasFlag(AutoDetachTypes.OnAttachNodeDestroy))
+            // We force a detach on destroy if there is a flag =or= if we are attached to a node that is being destroyed.
+            if (AutoDetach.HasFlag(AutoDetachTypes.OnAttachNodeDestroy) ||
+                (AutoDetach.HasFlag(AutoDetachTypes.OnDespawn) && m_AttachState == AttachState.Attached && m_AttachableNode && m_AttachableNode.IsDestroying))
+            {
+                ForceDetach();
+            }
+        }
+
+
+        /// <summary>
+        /// When we know this instance is being destroyed or will be destroyed
+        /// by something outside of NGO's realm of control, this gets invoked.
+        /// We should detach from any AttachableNode when this is invoked.
+        /// </summary>
+        protected internal override void OnIsDestroying()
+        {
+            // If we are not already marked as being destroyed, attached, this instance is the authority instance, and the node we are attached
+            // to is not in the middle of being destroyed...detach normally.
+            if (!IsDestroying && HasAuthority && m_AttachState == AttachState.Attached && m_AttachableNode && !m_AttachableNode.IsDestroying)
+            {
+                Detach();
+            }
+            else // Otherwise force the detach.
             {
-                // Force a detach
                 ForceDetach();
             }
+            base.OnIsDestroying();
         }
     }
 }

com.unity.netcode.gameobjects/Runtime/Components/Helpers/AttachableNode.cs

@@ -71,20 +71,20 @@ public override void OnNetworkPreDespawn()
         {
             for (int i = m_AttachedBehaviours.Count - 1; i >= 0; i--)
             {
-                if (!m_AttachedBehaviours[i])
+                var attachable = m_AttachedBehaviours[i];
+                if (!attachable)
                 {
                     continue;
                 }
-                // If we don't have authority but should detach on despawn,
-                // then proceed to detach.
-                if (!m_AttachedBehaviours[i].HasAuthority)
+
+                if (attachable.HasAuthority && attachable.IsSpawned)
                 {
-                    m_AttachedBehaviours[i].ForceDetach();
+                    // Detach the normal way with authority
+                    attachable.Detach();
                 }
-                else
+                else if (!attachable.HasAuthority || !attachable.IsDestroying)
                 {
-                    // Detach the normal way with authority
-                    m_AttachedBehaviours[i].Detach();
+                    attachable.ForceDetach();
                 }
             }
         }
@@ -93,12 +93,10 @@ public override void OnNetworkPreDespawn()
 
     internal override void InternalOnDestroy()
     {
-        // Notify any attached behaviours that this node is being destroyed.
-        for (int i = m_AttachedBehaviours.Count - 1; i >= 0; i--)
+        if (m_AttachedBehaviours.Count > 0)
         {
-            m_AttachedBehaviours[i]?.OnAttachNodeDestroy();
+            OnIsDestroying();
         }
-        m_AttachedBehaviours.Clear();
         base.InternalOnDestroy();
     }
 
@@ -141,4 +139,21 @@ internal void Detach(AttachableBehaviour attachableBehaviour)
         m_AttachedBehaviours.Remove(attachableBehaviour);
         OnDetached(attachableBehaviour);
     }
+
+    /// <summary>
+    /// When we know this instance is being destroyed or will be destroyed
+    /// by something outside of NGO's realm of control, this gets invoked.
+    /// We should notify any attached AttachableBehaviour that this node
+    /// is being destroyed.
+    /// </summary>
+    protected internal override void OnIsDestroying()
+    {
+        // Notify any attached behaviours that this node is being destroyed.
+        for (int i = m_AttachedBehaviours.Count - 1; i >= 0; i--)
+        {
+            m_AttachedBehaviours[i]?.OnAttachNodeDestroy();
+        }
+        m_AttachedBehaviours.Clear();
+        base.OnIsDestroying();
+    }
 }

com.unity.netcode.gameobjects/Runtime/Core/NetworkBehaviour.cs

@@ -643,6 +643,42 @@ protected NetworkBehaviour GetNetworkBehaviour(ushort behaviourId)
         /// </summary>
         public ulong OwnerClientId { get; internal set; }
 
+        /// <summary>
+        /// Returns true if the NetworkObject is in the middle of being destroyed.
+        /// </summary>
+        /// <remarks>
+        /// <see cref="SetIsDestroying"/>
+        /// </remarks>
+        internal bool IsDestroying { get; private set; }
+
+        /// <summary>
+        /// This provides us with a way to track when something is in the middle
+        /// of being destroyed or will be destroyed by something like SceneManager.
+        /// </summary>
+        protected internal virtual void OnIsDestroying()
+        {
+        }
+
+        /// <summary>
+        /// Invoked by <see cref="NetworkObject.SetIsDestroying"/>.
+        /// </summary>
+        /// <remarks>
+        /// We want to invoke the virtual method prior to setting the
+        /// IsDestroying flag to be able to distinguish between knowing
+        /// when something will be destroyed (i.e. scene manager unload
+        /// or load in single mode) or is in the middle of being
+        /// destroyed.
+        /// Setting the flag provides a way for other instances or internals
+        /// to determine if this <see cref="NetworkBehaviour"/> instance is
+        /// in the middle of being destroyed.
+        /// </remarks>
+        internal void SetIsDestroying()
+        {
+            // We intentionally invoke this before setting the IsDestroying flag.
+            OnIsDestroying();
+            IsDestroying = true;
+        }
+
         /// <summary>
         /// Updates properties with network session related
         /// dependencies such as a NetworkObject's spawned

com.unity.netcode.gameobjects/Runtime/Core/NetworkObject.cs

@@ -1688,8 +1688,52 @@ public static void NetworkHide(List<NetworkObject> networkObjects, ulong clientI
             }
         }
 
+        /// <summary>
+        /// Returns true if the NetworkObject is in the middle of being destroyed.
+        /// </summary>
+        /// <remarks>
+        /// This is particularly useful when determining if something is being de-spawned
+        /// normally or if it is being de-spawned because the NetworkObject/GameObject is
+        /// being destroyed.
+        /// </remarks>
+        internal bool IsDestroying { get; private set; }
+
+        /// <summary>
+        /// Applies the despawning flag for the local instance and
+        /// its child NetworkBehaviours. Private to assure this is
+        /// only invoked from within OnDestroy.
+        /// </summary>
+        internal void SetIsDestroying()
+        {
+            if (IsDestroying)
+            {
+                return;
+            }
+
+            if (m_ChildNetworkBehaviours != null)
+            {
+                foreach (var childBehaviour in m_ChildNetworkBehaviours)
+                {
+                    // Just ignore and continue processing through the entries
+                    if (!childBehaviour)
+                    {
+                        continue;
+                    }
+
+                    // Keeping the property a private set to assure this is
+                    // the only way it can be set as it should never be reset
+                    // back to false once invoked.
+                    childBehaviour.SetIsDestroying();
+                }
+            }
+            IsDestroying = true;
+        }
+
         private void OnDestroy()
         {
+            // Apply the is destroying flag
+            SetIsDestroying();
+
             var networkManager = NetworkManager;
             // If no NetworkManager is assigned, then just exit early
             if (!networkManager)

com.unity.netcode.gameobjects/Runtime/SceneManagement/DefaultSceneManagerHandler.cs

@@ -318,10 +318,19 @@ public void MoveObjectsFromSceneToDontDestroyOnLoad(ref NetworkManager networkMa
                 }
                 else if (networkObject.HasAuthority)
                 {
+                    // We know this instance is going to be destroyed (when it receives the destroy object message).
+                    // We have to invoke this prior to invoking despawn in order to know that we are de-spawning in
+                    // preparation of being destroyed by the SceneManager.
+                    networkObject.SetIsDestroying();
+                    // This sends a de-spawn message prior to the scene event.
                     networkObject.Despawn();
                 }
                 else // We are a client, migrate the object into the DDOL temporarily until it receives the destroy command from the server
                 {
+                    // We know this instance is going to be destroyed (when it receives the destroy object message).
+                    // We have to invoke this prior to invoking despawn in order to know that we are de-spawning in
+                    // preparation of being destroyed by the SceneManager.
+                    networkObject.SetIsDestroying();
                     UnityEngine.Object.DontDestroyOnLoad(networkObject.gameObject);
                 }
             }

com.unity.netcode.gameobjects/Runtime/SceneManagement/NetworkSceneManager.cs

@@ -2720,7 +2720,11 @@ internal void MoveObjectsToDontDestroyOnLoad()
                 }
                 else if (networkObject.HasAuthority)
                 {
-                    networkObject.Despawn();
+                    networkObject.SetIsDestroying();
+                    var isSceneObject = networkObject.IsSceneObject;
+                    // Only destroy non-scene placed NetworkObjects to avoid warnings about destroying in-scene placed NetworkObjects.
+                    // (MoveObjectsToDontDestroyOnLoad is only invoked during a scene event type of load and the load scene mode is single)
+                    networkObject.Despawn(isSceneObject.HasValue && isSceneObject.Value == false);
                 }
             }
         }