Skip to content

fix: Never distribute an object to a client who is not an observer #3323

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 8 commits into from
Mar 6, 2025
Merged

fix: Never distribute an object to a client who is not an observer #3323

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
ce45556
fix: Never distribute an object to a client who is not an observer
EmandM Feb 25, 2025
8c7ecea
Update CHANGELOG
EmandM Feb 25, 2025
518f491
Add test for changing ownership to non-observer
EmandM Feb 27, 2025
1a82660
Merge branch 'develop-2.0.0' of https://github.com/Unity-Technologies…
EmandM Feb 27, 2025
0fd34c8
Simplify finding targetOwner onClientDisconnect
EmandM Mar 3, 2025
740a9b1
Merge branch 'develop-2.0.0' of https://github.com/Unity-Technologies…
EmandM Mar 3, 2025
ee28dde
Update search for owner of child object on disconnect
EmandM Mar 4, 2025
5f72361
Merge branch 'develop-2.0.0' of https://github.com/Unity-Technologies…
EmandM Mar 4, 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
1 change: 1 addition & 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 @@ -15,6 +15,7 @@ Additional documentation and release notes are available at [Multiplayer Documen

### Fixed

- Fixed `ChangeOwnership` changing ownership to clients that are not observers. This also happened with automated object distribution. (#3323)
- Fixed issue where `AnticipatedNetworkVariable` previous value returned by `AnticipatedNetworkVariable.OnAuthoritativeValueChanged` is updated correctly on the non-authoritative side. (#3306)
- Fixed `OnClientConnectedCallback` passing incorrect `clientId` when scene management is disabled. (#3312)
- Fixed issue where the `NetworkObject.Ownership` custom editor did not take the default "Everything" flag into consideration. (#3305)
Expand Down
126 changes: 73 additions & 53 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 @@ -1122,18 +1122,16 @@ internal void OnClientDisconnectFromServer(ulong clientId)
{
if (!playerObject.DontDestroyWithOwner)
{
// DANGO-TODO: This is something that would be best for CMB Service to handle as it is part of the disconnection process
// If a player NetworkObject is being despawned, make sure to remove all children if they are marked to not be destroyed
// with the owner.
if (NetworkManager.DistributedAuthorityMode && NetworkManager.DAHost)
if (NetworkManager.DAHost)
{
// Remove any children from the player object if they are not going to be destroyed with the owner
var childNetworkObjects = playerObject.GetComponentsInChildren<NetworkObject>();
foreach (var child in childNetworkObjects)
{
// TODO: We have always just removed all children, but we might think about changing this to preserve the nested child
// hierarchy.
if (child.DontDestroyWithOwner && child.transform.transform.parent != null)
// TODO: We have always just removed all children, but we might think about changing this to preserve the nested child hierarchy.
if (child.DontDestroyWithOwner && child.transform.transform.parent)
{
// If we are here, then we are running in DAHost mode and have the authority to remove the child from its parent
child.AuthorityAppliedParenting = true;
Expand All @@ -1158,10 +1156,7 @@ internal void OnClientDisconnectFromServer(ulong clientId)
}
else if (!NetworkManager.ShutdownInProgress)
{
if (!NetworkManager.ShutdownInProgress)
{
playerObject.RemoveOwnership();
}
playerObject.RemoveOwnership();
}
}

Expand All @@ -1175,7 +1170,7 @@ internal void OnClientDisconnectFromServer(ulong clientId)
for (int i = clientOwnedObjects.Count - 1; i >= 0; i--)
{
var ownedObject = clientOwnedObjects[i];
if (ownedObject != null)
if (ownedObject)
{
// If destroying with owner, then always despawn and destroy (or defer destroying to prefab handler)
if (!ownedObject.DontDestroyWithOwner)
Expand All @@ -1196,68 +1191,93 @@ internal void OnClientDisconnectFromServer(ulong clientId)
}
else if (!NetworkManager.ShutdownInProgress)
{
// DANGO-TODO: We will want to match how the CMB service handles this. For now, we just try to evenly distribute
// ownership.
// 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 are not OwnershipSessionOwner are distributed.
// If the object has a parent - skip it for now, it will be distributed when its root parent is distributed.
if (!ownedObject.IsOwnershipDistributable || ownedObject.IsOwnershipSessionOwner || ownedObject.GetCachedParent())
{
continue;
}

if (ownedObject.IsOwnershipLocked)
{
ownedObject.SetOwnershipLock(false);
}

var targetOwner = NetworkManager.ServerClientId;
// Cycle through the full count of clients to find
// the next viable owner. If none are found, then
// the DAHost defaults to the owner.
for (int j = 0; j < remainingClients.Count; j++)
{
if (ownedObject.IsOwnershipLocked)
clientCounter++;
clientCounter = clientCounter % predictedClientCount;
if (ownedObject.Observers.Contains(remainingClients[clientCounter].ClientId))
{
ownedObject.SetOwnershipLock(false);
targetOwner = remainingClients[clientCounter].ClientId;
break;
}
}
if (EnableDistributeLogging)
{
Debug.Log($"[Disconnected][Client-{clientId}][NetworkObjectId-{ownedObject.NetworkObjectId} Distributed to Client-{targetOwner}");
}

// 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)
NetworkManager.SpawnManager.ChangeOwnership(ownedObject, targetOwner, true);

// Ownership gets passed down to all children that have the same owner.
var childNetworkObjects = ownedObject.GetComponentsInChildren<NetworkObject>();
foreach (var childObject in childNetworkObjects)
{
// We already changed ownership for this
if (childObject == ownedObject)
{
clientCounter++;
clientCounter = clientCounter % predictedClientCount;
targetOwner = remainingClients[clientCounter].ClientId;
continue;
}
if (EnableDistributeLogging)
// If the client owner disconnected, it is ok to unlock this at this point in time.
if (childObject.IsOwnershipLocked)
{
Debug.Log($"[Disconnected][Client-{clientId}][NetworkObjectId-{ownedObject.NetworkObjectId} Distributed to Client-{targetOwner}");
childObject.SetOwnershipLock(false);
}
NetworkManager.SpawnManager.ChangeOwnership(ownedObject, targetOwner, true);
// DANGO-TODO: Should we try handling inactive NetworkObjects?
// Ownership gets passed down to all children that have the same owner.
var childNetworkObjects = ownedObject.GetComponentsInChildren<NetworkObject>();
foreach (var childObject in childNetworkObjects)

// Ignore session owner marked objects
if (childObject.IsOwnershipSessionOwner)
{
// 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);
}
continue;
}

// Ignore session owner marked objects
if (childObject.IsOwnershipSessionOwner)
{
continue;
}
// If the child's owner is not the client disconnected and the objects are marked with either distributable or transferable, then
// do not change ownership.
if (childObject.OwnerClientId != clientId && (childObject.IsOwnershipDistributable || childObject.IsOwnershipTransferable))
{
continue;
}

// If the child's owner is not the client disconnected and the objects are marked with either distributable or transferable, then
// do not change ownership.
if (childObject.OwnerClientId != clientId && (childObject.IsOwnershipDistributable || childObject.IsOwnershipTransferable))
var childOwner = targetOwner;
if (!childObject.Observers.Contains(childOwner))
{
for (int j = 0; j < remainingClients.Count; j++)
{
continue;
clientCounter++;
clientCounter = clientCounter % predictedClientCount;
if (ownedObject.Observers.Contains(remainingClients[clientCounter].ClientId))
{
childOwner = remainingClients[clientCounter].ClientId;
break;
}
}
}

NetworkManager.SpawnManager.ChangeOwnership(childObject, targetOwner, true);
if (EnableDistributeLogging)
{
Debug.Log($"[Disconnected][Client-{clientId}][Child of {ownedObject.NetworkObjectId}][NetworkObjectId-{ownedObject.NetworkObjectId} Distributed to Client-{targetOwner}");
}
NetworkManager.SpawnManager.ChangeOwnership(childObject, childOwner, true);
if (EnableDistributeLogging)
{
Debug.Log($"[Disconnected][Client-{clientId}][Child of {ownedObject.NetworkObjectId}][NetworkObjectId-{ownedObject.NetworkObjectId} Distributed to Client-{targetOwner}");
}
}
}
Expand Down
Loading