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

@@ -205,6 +205,8 @@ public enum AttachState
         private Vector3 m_OriginalLocalPosition;
         private Quaternion m_OriginalLocalRotation;
 
+        internal bool IsDestroying { get; private set; }
+
         /// <inheritdoc/>
         protected override void OnSynchronize<T>(ref BufferSerializer<T> serializer)
         {
@@ -238,6 +240,42 @@ protected virtual void Awake()
             OnAwake();
         }
 
+        /// <inheritdoc/>
+        protected override void OnNetworkPreSpawn(ref NetworkManager networkManager)
+        {
+            IsDestroying = false;
+            // When attached to something else, the attachable needs to know if the
+            // default parent has been destroyed in order to not attempt to re-parent
+            // when detached (especially if it is being detached because it should be destroyed).
+            NetworkObject.OnDestroying += OnDefaultParentDestroying;
+
+            base.OnNetworkPreSpawn(ref networkManager);
+        }
+
+        private void OnDefaultParentDestroying()
+        {
+            NetworkObject.OnDestroying -= OnDefaultParentDestroying;
+            // Exit early if we are already being destroyed
+            if (IsDestroying)
+            {
+                return;
+            }
+            IsDestroying = true;
+            // If not completely detached, then destroy the GameObject for
+            // this attachable since the associated NetworkObject is being
+            // destroyed.
+            if (m_AttachState != AttachState.Detached)
+            {
+                Destroy(gameObject);
+            }
+        }
+
+        internal override void InternalOnDestroy()
+        {
+            IsDestroying = true;
+            base.InternalOnDestroy();
+        }
+
         /// <inheritdoc/>
         /// <remarks>
         /// If you create a custom <see cref="AttachableBehaviour"/> and override this method, you will want to
@@ -286,7 +324,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()
@@ -392,7 +430,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 +496,7 @@ public void Attach(AttachableNode attachableNode)
         /// </summary>
         internal void InternalDetach()
         {
-            if (m_AttachableNode)
+            if (!IsDestroying && m_AttachableNode && !m_AttachableNode.IsDestroying)
             {
                 if (m_DefaultParent)
                 {

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

@@ -24,6 +24,8 @@ public class AttachableNode : NetworkBehaviour
     /// </summary>
     public bool DetachOnDespawn = true;
 
+    internal bool IsDestroying { get; private set; }
+
     /// <summary>
     /// A <see cref="List{T}"/> of the currently attached <see cref="AttachableBehaviour"/>s.
     /// </summary>
@@ -32,6 +34,7 @@ public class AttachableNode : NetworkBehaviour
     /// <inheritdoc/>
     protected override void OnNetworkPreSpawn(ref NetworkManager networkManager)
     {
+        IsDestroying = false;
         m_AttachedBehaviours.Clear();
         base.OnNetworkPreSpawn(ref networkManager);
     }
@@ -71,20 +74,28 @@ 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)
                 {
-                    m_AttachedBehaviours[i].ForceDetach();
+                    attachable.ForceDetach();
                 }
                 else
                 {
-                    // Detach the normal way with authority
-                    m_AttachedBehaviours[i].Detach();
+                    if (attachable.IsSpawned)
+                    {
+                        // Detach the normal way with authority
+                        attachable.Detach();
+                    }
+                    else if (!attachable.IsDestroying)
+                    {
+                        attachable.ForceDetach();
+                    }
                 }
             }
         }
@@ -93,6 +104,7 @@ public override void OnNetworkPreDespawn()
 
     internal override void InternalOnDestroy()
     {
+        IsDestroying = true;
         // Notify any attached behaviours that this node is being destroyed.
         for (int i = m_AttachedBehaviours.Count - 1; i >= 0; i--)
         {

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

@@ -1688,8 +1688,14 @@ public static void NetworkHide(List<NetworkObject> networkObjects, ulong clientI
             }
         }
 
+        /// <summary>
+        /// Invoked when the NetworkObject is destroyed.
+        /// </summary>
+        internal event Action OnDestroying;
+
         private void OnDestroy()
         {
+            OnDestroying?.Invoke();
             var networkManager = NetworkManager;
             // If no NetworkManager is assigned, then just exit early
             if (!networkManager)

com.unity.netcode.gameobjects/Tests/Runtime/AttachableBehaviourTests.cs

@@ -55,7 +55,7 @@ protected override void OnServerAndClientsCreated()
             attachableNetworkObject.SetOwnershipStatus(NetworkObject.OwnershipStatus.Transferable);
 
             // The target prefab that the source prefab will attach
-            // will be parented under the target prefab.
+            // to will be parented under the target prefab.
             m_TargetNodePrefabA = CreateNetworkObjectPrefab("TargetA");
             m_TargetNodePrefabB = CreateNetworkObjectPrefab("TargetB");
             var sourceChild = new GameObject("SourceChild");
@@ -676,10 +676,13 @@ internal class TestAttachable : AttachableBehaviour
             public GameObject DefaultParent => m_DefaultParent;
             public AttachState State => m_AttachState;
 
+            public bool DestroyWithScene;
+
             public override void OnNetworkSpawn()
             {
                 AttachStateChange += OnAttachStateChangeEvent;
                 name = $"{name}-{NetworkManager.LocalClientId}";
+                NetworkObject.DestroyWithScene = DestroyWithScene;
                 base.OnNetworkSpawn();
             }
 
@@ -780,9 +783,16 @@ public bool CheckForState(bool checkAttached, bool checkEvent)
         /// </summary>
         internal class TestNode : AttachableNode
         {
+            public bool DestroyWithScene;
             public bool OnAttachedInvoked { get; private set; }
             public bool OnDetachedInvoked { get; private set; }
 
+            public override void OnNetworkSpawn()
+            {
+                NetworkObject.DestroyWithScene = DestroyWithScene;
+                base.OnNetworkSpawn();
+            }
+
             public bool IsAttached(AttachableBehaviour attachableBehaviour)
             {
                 return m_AttachedBehaviours.Contains(attachableBehaviour);