Skip to content

fix: client owned NetworkObject with prefabhandler destroy order incorrect on host-server side (Backport) #3202

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 6 commits into from
Jan 10, 2025
Merged

fix: client owned NetworkObject with prefabhandler destroy order incorrect on host-server side (Backport) #3202

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1aca8b0
fix
NoelStephensUnity Jan 10, 2025
59b963c
update
NoelStephensUnity Jan 10, 2025
3af04a1
test
NoelStephensUnity Jan 10, 2025
bf95825
update
NoelStephensUnity Jan 10, 2025
08b1a07
Merge branch 'develop' into fix/client-owned-networkobject-destroy-or…
NoelStephensUnity Jan 10, 2025
2ad215f
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
44 changes: 21 additions & 23 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 @@ -967,6 +967,11 @@ internal void OnClientDisconnectFromServer(ulong clientId)
{
if (NetworkManager.PrefabHandler.ContainsHandler(ConnectedClients[clientId].PlayerObject.GlobalObjectIdHash))
{
// If the player is spawned, then despawn before invoking HandleNetworkPrefabDestroy
if (playerObject.IsSpawned)
{
NetworkManager.SpawnManager.DespawnObject(ConnectedClients[clientId].PlayerObject, false);
}
NetworkManager.PrefabHandler.HandleNetworkPrefabDestroy(ConnectedClients[clientId].PlayerObject);
}
else if (playerObject.IsSpawned)
Expand All @@ -984,39 +989,32 @@ internal void OnClientDisconnectFromServer(ulong clientId)

// Get the NetworkObjects owned by the disconnected client
var clientOwnedObjects = NetworkManager.SpawnManager.GetClientOwnedObjects(clientId);
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 despawn & destroy or change ownership
for (int i = clientOwnedObjects.Count - 1; i >= 0; i--)
{
// Handle changing ownership and prefab handlers
for (int i = clientOwnedObjects.Count - 1; i >= 0; i--)
var ownedObject = clientOwnedObjects[i];
if (ownedObject != null)
{
var ownedObject = clientOwnedObjects[i];
if (ownedObject != null)
if (!ownedObject.DontDestroyWithOwner)
Copy link
Collaborator

@EmandM EmandM Jan 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a thought, this is a lot of nested ifs. What are your thoughts on nested ifs vs early exits in this type of Netcode context?

Copy link
Collaborator Author

@NoelStephensUnity NoelStephensUnity Jan 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I wouldn't exit early because there is other code that needs to run but I did change this to check if it is null and if so then it just continues.

Nesting doesn't bother me personally and in the end the op codes generated are pretty much identical where it ends up jumping back to the top and grabbing the next element (continue) or returning from the method (exit early).

However, it did remove one layer of a nested if which can be easier to read for users...so that is a good catch.

{
if (!ownedObject.DontDestroyWithOwner)
if (NetworkManager.PrefabHandler.ContainsHandler(clientOwnedObjects[i].GlobalObjectIdHash))
{
if (NetworkManager.PrefabHandler.ContainsHandler(clientOwnedObjects[i].GlobalObjectIdHash))
{
NetworkManager.PrefabHandler.HandleNetworkPrefabDestroy(clientOwnedObjects[i]);
}
else
if (ownedObject.IsSpawned)
{
Object.Destroy(ownedObject.gameObject);
NetworkManager.SpawnManager.DespawnObject(ownedObject, false);
}
NetworkManager.PrefabHandler.HandleNetworkPrefabDestroy(clientOwnedObjects[i]);
}
else if (!NetworkManager.ShutdownInProgress)
else
{
ownedObject.RemoveOwnership();
Object.Destroy(ownedObject.gameObject);
}
}
else if (!NetworkManager.ShutdownInProgress)
{
ownedObject.RemoveOwnership();
}
}
}

Expand Down
50 changes: 42 additions & 8 deletions com.unity.netcode.gameobjects/Runtime/Spawning/NetworkSpawnManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -435,10 +435,15 @@ internal NetworkObject InstantiateAndSpawnNoParameterChecks(NetworkObject networ

var networkObject = networkPrefab;
// 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.
if (forceOverride || NetworkManager.IsHost)
if (forceOverride || NetworkManager.IsHost || NetworkManager.PrefabHandler.ContainsHandler(networkPrefab.GlobalObjectIdHash))
{
networkObject = GetNetworkObjectToSpawn(networkPrefab.GlobalObjectIdHash, ownerClientId, position, rotation);
}
else // Under this case, server instantiate the prefab passed in.
{
networkObject = InstantiateNetworkPrefab(networkPrefab.gameObject, networkPrefab.GlobalObjectIdHash, position, rotation);
}

if (networkObject == null)
{
Debug.LogError($"Failed to instantiate and spawn {networkPrefab.name}!");
Expand All @@ -447,7 +452,15 @@ internal NetworkObject InstantiateAndSpawnNoParameterChecks(NetworkObject networ
networkObject.IsPlayerObject = isPlayerObject;
networkObject.transform.position = position;
networkObject.transform.rotation = rotation;
networkObject.SpawnWithOwnership(ownerClientId, destroyWithScene);
// If spawning as a player, then invoke SpawnAsPlayerObject
if (isPlayerObject)
{
networkObject.SpawnAsPlayerObject(ownerClientId, destroyWithScene);
}
else // Otherwise just spawn with ownership
{
networkObject.SpawnWithOwnership(ownerClientId, destroyWithScene);
}
return networkObject;
}

Expand Down Expand Up @@ -512,17 +525,38 @@ internal NetworkObject GetNetworkObjectToSpawn(uint globalObjectIdHash, ulong ow
}
else
{
// Create prefab instance
networkObject = UnityEngine.Object.Instantiate(networkPrefabReference).GetComponent<NetworkObject>();
networkObject.transform.position = position ?? networkObject.transform.position;
networkObject.transform.rotation = rotation ?? networkObject.transform.rotation;
networkObject.NetworkManagerOwner = NetworkManager;
networkObject.PrefabGlobalObjectIdHash = globalObjectIdHash;
// Create prefab instance while applying any pre-assigned position and rotation values
networkObject = InstantiateNetworkPrefab(networkPrefabReference, globalObjectIdHash, position, rotation);
}
}
return networkObject;
}

/// <summary>
/// Instantiates a network prefab instance, assigns the base prefab <see cref="NetworkObject.GlobalObjectIdHash"/>, positions, and orients
/// the instance.
/// !!! Should only be invoked by <see cref="GetNetworkObjectToSpawn"/> unless used by an integration test !!!
/// </summary>
/// <remarks>
/// <param name="prefabGlobalObjectIdHash"> should be the base prefab <see cref="NetworkObject.GlobalObjectIdHash"/> value and not the
/// overrided value.
/// (Can be used for integration testing)
/// </remarks>
/// <param name="networkPrefab">prefab to instantiate</param>
/// <param name="prefabGlobalObjectIdHash"><see cref="NetworkObject.GlobalObjectIdHash"/> of the base prefab instance</param>
/// <param name="position">conditional position in place of the network prefab's default position</param>
/// <param name="rotation">conditional rotation in place of the network prefab's default rotation</param>
/// <returns>the instance of the <see cref="NetworkObject"/></returns>
internal NetworkObject InstantiateNetworkPrefab(GameObject networkPrefab, uint prefabGlobalObjectIdHash, Vector3? position, Quaternion? rotation)
{
var networkObject = UnityEngine.Object.Instantiate(networkPrefab).GetComponent<NetworkObject>();
networkObject.transform.position = position ?? networkObject.transform.position;
networkObject.transform.rotation = rotation ?? networkObject.transform.rotation;
networkObject.NetworkManagerOwner = NetworkManager;
networkObject.PrefabGlobalObjectIdHash = prefabGlobalObjectIdHash;
return networkObject;
}

/// <summary>
/// Creates a local NetowrkObject to be spawned.
/// </summary>
Expand Down
31 changes: 24 additions & 7 deletions com.unity.netcode.gameobjects/TestHelpers/Runtime/NetcodeIntegrationTestHelpers.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -527,6 +527,29 @@ public static void MakeNetworkObjectTestPrefab(NetworkObject networkObject, uint
}
}

/// <summary>
/// Creates a <see cref="NetworkObject"/> to be used with integration testing
/// </summary>
/// <param name="baseName">namr of the object</param>
/// <param name="owner">owner of the object</param>
/// <param name="moveToDDOL">when true, the instance is automatically migrated into the DDOL</param>
/// <returns></returns>
internal static GameObject CreateNetworkObject(string baseName, NetworkManager owner, bool moveToDDOL = false)
{
var gameObject = new GameObject
{
name = baseName
};
var networkObject = gameObject.AddComponent<NetworkObject>();
networkObject.NetworkManagerOwner = owner;
MakeNetworkObjectTestPrefab(networkObject);
if (moveToDDOL)
{
Object.DontDestroyOnLoad(gameObject);
}
return gameObject;
}

public static GameObject CreateNetworkObjectPrefab(string baseName, NetworkManager server, params NetworkManager[] clients)
{
void AddNetworkPrefab(NetworkConfig config, NetworkPrefab prefab)
Expand All @@ -538,13 +561,7 @@ void AddNetworkPrefab(NetworkConfig config, NetworkPrefab prefab)
Assert.IsNotNull(server, prefabCreateAssertError);
Assert.IsFalse(server.IsListening, prefabCreateAssertError);

var gameObject = new GameObject
{
name = baseName
};
var networkObject = gameObject.AddComponent<NetworkObject>();
networkObject.NetworkManagerOwner = server;
MakeNetworkObjectTestPrefab(networkObject);
var gameObject = CreateNetworkObject(baseName, server);
var networkPrefab = new NetworkPrefab() { Prefab = gameObject };

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