From 63866272e40244220f820e3744f24ee3cc664baf Mon Sep 17 00:00:00 2001 From: NoelStephensUnity Date: Fri, 10 Jan 2025 11:27:16 -0600 Subject: [PATCH 1/7] fix Pass in false for the destroy object parameter when cleaning up objects owned by a client upon the client disconnecting and the spawned objects have a prefab handler registered for their associated network prefab. --- .../Runtime/Connection/NetworkConnectionManager.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/com.unity.netcode.gameobjects/Runtime/Connection/NetworkConnectionManager.cs b/com.unity.netcode.gameobjects/Runtime/Connection/NetworkConnectionManager.cs index fe6bc4816d..50fa15cdd2 100644 --- a/com.unity.netcode.gameobjects/Runtime/Connection/NetworkConnectionManager.cs +++ b/com.unity.netcode.gameobjects/Runtime/Connection/NetworkConnectionManager.cs @@ -1195,7 +1195,7 @@ internal void OnClientDisconnectFromServer(ulong clientId) { if (NetworkManager.PrefabHandler.ContainsHandler(clientOwnedObjects[i].GlobalObjectIdHash)) { - NetworkManager.SpawnManager.DespawnObject(ownedObject, true, true); + NetworkManager.SpawnManager.DespawnObject(ownedObject, false, true); NetworkManager.PrefabHandler.HandleNetworkPrefabDestroy(clientOwnedObjects[i]); } else From 2d3498befe7656404a015e3438c202c7eeca76f6 Mon Sep 17 00:00:00 2001 From: NoelStephensUnity Date: Fri, 10 Jan 2025 11:30:57 -0600 Subject: [PATCH 2/7] update adding change log entry. --- com.unity.netcode.gameobjects/CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/com.unity.netcode.gameobjects/CHANGELOG.md b/com.unity.netcode.gameobjects/CHANGELOG.md index 423096ac35..7d37af25f1 100644 --- a/com.unity.netcode.gameobjects/CHANGELOG.md +++ b/com.unity.netcode.gameobjects/CHANGELOG.md @@ -12,6 +12,8 @@ Additional documentation and release notes are available at [Multiplayer Documen ### Fixed +- Fixed issue where a spawned `NetworkObject` that was registered with a prefab handler and owned by a client would invoke destroy more than once on the host-server side if the client disconnected while the `NetworkObject` was still spawned. + ### Changed From 49195c55f50f540b565e1ed733b53a6ec29d553e Mon Sep 17 00:00:00 2001 From: NoelStephensUnity Date: Fri, 10 Jan 2025 13:17:19 -0600 Subject: [PATCH 3/7] update Cleaning up the connection manager. Removing the warning about a client having nothing that it owns spawned (could be a valid design pattern and no reason to spam the message upon a client disconnecting if it is intentional). --- .../Connection/NetworkConnectionManager.cs | 155 +++++++++--------- 1 file changed, 73 insertions(+), 82 deletions(-) diff --git a/com.unity.netcode.gameobjects/Runtime/Connection/NetworkConnectionManager.cs b/com.unity.netcode.gameobjects/Runtime/Connection/NetworkConnectionManager.cs index 50fa15cdd2..5e30694bce 100644 --- a/com.unity.netcode.gameobjects/Runtime/Connection/NetworkConnectionManager.cs +++ b/com.unity.netcode.gameobjects/Runtime/Connection/NetworkConnectionManager.cs @@ -1144,14 +1144,10 @@ internal void OnClientDisconnectFromServer(ulong clientId) if (NetworkManager.PrefabHandler.ContainsHandler(playerObject.GlobalObjectIdHash)) { - if (NetworkManager.DAHost && NetworkManager.DistributedAuthorityMode) - { - NetworkManager.SpawnManager.DespawnObject(playerObject, true, NetworkManager.DistributedAuthorityMode); - } - else - { - NetworkManager.PrefabHandler.HandleNetworkPrefabDestroy(playerObject); - } + // Despawn but don't destroy. DA Host will act like the service and send despawn notifications. + NetworkManager.SpawnManager.DespawnObject(playerObject, false, NetworkManager.DistributedAuthorityMode); + // Let the prefab handler determine if it will be destroyed + NetworkManager.PrefabHandler.HandleNetworkPrefabDestroy(playerObject); } else if (playerObject.IsSpawned) { @@ -1171,105 +1167,100 @@ internal void OnClientDisconnectFromServer(ulong clientId) // Get the NetworkObjects owned by the disconnected client var clientOwnedObjects = NetworkManager.SpawnManager.SpawnedObjectsList.Where((c) => c.OwnerClientId == clientId).ToList(); - if (clientOwnedObjects == null) - { - // This could happen if a client is never assigned a player object and is disconnected - // Only log this in verbose/developer mode - if (NetworkManager.LogLevel == LogLevel.Developer) - { - NetworkLog.LogWarning($"ClientID {clientId} disconnected with (0) zero owned objects! Was a player prefab not assigned?"); - } - } - else + + // Handle changing ownership and prefab handlers + var clientCounter = 0; + var predictedClientCount = ConnectedClientsList.Count - 1; + var remainingClients = NetworkManager.DistributedAuthorityMode ? ConnectedClientsList.Where((c) => c.ClientId != clientId).ToList() : null; + for (int i = clientOwnedObjects.Count - 1; i >= 0; i--) { - // Handle changing ownership and prefab handlers - var clientCounter = 0; - var predictedClientCount = ConnectedClientsList.Count - 1; - var remainingClients = NetworkManager.DistributedAuthorityMode ? ConnectedClientsList.Where((c) => c.ClientId != clientId).ToList() : null; - for (int i = clientOwnedObjects.Count - 1; i >= 0; i--) + var ownedObject = clientOwnedObjects[i]; + if (ownedObject != null) { - var ownedObject = clientOwnedObjects[i]; - if (ownedObject != null) + // If destroying with owner, then always despawn and destroy (or defer destroying to prefab handler) + if (!ownedObject.DontDestroyWithOwner) { - if (!ownedObject.DontDestroyWithOwner) + if (NetworkManager.PrefabHandler.ContainsHandler(clientOwnedObjects[i].GlobalObjectIdHash)) { - if (NetworkManager.PrefabHandler.ContainsHandler(clientOwnedObjects[i].GlobalObjectIdHash)) + if (ownedObject.IsSpawned) { + // Don't destroy (prefab handler will determine this, but always notify NetworkManager.SpawnManager.DespawnObject(ownedObject, false, true); - NetworkManager.PrefabHandler.HandleNetworkPrefabDestroy(clientOwnedObjects[i]); - } - else - { - NetworkManager.SpawnManager.DespawnObject(ownedObject, true, true); } + NetworkManager.PrefabHandler.HandleNetworkPrefabDestroy(clientOwnedObjects[i]); } - else if (!NetworkManager.ShutdownInProgress) + else + { + NetworkManager.SpawnManager.DespawnObject(ownedObject, true, true); + } + } + else if (!NetworkManager.ShutdownInProgress) + { + // NOTE: All of the below code only handles ownership transfer. + // For client-server, we just remove the ownership. + // For distributed authority, we need to change ownership based on parenting + if (NetworkManager.DistributedAuthorityMode) { - // NOTE: All of the below code only handles ownership transfer. - // For client-server, we just remove the ownership. - // For distributed authority, we need to change ownership based on parenting - if (NetworkManager.DistributedAuthorityMode) + // Only NetworkObjects that have the OwnershipStatus.Distributable flag set and no parent + // (ownership is transferred to all children) will have their ownership redistributed. + if (ownedObject.IsOwnershipDistributable && ownedObject.GetCachedParent() == null && !ownedObject.IsOwnershipSessionOwner) { - // Only NetworkObjects that have the OwnershipStatus.Distributable flag set and no parent - // (ownership is transferred to all children) will have their ownership redistributed. - if (ownedObject.IsOwnershipDistributable && ownedObject.GetCachedParent() == null && !ownedObject.IsOwnershipSessionOwner) + if (ownedObject.IsOwnershipLocked) { - if (ownedObject.IsOwnershipLocked) + ownedObject.SetOwnershipLock(false); + } + + // DANGO-TODO: We will want to match how the CMB service handles this. For now, we just try to evenly distribute + // ownership. + var targetOwner = NetworkManager.ServerClientId; + if (predictedClientCount > 1) + { + clientCounter++; + clientCounter = clientCounter % predictedClientCount; + targetOwner = remainingClients[clientCounter].ClientId; + } + if (EnableDistributeLogging) + { + Debug.Log($"[Disconnected][Client-{clientId}][NetworkObjectId-{ownedObject.NetworkObjectId} Distributed to Client-{targetOwner}"); + } + NetworkManager.SpawnManager.ChangeOwnership(ownedObject, targetOwner, true); + // DANGO-TODO: Should we try handling inactive NetworkObjects? + // Ownership gets passed down to all children + var childNetworkObjects = ownedObject.GetComponentsInChildren(); + foreach (var childObject in childNetworkObjects) + { + // We already changed ownership for this + if (childObject == ownedObject) { - ownedObject.SetOwnershipLock(false); + continue; } - - // DANGO-TODO: We will want to match how the CMB service handles this. For now, we just try to evenly distribute - // ownership. - var targetOwner = NetworkManager.ServerClientId; - if (predictedClientCount > 1) + // If the client owner disconnected, it is ok to unlock this at this point in time. + if (childObject.IsOwnershipLocked) { - clientCounter++; - clientCounter = clientCounter % predictedClientCount; - targetOwner = remainingClients[clientCounter].ClientId; + childObject.SetOwnershipLock(false); } - if (EnableDistributeLogging) + + // Ignore session owner marked objects + if (childObject.IsOwnershipSessionOwner) { - Debug.Log($"[Disconnected][Client-{clientId}][NetworkObjectId-{ownedObject.NetworkObjectId} Distributed to Client-{targetOwner}"); + continue; } - NetworkManager.SpawnManager.ChangeOwnership(ownedObject, targetOwner, true); - // DANGO-TODO: Should we try handling inactive NetworkObjects? - // Ownership gets passed down to all children - var childNetworkObjects = ownedObject.GetComponentsInChildren(); - foreach (var childObject in childNetworkObjects) + NetworkManager.SpawnManager.ChangeOwnership(childObject, targetOwner, true); + if (EnableDistributeLogging) { - // We already changed ownership for this - if (childObject == ownedObject) - { - continue; - } - // If the client owner disconnected, it is ok to unlock this at this point in time. - if (childObject.IsOwnershipLocked) - { - childObject.SetOwnershipLock(false); - } - - // Ignore session owner marked objects - if (childObject.IsOwnershipSessionOwner) - { - continue; - } - NetworkManager.SpawnManager.ChangeOwnership(childObject, targetOwner, true); - if (EnableDistributeLogging) - { - Debug.Log($"[Disconnected][Client-{clientId}][Child of {ownedObject.NetworkObjectId}][NetworkObjectId-{ownedObject.NetworkObjectId} Distributed to Client-{targetOwner}"); - } + Debug.Log($"[Disconnected][Client-{clientId}][Child of {ownedObject.NetworkObjectId}][NetworkObjectId-{ownedObject.NetworkObjectId} Distributed to Client-{targetOwner}"); } } } - else - { - ownedObject.RemoveOwnership(); - } + } + else + { + ownedObject.RemoveOwnership(); } } } } + // TODO: Could(should?) be replaced with more memory per client, by storing the visibility foreach (var sobj in NetworkManager.SpawnManager.SpawnedObjectsList) From b8f93f661c3cadfcdf1fd156f6992ead74cbd3ce Mon Sep 17 00:00:00 2001 From: NoelStephensUnity Date: Fri, 10 Jan 2025 13:18:38 -0600 Subject: [PATCH 4/7] test Updated NetworkPrefabOverrideTests to add a validation check for this PR --- .../Prefabs/NetworkPrefabOverrideTests.cs | 78 +++++++++++++++++++ 1 file changed, 78 insertions(+) diff --git a/com.unity.netcode.gameobjects/Tests/Runtime/Prefabs/NetworkPrefabOverrideTests.cs b/com.unity.netcode.gameobjects/Tests/Runtime/Prefabs/NetworkPrefabOverrideTests.cs index 84d9c56fec..a9fdded910 100644 --- a/com.unity.netcode.gameobjects/Tests/Runtime/Prefabs/NetworkPrefabOverrideTests.cs +++ b/com.unity.netcode.gameobjects/Tests/Runtime/Prefabs/NetworkPrefabOverrideTests.cs @@ -1,4 +1,5 @@ using System.Collections; +using System.Collections.Generic; using System.Linq; using System.Text; using NUnit.Framework; @@ -61,6 +62,46 @@ public void Destroy(NetworkObject networkObject) } } + + internal class SpawnDespawnDestroyNotifications : NetworkBehaviour + { + + public int Despawned { get; private set; } + public int Destroyed { get; private set; } + + private bool m_WasSpawned; + + private ulong m_LocalClientId; + + public override void OnNetworkSpawn() + { + m_WasSpawned = true; + m_LocalClientId = NetworkManager.LocalClientId; + base.OnNetworkSpawn(); + } + + public override void OnNetworkDespawn() + { + Assert.True(Destroyed == 0, $"{name} on client-{m_LocalClientId} should have a destroy invocation count of 0 but it is {Destroyed}!"); + Assert.True(Despawned == 0, $"{name} on client-{m_LocalClientId} should have a despawn invocation count of 0 but it is {Despawned}!"); + Despawned++; + base.OnNetworkDespawn(); + } + + public override void OnDestroy() + { + // When the original prefabs are destroyed, we want to ignore this check (those instances are never spawned) + if (m_WasSpawned) + { + Assert.True(Despawned == 1, $"{name} on client-{m_LocalClientId} should have a despawn invocation count of 1 but it is {Despawned}!"); + Assert.True(Destroyed == 0, $"{name} on client-{m_LocalClientId} should have a destroy invocation count of 0 but it is {Destroyed}!"); + } + Destroyed++; + + base.OnDestroy(); + } + } + /// /// Mock component for testing that the client-side player is using the right /// network prefab. @@ -95,7 +136,9 @@ protected override void OnServerAndClientsCreated() { // Create a NetworkPrefab with an override var basePrefab = NetcodeIntegrationTestHelpers.CreateNetworkObject($"{k_PrefabRootName}-base", m_ServerNetworkManager, true); + basePrefab.AddComponent(); var targetPrefab = NetcodeIntegrationTestHelpers.CreateNetworkObject($"{k_PrefabRootName}-over", m_ServerNetworkManager, true); + targetPrefab.AddComponent(); m_PrefabOverride = new NetworkPrefab() { Prefab = basePrefab, @@ -223,6 +266,7 @@ public IEnumerator PrefabOverrideTests() { networkManagerOwner = m_ClientNetworkManagers[0]; } + // Clients and Host will spawn the OverridingTargetPrefab while a dedicated server will spawn the SourcePrefabToOverride var expectedServerGlobalObjectIdHash = networkManagerOwner.IsClient ? m_PrefabOverride.OverridingTargetPrefab.GetComponent().GlobalObjectIdHash : m_PrefabOverride.SourcePrefabToOverride.GetComponent().GlobalObjectIdHash; var expectedClientGlobalObjectIdHash = m_PrefabOverride.OverridingTargetPrefab.GetComponent().GlobalObjectIdHash; @@ -263,6 +307,40 @@ bool ObjectSpawnedOnAllNetworkMangers() yield return WaitForConditionOrTimeOut(ObjectSpawnedOnAllNetworkMangers); AssertOnTimeout($"The spawned prefab override validation failed!\n {builder}"); + + // Verify that the despawn and destroy order of operations is correct for client owned NetworkObjects and the nunmber of times each is invoked is correct + expectedServerGlobalObjectIdHash = networkManagerOwner.IsClient ? m_PrefabOverride.OverridingTargetPrefab.GetComponent().GlobalObjectIdHash : m_PrefabOverride.SourcePrefabToOverride.GetComponent().GlobalObjectIdHash; + expectedClientGlobalObjectIdHash = m_PrefabOverride.OverridingTargetPrefab.GetComponent().GlobalObjectIdHash; + + spawnedInstance = NetworkObject.InstantiateAndSpawn(m_PrefabOverride.SourcePrefabToOverride, networkManagerOwner, m_ClientNetworkManagers[0].LocalClientId); + + + yield return WaitForConditionOrTimeOut(ObjectSpawnedOnAllNetworkMangers); + AssertOnTimeout($"The spawned prefab override validation failed!\n {builder}"); + var clientId = m_ClientNetworkManagers[0].LocalClientId; + m_ClientNetworkManagers[0].Shutdown(); + + // Wait until all of the client's owned objects are destroyed + // If no asserts occur, then the despawn & destroy order of operations and invocation count is correct + /// For more information look at: + bool ClientDisconnected(ulong clientId) + { + var clientOwnedObjects = m_ServerNetworkManager.SpawnManager.SpawnedObjects.Where((c) => c.Value.OwnerClientId == clientId).ToList(); + if (clientOwnedObjects.Count > 0) + { + return false; + } + + clientOwnedObjects = m_ClientNetworkManagers[1].SpawnManager.SpawnedObjects.Where((c) => c.Value.OwnerClientId == clientId).ToList(); + if (clientOwnedObjects.Count > 0) + { + return false; + } + return true; + } + + yield return WaitForConditionOrTimeOut(() => ClientDisconnected(clientId)); + AssertOnTimeout($"Timed out waiting for client to disconnect!"); } } } From e22f14d892114cc6b08ac4f2659c5dd918fb1648 Mon Sep 17 00:00:00 2001 From: NoelStephensUnity Date: Fri, 10 Jan 2025 13:20:03 -0600 Subject: [PATCH 5/7] update Adding PR number to changelog entry. --- com.unity.netcode.gameobjects/CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/com.unity.netcode.gameobjects/CHANGELOG.md b/com.unity.netcode.gameobjects/CHANGELOG.md index 7d37af25f1..c20a80df87 100644 --- a/com.unity.netcode.gameobjects/CHANGELOG.md +++ b/com.unity.netcode.gameobjects/CHANGELOG.md @@ -12,7 +12,7 @@ Additional documentation and release notes are available at [Multiplayer Documen ### Fixed -- Fixed issue where a spawned `NetworkObject` that was registered with a prefab handler and owned by a client would invoke destroy more than once on the host-server side if the client disconnected while the `NetworkObject` was still spawned. +- Fixed issue where a spawned `NetworkObject` that was registered with a prefab handler and owned by a client would invoke destroy more than once on the host-server side if the client disconnected while the `NetworkObject` was still spawned. (#3200) ### Changed From f34d34e1e7c32b0261639d7e98fbcbf48b0d851b Mon Sep 17 00:00:00 2001 From: NoelStephensUnity Date: Fri, 10 Jan 2025 13:32:49 -0600 Subject: [PATCH 6/7] style remove whitespaces --- .../Runtime/Connection/NetworkConnectionManager.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/com.unity.netcode.gameobjects/Runtime/Connection/NetworkConnectionManager.cs b/com.unity.netcode.gameobjects/Runtime/Connection/NetworkConnectionManager.cs index 5e30694bce..06a07acfc4 100644 --- a/com.unity.netcode.gameobjects/Runtime/Connection/NetworkConnectionManager.cs +++ b/com.unity.netcode.gameobjects/Runtime/Connection/NetworkConnectionManager.cs @@ -1260,7 +1260,7 @@ internal void OnClientDisconnectFromServer(ulong clientId) } } } - + // TODO: Could(should?) be replaced with more memory per client, by storing the visibility foreach (var sobj in NetworkManager.SpawnManager.SpawnedObjectsList) From ab9b1b9b5d2886bf3823243a2b66fbb08a84d7f0 Mon Sep 17 00:00:00 2001 From: NoelStephensUnity Date: Fri, 10 Jan 2025 13:35:22 -0600 Subject: [PATCH 7/7] style remove unused namespace --- .../Tests/Runtime/Prefabs/NetworkPrefabOverrideTests.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/com.unity.netcode.gameobjects/Tests/Runtime/Prefabs/NetworkPrefabOverrideTests.cs b/com.unity.netcode.gameobjects/Tests/Runtime/Prefabs/NetworkPrefabOverrideTests.cs index a9fdded910..f016286ac0 100644 --- a/com.unity.netcode.gameobjects/Tests/Runtime/Prefabs/NetworkPrefabOverrideTests.cs +++ b/com.unity.netcode.gameobjects/Tests/Runtime/Prefabs/NetworkPrefabOverrideTests.cs @@ -1,5 +1,4 @@ using System.Collections; -using System.Collections.Generic; using System.Linq; using System.Text; using NUnit.Framework;