update

Modifications from 1:1 PR review session with Emma.
Removed some redundant logic.
Re-organized a few areas for better readability.
Renamed the `AttachableBehaviour.AttachableNode` to  `AttachableBehaviour.InternalAttachableNode`.
Removed the authority check within `AttachableNode.OnNetworkPreDespawn`.

Author: NoelStephensUnity

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

@@ -198,7 +198,7 @@ public enum AttachState
         /// If attached, attaching, or detaching this will be the <see cref="AttachableNode"/> this <see cref="AttachableBehaviour"/> instance is attached to.
         /// </summary>
         protected AttachableNode m_AttachableNode { get; private set; }
-        internal AttachableNode AttachableNode => m_AttachableNode;
+        internal AttachableNode InternalAttachableNode => m_AttachableNode;
 
         private NetworkBehaviourReference m_AttachedNodeReference = new NetworkBehaviourReference(null);
         private Vector3 m_OriginalLocalPosition;
@@ -287,36 +287,37 @@ public override void OnNetworkPreDespawn()
         private void UpdateAttachedState()
         {
             var attachableNode = (AttachableNode)null;
-            var isAttaching = m_AttachedNodeReference.TryGet(out attachableNode, NetworkManager);
-            var preState = isAttaching ? AttachState.Attaching : AttachState.Detaching;
+            var referenceHasNode = m_AttachedNodeReference.TryGet(out attachableNode, NetworkManager);
 
+            /////////////////////////////////////////////////////////////
             // Exit early if we are already in the correct attached state and the incoming
             // AttachableNode reference is the same as the local AttachableNode property.
             if (attachableNode == m_AttachableNode &&
-                ((isAttaching && m_AttachState == AttachState.Attached) ||
-                (!isAttaching && m_AttachState == AttachState.Detached)))
+                ((referenceHasNode && m_AttachState == AttachState.Attached) ||
+                (!referenceHasNode && m_AttachState == AttachState.Detached)))
             {
                 return;
             }
 
-            var preNode = isAttaching ? attachableNode : m_AttachableNode;
-            isAttaching = isAttaching && attachableNode != null;
+            // If we are in an attaching state but the node is null then we are still not attaching.
+            var isAttaching = referenceHasNode && attachableNode != null;
 
+            // If we are attached to some other AttachableNode, then detach from that before attaching to a new one.
             if (isAttaching && m_AttachableNode != null && m_AttachState == AttachState.Attached)
             {
-                // If we are attached to some other AttachableNode, then detach from that before attaching to a new one.
-                if (m_AttachableNode != attachableNode)
-                {
-                    // Run through the same process without being triggerd by a NetVar update.
-                    UpdateAttachState(AttachState.Detaching, m_AttachableNode);
-                    InternalDetach();
-                    UpdateAttachState(AttachState.Detached, m_AttachableNode);
+                // Run through the same process without being triggerd by a NetVar update.
+                UpdateAttachState(AttachState.Detaching, m_AttachableNode);
+                InternalDetach();
+                UpdateAttachState(AttachState.Detached, m_AttachableNode);
 
-                    m_AttachableNode.Detach(this);
-                    m_AttachableNode = null;
-                }
+                m_AttachableNode.Detach(this);
+                m_AttachableNode = null;
             }
 
+            // Used for attaching or detatching notifications
+            var preNode = referenceHasNode ? attachableNode : m_AttachableNode;
+            var preState = referenceHasNode ? AttachState.Attaching : AttachState.Detaching;
+
             // Change the state to attaching or detaching
             UpdateAttachState(preState, preNode);
 

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

@@ -51,7 +51,7 @@ protected override void OnOwnershipChanged(ulong previous, ulong current)
             var attachables = NetworkObject.transform.GetComponentsInChildren<AttachableBehaviour>();
             foreach (var attachable in attachables)
             {
-                if (attachable.AttachableNode == this)
+                if (attachable.InternalAttachableNode == this)
                 {
                     m_AttachedBehaviours.Add(attachable);
                 }
@@ -67,7 +67,7 @@ protected override void OnOwnershipChanged(ulong previous, ulong current)
     /// </remarks>
     public override void OnNetworkPreDespawn()
     {
-        if (IsSpawned && HasAuthority && DetachOnDespawn)
+        if (IsSpawned && DetachOnDespawn)
         {
             for (int i = m_AttachedBehaviours.Count - 1; i >= 0; i--)
             {

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

@@ -387,10 +387,10 @@ private bool AllInstancesDetached()
                     m_ErrorLog.AppendLine($"[Client-{networkManager.LocalClientId}][{attachable.name}] Did not have its event invoked!");
                 }
 
-                if (attachable.AttachableNode != null)
+                if (attachable.InternalAttachableNode != null)
                 {
-                    var nodeHasAttachments = attachable.AttachableNode.HasAttachments ? $" {attachable.AttachableNode.name} still has attachments!" : "";
-                    m_ErrorLog.AppendLine($"[Client-{networkManager.LocalClientId}][{attachable.name}] Still refers to {attachable.AttachableNode.name}!{nodeHasAttachments}");
+                    var nodeHasAttachments = attachable.InternalAttachableNode.HasAttachments ? $" {attachable.InternalAttachableNode.name} still has attachments!" : "";
+                    m_ErrorLog.AppendLine($"[Client-{networkManager.LocalClientId}][{attachable.name}] Still refers to {attachable.InternalAttachableNode.name}!{nodeHasAttachments}");
                 }
             }
             return m_ErrorLog.Length == 0;