Skip to content

fix: client owned NetworkObject with prefabhandler destroy order incorrect on host-server side #3200

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Jan 10, 2025
Merged

fix: client owned NetworkObject with prefabhandler destroy order incorrect on host-server side #3200

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
6386627
fix
NoelStephensUnity Jan 10, 2025
2d3498b
update
NoelStephensUnity Jan 10, 2025
49195c5
update
NoelStephensUnity Jan 10, 2025
b8f93f6
test
NoelStephensUnity Jan 10, 2025
e22f14d
update
NoelStephensUnity Jan 10, 2025
f34d34e
style
NoelStephensUnity Jan 10, 2025
ab9b1b9
style
NoelStephensUnity Jan 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions com.unity.netcode.gameobjects/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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. (#3200)

### Changed


Expand Down
157 changes: 74 additions & 83 deletions com.unity.netcode.gameobjects/Runtime/Connection/NetworkConnectionManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
{
Expand All @@ -1171,106 +1167,101 @@ 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)
{
NetworkManager.SpawnManager.DespawnObject(ownedObject, true, true);
NetworkManager.PrefabHandler.HandleNetworkPrefabDestroy(clientOwnedObjects[i]);
}
else
{
NetworkManager.SpawnManager.DespawnObject(ownedObject, true, true);
// Don't destroy (prefab handler will determine this, but always notify
NetworkManager.SpawnManager.DespawnObject(ownedObject, false, 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<NetworkObject>();
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<NetworkObject>();
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)
{
Expand Down
77 changes: 77 additions & 0 deletions com.unity.netcode.gameobjects/Tests/Runtime/Prefabs/NetworkPrefabOverrideTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,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();
}
}

/// <summary>
/// Mock component for testing that the client-side player is using the right
/// network prefab.
Expand Down Expand Up @@ -95,7 +135,9 @@ protected override void OnServerAndClientsCreated()
{
// Create a NetworkPrefab with an override
var basePrefab = NetcodeIntegrationTestHelpers.CreateNetworkObject($"{k_PrefabRootName}-base", m_ServerNetworkManager, true);
basePrefab.AddComponent<SpawnDespawnDestroyNotifications>();
var targetPrefab = NetcodeIntegrationTestHelpers.CreateNetworkObject($"{k_PrefabRootName}-over", m_ServerNetworkManager, true);
targetPrefab.AddComponent<SpawnDespawnDestroyNotifications>();
m_PrefabOverride = new NetworkPrefab()
{
Prefab = basePrefab,
Expand Down Expand Up @@ -223,6 +265,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<NetworkObject>().GlobalObjectIdHash : m_PrefabOverride.SourcePrefabToOverride.GetComponent<NetworkObject>().GlobalObjectIdHash;
var expectedClientGlobalObjectIdHash = m_PrefabOverride.OverridingTargetPrefab.GetComponent<NetworkObject>().GlobalObjectIdHash;
Expand Down Expand Up @@ -263,6 +306,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<NetworkObject>().GlobalObjectIdHash : m_PrefabOverride.SourcePrefabToOverride.GetComponent<NetworkObject>().GlobalObjectIdHash;
expectedClientGlobalObjectIdHash = m_PrefabOverride.OverridingTargetPrefab.GetComponent<NetworkObject>().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: <see cref="SpawnDespawnDestroyNotifications"/>
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!");
}
}
}
Loading