From 2dd2918e9bb6489b0256e9adf600f504535b2046 Mon Sep 17 00:00:00 2001 From: Noel Stephens Date: Fri, 3 Apr 2026 15:47:31 -0500 Subject: [PATCH 01/14] fix Fixing some issues based on varying destroy order of operations (more recent change where instantiation order does not reflect the order in which they will be destroyed via SceneManager during an in-session scene load (single mode). --- .../Components/Helpers/AttachableBehaviour.cs | 12 +++++++-- .../Components/Helpers/AttachableNode.cs | 26 +++++++++++++++---- 2 files changed, 31 insertions(+), 7 deletions(-) diff --git a/com.unity.netcode.gameobjects/Runtime/Components/Helpers/AttachableBehaviour.cs b/com.unity.netcode.gameobjects/Runtime/Components/Helpers/AttachableBehaviour.cs index 7038e74cd1..bc435e73c2 100644 --- a/com.unity.netcode.gameobjects/Runtime/Components/Helpers/AttachableBehaviour.cs +++ b/com.unity.netcode.gameobjects/Runtime/Components/Helpers/AttachableBehaviour.cs @@ -392,7 +392,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); } @@ -459,7 +460,14 @@ internal void InternalDetach() { if (m_AttachableNode) { - if (m_DefaultParent) + // TODO-FIX: We might track if something has been "destroyed" in order + // to be able to be 100% sure in the event a user disables the world item + // when detatched. Otherwise, we keep this in place and make note of it + // in documentation. + // Issue: + // Edge-case where the parent could be in the middle of being destroyed. + // If not active in the hierarchy, then don't attempt to set the parent. + if (m_DefaultParent && m_DefaultParent.activeInHierarchy) { // Set the original parent and origianl local position and rotation transform.SetParent(m_DefaultParent.transform, false); diff --git a/com.unity.netcode.gameobjects/Runtime/Components/Helpers/AttachableNode.cs b/com.unity.netcode.gameobjects/Runtime/Components/Helpers/AttachableNode.cs index d1a2ca9c16..e15b71cb4d 100644 --- a/com.unity.netcode.gameobjects/Runtime/Components/Helpers/AttachableNode.cs +++ b/com.unity.netcode.gameobjects/Runtime/Components/Helpers/AttachableNode.cs @@ -71,20 +71,36 @@ 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(); + // TODO-FIX: We might track if something has been "destroyed" in order + // to be able to be 100% sure this is specific to being destroyed. + // Otherwise, we keep this in place and make note of it + // in documentation that you cannot detatch from something already despawned. + // Issue: When trying to detatch if the thing attached is no longer + // spawned. Instantiation order recently changed such that + // the attachable =or= the attach node target could be despawned + // and in the middle of being destroyed. Resolution for this + // is to skip over destroyed object (default) and then only sort + // through the things the local NetworkManager instance has authority + // over. Even then, we have to check if the attached object is still + // spawned before attempting to detatch it. + if (attachable.IsSpawned) + { + // Detach the normal way with authority + attachable.Detach(); + } } } } From 21ff894ae3f16c2aa526a8ac6fabd4e44d6102d2 Mon Sep 17 00:00:00 2001 From: Noel Stephens Date: Tue, 7 Apr 2026 22:05:29 -0500 Subject: [PATCH 02/14] test Adding the test for this fix. --- .../Tests/Runtime/AttachableBehaviourTests.cs | 12 +- .../AttachableBehaviourSceneLoadTests.cs | 413 ++++++++++++++++++ .../AttachableBehaviourSceneLoadTests.cs.meta | 2 + 3 files changed, 426 insertions(+), 1 deletion(-) create mode 100644 testproject/Assets/Tests/Runtime/NetworkSceneManager/AttachableBehaviourSceneLoadTests.cs create mode 100644 testproject/Assets/Tests/Runtime/NetworkSceneManager/AttachableBehaviourSceneLoadTests.cs.meta diff --git a/com.unity.netcode.gameobjects/Tests/Runtime/AttachableBehaviourTests.cs b/com.unity.netcode.gameobjects/Tests/Runtime/AttachableBehaviourTests.cs index 383651d446..7324fe6df4 100644 --- a/com.unity.netcode.gameobjects/Tests/Runtime/AttachableBehaviourTests.cs +++ b/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) /// 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); diff --git a/testproject/Assets/Tests/Runtime/NetworkSceneManager/AttachableBehaviourSceneLoadTests.cs b/testproject/Assets/Tests/Runtime/NetworkSceneManager/AttachableBehaviourSceneLoadTests.cs new file mode 100644 index 0000000000..13b919158b --- /dev/null +++ b/testproject/Assets/Tests/Runtime/NetworkSceneManager/AttachableBehaviourSceneLoadTests.cs @@ -0,0 +1,413 @@ +using System.Collections; +using System.Collections.Generic; +using System.Text; +using NUnit.Framework; +using Unity.Netcode; +using Unity.Netcode.Components; +using Unity.Netcode.TestHelpers.Runtime; +using UnityEngine; +using UnityEngine.SceneManagement; +using UnityEngine.TestTools; +using static Unity.Netcode.RuntimeTests.AttachableBehaviourTests; + +namespace TestProject.RuntimeTests +{ + /// + /// Validates that attachables do not log errors or throw exceptions + /// during a scene transition (or unload) where either the attachable + /// or the node persists and the other does not but they are attached + /// to each other prior to the scene being unloaded. + /// + [TestFixture(HostOrServer.Host, Persists.AttachableBehaviour)] + [TestFixture(HostOrServer.Server, Persists.AttachableBehaviour)] + [TestFixture(HostOrServer.DAHost, Persists.AttachableBehaviour)] + [TestFixture(HostOrServer.Host, Persists.AttachableNode)] + [TestFixture(HostOrServer.Server, Persists.AttachableNode)] + [TestFixture(HostOrServer.DAHost, Persists.AttachableNode)] + internal class AttachableBehaviourSceneLoadTests : NetcodeIntegrationTest + { + public enum Persists + { + AttachableBehaviour, + AttachableNode + } + + protected override int NumberOfClients => 2; + + private const string k_SceneToLoad = "EmptyScene"; + + private GameObject m_AttachablePrefab; + private GameObject m_TargetNodePrefabA; + private TestAttachable m_PrefabAttachableBehaviour; + private TestNode m_PrefabAttachableNode; + + private NetworkObject m_SourceInstance; + private NetworkObject m_TargetInstance; + private TestAttachable m_AttachableBehaviourInstance; + private AttachableNode m_AttachableNodeInstance; + + private List m_DoesNotPersistNetworkObjectIds = new List(); + + private Scene m_AuthoritySceneLoaded; + private Scene m_TestRunnerScene; + + private bool m_SceneLoadCompleted; + + private Persists m_Persists; + public AttachableBehaviourSceneLoadTests(HostOrServer hostOrServer, Persists persists) : base(hostOrServer) + { + m_Persists = persists; + } + + protected override IEnumerator OnSetup() + { + m_TestRunnerScene = SceneManager.GetActiveScene(); + m_DoesNotPersistNetworkObjectIds.Clear(); + return base.OnSetup(); + } + + protected override void OnCreatePlayerPrefab() + { + var networkObject = m_PlayerPrefab.GetComponent(); + networkObject.ActiveSceneSynchronization = false; + base.OnCreatePlayerPrefab(); + } + + /// + /// Create an attachable with a node that should be destroyed upon + /// the scene it currently resides in being unloaded. + /// + protected override void OnServerAndClientsCreated() + { + // The source prefab contains the nested NetworkBehaviour that + // will be parented under the target prefab. + m_AttachablePrefab = CreateNetworkObjectPrefab("Source"); + m_AttachablePrefab.AddComponent(); + var attachableNetworkObject = m_AttachablePrefab.GetComponent(); + attachableNetworkObject.DontDestroyWithOwner = false; + attachableNetworkObject.SetOwnershipStatus(NetworkObject.OwnershipStatus.Transferable); + attachableNetworkObject.ActiveSceneSynchronization = false; + attachableNetworkObject.SceneMigrationSynchronization = false; + + // The "attachable" that has a world item (source) as its original root parent. + var sourceChild = new GameObject("SourceChild"); + sourceChild.transform.parent = m_AttachablePrefab.transform; + m_PrefabAttachableBehaviour = sourceChild.AddComponent(); + m_PrefabAttachableBehaviour.AutoDetach = AttachableBehaviour.AutoDetachTypes.OnDespawn | AttachableBehaviour.AutoDetachTypes.OnAttachNodeDestroy; + + // This particular test validates that the attachable's world item is destroyed on a scene transition while the attachable + // is attached to something that persists through scene loading. + m_PrefabAttachableBehaviour.DestroyWithScene = m_Persists != Persists.AttachableBehaviour; + + // The target prefab that the source prefab will attach + // to will be parented under the target prefab. + m_TargetNodePrefabA = CreateNetworkObjectPrefab("TargetA"); + m_TargetNodePrefabA.AddComponent(); + var targetNetworkObject = m_TargetNodePrefabA.GetComponent(); + targetNetworkObject.DontDestroyWithOwner = true; + targetNetworkObject.SceneMigrationSynchronization = false; + + // The "target node" to attach the source child to + var targetChildA = new GameObject("TargetChildA"); + targetChildA.transform.parent = m_TargetNodePrefabA.transform; + m_PrefabAttachableNode = targetChildA.AddComponent(); + m_PrefabAttachableNode.DetachOnDespawn = true; + + // This particular test validates that the attachable's world item is destroyed on a scene transition while the attachable + // is attached to something that persists through scene loading. + m_PrefabAttachableNode.DestroyWithScene = m_Persists != Persists.AttachableNode; + + // For this test & only when using a distributed authority topology, we want to switch the default synchronization + // mode back to single (even though it still is effectively loaded additively). + if (m_DistributedAuthority) + { + foreach (var networkManager in m_NetworkManagers) + { + m_ApplyClientSynchronizationModeInstances.Add(new ApplyClientSynchronizationMode(networkManager, LoadSceneMode.Single)); + } + } + base.OnServerAndClientsCreated(); + } + + #region Silly helper class to handle setting client synchronization mode for all distributed authority clients + // TODO: We should be able to apply NetworkSceneManager properties like this within the NetworkConfig. + private List m_ApplyClientSynchronizationModeInstances = new List(); + internal class ApplyClientSynchronizationMode + { + private NetworkManager m_NetworkManager; + private LoadSceneMode m_LoadSceneMode; + public ApplyClientSynchronizationMode(NetworkManager networkManager, LoadSceneMode loadSceneMode) + { + m_NetworkManager = networkManager; + m_LoadSceneMode = loadSceneMode; + m_NetworkManager.OnClientStarted += OnClientStarted; + } + + private void OnClientStarted() + { + m_NetworkManager.OnClientStarted -= OnClientStarted; + m_NetworkManager.SceneManager.SetClientSynchronizationMode(m_LoadSceneMode); + } + } + #endregion + + protected override IEnumerator OnServerAndClientsConnected() + { + m_ApplyClientSynchronizationModeInstances.Clear(); + foreach (var networkManager in m_NetworkManagers) + { + networkManager.SceneManager.ActiveSceneSynchronizationEnabled = true; + } + return base.OnServerAndClientsConnected(); + } + + /// + /// Conditional that validates a specific spawned attachable instance + /// has been attached for the original and all cloned instances. + /// + private bool AllInstancesAttachedStateChanged(StringBuilder errorLog) + { + var target = m_TargetInstance; + var targetId = target.NetworkObjectId; + // The attachable can move between the two spawned instances so we have to use the appropriate one depending upon the authority's current state. + var currentAttachableRoot = m_AttachableBehaviourInstance.State == AttachableBehaviour.AttachState.Attached ? target : m_SourceInstance; + var attachable = (TestAttachable)null; + var node = (TestNode)null; + foreach (var networkManager in m_NetworkManagers) + { + if (!networkManager.SpawnManager.SpawnedObjects.ContainsKey(currentAttachableRoot.NetworkObjectId)) + { + errorLog.AppendLine($"[Client-{networkManager.LocalClientId}] Has no spawned instance of {currentAttachableRoot.name}!"); + continue; + } + else + { + attachable = networkManager.SpawnManager.SpawnedObjects[currentAttachableRoot.NetworkObjectId].GetComponentInChildren(); + } + + if (!attachable) + { + attachable = networkManager.SpawnManager.SpawnedObjects[targetId].GetComponentInChildren(); + if (!attachable) + { + errorLog.AppendLine($"[Client-{networkManager.LocalClientId}][Attachable] Attachable was not found!"); + } + continue; + } + + if (!networkManager.SpawnManager.SpawnedObjects.ContainsKey(targetId)) + { + errorLog.AppendLine($"[Client-{networkManager.LocalClientId}] Has no spawned instance of {target.name}!"); + continue; + } + else + { + node = networkManager.SpawnManager.SpawnedObjects[targetId].GetComponentInChildren(); + } + + if (!attachable.CheckForState(true, false)) + { + errorLog.AppendLine($"[Client-{networkManager.LocalClientId}][{attachable.name}] Did not have its override invoked!"); + } + if (!attachable.CheckForState(true, true)) + { + errorLog.AppendLine($"[Client-{networkManager.LocalClientId}][{attachable.name}] Did not have its event invoked!"); + } + if (!node.OnAttachedInvoked) + { + errorLog.AppendLine($"[Client-{networkManager.LocalClientId}][{node.name}] Did not have its override invoked!"); + } + if (attachable.transform.parent != node.transform) + { + errorLog.AppendLine($"[Client-{networkManager.LocalClientId}][{attachable.name}] {node.name} is not the parent of {attachable.name}!"); + } + } + return errorLog.Length == 0; + } + + /// + /// Conditional that waits for the expected, scene load non-persistent, spawned objects + /// to be despawned. + /// + private bool WaitForAllToDespawn(StringBuilder errorLog) + { + foreach (var networkManager in m_NetworkManagers) + { + for (int i = 0; i < m_DoesNotPersistNetworkObjectIds.Count; i++) + { + if (networkManager.SpawnManager == null) + { + continue; + } + var sourceId = m_DoesNotPersistNetworkObjectIds[i]; + if (networkManager.SpawnManager.SpawnedObjects.ContainsKey(sourceId)) + { + errorLog.AppendLine($"[{networkManager.name}] Still has NetworkObjectId-{sourceId} spawned!"); + } + } + } + return errorLog.Length == 0; + } + + /// + /// Since everything spawns in the active scene, we want to assure that these instances are in their + /// NetworkManager relative scene so upon receiving a scene unload event each client will handle destroying + /// anything still spawned that does not persist a scene loading event. + /// This is required to replicate of this issue. + /// + private void SynchronizeScene() + { + var sourceId = m_SourceInstance.NetworkObjectId; + var targetId = m_TargetInstance.NetworkObjectId; + foreach (var networkManager in m_NetworkManagers) + { + var synchronizer = networkManager.SpawnManager.SpawnedObjects[sourceId].GetComponent(); + Assert.IsTrue(synchronizer.SynchronizeScene(), $"[{synchronizer.name}] Failed to synchronize instance to local scene!"); + synchronizer = networkManager.SpawnManager.SpawnedObjects[targetId].GetComponent(); + Assert.IsTrue(synchronizer.SynchronizeScene(), $"[{synchronizer.name}] Failed to synchronize instance to local scene!"); + } + } + + [UnityTest] + public IEnumerator AttachedUponSceneTransition([Values] bool detachOnDespawn) + { + m_SceneLoadCompleted = false; + + // Handle detatch on despawn differently to validate both paths. + m_PrefabAttachableNode.DetachOnDespawn = detachOnDespawn; + + var authority = GetAuthorityNetworkManager(); + + // Load a scene so all clients load this scene. + authority.SceneManager.OnSceneEvent += OnSceneEvent; + var response = authority.SceneManager.LoadScene(k_SceneToLoad, LoadSceneMode.Additive); + 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}!"); + + // 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) + { + // Spawn our instances for this client + m_SourceInstance = SpawnObject(m_AttachablePrefab, networkManager).GetComponent(); + m_TargetInstance = SpawnObject(m_TargetNodePrefabA, networkManager).GetComponent(); + + yield return WaitForSpawnedOnAllOrTimeOut(m_SourceInstance); + AssertOnTimeout($"Timed out waiting for all clients to spawn {m_SourceInstance.name}!"); + + yield return WaitForSpawnedOnAllOrTimeOut(m_TargetInstance); + AssertOnTimeout($"Timed out waiting for all clients to spawn {m_TargetInstance.name}!"); + + m_AttachableBehaviourInstance = m_SourceInstance.GetComponentInChildren(); + Assert.NotNull(m_AttachableBehaviourInstance, $"{m_SourceInstance.name} does not have a nested child {nameof(AttachableBehaviour)}!"); + + m_AttachableNodeInstance = m_TargetInstance.GetComponentInChildren(); + Assert.NotNull(m_AttachableNodeInstance, $"{m_TargetInstance.name} does not have a nested child {nameof(AttachableNode)}!"); + + m_AttachableBehaviourInstance.Attach(m_AttachableNodeInstance); + + yield return WaitForConditionOrTimeOut(AllInstancesAttachedStateChanged); + AssertOnTimeout($"Timed out waiting for all clients to attach {m_AttachableBehaviourInstance.name} to {m_AttachableNodeInstance.name}!"); + + // Now migrate all instances from the scene authority's scene instance to the NetworkManager relative scene. + SynchronizeScene(); + + // 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; + m_DoesNotPersistNetworkObjectIds.Add(doesNotPersist); + } + + // This is the actual validation point where the scene is unloaded and either the attachable or + // the node will be destroyed while the other persists (and detects that the other has been despawned and destroyed). + response = authority.SceneManager.UnloadScene(m_AuthoritySceneLoaded); + Assert.IsTrue(response == SceneEventProgressStatus.Started, $"Failed to begin scene unloading event for {k_SceneToLoad} with a status of {response}!"); + + yield return WaitForConditionOrTimeOut(WaitForAllToDespawn); + AssertOnTimeout($"Timed out waiting for all clients to despawn NetworkObjects!"); + } + + private void OnSceneEvent(SceneEvent sceneEvent) + { + if ((sceneEvent.SceneEventType != SceneEventType.LoadEventCompleted && sceneEvent.SceneEventType != SceneEventType.LoadComplete) + || sceneEvent.ClientId != GetAuthorityNetworkManager().LocalClientId) + { + return; + } + + if (sceneEvent.SceneName == k_SceneToLoad) + { + if (sceneEvent.SceneEventType == SceneEventType.LoadComplete) + { + m_AuthoritySceneLoaded = sceneEvent.Scene; + } + + if (sceneEvent.SceneEventType == SceneEventType.LoadEventCompleted) + { + m_SceneLoadCompleted = true; + } + } + } + + protected override IEnumerator OnTearDown() + { + // Assure we set the active scene back to the integration test's scene if needed. + var currentActiveScene = SceneManager.GetActiveScene(); + if (m_AuthoritySceneLoaded != null && currentActiveScene == m_AuthoritySceneLoaded && m_AuthoritySceneLoaded.isLoaded && m_AuthoritySceneLoaded.IsValid()) + { + SceneManager.SetActiveScene(m_TestRunnerScene); + SceneManager.UnloadSceneAsync(m_AuthoritySceneLoaded); + } + return base.OnTearDown(); + } + } + + /// + /// This helper class will assure that the spawned object clone + /// (or original) instance will be migrated into its appropriate + /// NetworkManager relative instance. + /// + /// + /// Since we share the same scene root, there is only 1 active scene + /// and all new instances are always instantiated within that. + /// If you load a new scene and make that the active scene prior to + /// spawning instances, then all instances will reside in the scene + /// authority's loaded scene instance. + /// This helper assures all instances are migrated into their NetworkManager + /// relative scene in order to replicate a more "real world" scenario + /// where each spawned clone instance for each connected client will be + /// destroyed during that client's processing of the unload scene + /// event. + /// Without this helper (or similar logic elsewhere), everything would be + /// destroyed upon the authority's scene being unloaded while the rest of + /// the clients would unload empty scenes. + /// + internal class SceneSynchronizer : NetworkBehaviour + { + /// + /// We are using the NetworkBehaviour to provide us with the relative + /// NetworkManager instance (for this spawned instance) in order to + /// then check against the client relative scenes loaded. + /// + /// Success if true. Failed to find the scene if false. + public bool SynchronizeScene() + { + var scenes = NetworkManager.SceneManager.GetSynchronizedScenes(); + foreach (var scene in scenes) + { + // Find the matching scene name + if (gameObject.scene.name == scene.name) + { + // If the scene handles are different, then migrate to the + // one registered with this NetworkSceneManager instance. + if (scene.handle != gameObject.scene.handle) + { + SceneManager.MoveGameObjectToScene(gameObject, scene); + } + return true; + } + } + return false; + } + } +} diff --git a/testproject/Assets/Tests/Runtime/NetworkSceneManager/AttachableBehaviourSceneLoadTests.cs.meta b/testproject/Assets/Tests/Runtime/NetworkSceneManager/AttachableBehaviourSceneLoadTests.cs.meta new file mode 100644 index 0000000000..4166ba969e --- /dev/null +++ b/testproject/Assets/Tests/Runtime/NetworkSceneManager/AttachableBehaviourSceneLoadTests.cs.meta @@ -0,0 +1,2 @@ +fileFormatVersion: 2 +guid: d0fa3eb3f5e1b6f4da462ff095a46f84 \ No newline at end of file From 842b67b00c78889a35d71dff7fd19cbbf663a004 Mon Sep 17 00:00:00 2001 From: Noel Stephens Date: Tue, 7 Apr 2026 22:49:16 -0500 Subject: [PATCH 03/14] fix The actual fix for the issue with additional handling when an attachable's NetworkObject is destroyed while it is still attached to something else. --- .../Components/Helpers/AttachableBehaviour.cs | 44 +++++++++++++++---- .../Components/Helpers/AttachableNode.cs | 20 ++++----- .../Runtime/Core/NetworkObject.cs | 6 +++ 3 files changed, 49 insertions(+), 21 deletions(-) diff --git a/com.unity.netcode.gameobjects/Runtime/Components/Helpers/AttachableBehaviour.cs b/com.unity.netcode.gameobjects/Runtime/Components/Helpers/AttachableBehaviour.cs index bc435e73c2..4155393019 100644 --- a/com.unity.netcode.gameobjects/Runtime/Components/Helpers/AttachableBehaviour.cs +++ b/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; } + /// protected override void OnSynchronize(ref BufferSerializer serializer) { @@ -238,6 +240,37 @@ protected virtual void Awake() OnAwake(); } + 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 detatched 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; + // Just destroy the GameObject for this attachable since + // the associated NetworkObject is being destroyed. + Destroy(gameObject); + } + + internal override void InternalOnDestroy() + { + IsDestroying = true; + base.InternalOnDestroy(); + } + /// /// /// If you create a custom and override this method, you will want to @@ -458,16 +491,9 @@ public void Attach(AttachableNode attachableNode) /// internal void InternalDetach() { - if (m_AttachableNode) + if (!IsDestroying && m_AttachableNode && !m_AttachableNode.IsDestroying) { - // TODO-FIX: We might track if something has been "destroyed" in order - // to be able to be 100% sure in the event a user disables the world item - // when detatched. Otherwise, we keep this in place and make note of it - // in documentation. - // Issue: - // Edge-case where the parent could be in the middle of being destroyed. - // If not active in the hierarchy, then don't attempt to set the parent. - if (m_DefaultParent && m_DefaultParent.activeInHierarchy) + if (m_DefaultParent) { // Set the original parent and origianl local position and rotation transform.SetParent(m_DefaultParent.transform, false); diff --git a/com.unity.netcode.gameobjects/Runtime/Components/Helpers/AttachableNode.cs b/com.unity.netcode.gameobjects/Runtime/Components/Helpers/AttachableNode.cs index e15b71cb4d..248c487e89 100644 --- a/com.unity.netcode.gameobjects/Runtime/Components/Helpers/AttachableNode.cs +++ b/com.unity.netcode.gameobjects/Runtime/Components/Helpers/AttachableNode.cs @@ -24,6 +24,8 @@ public class AttachableNode : NetworkBehaviour /// public bool DetachOnDespawn = true; + internal bool IsDestroying { get; private set; } + /// /// A of the currently attached s. /// @@ -32,6 +34,7 @@ public class AttachableNode : NetworkBehaviour /// protected override void OnNetworkPreSpawn(ref NetworkManager networkManager) { + IsDestroying = false; m_AttachedBehaviours.Clear(); base.OnNetworkPreSpawn(ref networkManager); } @@ -84,23 +87,15 @@ public override void OnNetworkPreDespawn() } else { - // TODO-FIX: We might track if something has been "destroyed" in order - // to be able to be 100% sure this is specific to being destroyed. - // Otherwise, we keep this in place and make note of it - // in documentation that you cannot detatch from something already despawned. - // Issue: When trying to detatch if the thing attached is no longer - // spawned. Instantiation order recently changed such that - // the attachable =or= the attach node target could be despawned - // and in the middle of being destroyed. Resolution for this - // is to skip over destroyed object (default) and then only sort - // through the things the local NetworkManager instance has authority - // over. Even then, we have to check if the attached object is still - // spawned before attempting to detatch it. if (attachable.IsSpawned) { // Detach the normal way with authority attachable.Detach(); } + else if (!attachable.IsDestroying) + { + attachable.ForceDetach(); + } } } } @@ -109,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--) { diff --git a/com.unity.netcode.gameobjects/Runtime/Core/NetworkObject.cs b/com.unity.netcode.gameobjects/Runtime/Core/NetworkObject.cs index bd518b3048..445508d615 100644 --- a/com.unity.netcode.gameobjects/Runtime/Core/NetworkObject.cs +++ b/com.unity.netcode.gameobjects/Runtime/Core/NetworkObject.cs @@ -1688,8 +1688,14 @@ public static void NetworkHide(List networkObjects, ulong clientI } } + /// + /// Invoked when the NetworkObject is destroyed. + /// + internal event Action OnDestroying; + private void OnDestroy() { + OnDestroying?.Invoke(); var networkManager = NetworkManager; // If no NetworkManager is assigned, then just exit early if (!networkManager) From 84a5dd98e41d55a129b0e50e778a2c623e431a77 Mon Sep 17 00:00:00 2001 From: Noel Stephens Date: Tue, 7 Apr 2026 22:59:42 -0500 Subject: [PATCH 04/14] update & style Adding change log entry. Fixing some typos. --- com.unity.netcode.gameobjects/CHANGELOG.md | 1 + .../Components/Helpers/AttachableBehaviour.cs | 14 +++++++++----- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/com.unity.netcode.gameobjects/CHANGELOG.md b/com.unity.netcode.gameobjects/CHANGELOG.md index ed55793aba..09ffeee3e6 100644 --- a/com.unity.netcode.gameobjects/CHANGELOG.md +++ b/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 diff --git a/com.unity.netcode.gameobjects/Runtime/Components/Helpers/AttachableBehaviour.cs b/com.unity.netcode.gameobjects/Runtime/Components/Helpers/AttachableBehaviour.cs index 4155393019..f506c4738a 100644 --- a/com.unity.netcode.gameobjects/Runtime/Components/Helpers/AttachableBehaviour.cs +++ b/com.unity.netcode.gameobjects/Runtime/Components/Helpers/AttachableBehaviour.cs @@ -245,7 +245,7 @@ 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 detatched because it should be destroyed). + // when detached (especially if it is being detached because it should be destroyed). NetworkObject.OnDestroying += OnDefaultParentDestroying; base.OnNetworkPreSpawn(ref networkManager); @@ -260,9 +260,13 @@ private void OnDefaultParentDestroying() return; } IsDestroying = true; - // Just destroy the GameObject for this attachable since - // the associated NetworkObject is being destroyed. - Destroy(gameObject); + // 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() @@ -319,7 +323,7 @@ public override void OnNetworkPreDespawn() } /// - /// This will apply the final attach or detatch state based on the current value of . + /// This will apply the final attach or detach state based on the current value of . /// [MethodImpl(MethodImplOptions.AggressiveInlining)] private void UpdateAttachedState() From ce4f577e5941259eb537e1b47f5d4506a5783d9f Mon Sep 17 00:00:00 2001 From: Noel Stephens Date: Wed, 8 Apr 2026 00:55:27 -0500 Subject: [PATCH 05/14] style removing white space. --- .../Runtime/Components/Helpers/AttachableNode.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/com.unity.netcode.gameobjects/Runtime/Components/Helpers/AttachableNode.cs b/com.unity.netcode.gameobjects/Runtime/Components/Helpers/AttachableNode.cs index 248c487e89..e8e560d510 100644 --- a/com.unity.netcode.gameobjects/Runtime/Components/Helpers/AttachableNode.cs +++ b/com.unity.netcode.gameobjects/Runtime/Components/Helpers/AttachableNode.cs @@ -24,7 +24,7 @@ public class AttachableNode : NetworkBehaviour /// public bool DetachOnDespawn = true; - internal bool IsDestroying { get; private set; } + internal bool IsDestroying { get; private set; } /// /// A of the currently attached s. From ffca0f35f52071b083e4d6b8b946fb27fe33129f Mon Sep 17 00:00:00 2001 From: Noel Stephens Date: Wed, 8 Apr 2026 00:57:28 -0500 Subject: [PATCH 06/14] style fixing spelling typo. --- .../NetworkSceneManager/AttachableBehaviourSceneLoadTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/testproject/Assets/Tests/Runtime/NetworkSceneManager/AttachableBehaviourSceneLoadTests.cs b/testproject/Assets/Tests/Runtime/NetworkSceneManager/AttachableBehaviourSceneLoadTests.cs index 13b919158b..619bab5dab 100644 --- a/testproject/Assets/Tests/Runtime/NetworkSceneManager/AttachableBehaviourSceneLoadTests.cs +++ b/testproject/Assets/Tests/Runtime/NetworkSceneManager/AttachableBehaviourSceneLoadTests.cs @@ -273,7 +273,7 @@ public IEnumerator AttachedUponSceneTransition([Values] bool detachOnDespawn) { m_SceneLoadCompleted = false; - // Handle detatch on despawn differently to validate both paths. + // Handle detach on despawn differently to validate both paths. m_PrefabAttachableNode.DetachOnDespawn = detachOnDespawn; var authority = GetAuthorityNetworkManager(); From e2e0eee0167eb7669a5fb358f5038d60689b3196 Mon Sep 17 00:00:00 2001 From: Noel Stephens Date: Wed, 8 Apr 2026 01:09:52 -0500 Subject: [PATCH 07/14] style-pvp Adding inheritdoc --- .../Runtime/Components/Helpers/AttachableBehaviour.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/com.unity.netcode.gameobjects/Runtime/Components/Helpers/AttachableBehaviour.cs b/com.unity.netcode.gameobjects/Runtime/Components/Helpers/AttachableBehaviour.cs index f506c4738a..21c69637db 100644 --- a/com.unity.netcode.gameobjects/Runtime/Components/Helpers/AttachableBehaviour.cs +++ b/com.unity.netcode.gameobjects/Runtime/Components/Helpers/AttachableBehaviour.cs @@ -240,6 +240,7 @@ protected virtual void Awake() OnAwake(); } + /// protected override void OnNetworkPreSpawn(ref NetworkManager networkManager) { IsDestroying = false; From 6daf3eaf68c4f52204a9801c2192aab41239ad06 Mon Sep 17 00:00:00 2001 From: Noel Stephens Date: Wed, 8 Apr 2026 17:43:25 -0500 Subject: [PATCH 08/14] refactor Going ahead and adding an (currently) internal "IsDestroying" flag that is set only when the NetworkObject.OnDestroy method is invoked. --- .../Components/Helpers/AttachableBehaviour.cs | 46 ++++--------------- .../Components/Helpers/AttachableNode.cs | 4 -- .../Runtime/Core/NetworkBehaviour.cs | 14 ++++++ .../Runtime/Core/NetworkObject.cs | 43 +++++++++++++++-- 4 files changed, 62 insertions(+), 45 deletions(-) diff --git a/com.unity.netcode.gameobjects/Runtime/Components/Helpers/AttachableBehaviour.cs b/com.unity.netcode.gameobjects/Runtime/Components/Helpers/AttachableBehaviour.cs index 21c69637db..52997eb7e5 100644 --- a/com.unity.netcode.gameobjects/Runtime/Components/Helpers/AttachableBehaviour.cs +++ b/com.unity.netcode.gameobjects/Runtime/Components/Helpers/AttachableBehaviour.cs @@ -205,8 +205,6 @@ public enum AttachState private Vector3 m_OriginalLocalPosition; private Quaternion m_OriginalLocalRotation; - internal bool IsDestroying { get; private set; } - /// protected override void OnSynchronize(ref BufferSerializer serializer) { @@ -240,42 +238,6 @@ protected virtual void Awake() OnAwake(); } - /// - 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(); - } - /// /// /// If you create a custom and override this method, you will want to @@ -316,6 +278,14 @@ internal void ForceDetach() /// 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(); diff --git a/com.unity.netcode.gameobjects/Runtime/Components/Helpers/AttachableNode.cs b/com.unity.netcode.gameobjects/Runtime/Components/Helpers/AttachableNode.cs index e8e560d510..b9037ff1c0 100644 --- a/com.unity.netcode.gameobjects/Runtime/Components/Helpers/AttachableNode.cs +++ b/com.unity.netcode.gameobjects/Runtime/Components/Helpers/AttachableNode.cs @@ -24,8 +24,6 @@ public class AttachableNode : NetworkBehaviour /// public bool DetachOnDespawn = true; - internal bool IsDestroying { get; private set; } - /// /// A of the currently attached s. /// @@ -34,7 +32,6 @@ public class AttachableNode : NetworkBehaviour /// protected override void OnNetworkPreSpawn(ref NetworkManager networkManager) { - IsDestroying = false; m_AttachedBehaviours.Clear(); base.OnNetworkPreSpawn(ref networkManager); } @@ -104,7 +101,6 @@ 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--) { diff --git a/com.unity.netcode.gameobjects/Runtime/Core/NetworkBehaviour.cs b/com.unity.netcode.gameobjects/Runtime/Core/NetworkBehaviour.cs index 049f28070b..12decd3b7a 100644 --- a/com.unity.netcode.gameobjects/Runtime/Core/NetworkBehaviour.cs +++ b/com.unity.netcode.gameobjects/Runtime/Core/NetworkBehaviour.cs @@ -643,6 +643,20 @@ protected NetworkBehaviour GetNetworkBehaviour(ushort behaviourId) /// public ulong OwnerClientId { get; internal set; } + /// + /// Returns true if the NetworkObject is in the middle of being destroyed or + /// if there is no valid assigned NetworkObject. + /// + /// + /// + /// + internal bool IsDestroying { get; private set; } + + internal void SetDestroying() + { + IsDestroying = true; + } + /// /// Updates properties with network session related /// dependencies such as a NetworkObject's spawned diff --git a/com.unity.netcode.gameobjects/Runtime/Core/NetworkObject.cs b/com.unity.netcode.gameobjects/Runtime/Core/NetworkObject.cs index 445508d615..6afb97475d 100644 --- a/com.unity.netcode.gameobjects/Runtime/Core/NetworkObject.cs +++ b/com.unity.netcode.gameobjects/Runtime/Core/NetworkObject.cs @@ -1689,13 +1689,50 @@ public static void NetworkHide(List networkObjects, ulong clientI } /// - /// Invoked when the NetworkObject is destroyed. + /// Returns true if the NetworkObject is in the middle of being destroyed. /// - internal event Action OnDestroying; + /// + /// 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. + /// + internal bool IsDestroying { get; private set; } + + /// + /// Applies the despawning flag for the local instance and + /// its child NetworkBehaviours. Private to assure this is + /// only invoked from within OnDestroy. + /// + private void SetIsDestroying() + { + IsDestroying = true; + + // Exit early if null + if (m_ChildNetworkBehaviours == null) + { + return; + } + + 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.SetDestroying(); + } + } private void OnDestroy() { - OnDestroying?.Invoke(); + // Apply the is destroying flag + SetIsDestroying(); + var networkManager = NetworkManager; // If no NetworkManager is assigned, then just exit early if (!networkManager) From a73b5977554626bfba4c34aa191d09124125213f Mon Sep 17 00:00:00 2001 From: Noel Stephens Date: Wed, 8 Apr 2026 17:46:52 -0500 Subject: [PATCH 09/14] update Applying suggested adjustment to logic within OnNetworkPreDespawn. --- .../Components/Helpers/AttachableNode.cs | 20 ++++++------------- 1 file changed, 6 insertions(+), 14 deletions(-) diff --git a/com.unity.netcode.gameobjects/Runtime/Components/Helpers/AttachableNode.cs b/com.unity.netcode.gameobjects/Runtime/Components/Helpers/AttachableNode.cs index b9037ff1c0..edd507cd67 100644 --- a/com.unity.netcode.gameobjects/Runtime/Components/Helpers/AttachableNode.cs +++ b/com.unity.netcode.gameobjects/Runtime/Components/Helpers/AttachableNode.cs @@ -76,23 +76,15 @@ public override void OnNetworkPreDespawn() { continue; } - // If we don't have authority but should detach on despawn, - // then proceed to detach. - if (!attachable.HasAuthority) + + if (attachable.HasAuthority && attachable.IsSpawned) { - attachable.ForceDetach(); + // Detach the normal way with authority + attachable.Detach(); } - else + else if (!attachable.HasAuthority || !attachable.IsDestroying) { - if (attachable.IsSpawned) - { - // Detach the normal way with authority - attachable.Detach(); - } - else if (!attachable.IsDestroying) - { - attachable.ForceDetach(); - } + attachable.ForceDetach(); } } } From 9e4f0e55b0e1b5e18f046e00c17729ed8f9c5c40 Mon Sep 17 00:00:00 2001 From: Noel Stephens Date: Wed, 8 Apr 2026 17:59:18 -0500 Subject: [PATCH 10/14] style adding a whitespace --- com.unity.netcode.gameobjects/Runtime/Core/NetworkObject.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/com.unity.netcode.gameobjects/Runtime/Core/NetworkObject.cs b/com.unity.netcode.gameobjects/Runtime/Core/NetworkObject.cs index 6afb97475d..0966d7965d 100644 --- a/com.unity.netcode.gameobjects/Runtime/Core/NetworkObject.cs +++ b/com.unity.netcode.gameobjects/Runtime/Core/NetworkObject.cs @@ -1713,7 +1713,7 @@ private void SetIsDestroying() return; } - foreach(var childBehaviour in m_ChildNetworkBehaviours) + foreach (var childBehaviour in m_ChildNetworkBehaviours) { // Just ignore and continue processing through the entries if (!childBehaviour) From ffece95982e0beeb2bedd7aab89b5fdacd3ea065 Mon Sep 17 00:00:00 2001 From: Noel Stephens Date: Tue, 14 Apr 2026 19:29:43 -0500 Subject: [PATCH 11/14] 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. --- .../Components/Helpers/AttachableBehaviour.cs | 52 +++++++++++++------ .../Components/Helpers/AttachableNode.cs | 23 ++++++-- .../Runtime/Core/NetworkBehaviour.cs | 25 ++++++++- .../Runtime/Core/NetworkObject.cs | 29 ++++++----- .../DefaultSceneManagerHandler.cs | 9 ++++ .../Tests/Runtime/AttachableBehaviourTests.cs | 4 +- .../IntegrationTestSceneHandler.cs | 12 +++++ .../AttachableBehaviourSceneLoadTests.cs | 5 +- 8 files changed, 123 insertions(+), 36 deletions(-) diff --git a/com.unity.netcode.gameobjects/Runtime/Components/Helpers/AttachableBehaviour.cs b/com.unity.netcode.gameobjects/Runtime/Components/Helpers/AttachableBehaviour.cs index 52997eb7e5..0fc638bd54 100644 --- a/com.unity.netcode.gameobjects/Runtime/Components/Helpers/AttachableBehaviour.cs +++ b/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; } /// @@ -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) /// 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 /// 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(); + } + } + + + /// + /// 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. + /// + 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(); } } } diff --git a/com.unity.netcode.gameobjects/Runtime/Components/Helpers/AttachableNode.cs b/com.unity.netcode.gameobjects/Runtime/Components/Helpers/AttachableNode.cs index edd507cd67..f8e08dda0a 100644 --- a/com.unity.netcode.gameobjects/Runtime/Components/Helpers/AttachableNode.cs +++ b/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); } + + /// + /// 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. + /// + 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(); + } } diff --git a/com.unity.netcode.gameobjects/Runtime/Core/NetworkBehaviour.cs b/com.unity.netcode.gameobjects/Runtime/Core/NetworkBehaviour.cs index 12decd3b7a..a1a14dab15 100644 --- a/com.unity.netcode.gameobjects/Runtime/Core/NetworkBehaviour.cs +++ b/com.unity.netcode.gameobjects/Runtime/Core/NetworkBehaviour.cs @@ -652,8 +652,31 @@ protected NetworkBehaviour GetNetworkBehaviour(ushort behaviourId) /// internal bool IsDestroying { get; private set; } - internal void SetDestroying() + /// + /// 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 is + /// + protected internal virtual void OnIsDestroying() + { + } + + /// + /// Invoked by . + /// + /// + /// 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 instance is + /// in the middle of being destroyed. + /// + internal void SetIsDestroying() { + OnIsDestroying(); IsDestroying = true; } diff --git a/com.unity.netcode.gameobjects/Runtime/Core/NetworkObject.cs b/com.unity.netcode.gameobjects/Runtime/Core/NetworkObject.cs index 0966d7965d..d529a46764 100644 --- a/com.unity.netcode.gameobjects/Runtime/Core/NetworkObject.cs +++ b/com.unity.netcode.gameobjects/Runtime/Core/NetworkObject.cs @@ -1703,29 +1703,30 @@ public static void NetworkHide(List networkObjects, ulong clientI /// its child NetworkBehaviours. Private to assure this is /// only invoked from within OnDestroy. /// - 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() diff --git a/com.unity.netcode.gameobjects/Runtime/SceneManagement/DefaultSceneManagerHandler.cs b/com.unity.netcode.gameobjects/Runtime/SceneManagement/DefaultSceneManagerHandler.cs index 5ed894402e..5a11a01b4a 100644 --- a/com.unity.netcode.gameobjects/Runtime/SceneManagement/DefaultSceneManagerHandler.cs +++ b/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); } } diff --git a/com.unity.netcode.gameobjects/Tests/Runtime/AttachableBehaviourTests.cs b/com.unity.netcode.gameobjects/Tests/Runtime/AttachableBehaviourTests.cs index 7324fe6df4..3574e56efb 100644 --- a/com.unity.netcode.gameobjects/Tests/Runtime/AttachableBehaviourTests.cs +++ b/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}"); } } diff --git a/com.unity.netcode.gameobjects/Tests/Runtime/TestHelpers/IntegrationTestSceneHandler.cs b/com.unity.netcode.gameobjects/Tests/Runtime/TestHelpers/IntegrationTestSceneHandler.cs index e1959de8a0..2a5c2c35f9 100644 --- a/com.unity.netcode.gameobjects/Tests/Runtime/TestHelpers/IntegrationTestSceneHandler.cs +++ b/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); } } diff --git a/testproject/Assets/Tests/Runtime/NetworkSceneManager/AttachableBehaviourSceneLoadTests.cs b/testproject/Assets/Tests/Runtime/NetworkSceneManager/AttachableBehaviourSceneLoadTests.cs index 619bab5dab..58d056d8d0 100644 --- a/testproject/Assets/Tests/Runtime/NetworkSceneManager/AttachableBehaviourSceneLoadTests.cs +++ b/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(); // 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 From aec2ae680c8fc24d65f045b61cbf7763a54f5873 Mon Sep 17 00:00:00 2001 From: Noel Stephens Date: Tue, 14 Apr 2026 19:34:38 -0500 Subject: [PATCH 12/14] style Removing comment that was just mistakenly left in. --- com.unity.netcode.gameobjects/Runtime/Core/NetworkBehaviour.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/com.unity.netcode.gameobjects/Runtime/Core/NetworkBehaviour.cs b/com.unity.netcode.gameobjects/Runtime/Core/NetworkBehaviour.cs index a1a14dab15..2005efa7c4 100644 --- a/com.unity.netcode.gameobjects/Runtime/Core/NetworkBehaviour.cs +++ b/com.unity.netcode.gameobjects/Runtime/Core/NetworkBehaviour.cs @@ -655,7 +655,6 @@ protected NetworkBehaviour GetNetworkBehaviour(ushort behaviourId) /// /// 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 is /// protected internal virtual void OnIsDestroying() { From 42e3d8325bffb2aa0a08c1976c3c73adab301d5e Mon Sep 17 00:00:00 2001 From: Noel Stephens Date: Wed, 15 Apr 2026 15:02:29 -0500 Subject: [PATCH 13/14] update + fix Fixing an edge case that requires manually testing due to how integration tests additively load everything to avoid unloading the test runner test scene when loading a scene. Two parts: - When a server is destroying spawned scene objects due to a single mode scene load, it should also include a check for this and handle marking the NetworkObject and associated NetworkBehaviours as being destroyed and then destroy the object. - Just prior to handling a scene loading event, we need to mark anything that will be destroyed (i.e. destroy with scene) as being destroyed when migrating spawned objects into the DDOL. --- .../SceneManagement/NetworkSceneManager.cs | 6 ++- .../Runtime/Spawning/NetworkSpawnManager.cs | 40 ++++++++++++++----- 2 files changed, 35 insertions(+), 11 deletions(-) diff --git a/com.unity.netcode.gameobjects/Runtime/SceneManagement/NetworkSceneManager.cs b/com.unity.netcode.gameobjects/Runtime/SceneManagement/NetworkSceneManager.cs index bf1ed0d734..5e2d9aa121 100644 --- a/com.unity.netcode.gameobjects/Runtime/SceneManagement/NetworkSceneManager.cs +++ b/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); } } } diff --git a/com.unity.netcode.gameobjects/Runtime/Spawning/NetworkSpawnManager.cs b/com.unity.netcode.gameobjects/Runtime/Spawning/NetworkSpawnManager.cs index bf4eacbc1b..0b320da1da 100644 --- a/com.unity.netcode.gameobjects/Runtime/Spawning/NetworkSpawnManager.cs +++ b/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() } /// - /// Gets called only by NetworkSceneManager.SwitchScene + /// Gets called only by and the load scene mode + /// is set to . /// 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 From eace6b84ede7ea51a717d7d840295721d0507e20 Mon Sep 17 00:00:00 2001 From: Noel Stephens Date: Thu, 16 Apr 2026 09:57:25 -0500 Subject: [PATCH 14/14] style Updating some comments based on AI suggestions. --- .../Runtime/Components/Helpers/AttachableBehaviour.cs | 2 ++ .../Runtime/Core/NetworkBehaviour.cs | 6 +++--- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/com.unity.netcode.gameobjects/Runtime/Components/Helpers/AttachableBehaviour.cs b/com.unity.netcode.gameobjects/Runtime/Components/Helpers/AttachableBehaviour.cs index 0fc638bd54..f17ae300ec 100644 --- a/com.unity.netcode.gameobjects/Runtime/Components/Helpers/AttachableBehaviour.cs +++ b/com.unity.netcode.gameobjects/Runtime/Components/Helpers/AttachableBehaviour.cs @@ -467,6 +467,8 @@ public void Attach(AttachableNode attachableNode) /// internal void InternalDetach() { + // 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) diff --git a/com.unity.netcode.gameobjects/Runtime/Core/NetworkBehaviour.cs b/com.unity.netcode.gameobjects/Runtime/Core/NetworkBehaviour.cs index 2005efa7c4..14f8ea5dfb 100644 --- a/com.unity.netcode.gameobjects/Runtime/Core/NetworkBehaviour.cs +++ b/com.unity.netcode.gameobjects/Runtime/Core/NetworkBehaviour.cs @@ -644,11 +644,10 @@ protected NetworkBehaviour GetNetworkBehaviour(ushort behaviourId) public ulong OwnerClientId { get; internal set; } /// - /// Returns true if the NetworkObject is in the middle of being destroyed or - /// if there is no valid assigned NetworkObject. + /// Returns true if the NetworkObject is in the middle of being destroyed. /// /// - /// + /// /// internal bool IsDestroying { get; private set; } @@ -675,6 +674,7 @@ protected internal virtual void OnIsDestroying() /// internal void SetIsDestroying() { + // We intentionally invoke this before setting the IsDestroying flag. OnIsDestroying(); IsDestroying = true; }