diff --git a/com.unity.netcode.gameobjects/CHANGELOG.md b/com.unity.netcode.gameobjects/CHANGELOG.md index 937a0193b4..f522ca936a 100644 --- a/com.unity.netcode.gameobjects/CHANGELOG.md +++ b/com.unity.netcode.gameobjects/CHANGELOG.md @@ -6,6 +6,13 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/) Additional documentation and release notes are available at [Multiplayer Documentation](https://docs-multiplayer.unity3d.com). +## [Unreleased] + +### Fixed + +- Fixed issue where spawned objects with `NetworkObject.DontDestroyWithOwner` is set to `false` would not be destroyed when using a client-server network topology. (#3522) + + ## [2.4.2] - 2025-06-13 ### Fixed diff --git a/com.unity.netcode.gameobjects/Runtime/Connection/NetworkConnectionManager.cs b/com.unity.netcode.gameobjects/Runtime/Connection/NetworkConnectionManager.cs index ade21e09ce..cedb01630e 100644 --- a/com.unity.netcode.gameobjects/Runtime/Connection/NetworkConnectionManager.cs +++ b/com.unity.netcode.gameobjects/Runtime/Connection/NetworkConnectionManager.cs @@ -1178,7 +1178,7 @@ internal void OnClientDisconnectFromServer(ulong clientId) { // If destroying with owner, then always despawn and destroy (or defer destroying to prefab handler) // Handle an object with no observers other than the current disconnecting client as destroying with owner - if (!ownedObject.DontDestroyWithOwner && (ownedObject.Observers.Count == 0 || (ownedObject.Observers.Count == 1 && ownedObject.Observers.Contains(clientId)))) + if (!ownedObject.DontDestroyWithOwner && (ownedObject.Observers.Count == 0 || (ownedObject.Observers.Count >= 1 && ownedObject.Observers.Contains(clientId)))) { if (NetworkManager.PrefabHandler.ContainsHandler(ownedObject.GlobalObjectIdHash)) { diff --git a/com.unity.netcode.gameobjects/Tests/Runtime/NetworkObject/NetworkObjectDontDestroyWithOwnerTests.cs b/com.unity.netcode.gameobjects/Tests/Runtime/NetworkObject/NetworkObjectDontDestroyWithOwnerTests.cs index 9466148d38..749c90b111 100644 --- a/com.unity.netcode.gameobjects/Tests/Runtime/NetworkObject/NetworkObjectDontDestroyWithOwnerTests.cs +++ b/com.unity.netcode.gameobjects/Tests/Runtime/NetworkObject/NetworkObjectDontDestroyWithOwnerTests.cs @@ -1,5 +1,7 @@ using System.Collections; +using System.Collections.Generic; using System.Linq; +using System.Text; using NUnit.Framework; using Unity.Netcode.TestHelpers.Runtime; using UnityEngine; @@ -13,24 +15,26 @@ namespace Unity.Netcode.RuntimeTests [TestFixture(HostOrServer.Server)] internal class NetworkObjectDontDestroyWithOwnerTests : NetcodeIntegrationTest { - private const int k_NumberObjectsToSpawn = 32; - protected override int NumberOfClients => 1; + private const int k_NumberObjectsToSpawn = 16; + protected override int NumberOfClients => 3; - // TODO: [CmbServiceTests] Adapt to run with the service - protected override bool UseCMBService() - { - return false; - } - - protected GameObject m_PrefabToSpawn; + protected GameObject m_DestroyWithOwnerPrefab; + protected GameObject m_DontDestroyWithOwnerPrefab; protected GameObject m_PrefabNoObserversSpawn; + private ulong m_NonAuthorityClientId; + + private List m_DontDestroyObjectIds = new List(); + private List m_DestroyObjectIds = new List(); + public NetworkObjectDontDestroyWithOwnerTests(HostOrServer hostOrServer) : base(hostOrServer) { } protected override void OnServerAndClientsCreated() { - m_PrefabToSpawn = CreateNetworkObjectPrefab("ClientOwnedObject"); - m_PrefabToSpawn.GetComponent().DontDestroyWithOwner = true; + m_DontDestroyWithOwnerPrefab = CreateNetworkObjectPrefab("DestroyWith"); + m_DontDestroyWithOwnerPrefab.GetComponent().DontDestroyWithOwner = true; + + m_DestroyWithOwnerPrefab = CreateNetworkObjectPrefab("DontDestroyWith"); m_PrefabNoObserversSpawn = CreateNetworkObjectPrefab("NoObserversObject"); var prefabNoObserversNetworkObject = m_PrefabNoObserversSpawn.GetComponent(); @@ -38,47 +42,135 @@ protected override void OnServerAndClientsCreated() prefabNoObserversNetworkObject.DontDestroyWithOwner = true; } - [UnityTest] - public IEnumerator DontDestroyWithOwnerTest() + /// + /// Validates all instances of both prefab types have spawned on all clients. + /// + private bool HaveAllObjectInstancesSpawned(StringBuilder errorLog) + { + foreach (var networkManager in m_NetworkManagers) + { + var relativeSpawnedObjects = networkManager.SpawnManager.SpawnedObjects; + for (int i = 0; i < k_NumberObjectsToSpawn; i++) + { + var dontDestroyObjectId = m_DontDestroyObjectIds[i]; + var destroyObjectId = m_DestroyObjectIds[i]; + if (!relativeSpawnedObjects.ContainsKey(dontDestroyObjectId)) + { + errorLog.AppendLine($"[Client-{networkManager.LocalClientId}][DontDestroyWithOwner] Has not spawned {nameof(NetworkObject)}-{dontDestroyObjectId}!"); + } + if (!relativeSpawnedObjects.ContainsKey(destroyObjectId)) + { + errorLog.AppendLine($"[Client-{networkManager.LocalClientId}][DestroyWithOwner] Has not spawned {nameof(NetworkObject)}-{destroyObjectId}!"); + } + } + } + return errorLog.Length == 0; + } + + /// + /// Helper method to spawn the two different sets of objects. + /// Those that will destroy with the owner and those that will not. + /// + /// type of prefab to use for spawning + private void SpawnAllObjects(bool dontDestroyWithOwner) { - var client = m_ClientNetworkManagers[0]; - var clientId = client.LocalClientId; - var networkObjects = SpawnObjects(m_PrefabToSpawn, m_ClientNetworkManagers[0], k_NumberObjectsToSpawn); + var networkObjectIds = new List(); + var nonAuthority = m_NetworkManagers.Where((c) => c.LocalClientId == m_NonAuthorityClientId).First(); + var objectToSpawn = dontDestroyWithOwner ? m_DontDestroyWithOwnerPrefab : m_DestroyWithOwnerPrefab; + var spawnedObjects = SpawnObjects(objectToSpawn, nonAuthority, k_NumberObjectsToSpawn); + foreach (var spawnedObject in spawnedObjects) + { + networkObjectIds.Add(spawnedObject.GetComponent().NetworkObjectId); + } - // wait for object spawn on client to reach k_NumberObjectsToSpawn + 1 (k_NumberObjectsToSpawn and 1 for the player) - yield return WaitForConditionOrTimeOut(() => client.SpawnManager.GetClientOwnedObjects(clientId).Count() == k_NumberObjectsToSpawn + 1); - Assert.False(s_GlobalTimeoutHelper.TimedOut, $"Timed out waiting for client to have 33 NetworkObjects spawned! Only {client.SpawnManager.GetClientOwnedObjects(clientId).Count()} were assigned!"); + if (dontDestroyWithOwner) + { + m_DontDestroyObjectIds.Clear(); + m_DontDestroyObjectIds.AddRange(networkObjectIds); + } + else + { + m_DestroyObjectIds.Clear(); + m_DestroyObjectIds.AddRange(networkObjectIds); + } + } - // Since clients spawn their objects locally in distributed authority mode, we have to rebuild the list of the client - // owned objects on the (DAHost) server-side because when the client disconnects it will destroy its local instances. - if (m_DistributedAuthority) + /// + /// Validates that the non-authority owner client disconnection + /// was registered on all clients. + /// + private bool NonAuthorityHasDisconnected(StringBuilder errorLog) + { + foreach (var networkManager in m_NetworkManagers) { - networkObjects.Clear(); - var serversideClientOwnedObjects = m_ServerNetworkManager.SpawnManager.GetClientOwnedObjects(clientId); + if (!networkManager.IsListening) + { + continue; + } + + if (networkManager.ConnectedClientsIds.Contains(m_NonAuthorityClientId)) + { + errorLog.AppendLine($"[Client-{networkManager.LocalClientId}][ClientDisconnect] Still thinks Client-{m_NonAuthorityClientId} is connected!"); + } + } + return errorLog.Length == 0; + } - foreach (var networkObject in serversideClientOwnedObjects) + /// + /// The primary validation for the . + /// This validates that: + /// - Spawned objects that are set to destroy with the owner gets destroyed/despawned when the owning client disconnects. + /// - Spawned objects that are set to not destroy with the owner are not destroyed/despawned when the owning client disconnects. + /// + private bool ValidateDontDestroyWithOwner(StringBuilder errorLog) + { + foreach (var networkManager in m_NetworkManagers) + { + var relativeSpawnedObjects = networkManager.SpawnManager.SpawnedObjects; + for (int i = 0; i < k_NumberObjectsToSpawn; i++) { - if (!networkObject.IsPlayerObject) + var dontDestroyObjectId = m_DontDestroyObjectIds[i]; + var destroyObjectId = m_DestroyObjectIds[i]; + if (!relativeSpawnedObjects.ContainsKey(dontDestroyObjectId)) + { + errorLog.AppendLine($"[Client-{networkManager.LocalClientId}][DontDestroyWithOwner][!Despawned!] {nameof(NetworkObject)}-{dontDestroyObjectId} should not despawn upon the owner disconnecting!"); + } + if (relativeSpawnedObjects.ContainsKey(destroyObjectId)) { - networkObjects.Add(networkObject.gameObject); + errorLog.AppendLine($"[Client-{networkManager.LocalClientId}][DestroyWithOwner][!Not Despawned!] {nameof(NetworkObject)}-{destroyObjectId} should have despawned upon the owner disconnecting!"); } } } + return errorLog.Length == 0; + } - // disconnect the client that owns all the clients - NetcodeIntegrationTestHelpers.StopOneClient(client); + /// + /// This validates that: + /// - Spawned objects that are set to destroy with the owner gets destroyed/despawned when the owning client disconnects. + /// - Spawned objects that are set to not destroy with the owner are not destroyed/despawned when the owning client disconnects. + /// + [UnityTest] + public IEnumerator DontDestroyWithOwnerTest() + { + var authority = GetAuthorityNetworkManager(); + var nonAuthority = GetNonAuthorityNetworkManager(); + m_NonAuthorityClientId = nonAuthority.LocalClientId; + SpawnAllObjects(true); + SpawnAllObjects(false); - var remainingClients = Mathf.Max(0, TotalClients - 1); - // wait for disconnect - yield return WaitForConditionOrTimeOut(() => m_ServerNetworkManager.ConnectedClients.Count == remainingClients); - Assert.False(s_GlobalTimeoutHelper.TimedOut, "Timed out waiting for client to disconnect!"); + // This should never fail. + Assert.IsTrue(m_DontDestroyObjectIds.Count == m_DestroyObjectIds.Count, $"Mismatch in spawn count! ({m_DontDestroyObjectIds.Count}) vs ({m_DestroyObjectIds.Count})"); - for (int i = 0; i < networkObjects.Count; i++) - { - var networkObject = networkObjects[i].GetComponent(); - // ensure ownership was transferred back - Assert.That(networkObject.OwnerClientId == m_ServerNetworkManager.LocalClientId); - } + yield return WaitForConditionOrTimeOut(HaveAllObjectInstancesSpawned); + AssertOnTimeout($"Timed out waiting for all clients to spawn objects!"); + + yield return StopOneClient(nonAuthority); + + yield return WaitForConditionOrTimeOut(NonAuthorityHasDisconnected); + AssertOnTimeout($"Timed out waiting for all clients to register that Client-{m_NonAuthorityClientId} has disconnected!"); + + yield return WaitForConditionOrTimeOut(ValidateDontDestroyWithOwner); + AssertOnTimeout($"Timed out while validating the DontDestroyWithOwnerTest results!"); } [UnityTest] @@ -87,22 +179,55 @@ public IEnumerator NetworkShowThenClientDisconnects() var authorityManager = GetAuthorityNetworkManager(); var networkObject = SpawnObject(m_PrefabNoObserversSpawn, authorityManager).GetComponent(); var longWait = new WaitForSeconds(0.25f); + // Wait long enough to assure that no client receives the spawn notification yield return longWait; + + foreach (var networkManager in m_NetworkManagers) + { + // Skip the authority as it will have an instance + if (networkManager == authorityManager) + { + continue; + } + Assert.False(networkManager.SpawnManager.SpawnedObjects.ContainsKey(networkObject.NetworkObjectId), $"[Client-{networkManager.LocalClientId}]" + + $" Spawned an instance of {networkObject.name} when it was spawned with no observers!"); + } + + // Get a non-authority client, show the spawned object to it, and then make that client the owner. var nonAuthorityManager = GetNonAuthorityNetworkManager(); - Assert.False(nonAuthorityManager.SpawnManager.SpawnedObjects.ContainsKey(networkObject.NetworkObjectId), $"[Client-{nonAuthorityManager.LocalClientId}] " + - $"Already has an instance of {networkObject.name} when it should not!"); networkObject.NetworkShow(nonAuthorityManager.LocalClientId); + // This validates that the change in ownership is not sent when making a NetworkObject visible and changing the ownership + // within the same frame/callstack. networkObject.ChangeOwnership(nonAuthorityManager.LocalClientId); + // Verifies the object was spawned on the non-authority client and that the non-authority client is the owner. yield return WaitForConditionOrTimeOut(() => nonAuthorityManager.SpawnManager.SpawnedObjects.ContainsKey(networkObject.NetworkObjectId) && nonAuthorityManager.SpawnManager.SpawnedObjects[networkObject.NetworkObjectId].OwnerClientId == nonAuthorityManager.LocalClientId); AssertOnTimeout($"[Client-{nonAuthorityManager.LocalClientId}] Failed to spawn {networkObject.name} when it was shown!"); + foreach (var networkManager in m_NetworkManagers) + { + // Skip the authority and the non-authority client as they should now both have instances + if (networkManager == authorityManager || networkManager == nonAuthorityManager) + { + continue; + } + + // No other client should have an instance + Assert.False(networkManager.SpawnManager.SpawnedObjects.ContainsKey(networkObject.NetworkObjectId), $"[Client-{networkManager.LocalClientId}]" + + $" Spawned an instance of {networkObject.name} when it was shown to Client-{nonAuthorityManager.LocalClientId}!"); + } + + // Wait a few frames yield return s_DefaultWaitForTick; + // Shutdown the non-authority client to assure this does not cause the spawned object to despawn nonAuthorityManager.Shutdown(); + // Wait long enough to assure all messages generated from the client shutting down have been processed yield return longWait; + + // Validate the object is still spawned Assert.True(networkObject.IsSpawned, $"The spawned test prefab was despawned on the authority side when it shouldn't have been!"); } }