diff --git a/com.unity.netcode.gameobjects/CHANGELOG.md b/com.unity.netcode.gameobjects/CHANGELOG.md index b92f716fc4..e2a06958ec 100644 --- a/com.unity.netcode.gameobjects/CHANGELOG.md +++ b/com.unity.netcode.gameobjects/CHANGELOG.md @@ -28,6 +28,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 7038e74cd1..f17ae300ec 100644 --- a/com.unity.netcode.gameobjects/Runtime/Components/Helpers/AttachableBehaviour.cs +++ b/com.unity.netcode.gameobjects/Runtime/Components/Helpers/AttachableBehaviour.cs @@ -261,23 +261,30 @@ internal void ForceDetach() ForceComponentChange(false, true); InternalDetach(); - // Notify of the changed attached state - NotifyAttachedStateChanged(m_AttachState, m_AttachableNode); - - m_AttachedNodeReference = new NetworkBehaviourReference(null); - // When detaching, we want to make our final action - // the invocation of the AttachableNode's Detach method. - if (m_AttachableNode) + if (m_AttachableNode != null && !m_AttachableNode.IsDestroying) { + // Notify of the changed attached state + NotifyAttachedStateChanged(m_AttachState, m_AttachableNode); + // Only notify of the detach if the node is still valid. m_AttachableNode.Detach(this); - m_AttachableNode = null; } + + m_AttachedNodeReference = new NetworkBehaviourReference(null); + m_AttachableNode = null; } /// public override void OnNetworkPreDespawn() { + // If the NetworkObject is being destroyed and not completely detached, then destroy the GameObject for + // this attachable since the associated default parent is being destroyed. + if (IsDestroying && m_AttachState != AttachState.Detached) + { + Destroy(gameObject); + return; + } + if (NetworkManager.ShutdownInProgress || AutoDetach.HasFlag(AutoDetachTypes.OnDespawn)) { ForceDetach(); @@ -286,7 +293,7 @@ public override void OnNetworkPreDespawn() } /// - /// 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() @@ -304,16 +311,18 @@ private void UpdateAttachedState() return; } - // If we are attached to some other AttachableNode, then detach from that before attaching to a new one. + // If we are attaching and already attached to some other AttachableNode, + // then detach from that before attaching to a new one. if (isAttaching && m_AttachableNode != null && m_AttachState == AttachState.Attached) { - // Run through the same process without being triggerd by a NetVar update. + // Detach the current attachable NotifyAttachedStateChanged(AttachState.Detaching, m_AttachableNode); InternalDetach(); NotifyAttachedStateChanged(AttachState.Detached, m_AttachableNode); - m_AttachableNode.Detach(this); m_AttachableNode = null; + + // Now attach the new attachable } // Change the state to attaching or detaching @@ -392,7 +401,8 @@ internal void ForceComponentChange(bool isAttaching, bool forcedChange) foreach (var componentControllerEntry in ComponentControllers) { - if (componentControllerEntry.AutoTrigger.HasFlag(triggerType)) + // Only if the component controller still exists and has the appropriate flag. + if (componentControllerEntry.ComponentController && componentControllerEntry.AutoTrigger.HasFlag(triggerType)) { componentControllerEntry.ComponentController.ForceChangeEnabled(componentControllerEntry.EnableOnAttach ? isAttaching : !isAttaching, forcedChange); } @@ -457,7 +467,9 @@ public void Attach(AttachableNode attachableNode) /// internal void InternalDetach() { - if (m_AttachableNode) + // If this instance is not in the middle of being destroyed, the attachable node is not null, and the node is not destroying + // =or= the scene it is located in is in the middle of being unloaded, then re-parent under the default parent. + if (!IsDestroying && m_AttachableNode && (!m_AttachableNode.IsDestroying || m_AttachableNode.gameObject.scene.isLoaded)) { if (m_DefaultParent) { @@ -553,12 +565,33 @@ private void UpdateAttachStateRpc(NetworkBehaviourReference attachedNodeReferenc /// 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 d1a2ca9c16..f8e08dda0a 100644 --- a/com.unity.netcode.gameobjects/Runtime/Components/Helpers/AttachableNode.cs +++ b/com.unity.netcode.gameobjects/Runtime/Components/Helpers/AttachableNode.cs @@ -71,20 +71,20 @@ public override void OnNetworkPreDespawn() { for (int i = m_AttachedBehaviours.Count - 1; i >= 0; i--) { - if (!m_AttachedBehaviours[i]) + var attachable = m_AttachedBehaviours[i]; + if (!attachable) { continue; } - // If we don't have authority but should detach on despawn, - // then proceed to detach. - if (!m_AttachedBehaviours[i].HasAuthority) + + if (attachable.HasAuthority && attachable.IsSpawned) { - m_AttachedBehaviours[i].ForceDetach(); + // Detach the normal way with authority + attachable.Detach(); } - else + else if (!attachable.HasAuthority || !attachable.IsDestroying) { - // Detach the normal way with authority - m_AttachedBehaviours[i].Detach(); + attachable.ForceDetach(); } } } @@ -93,12 +93,10 @@ public override void OnNetworkPreDespawn() internal override void InternalOnDestroy() { - // Notify any attached behaviours that this node is being destroyed. - for (int i = m_AttachedBehaviours.Count - 1; i >= 0; i--) + if (m_AttachedBehaviours.Count > 0) { - m_AttachedBehaviours[i]?.OnAttachNodeDestroy(); + OnIsDestroying(); } - m_AttachedBehaviours.Clear(); base.InternalOnDestroy(); } @@ -141,4 +139,21 @@ internal void Detach(AttachableBehaviour attachableBehaviour) m_AttachedBehaviours.Remove(attachableBehaviour); OnDetached(attachableBehaviour); } + + /// + /// 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 049f28070b..14f8ea5dfb 100644 --- a/com.unity.netcode.gameobjects/Runtime/Core/NetworkBehaviour.cs +++ b/com.unity.netcode.gameobjects/Runtime/Core/NetworkBehaviour.cs @@ -643,6 +643,42 @@ protected NetworkBehaviour GetNetworkBehaviour(ushort behaviourId) /// public ulong OwnerClientId { get; internal set; } + /// + /// Returns true if the NetworkObject is in the middle of being destroyed. + /// + /// + /// + /// + internal bool IsDestroying { get; private set; } + + /// + /// 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. + /// + 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() + { + // We intentionally invoke this before setting the IsDestroying flag. + OnIsDestroying(); + 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 c351f7d53b..7cfcc77be8 100644 --- a/com.unity.netcode.gameobjects/Runtime/Core/NetworkObject.cs +++ b/com.unity.netcode.gameobjects/Runtime/Core/NetworkObject.cs @@ -1688,8 +1688,52 @@ public static void NetworkHide(List networkObjects, ulong clientI } } + /// + /// Returns true if the NetworkObject is in the middle of being destroyed. + /// + /// + /// 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. + /// + internal void SetIsDestroying() + { + if (IsDestroying) + { + return; + } + + if (m_ChildNetworkBehaviours != null) + { + foreach (var childBehaviour in m_ChildNetworkBehaviours) + { + // Just ignore and continue processing through the entries + if (!childBehaviour) + { + continue; + } + + // Keeping the property a private set to assure this is + // the only way it can be set as it should never be reset + // back to false once invoked. + childBehaviour.SetIsDestroying(); + } + } + IsDestroying = true; + } + private void OnDestroy() { + // Apply the is destroying flag + SetIsDestroying(); + var networkManager = NetworkManager; // If no NetworkManager is assigned, then just exit early if (!networkManager) 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/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 928133e5f0..652f1d6983 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; + } } } @@ -1443,21 +1449,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); + } } } } @@ -1705,6 +1721,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 diff --git a/com.unity.netcode.gameobjects/Tests/Runtime/AttachableBehaviourTests.cs b/com.unity.netcode.gameobjects/Tests/Runtime/AttachableBehaviourTests.cs index 383651d446..3574e56efb 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"); @@ -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}"); } } @@ -676,10 +678,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 +785,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/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 new file mode 100644 index 0000000000..58d056d8d0 --- /dev/null +++ b/testproject/Assets/Tests/Runtime/NetworkSceneManager/AttachableBehaviourSceneLoadTests.cs @@ -0,0 +1,416 @@ +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 detach 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}!"); + 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) + { + // 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; + 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 + // 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