fix: Attachables destroy order of operations

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);
  }
  }
  }