Skip to content

Commit 53ec0e6

Browse files
NoelStephensUnityNoelStephensUnity
authored
fix: client owned NetworkObject with prefabhandler destroy order incorrect on host-server side (#3200)
* 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. * update adding change log entry. * 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). * test Updated NetworkPrefabOverrideTests to add a validation check for this PR * update Adding PR number to changelog entry. * style remove whitespaces * style remove unused namespace
1 parent ba35384 commit 53ec0e6

File tree

3 files changed

+153
-83
lines changed

3 files changed

+153
-83
lines changed

‎com.unity.netcode.gameobjects/CHANGELOG.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,8 @@ Additional documentation and release notes are available at [Multiplayer Documen
1212

1313
### Fixed
1414

15+
- 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)
16+
1517
### Changed
1618

1719

‎com.unity.netcode.gameobjects/Runtime/Connection/NetworkConnectionManager.cs‎

Lines changed: 74 additions & 83 deletions
Original file line numberDiff line numberDiff line change
@@ -1144,14 +1144,10 @@ internal void OnClientDisconnectFromServer(ulong clientId)
11441144

11451145
if (NetworkManager.PrefabHandler.ContainsHandler(playerObject.GlobalObjectIdHash))
11461146
{
1147-
if (NetworkManager.DAHost && NetworkManager.DistributedAuthorityMode)
1148-
{
1149-
NetworkManager.SpawnManager.DespawnObject(playerObject, true, NetworkManager.DistributedAuthorityMode);
1150-
}
1151-
else
1152-
{
1153-
NetworkManager.PrefabHandler.HandleNetworkPrefabDestroy(playerObject);
1154-
}
1147+
// Despawn but don't destroy. DA Host will act like the service and send despawn notifications.
1148+
NetworkManager.SpawnManager.DespawnObject(playerObject, false, NetworkManager.DistributedAuthorityMode);
1149+
// Let the prefab handler determine if it will be destroyed
1150+
NetworkManager.PrefabHandler.HandleNetworkPrefabDestroy(playerObject);
11551151
}
11561152
else if (playerObject.IsSpawned)
11571153
{
@@ -1171,106 +1167,101 @@ internal void OnClientDisconnectFromServer(ulong clientId)
11711167

11721168
// Get the NetworkObjects owned by the disconnected client
11731169
var clientOwnedObjects = NetworkManager.SpawnManager.SpawnedObjectsList.Where((c) => c.OwnerClientId == clientId).ToList();
1174-
if (clientOwnedObjects == null)
1175-
{
1176-
// This could happen if a client is never assigned a player object and is disconnected
1177-
// Only log this in verbose/developer mode
1178-
if (NetworkManager.LogLevel == LogLevel.Developer)
1179-
{
1180-
NetworkLog.LogWarning($"ClientID {clientId} disconnected with (0) zero owned objects! Was a player prefab not assigned?");
1181-
}
1182-
}
1183-
else
1170+
1171+
// Handle changing ownership and prefab handlers
1172+
var clientCounter = 0;
1173+
var predictedClientCount = ConnectedClientsList.Count - 1;
1174+
var remainingClients = NetworkManager.DistributedAuthorityMode ? ConnectedClientsList.Where((c) => c.ClientId != clientId).ToList() : null;
1175+
for (int i = clientOwnedObjects.Count - 1; i >= 0; i--)
11841176
{
1185-
// Handle changing ownership and prefab handlers
1186-
var clientCounter = 0;
1187-
var predictedClientCount = ConnectedClientsList.Count - 1;
1188-
var remainingClients = NetworkManager.DistributedAuthorityMode ? ConnectedClientsList.Where((c) => c.ClientId != clientId).ToList() : null;
1189-
for (int i = clientOwnedObjects.Count - 1; i >= 0; i--)
1177+
var ownedObject = clientOwnedObjects[i];
1178+
if (ownedObject != null)
11901179
{
1191-
var ownedObject = clientOwnedObjects[i];
1192-
if (ownedObject != null)
1180+
// If destroying with owner, then always despawn and destroy (or defer destroying to prefab handler)
1181+
if (!ownedObject.DontDestroyWithOwner)
11931182
{
1194-
if (!ownedObject.DontDestroyWithOwner)
1183+
if (NetworkManager.PrefabHandler.ContainsHandler(clientOwnedObjects[i].GlobalObjectIdHash))
11951184
{
1196-
if (NetworkManager.PrefabHandler.ContainsHandler(clientOwnedObjects[i].GlobalObjectIdHash))
1185+
if (ownedObject.IsSpawned)
11971186
{
1198-
NetworkManager.SpawnManager.DespawnObject(ownedObject, true, true);
1199-
NetworkManager.PrefabHandler.HandleNetworkPrefabDestroy(clientOwnedObjects[i]);
1200-
}
1201-
else
1202-
{
1203-
NetworkManager.SpawnManager.DespawnObject(ownedObject, true, true);
1187+
// Don't destroy (prefab handler will determine this, but always notify
1188+
NetworkManager.SpawnManager.DespawnObject(ownedObject, false, true);
12041189
}
1190+
NetworkManager.PrefabHandler.HandleNetworkPrefabDestroy(clientOwnedObjects[i]);
12051191
}
1206-
else if (!NetworkManager.ShutdownInProgress)
1192+
else
1193+
{
1194+
NetworkManager.SpawnManager.DespawnObject(ownedObject, true, true);
1195+
}
1196+
}
1197+
else if (!NetworkManager.ShutdownInProgress)
1198+
{
1199+
// NOTE: All of the below code only handles ownership transfer.
1200+
// For client-server, we just remove the ownership.
1201+
// For distributed authority, we need to change ownership based on parenting
1202+
if (NetworkManager.DistributedAuthorityMode)
12071203
{
1208-
// NOTE: All of the below code only handles ownership transfer.
1209-
// For client-server, we just remove the ownership.
1210-
// For distributed authority, we need to change ownership based on parenting
1211-
if (NetworkManager.DistributedAuthorityMode)
1204+
// Only NetworkObjects that have the OwnershipStatus.Distributable flag set and no parent
1205+
// (ownership is transferred to all children) will have their ownership redistributed.
1206+
if (ownedObject.IsOwnershipDistributable && ownedObject.GetCachedParent() == null && !ownedObject.IsOwnershipSessionOwner)
12121207
{
1213-
// Only NetworkObjects that have the OwnershipStatus.Distributable flag set and no parent
1214-
// (ownership is transferred to all children) will have their ownership redistributed.
1215-
if (ownedObject.IsOwnershipDistributable && ownedObject.GetCachedParent() == null && !ownedObject.IsOwnershipSessionOwner)
1208+
if (ownedObject.IsOwnershipLocked)
12161209
{
1217-
if (ownedObject.IsOwnershipLocked)
1210+
ownedObject.SetOwnershipLock(false);
1211+
}
1212+
1213+
// DANGO-TODO: We will want to match how the CMB service handles this. For now, we just try to evenly distribute
1214+
// ownership.
1215+
var targetOwner = NetworkManager.ServerClientId;
1216+
if (predictedClientCount > 1)
1217+
{
1218+
clientCounter++;
1219+
clientCounter = clientCounter % predictedClientCount;
1220+
targetOwner = remainingClients[clientCounter].ClientId;
1221+
}
1222+
if (EnableDistributeLogging)
1223+
{
1224+
Debug.Log($"[Disconnected][Client-{clientId}][NetworkObjectId-{ownedObject.NetworkObjectId} Distributed to Client-{targetOwner}");
1225+
}
1226+
NetworkManager.SpawnManager.ChangeOwnership(ownedObject, targetOwner, true);
1227+
// DANGO-TODO: Should we try handling inactive NetworkObjects?
1228+
// Ownership gets passed down to all children
1229+
var childNetworkObjects = ownedObject.GetComponentsInChildren<NetworkObject>();
1230+
foreach (var childObject in childNetworkObjects)
1231+
{
1232+
// We already changed ownership for this
1233+
if (childObject == ownedObject)
12181234
{
1219-
ownedObject.SetOwnershipLock(false);
1235+
continue;
12201236
}
1221-
1222-
// DANGO-TODO: We will want to match how the CMB service handles this. For now, we just try to evenly distribute
1223-
// ownership.
1224-
var targetOwner = NetworkManager.ServerClientId;
1225-
if (predictedClientCount > 1)
1237+
// If the client owner disconnected, it is ok to unlock this at this point in time.
1238+
if (childObject.IsOwnershipLocked)
12261239
{
1227-
clientCounter++;
1228-
clientCounter = clientCounter % predictedClientCount;
1229-
targetOwner = remainingClients[clientCounter].ClientId;
1240+
childObject.SetOwnershipLock(false);
12301241
}
1231-
if (EnableDistributeLogging)
1242+
1243+
// Ignore session owner marked objects
1244+
if (childObject.IsOwnershipSessionOwner)
12321245
{
1233-
Debug.Log($"[Disconnected][Client-{clientId}][NetworkObjectId-{ownedObject.NetworkObjectId} Distributed to Client-{targetOwner}");
1246+
continue;
12341247
}
1235-
NetworkManager.SpawnManager.ChangeOwnership(ownedObject, targetOwner, true);
1236-
// DANGO-TODO: Should we try handling inactive NetworkObjects?
1237-
// Ownership gets passed down to all children
1238-
var childNetworkObjects = ownedObject.GetComponentsInChildren<NetworkObject>();
1239-
foreach (var childObject in childNetworkObjects)
1248+
NetworkManager.SpawnManager.ChangeOwnership(childObject, targetOwner, true);
1249+
if (EnableDistributeLogging)
12401250
{
1241-
// We already changed ownership for this
1242-
if (childObject == ownedObject)
1243-
{
1244-
continue;
1245-
}
1246-
// If the client owner disconnected, it is ok to unlock this at this point in time.
1247-
if (childObject.IsOwnershipLocked)
1248-
{
1249-
childObject.SetOwnershipLock(false);
1250-
}
1251-
1252-
// Ignore session owner marked objects
1253-
if (childObject.IsOwnershipSessionOwner)
1254-
{
1255-
continue;
1256-
}
1257-
NetworkManager.SpawnManager.ChangeOwnership(childObject, targetOwner, true);
1258-
if (EnableDistributeLogging)
1259-
{
1260-
Debug.Log($"[Disconnected][Client-{clientId}][Child of {ownedObject.NetworkObjectId}][NetworkObjectId-{ownedObject.NetworkObjectId} Distributed to Client-{targetOwner}");
1261-
}
1251+
Debug.Log($"[Disconnected][Client-{clientId}][Child of {ownedObject.NetworkObjectId}][NetworkObjectId-{ownedObject.NetworkObjectId} Distributed to Client-{targetOwner}");
12621252
}
12631253
}
12641254
}
1265-
else
1266-
{
1267-
ownedObject.RemoveOwnership();
1268-
}
1255+
}
1256+
else
1257+
{
1258+
ownedObject.RemoveOwnership();
12691259
}
12701260
}
12711261
}
12721262
}
12731263

1264+
12741265
// TODO: Could(should?) be replaced with more memory per client, by storing the visibility
12751266
foreach (var sobj in NetworkManager.SpawnManager.SpawnedObjectsList)
12761267
{

‎com.unity.netcode.gameobjects/Tests/Runtime/Prefabs/NetworkPrefabOverrideTests.cs‎

Lines changed: 77 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,46 @@ public void Destroy(NetworkObject networkObject)
6161
}
6262
}
6363

64+
65+
internal class SpawnDespawnDestroyNotifications : NetworkBehaviour
66+
{
67+
68+
public int Despawned { get; private set; }
69+
public int Destroyed { get; private set; }
70+
71+
private bool m_WasSpawned;
72+
73+
private ulong m_LocalClientId;
74+
75+
public override void OnNetworkSpawn()
76+
{
77+
m_WasSpawned = true;
78+
m_LocalClientId = NetworkManager.LocalClientId;
79+
base.OnNetworkSpawn();
80+
}
81+
82+
public override void OnNetworkDespawn()
83+
{
84+
Assert.True(Destroyed == 0, $"{name} on client-{m_LocalClientId} should have a destroy invocation count of 0 but it is {Destroyed}!");
85+
Assert.True(Despawned == 0, $"{name} on client-{m_LocalClientId} should have a despawn invocation count of 0 but it is {Despawned}!");
86+
Despawned++;
87+
base.OnNetworkDespawn();
88+
}
89+
90+
public override void OnDestroy()
91+
{
92+
// When the original prefabs are destroyed, we want to ignore this check (those instances are never spawned)
93+
if (m_WasSpawned)
94+
{
95+
Assert.True(Despawned == 1, $"{name} on client-{m_LocalClientId} should have a despawn invocation count of 1 but it is {Despawned}!");
96+
Assert.True(Destroyed == 0, $"{name} on client-{m_LocalClientId} should have a destroy invocation count of 0 but it is {Destroyed}!");
97+
}
98+
Destroyed++;
99+
100+
base.OnDestroy();
101+
}
102+
}
103+
64104
/// <summary>
65105
/// Mock component for testing that the client-side player is using the right
66106
/// network prefab.
@@ -95,7 +135,9 @@ protected override void OnServerAndClientsCreated()
95135
{
96136
// Create a NetworkPrefab with an override
97137
var basePrefab = NetcodeIntegrationTestHelpers.CreateNetworkObject($"{k_PrefabRootName}-base", m_ServerNetworkManager, true);
138+
basePrefab.AddComponent<SpawnDespawnDestroyNotifications>();
98139
var targetPrefab = NetcodeIntegrationTestHelpers.CreateNetworkObject($"{k_PrefabRootName}-over", m_ServerNetworkManager, true);
140+
targetPrefab.AddComponent<SpawnDespawnDestroyNotifications>();
99141
m_PrefabOverride = new NetworkPrefab()
100142
{
101143
Prefab = basePrefab,
@@ -223,6 +265,7 @@ public IEnumerator PrefabOverrideTests()
223265
{
224266
networkManagerOwner = m_ClientNetworkManagers[0];
225267
}
268+
226269
// Clients and Host will spawn the OverridingTargetPrefab while a dedicated server will spawn the SourcePrefabToOverride
227270
var expectedServerGlobalObjectIdHash = networkManagerOwner.IsClient ? m_PrefabOverride.OverridingTargetPrefab.GetComponent<NetworkObject>().GlobalObjectIdHash : m_PrefabOverride.SourcePrefabToOverride.GetComponent<NetworkObject>().GlobalObjectIdHash;
228271
var expectedClientGlobalObjectIdHash = m_PrefabOverride.OverridingTargetPrefab.GetComponent<NetworkObject>().GlobalObjectIdHash;
@@ -263,6 +306,40 @@ bool ObjectSpawnedOnAllNetworkMangers()
263306

264307
yield return WaitForConditionOrTimeOut(ObjectSpawnedOnAllNetworkMangers);
265308
AssertOnTimeout($"The spawned prefab override validation failed!\n {builder}");
309+
310+
// 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
311+
expectedServerGlobalObjectIdHash = networkManagerOwner.IsClient ? m_PrefabOverride.OverridingTargetPrefab.GetComponent<NetworkObject>().GlobalObjectIdHash : m_PrefabOverride.SourcePrefabToOverride.GetComponent<NetworkObject>().GlobalObjectIdHash;
312+
expectedClientGlobalObjectIdHash = m_PrefabOverride.OverridingTargetPrefab.GetComponent<NetworkObject>().GlobalObjectIdHash;
313+
314+
spawnedInstance = NetworkObject.InstantiateAndSpawn(m_PrefabOverride.SourcePrefabToOverride, networkManagerOwner, m_ClientNetworkManagers[0].LocalClientId);
315+
316+
317+
yield return WaitForConditionOrTimeOut(ObjectSpawnedOnAllNetworkMangers);
318+
AssertOnTimeout($"The spawned prefab override validation failed!\n {builder}");
319+
var clientId = m_ClientNetworkManagers[0].LocalClientId;
320+
m_ClientNetworkManagers[0].Shutdown();
321+
322+
// Wait until all of the client's owned objects are destroyed
323+
// If no asserts occur, then the despawn & destroy order of operations and invocation count is correct
324+
/// For more information look at: <see cref="SpawnDespawnDestroyNotifications"/>
325+
bool ClientDisconnected(ulong clientId)
326+
{
327+
var clientOwnedObjects = m_ServerNetworkManager.SpawnManager.SpawnedObjects.Where((c) => c.Value.OwnerClientId == clientId).ToList();
328+
if (clientOwnedObjects.Count > 0)
329+
{
330+
return false;
331+
}
332+
333+
clientOwnedObjects = m_ClientNetworkManagers[1].SpawnManager.SpawnedObjects.Where((c) => c.Value.OwnerClientId == clientId).ToList();
334+
if (clientOwnedObjects.Count > 0)
335+
{
336+
return false;
337+
}
338+
return true;
339+
}
340+
341+
yield return WaitForConditionOrTimeOut(() => ClientDisconnected(clientId));
342+
AssertOnTimeout($"Timed out waiting for client to disconnect!");
266343
}
267344
}
268345
}

0 commit comments

Comments
 (0)