fix: player networkobject not added to client owned object list (#1801)

* fix

This fixes the issue with a player not being added to the client's owned objects list.  This also fixes the issue where dynamically spawned NetworkObjects were not getting added to the client's owned object list.
This fix migrates the table into NetworkSpawnManager, removes the OwnedObjects list from NetworkClient (replaces it with a method), and provides automatic assignment and de-assignment of ownership to the NetworkSpawnManager's OwnershipToObjectsTable.

* tests

Updates some of the ownership integration tests to the changes.

* Update and Style

Log message and exception additions.
White space removal.
Extra-LF removal.

* fix

removed extra '}' char.

* fix

Fix for server ownership invocation.

* style

whitespace

* fix

Fixing minor issue with NetworkTransform having ownership applied before OnNetworkSpawn being invoked.

* test

This is the test for this PR

* update `NetworkObjectOwnershipTests`

* fix

set previous owner whether removing or not.

* minor stuff

* fix

Set the ownership before invoking lost or gained ownership methods.

* style

* style

* fix and update

fixing some order of operations issues.
reverting back to OwnedObjects get.

* style

* update

removed the internal OwnerClientId.

* update

Removing nullable client id as a parameter in spawn methods.

* test

Minor update to the ownership test where it double checks the update to NetworkClient when checking if a client's player is included in the owned objects list.

* Update CHANGELOG.md

* style

Minor clean up.

* fix

Assure that clients that are neither the current or new owner of the NetworkObject update their properties to reflect the new owner.

* test

Added a test to verify that switching ownership between several clients works.

* style

updating small comment for clarity

* style

updated comments for clarity.

* minor update

Co-authored-by: Fatih Mar <mfatihmar@gmail.com>

Author: NoelStephensUnity

com.unity.netcode.gameobjects/CHANGELOG.md

@@ -13,6 +13,8 @@ Additional documentation and release notes are available at [Multiplayer Documen
 ### Changed
 
 ### Fixed
+
+- Fixed PlayerObjects and dynamically spawned NetworkObjects not being added to the NetworkClient's OwnedObjects (#1801)
 - Fixed issue where NetworkManager would continue starting even if the NetworkTransport selected failed. (#1780)
 - Fixed issue when spawning new player if an already existing player exists it does not remove IsPlayer from the previous player (#1779)
 - Fixed lack of notification that NetworkManager and NetworkObject cannot be added to the same GameObject with in-editor notifications (#1777)

com.unity.netcode.gameobjects/Components/NetworkTransform.cs

@@ -785,7 +785,7 @@ private void Initialize()
             {
                 m_ReplicatedNetworkState.SetDirty(true);
             }
-            else
+            else if (m_Transform != null)
             {
                 ApplyInterpolatedNetworkStateToTransform(m_ReplicatedNetworkState.Value, m_Transform);
             }

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

@@ -20,6 +20,17 @@ public class NetworkClient
         /// <summary>
         /// The NetworkObject's owned by this Client
         /// </summary>
-        public readonly List<NetworkObject> OwnedObjects = new List<NetworkObject>();
+        public List<NetworkObject> OwnedObjects
+        {
+            get
+            {
+                if (PlayerObject != null && PlayerObject.NetworkManager != null && PlayerObject.NetworkManager.IsListening)
+                {
+                    return PlayerObject.NetworkManager.SpawnManager.GetClientOwnedObjects(ClientId);
+                }
+
+                return new List<NetworkObject>();
+            }
+        }
     }
 }

com.unity.netcode.gameobjects/Runtime/Core/NetworkManager.cs

@@ -125,13 +125,11 @@ public bool OnVerifyCanSend(ulong destinationId, Type messageType, NetworkDelive
             public bool OnVerifyCanReceive(ulong senderId, Type messageType)
             {
                 if (m_NetworkManager.PendingClients.TryGetValue(senderId, out PendingClient client) &&
-                       (client.ConnectionState == PendingClient.State.PendingApproval ||
-                        (client.ConnectionState == PendingClient.State.PendingConnection &&
-                         messageType != typeof(ConnectionRequestMessage))))
+                    (client.ConnectionState == PendingClient.State.PendingApproval || (client.ConnectionState == PendingClient.State.PendingConnection && messageType != typeof(ConnectionRequestMessage))))
                 {
                     if (NetworkLog.CurrentLogLevel <= LogLevel.Normal)
                     {
-                        NetworkLog.LogWarning($"Message received from {nameof(senderId)}={senderId.ToString()} before it has been accepted");
+                        NetworkLog.LogWarning($"Message received from {nameof(senderId)}={senderId} before it has been accepted");
                     }
 
                     return false;
@@ -365,16 +363,14 @@ public IReadOnlyList<ulong> ConnectedClientsIds
         /// <param name="approved">Whether or not the client was approved</param>
         /// <param name="position">The position to spawn the client at. If null, the prefab position is used.</param>
         /// <param name="rotation">The rotation to spawn the client with. If null, the prefab position is used.</param>
-        public delegate void ConnectionApprovedDelegate(bool createPlayerObject, uint? playerPrefabHash, bool approved,
-            Vector3? position, Quaternion? rotation);
+        public delegate void ConnectionApprovedDelegate(bool createPlayerObject, uint? playerPrefabHash, bool approved, Vector3? position, Quaternion? rotation);
 
         /// <summary>
         /// The callback to invoke during connection approval
         /// </summary>
         public event Action<byte[], ulong, ConnectionApprovedDelegate> ConnectionApprovalCallback = null;
 
-        internal void InvokeConnectionApproval(byte[] payload, ulong clientId, ConnectionApprovedDelegate action) =>
-            ConnectionApprovalCallback?.Invoke(payload, clientId, action);
+        internal void InvokeConnectionApproval(byte[] payload, ulong clientId, ConnectionApprovedDelegate action) => ConnectionApprovalCallback?.Invoke(payload, clientId, action);
 
         /// <summary>
         /// The current NetworkConfig
@@ -1612,32 +1608,45 @@ private void OnClientDisconnectFromServer(ulong clientId)
                         }
                     }
 
-                    for (int i = networkClient.OwnedObjects.Count - 1; i >= 0; i--)
+                    // Get the NetworkObjects owned by the disconnected client
+                    var clientOwnedObjects = SpawnManager.GetClientOwnedObjects(clientId);
+                    if (clientOwnedObjects == null)
                     {
-                        var ownedObject = networkClient.OwnedObjects[i];
-                        if (ownedObject != null)
+                        // This could happen if a client is never assigned a player object and is disconnected
+                        // Only log this in verbose/developer mode
+                        if (LogLevel == LogLevel.Developer)
                         {
-                            if (!ownedObject.DontDestroyWithOwner)
+                            NetworkLog.LogWarning($"ClientID {clientId} disconnected with (0) zero owned objects!  Was a player prefab not assigned?");
+                        }
+                    }
+                    else
+                    {
+                        // Handle changing ownership and prefab handlers
+                        for (int i = clientOwnedObjects.Count - 1; i >= 0; i--)
+                        {
+                            var ownedObject = clientOwnedObjects[i];
+                            if (ownedObject != null)
                             {
-                                if (PrefabHandler.ContainsHandler(ConnectedClients[clientId].OwnedObjects[i]
-                                    .GlobalObjectIdHash))
+                                if (!ownedObject.DontDestroyWithOwner)
                                 {
-                                    PrefabHandler.HandleNetworkPrefabDestroy(ConnectedClients[clientId].OwnedObjects[i]);
+                                    if (PrefabHandler.ContainsHandler(clientOwnedObjects[i].GlobalObjectIdHash))
+                                    {
+                                        PrefabHandler.HandleNetworkPrefabDestroy(clientOwnedObjects[i]);
+                                    }
+                                    else
+                                    {
+                                        Destroy(ownedObject.gameObject);
+                                    }
                                 }
                                 else
                                 {
-                                    Destroy(ownedObject.gameObject);
+                                    ownedObject.RemoveOwnership();
                                 }
                             }
-                            else
-                            {
-                                ownedObject.RemoveOwnership();
-                            }
                         }
                     }
 
                     // TODO: Could(should?) be replaced with more memory per client, by storing the visibility
-
                     foreach (var sobj in SpawnManager.SpawnedObjectsList)
                     {
                         sobj.Observers.Remove(clientId);

com.unity.netcode.gameobjects/Runtime/Core/NetworkObject.cs

@@ -64,33 +64,7 @@ internal void GenerateGlobalObjectIdHash()
         /// <summary>
         /// Gets the ClientId of the owner of this NetworkObject
         /// </summary>
-        public ulong OwnerClientId
-        {
-            get
-            {
-                if (OwnerClientIdInternal == null)
-                {
-                    return NetworkManager != null ? NetworkManager.ServerClientId : 0;
-                }
-                else
-                {
-                    return OwnerClientIdInternal.Value;
-                }
-            }
-            internal set
-            {
-                if (NetworkManager != null && value == NetworkManager.ServerClientId)
-                {
-                    OwnerClientIdInternal = null;
-                }
-                else
-                {
-                    OwnerClientIdInternal = value;
-                }
-            }
-        }
-
-        internal ulong? OwnerClientIdInternal = null;
+        public ulong OwnerClientId { get; internal set; }
 
         /// <summary>
         /// If true, the object will always be replicated as root on clients and the parent will be ignored.
@@ -384,8 +358,8 @@ public static void NetworkHide(List<NetworkObject> networkObjects, ulong clientI
 
         private void OnDestroy()
         {
-            if (NetworkManager != null && NetworkManager.IsListening && NetworkManager.IsServer == false && IsSpawned
-                && (IsSceneObject == null || (IsSceneObject != null && IsSceneObject.Value != true)))
+            if (NetworkManager != null && NetworkManager.IsListening && NetworkManager.IsServer == false && IsSpawned &&
+                (IsSceneObject == null || (IsSceneObject != null && IsSceneObject.Value != true)))
             {
                 throw new NotServerException($"Destroy a spawned {nameof(NetworkObject)} on a non-host client is not valid. Call {nameof(Destroy)} or {nameof(Despawn)} on the server/host instead.");
             }
@@ -461,7 +435,7 @@ internal void SnapshotDespawn(ulong clientId)
         }
 
         [MethodImpl(MethodImplOptions.AggressiveInlining)]
-        private void SpawnInternal(bool destroyWithScene, ulong? ownerClientId, bool playerObject)
+        private void SpawnInternal(bool destroyWithScene, ulong ownerClientId, bool playerObject)
         {
             if (!NetworkManager.IsListening)
             {
@@ -480,7 +454,6 @@ private void SpawnInternal(bool destroyWithScene, ulong? ownerClientId, bool pla
                 SnapshotSpawn();
             }
 
-            ulong ownerId = ownerClientId != null ? ownerClientId.Value : NetworkManager.ServerClientId;
             for (int i = 0; i < NetworkManager.ConnectedClientsList.Count; i++)
             {
                 if (Observers.Contains(NetworkManager.ConnectedClientsList[i].ClientId))
@@ -496,7 +469,7 @@ private void SpawnInternal(bool destroyWithScene, ulong? ownerClientId, bool pla
         /// <param name="destroyWithScene">Should the object be destroyed when the scene is changed</param>
         public void Spawn(bool destroyWithScene = false)
         {
-            SpawnInternal(destroyWithScene, null, false);
+            SpawnInternal(destroyWithScene, NetworkManager.ServerClientId, false);
         }
 
         /// <summary>
@@ -547,6 +520,12 @@ public void ChangeOwnership(ulong newOwnerClientId)
 
         internal void InvokeBehaviourOnLostOwnership()
         {
+            // Server already handles this earlier, hosts should ignore
+            if (!NetworkManager.IsServer && NetworkManager.LocalClientId == OwnerClientId)
+            {
+                NetworkManager.SpawnManager.UpdateOwnershipTable(this, OwnerClientId, true);
+            }
+
             for (int i = 0; i < ChildNetworkBehaviours.Count; i++)
             {
                 ChildNetworkBehaviours[i].InternalOnLostOwnership();
@@ -555,6 +534,12 @@ internal void InvokeBehaviourOnLostOwnership()
 
         internal void InvokeBehaviourOnGainedOwnership()
         {
+            // Server already handles this earlier, hosts should ignore
+            if (!NetworkManager.IsServer && NetworkManager.LocalClientId == OwnerClientId)
+            {
+                NetworkManager.SpawnManager.UpdateOwnershipTable(this, OwnerClientId);
+            }
+
             for (int i = 0; i < ChildNetworkBehaviours.Count; i++)
             {
                 ChildNetworkBehaviours[i].InternalOnGainedOwnership();
@@ -793,6 +778,8 @@ internal static void CheckOrphanChildren()
 
         internal void InvokeBehaviourNetworkSpawn()
         {
+            NetworkManager.SpawnManager.UpdateOwnershipTable(this, OwnerClientId);
+
             for (int i = 0; i < ChildNetworkBehaviours.Count; i++)
             {
                 ChildNetworkBehaviours[i].InternalOnNetworkSpawn();
@@ -801,6 +788,8 @@ internal void InvokeBehaviourNetworkSpawn()
 
         internal void InvokeBehaviourNetworkDespawn()
         {
+            NetworkManager.SpawnManager.UpdateOwnershipTable(this, OwnerClientId, true);
+
             for (int i = 0; i < ChildNetworkBehaviours.Count; i++)
             {
                 ChildNetworkBehaviours[i].InternalOnNetworkDespawn();

com.unity.netcode.gameobjects/Runtime/Messaging/Messages/ChangeOwnershipMessage.cs

@@ -29,24 +29,33 @@ public bool Deserialize(FastBufferReader reader, ref NetworkContext context)
 
         public void Handle(ref NetworkContext context)
         {
-
             var networkManager = (NetworkManager)context.SystemOwner;
             var networkObject = networkManager.SpawnManager.SpawnedObjects[NetworkObjectId];
+            var originalOwner = networkObject.OwnerClientId;
+
+            networkObject.OwnerClientId = OwnerClientId;
 
-            if (networkObject.OwnerClientId == networkManager.LocalClientId)
+            // We are current owner.
+            if (originalOwner == networkManager.LocalClientId)
             {
-                //We are current owner.
                 networkObject.InvokeBehaviourOnLostOwnership();
             }
 
-            networkObject.OwnerClientId = OwnerClientId;
-
+            // We are new owner.
             if (OwnerClientId == networkManager.LocalClientId)
             {
-                //We are new owner.
                 networkObject.InvokeBehaviourOnGainedOwnership();
             }
 
+            // For all other clients that are neither the former or current owner, update the behaviours' properties
+            if (OwnerClientId != networkManager.LocalClientId && originalOwner != networkManager.LocalClientId)
+            {
+                for (int i = 0; i < networkObject.ChildNetworkBehaviours.Count; i++)
+                {
+                    networkObject.ChildNetworkBehaviours[i].UpdateNetworkProperties();
+                }
+            }
+
             networkManager.NetworkMetrics.TrackOwnershipChangeReceived(context.SenderId, networkObject, context.MessageSize);
         }
     }

com.unity.netcode.gameobjects/Runtime/SceneManagement/NetworkSceneManager.cs

@@ -1288,7 +1288,9 @@ private void OnServerLoadedScene(uint sceneEventId, Scene scene)
                 {
                     if (!keyValuePairBySceneHandle.Value.IsPlayerObject)
                     {
-                        m_NetworkManager.SpawnManager.SpawnNetworkObjectLocally(keyValuePairBySceneHandle.Value, m_NetworkManager.SpawnManager.GetNetworkObjectId(), true, false, null, true);
+                        // All in-scene placed NetworkObjects default to being owned by the server
+                        m_NetworkManager.SpawnManager.SpawnNetworkObjectLocally(keyValuePairBySceneHandle.Value,
+                            m_NetworkManager.SpawnManager.GetNetworkObjectId(), true, false, NetworkManager.ServerClientId, true);
                     }
                 }
             }