From 2dd2918e9bb6489b0256e9adf600f504535b2046 Mon Sep 17 00:00:00 2001 From: Noel Stephens Date: Fri, 3 Apr 2026 15:47:31 -0500 Subject: [PATCH 01/10] 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/10] 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/10] 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/10] 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/10] 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/10] 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/10] 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/10] 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/10] 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/10] 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)