Skip to content

Commit eec1bdd

Browse files
NoelStephensUnityNoelStephensUnity
authored
fix: client owned NetworkObject with prefabhandler destroy order incorrect on host-server side (Backport) (#3202)
* 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. Cleaned up NetworkConnectionManager client disconnection a bit. * update migrating some NetworkSpawnManager updates from v2 to v1 and also fixing a few issues that were not back ported. * test Bringing the NetworkPrefabOverrideTests into v1. Updating the NetcodeIntegrtionTestHelpers with some v2 improvements. * update adding changelog entry. * style continue vs nested if update.
1 parent bd738b3 commit eec1bdd

File tree

6 files changed

+443
-40
lines changed

6 files changed

+443
-40
lines changed

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ 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. (#3202)
1516
- Fixed issue where `NetworkRigidBody2D` was still using the deprecated `isKinematic` property in Unity versions 2022.3 and newer. (#3199)
1617
- Fixed issue where an exception was thrown when calling `NetworkManager.Shutdown` after calling `UnityTransport.Shutdown`. (#3118)
1718

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

Lines changed: 24 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -967,6 +967,11 @@ internal void OnClientDisconnectFromServer(ulong clientId)
967967
{
968968
if (NetworkManager.PrefabHandler.ContainsHandler(ConnectedClients[clientId].PlayerObject.GlobalObjectIdHash))
969969
{
970+
// If the player is spawned, then despawn before invoking HandleNetworkPrefabDestroy
971+
if (playerObject.IsSpawned)
972+
{
973+
NetworkManager.SpawnManager.DespawnObject(ConnectedClients[clientId].PlayerObject, false);
974+
}
970975
NetworkManager.PrefabHandler.HandleNetworkPrefabDestroy(ConnectedClients[clientId].PlayerObject);
971976
}
972977
else if (playerObject.IsSpawned)
@@ -984,40 +989,34 @@ internal void OnClientDisconnectFromServer(ulong clientId)
984989

985990
// Get the NetworkObjects owned by the disconnected client
986991
var clientOwnedObjects = NetworkManager.SpawnManager.GetClientOwnedObjects(clientId);
987-
if (clientOwnedObjects == null)
992+
993+
// Handle despawn & destroy or change ownership
994+
for (int i = clientOwnedObjects.Count - 1; i >= 0; i--)
988995
{
989-
// This could happen if a client is never assigned a player object and is disconnected
990-
// Only log this in verbose/developer mode
991-
if (NetworkManager.LogLevel == LogLevel.Developer)
996+
var ownedObject = clientOwnedObjects[i];
997+
if (!ownedObject)
992998
{
993-
NetworkLog.LogWarning($"ClientID {clientId} disconnected with (0) zero owned objects! Was a player prefab not assigned?");
999+
continue;
9941000
}
995-
}
996-
else
997-
{
998-
// Handle changing ownership and prefab handlers
999-
for (int i = clientOwnedObjects.Count - 1; i >= 0; i--)
1001+
if (!ownedObject.DontDestroyWithOwner)
10001002
{
1001-
var ownedObject = clientOwnedObjects[i];
1002-
if (ownedObject != null)
1003+
if (NetworkManager.PrefabHandler.ContainsHandler(clientOwnedObjects[i].GlobalObjectIdHash))
10031004
{
1004-
if (!ownedObject.DontDestroyWithOwner)
1005+
if (ownedObject.IsSpawned)
10051006
{
1006-
if (NetworkManager.PrefabHandler.ContainsHandler(clientOwnedObjects[i].GlobalObjectIdHash))
1007-
{
1008-
NetworkManager.PrefabHandler.HandleNetworkPrefabDestroy(clientOwnedObjects[i]);
1009-
}
1010-
else
1011-
{
1012-
Object.Destroy(ownedObject.gameObject);
1013-
}
1014-
}
1015-
else if (!NetworkManager.ShutdownInProgress)
1016-
{
1017-
ownedObject.RemoveOwnership();
1007+
NetworkManager.SpawnManager.DespawnObject(ownedObject, false);
10181008
}
1009+
NetworkManager.PrefabHandler.HandleNetworkPrefabDestroy(clientOwnedObjects[i]);
1010+
}
1011+
else
1012+
{
1013+
Object.Destroy(ownedObject.gameObject);
10191014
}
10201015
}
1016+
else if (!NetworkManager.ShutdownInProgress)
1017+
{
1018+
ownedObject.RemoveOwnership();
1019+
}
10211020
}
10221021

10231022
// TODO: Could(should?) be replaced with more memory per client, by storing the visibility

‎com.unity.netcode.gameobjects/Runtime/Spawning/NetworkSpawnManager.cs‎

Lines changed: 42 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -435,10 +435,15 @@ internal NetworkObject InstantiateAndSpawnNoParameterChecks(NetworkObject networ
435435

436436
var networkObject = networkPrefab;
437437
// Host spawns the ovveride and server spawns the original prefab unless forceOverride is set to true where both server or host will spawn the override.
438-
if (forceOverride || NetworkManager.IsHost)
438+
if (forceOverride || NetworkManager.IsHost || NetworkManager.PrefabHandler.ContainsHandler(networkPrefab.GlobalObjectIdHash))
439439
{
440440
networkObject = GetNetworkObjectToSpawn(networkPrefab.GlobalObjectIdHash, ownerClientId, position, rotation);
441441
}
442+
else // Under this case, server instantiate the prefab passed in.
443+
{
444+
networkObject = InstantiateNetworkPrefab(networkPrefab.gameObject, networkPrefab.GlobalObjectIdHash, position, rotation);
445+
}
446+
442447
if (networkObject == null)
443448
{
444449
Debug.LogError($"Failed to instantiate and spawn {networkPrefab.name}!");
@@ -447,7 +452,15 @@ internal NetworkObject InstantiateAndSpawnNoParameterChecks(NetworkObject networ
447452
networkObject.IsPlayerObject = isPlayerObject;
448453
networkObject.transform.position = position;
449454
networkObject.transform.rotation = rotation;
450-
networkObject.SpawnWithOwnership(ownerClientId, destroyWithScene);
455+
// If spawning as a player, then invoke SpawnAsPlayerObject
456+
if (isPlayerObject)
457+
{
458+
networkObject.SpawnAsPlayerObject(ownerClientId, destroyWithScene);
459+
}
460+
else // Otherwise just spawn with ownership
461+
{
462+
networkObject.SpawnWithOwnership(ownerClientId, destroyWithScene);
463+
}
451464
return networkObject;
452465
}
453466

@@ -512,17 +525,38 @@ internal NetworkObject GetNetworkObjectToSpawn(uint globalObjectIdHash, ulong ow
512525
}
513526
else
514527
{
515-
// Create prefab instance
516-
networkObject = UnityEngine.Object.Instantiate(networkPrefabReference).GetComponent<NetworkObject>();
517-
networkObject.transform.position = position ?? networkObject.transform.position;
518-
networkObject.transform.rotation = rotation ?? networkObject.transform.rotation;
519-
networkObject.NetworkManagerOwner = NetworkManager;
520-
networkObject.PrefabGlobalObjectIdHash = globalObjectIdHash;
528+
// Create prefab instance while applying any pre-assigned position and rotation values
529+
networkObject = InstantiateNetworkPrefab(networkPrefabReference, globalObjectIdHash, position, rotation);
521530
}
522531
}
523532
return networkObject;
524533
}
525534

535+
/// <summary>
536+
/// Instantiates a network prefab instance, assigns the base prefab <see cref="NetworkObject.GlobalObjectIdHash"/>, positions, and orients
537+
/// the instance.
538+
/// !!! Should only be invoked by <see cref="GetNetworkObjectToSpawn"/> unless used by an integration test !!!
539+
/// </summary>
540+
/// <remarks>
541+
/// <param name="prefabGlobalObjectIdHash"> should be the base prefab <see cref="NetworkObject.GlobalObjectIdHash"/> value and not the
542+
/// overrided value.
543+
/// (Can be used for integration testing)
544+
/// </remarks>
545+
/// <param name="networkPrefab">prefab to instantiate</param>
546+
/// <param name="prefabGlobalObjectIdHash"><see cref="NetworkObject.GlobalObjectIdHash"/> of the base prefab instance</param>
547+
/// <param name="position">conditional position in place of the network prefab's default position</param>
548+
/// <param name="rotation">conditional rotation in place of the network prefab's default rotation</param>
549+
/// <returns>the instance of the <see cref="NetworkObject"/></returns>
550+
internal NetworkObject InstantiateNetworkPrefab(GameObject networkPrefab, uint prefabGlobalObjectIdHash, Vector3? position, Quaternion? rotation)
551+
{
552+
var networkObject = UnityEngine.Object.Instantiate(networkPrefab).GetComponent<NetworkObject>();
553+
networkObject.transform.position = position ?? networkObject.transform.position;
554+
networkObject.transform.rotation = rotation ?? networkObject.transform.rotation;
555+
networkObject.NetworkManagerOwner = NetworkManager;
556+
networkObject.PrefabGlobalObjectIdHash = prefabGlobalObjectIdHash;
557+
return networkObject;
558+
}
559+
526560
/// <summary>
527561
/// Creates a local NetowrkObject to be spawned.
528562
/// </summary>

‎com.unity.netcode.gameobjects/TestHelpers/Runtime/NetcodeIntegrationTestHelpers.cs‎

Lines changed: 24 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -527,6 +527,29 @@ public static void MakeNetworkObjectTestPrefab(NetworkObject networkObject, uint
527527
}
528528
}
529529

530+
/// <summary>
531+
/// Creates a <see cref="NetworkObject"/> to be used with integration testing
532+
/// </summary>
533+
/// <param name="baseName">namr of the object</param>
534+
/// <param name="owner">owner of the object</param>
535+
/// <param name="moveToDDOL">when true, the instance is automatically migrated into the DDOL</param>
536+
/// <returns></returns>
537+
internal static GameObject CreateNetworkObject(string baseName, NetworkManager owner, bool moveToDDOL = false)
538+
{
539+
var gameObject = new GameObject
540+
{
541+
name = baseName
542+
};
543+
var networkObject = gameObject.AddComponent<NetworkObject>();
544+
networkObject.NetworkManagerOwner = owner;
545+
MakeNetworkObjectTestPrefab(networkObject);
546+
if (moveToDDOL)
547+
{
548+
Object.DontDestroyOnLoad(gameObject);
549+
}
550+
return gameObject;
551+
}
552+
530553
public static GameObject CreateNetworkObjectPrefab(string baseName, NetworkManager server, params NetworkManager[] clients)
531554
{
532555
void AddNetworkPrefab(NetworkConfig config, NetworkPrefab prefab)
@@ -538,13 +561,7 @@ void AddNetworkPrefab(NetworkConfig config, NetworkPrefab prefab)
538561
Assert.IsNotNull(server, prefabCreateAssertError);
539562
Assert.IsFalse(server.IsListening, prefabCreateAssertError);
540563

541-
var gameObject = new GameObject
542-
{
543-
name = baseName
544-
};
545-
var networkObject = gameObject.AddComponent<NetworkObject>();
546-
networkObject.NetworkManagerOwner = server;
547-
MakeNetworkObjectTestPrefab(networkObject);
564+
var gameObject = CreateNetworkObject(baseName, server);
548565
var networkPrefab = new NetworkPrefab() { Prefab = gameObject };
549566

550567
// We could refactor this test framework to share a NetworkPrefabList instance, but at this point it's

0 commit comments

Comments
 (0)