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.

Author: NoelStephensUnity

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

@@ -261,18 +261,17 @@ 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/>
@@ -312,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
@@ -466,7 +467,7 @@ public void Attach(AttachableNode attachableNode)
         /// </summary>
         internal void InternalDetach()
         {
-            if (!IsDestroying && m_AttachableNode && !m_AttachableNode.IsDestroying)
+            if (!IsDestroying && m_AttachableNode && (!m_AttachableNode.IsDestroying || m_AttachableNode.gameObject.scene.isLoaded))
             {
                 if (m_DefaultParent)
                 {
@@ -562,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

@@ -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

@@ -652,8 +652,31 @@ protected NetworkBehaviour GetNetworkBehaviour(ushort behaviourId)
         /// </remarks>
         internal bool IsDestroying { get; private set; }
 
-        internal void SetDestroying()
+        /// <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.
+        /// root <see cref="GameObject"/> is 
+        /// </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;
         }
 

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

@@ -1703,29 +1703,30 @@ public static void NetworkHide(List<NetworkObject> networkObjects, ulong clientI
         /// its child NetworkBehaviours. Private to assure this is
         /// only invoked from within OnDestroy.
         /// </summary>
-        private void SetIsDestroying()
+        internal void SetIsDestroying()
         {
-            IsDestroying = true;
-
-            // Exit early if null
-            if (m_ChildNetworkBehaviours == null)
+            if (IsDestroying)
             {
                 return;
             }
 
-            foreach (var childBehaviour in m_ChildNetworkBehaviours)
+            if (m_ChildNetworkBehaviours != null)
             {
-                // Just ignore and continue processing through the entries
-                if (!childBehaviour)
+                foreach (var childBehaviour in m_ChildNetworkBehaviours)
                 {
-                    continue;
-                }
+                    // 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.SetDestroying();
+                    // 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()

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/Tests/Runtime/AttachableBehaviourTests.cs

@@ -653,9 +653,11 @@ public IEnumerator AutoDetachTests([Values] DetachCombinations detachCombination
                     attachable.AutoDetach = AttachableBehaviour.AutoDetachTypes.OnAttachNodeDestroy;
                 }
                 var attachableNodeName = m_AttachableNodeInstance.name;
+                var attachableBehaviourName = m_AttachableBehaviourInstance.name;
+
                 Object.Destroy(m_TargetInstance.gameObject);
                 yield return WaitForConditionOrTimeOut(AllInstancesDetached);
-                AssertOnTimeout($"[OnAttachNodeDestroy] Timed out waiting for all clients to detach {m_AttachableBehaviourInstance.name} from {attachableNodeName}!\n {m_ErrorLog}");
+                AssertOnTimeout($"[OnAttachNodeDestroy] Timed out waiting for all clients to detach {attachableBehaviourName} from {attachableNodeName}!\n {m_ErrorLog}");
             }
         }
 

com.unity.netcode.gameobjects/Tests/Runtime/TestHelpers/IntegrationTestSceneHandler.cs

@@ -761,17 +761,29 @@ public void MoveObjectsFromSceneToDontDestroyOnLoad(ref NetworkManager networkMa
                     if (networkObject.HasAuthority && networkObject.NetworkManager == networkManager)
                     {
                         VerboseDebug($"[MoveObjects from {scene.name} | {scene.handle}] Destroying {networkObject.gameObject.name} because it is in scene {networkObject.gameObject.scene.name} with DWS = {networkObject.DestroyWithScene}.");
+                        // 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();
                         networkObject.Despawn();
                     }
                     else //For integration testing purposes, migrate remaining into DDOL
                     {
                         if (networkObject.DestroyPendingSceneEvent)
                         {
+                            // We know this instance is going to be destroyed (for integration testing we simulate the destroy).
+                            // 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();
                             Object.Destroy(networkObject.gameObject);
                         }
                         else
                         {
                             VerboseDebug($"[MoveObjects from {scene.name} | {scene.handle}] Temporarily migrating {networkObject.gameObject.name} into DDOL to await server destroy message.");
+                            // 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();
                             Object.DontDestroyOnLoad(networkObject.gameObject);
                         }
                     }

testproject/Assets/Tests/Runtime/NetworkSceneManager/AttachableBehaviourSceneLoadTests.cs

@@ -284,7 +284,7 @@ public IEnumerator AttachedUponSceneTransition([Values] bool detachOnDespawn)
             Assert.IsTrue(response == SceneEventProgressStatus.Started, $"Failed to begin scene loading event for {k_SceneToLoad} with a status of {response}!");
             yield return WaitForConditionOrTimeOut(() => m_AuthoritySceneLoaded.IsValid() && m_AuthoritySceneLoaded.isLoaded && m_SceneLoadCompleted);
             AssertOnTimeout($"Timed out waiting for all clients to load scene {k_SceneToLoad}!");
-
+            var persistedObjects = new Dictionary<NetworkManager, ulong>();
             // Now, make the newly loaded scene the currently active scene so everything instantiates in the scene authority's newly loaded scene instance.
             SceneManager.SetActiveScene(m_AuthoritySceneLoaded);
             foreach (var networkManager in m_NetworkManagers)
@@ -315,7 +315,10 @@ public IEnumerator AttachedUponSceneTransition([Values] bool detachOnDespawn)
 
                 // Keep track of the spawned instances we expect to not persist a scene load
                 var doesNotPersist = m_Persists == Persists.AttachableNode ? m_SourceInstance.NetworkObjectId : m_TargetInstance.NetworkObjectId;
+                var persists = m_Persists == Persists.AttachableNode ? m_TargetInstance.NetworkObjectId : m_SourceInstance.NetworkObjectId;
                 m_DoesNotPersistNetworkObjectIds.Add(doesNotPersist);
+                persistedObjects.Add(networkManager, persists);
+                Debug.Log($"[{networkManager.name}] Spawned attachable and attached it.");
             }
 
             // This is the actual validation point where the scene is unloaded and either the attachable or