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

<!-- Replace this block with what this PR does and why. Describe what
you'd like reviewers to know, how you applied the engineering
principles, and any interesting tradeoffs made. Delete bullet points
below that don't apply, and update the changelog section as appropriate.
-->

<!-- Add short version of the JIRA ticket to the PR title (e.g. "feat:
new shiny feature [MTT-123]") -->
[MTTB-1041](https://jira.unity3d.com/browse/MTTB-1041)

<!-- Add RFC link here if applicable. -->

## Changelog

- Fixed: ChangeOwnership should never distribute to a client that is not
an observer of that object.

## Testing and Documentation

- No tests have been added.

<!-- Uncomment and mark items off with a * if this PR deprecates any
API:
### Deprecated API
- [ ] An `[Obsolete]` attribute was added along with a `(RemovedAfter
yyyy-mm-dd)` entry.
- [ ] An [api updater] was added.
- [ ] Deprecation of the API is explained in the CHANGELOG.
- [ ] The users can understand why this API was removed and what they
should use instead.
-->

Closes #3299

Author: EmandM

com.unity.netcode.gameobjects/CHANGELOG.md

@@ -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)

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

@@ -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;
@@ -1158,10 +1156,7 @@ internal void OnClientDisconnectFromServer(ulong clientId)
                     }
                     else if (!NetworkManager.ShutdownInProgress)
                     {
-                        if (!NetworkManager.ShutdownInProgress)
-                        {
-                            playerObject.RemoveOwnership();
-                        }
+                        playerObject.RemoveOwnership();
                     }
                 }
 
@@ -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)
@@ -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}");
                                     }
                                 }
                             }