fix: Attachables destroy order of operations

com.unity.netcode.gameobjects/CHANGELOG.md

@@ -26,6 +26,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,7 @@ public void Attach(AttachableNode attachableNode)
         /// </summary>
         internal void InternalDetach()
         {
-            if (m_AttachableNode)
+            if (!IsDestroying && m_AttachableNode && (!m_AttachableNode.IsDestroying || m_AttachableNode.gameObject.scene.isLoaded))
             {
                 if (m_DefaultParent)
                 {
@@ -553,12 +563,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 or
+        /// if there is no valid assigned NetworkObject.
+        /// </summary>
+        /// <remarks>
+        /// <see cref="NetworkObject.IsDestroying"/>
+        /// </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()
+        {
+            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

@@ -2709,7 +2709,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);
                 }
             }
         }

com.unity.netcode.gameobjects/Runtime/Spawning/NetworkSpawnManager.cs

@@ -164,10 +164,16 @@ private void RemovePlayerObject(NetworkObject playerObject, bool destroyingObjec
                     m_PlayerObjectsTable.Remove(playerObject.OwnerClientId);
                 }
             }
-
+            // If the client exists locally and we are destroying...
             if (NetworkManager.ConnectionManager.ConnectedClients.ContainsKey(playerObject.OwnerClientId) && destroyingObject)
             {
-                NetworkManager.ConnectionManager.ConnectedClients[playerObject.OwnerClientId].PlayerObject = null;
+                var client = NetworkManager.ConnectionManager.ConnectedClients[playerObject.OwnerClientId];
+                // and the client's currently assigned player object is what is being destroyed...
+                if (client != null && client.PlayerObject == playerObject)
+                {
+                    // then clear out the clients currently assigned player object.
+                    client.PlayerObject = null;
+                }
             }
         }
 
@@ -1380,21 +1386,31 @@ internal void ServerResetShudownStateForSceneObjects()
         }
 
         /// <summary>
-        /// Gets called only by NetworkSceneManager.SwitchScene
+        /// Gets called only by <see cref="NetworkManager.Load"/> and the load scene mode
+        /// is set to <see cref="UnityEngine.SceneManagement.LoadSceneMode.Single"/>.
         /// </summary>
         internal void ServerDestroySpawnedSceneObjects()
         {
-            // This Allocation is "OK" for now because this code only executes when a new scene is switched to
-            // We need to create a new copy the HashSet of NetworkObjects (SpawnedObjectsList) so we can remove
-            // objects from the HashSet (SpawnedObjectsList) without causing a list has been modified exception to occur.
+            // This Allocation is "OK" for now because this code only executes when transitioning to a new scene (i.e. lots of allocations and de-allocations).
+            // We create new copy of the NetworkObjects (SpawnedObjectsList) HashSet so we can remove from the original list as needed.
             var spawnedObjects = SpawnedObjectsList.ToList();
 
-            foreach (var sobj in spawnedObjects)
+            foreach (var networkObject in spawnedObjects)
             {
-                if (sobj.IsSceneObject != null && sobj.IsSceneObject.Value && sobj.DestroyWithScene && sobj.gameObject.scene != NetworkManager.SceneManager.DontDestroyOnLoadScene)
+                if (networkObject.IsSceneObject != null && networkObject.IsSceneObject.Value && networkObject.DestroyWithScene
+                    && networkObject.gameObject.scene != NetworkManager.SceneManager.DontDestroyOnLoadScene)
                 {
-                    SpawnedObjectsList.Remove(sobj);
-                    UnityEngine.Object.Destroy(sobj.gameObject);
+                    if (networkObject.IsSpawned && networkObject.HasAuthority)
+                    {
+                        networkObject.Despawn(false);
+                    }
+                    else // Non-authority objects should just be destroyed (i.e. DAHost)
+                    {
+                        // Mark the object and associated NetworkBehaviours as in the process (or will be) destroyed.
+                        networkObject.SetIsDestroying();
+                        UnityEngine.Object.Destroy(networkObject.gameObject);
+                        SpawnedObjectsList.Remove(networkObject);
+                    }
                 }
             }
         }
@@ -1630,6 +1646,10 @@ internal void OnDespawnObject(NetworkObject networkObject, bool destroyGameObjec
                 }
             }
 
+            if (destroyGameObject)
+            {
+                networkObject.SetIsDestroying();
+            }
             networkObject.InvokeBehaviourNetworkDespawn();
 
             // Whether we are in distributedAuthority mode and have authority on this object