fix: NetworkClient.OwnedObjects always returning a count of zero (#2631)

* fix

This fixes the issue with NetworkClient.OwnedObjects always returning a zero count.

* style

removing TODO 2023-Q2 comments

* test

This validates the fixes for the owned objects list.

* update

removing an unrequired change.

* update

Adding the change log entry for this PR.

* test fix

Only test host for spawned objects locally, ignore that one portion when testing server only pass.

Author: NoelStephensUnity

com.unity.netcode.gameobjects/CHANGELOG.md

@@ -12,6 +12,7 @@ Additional documentation and release notes are available at [Multiplayer Documen
 
 ### Fixed
 
+- Fixed issue where `NetworkClient.OwnedObjects` was not returning any owned objects due to the `NetworkClient.IsConnected` not being properly set. (#2631)
 - Fixed a crash when calling TrySetParent with a null Transform (#2625)
 - Fixed issue where a `NetworkTransform` using full precision state updates was losing transform state updates when interpolation was enabled. (#2624)
 - Fixed issue where `NetworkObject.SpawnWithObservers` was not being honored for late joining clients. (#2623)

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

@@ -36,15 +36,11 @@ public class NetworkClient
         /// <summary>
         /// The ClientId of the NetworkClient
         /// </summary>
-        // TODO-2023-Q2: Determine if we want to make this property a public get and internal/private set
-        // There is no reason for a user to want to set this, but this will fail the package-validation-suite
         public ulong ClientId;
 
         /// <summary>
         /// The PlayerObject of the Client
         /// </summary>
-        // TODO-2023-Q2: Determine if we want to make this property a public get and internal/private set
-        // There is no reason for a user to want to set this, but this will fail the package-validation-suite
         public NetworkObject PlayerObject;
 
         /// <summary>

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

@@ -17,7 +17,6 @@ namespace Unity.Netcode
     /// - Processing <see cref="NetworkEvent"/>s.
     /// - Client Disconnection
     /// </summary>
-    // TODO 2023-Q2: Discuss what kind of public API exposure we want for this
     public sealed class NetworkConnectionManager
     {
 #if DEVELOPMENT_BUILD || UNITY_EDITOR
@@ -656,6 +655,7 @@ internal void HandleConnectionApproval(ulong ownerClientId, NetworkManager.Conne
                     // If scene management is disabled, then we are done and notify the local host-server the client is connected
                     if (!NetworkManager.NetworkConfig.EnableSceneManagement)
                     {
+                        NetworkManager.ConnectedClients[ownerClientId].IsConnected = true;
                         InvokeOnClientConnectedCallback(ownerClientId);
                     }
                     else // Otherwise, let NetworkSceneManager handle the initial scene and NetworkObject synchronization
@@ -667,6 +667,7 @@ internal void HandleConnectionApproval(ulong ownerClientId, NetworkManager.Conne
                 {
                     LocalClient = client;
                     NetworkManager.SpawnManager.UpdateObservedNetworkObjects(ownerClientId);
+                    LocalClient.IsConnected = true;
                 }
 
                 if (!response.CreatePlayerObject || (response.PlayerPrefabHash == null && NetworkManager.NetworkConfig.PlayerPrefab == null))
@@ -732,12 +733,10 @@ internal void ApprovedPlayerSpawn(ulong clientId, uint playerPrefabHash)
         internal NetworkClient AddClient(ulong clientId)
         {
             var networkClient = LocalClient;
-            if (clientId != NetworkManager.ServerClientId)
-            {
-                networkClient = new NetworkClient();
-                networkClient.SetRole(isServer: false, isClient: true, NetworkManager);
-                networkClient.ClientId = clientId;
-            }
+
+            networkClient = new NetworkClient();
+            networkClient.SetRole(clientId == NetworkManager.ServerClientId, isClient: true, NetworkManager);
+            networkClient.ClientId = clientId;
 
             ConnectedClients.Add(clientId, networkClient);
             ConnectedClientsList.Add(networkClient);
@@ -800,8 +799,7 @@ internal void OnClientDisconnectFromServer(ulong clientId)
                 }
                 else
                 {
-                    // Handle changing ownership and prefab handlers
-                    // TODO-2023: Look into whether in-scene placed NetworkObjects could be destroyed if ownership changes to a client
+                    // Handle changing ownership and prefab handlers                    
                     for (int i = clientOwnedObjects.Count - 1; i >= 0; i--)
                     {
                         var ownedObject = clientOwnedObjects[i];

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

@@ -118,7 +118,7 @@ internal void Shutdown()
             m_NetworkManager.NetworkTickSystem.Tick -= NetworkBehaviourUpdater_Tick;
         }
 
-        // TODO 2023-Q2: Order of operations requires NetworkVariable updates first then showing NetworkObjects
+        // Order of operations requires NetworkVariable updates first then showing NetworkObjects
         private void NetworkBehaviourUpdater_Tick()
         {
             // First update NetworkVariables

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

@@ -59,13 +59,12 @@ public void NetworkUpdate(NetworkUpdateStage updateStage)
                         // Metrics update needs to be driven by NetworkConnectionManager's update to assure metrics are dispatched after the send queue is processed.
                         MetricsManager.UpdateMetrics();
 
-                        // TODO 2023-Q2: Determine a better way to handle this
+                        // TODO: Determine a better way to handle this
                         NetworkObject.VerifyParentingStatus();
 
                         // This is "ok" to invoke when not processing messages since it is just cleaning up messages that never got handled within their timeout period.
                         DeferredMessageManager.CleanupStaleTriggers();
 
-                        // TODO 2023-Q2: Determine a better way to handle this
                         if (m_ShuttingDown)
                         {
                             ShutdownInternal();
@@ -834,9 +833,7 @@ public bool StartHost()
             }
 
             ConnectionManager.LocalClient.SetRole(true, true, this);
-
             Initialize(true);
-
             try
             {
                 IsListening = NetworkConfig.NetworkTransport.StartServer();

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

@@ -2191,6 +2191,10 @@ private void HandleServerSceneEvent(uint sceneEventId, ulong clientId)
                             ClientId = clientId
                         });
 
+                        // At this point the client is considered fully "connected"
+                        NetworkManager.ConnectedClients[clientId].IsConnected = true;
+
+                        // All scenes are synchronized, let the server know we are done synchronizing
                         OnSynchronizeComplete?.Invoke(clientId);
 
                         // At this time the client is fully synchronized with all loaded scenes and

com.unity.netcode.gameobjects/Runtime/Timing/NetworkTimeSystem.cs

@@ -9,9 +9,6 @@ namespace Unity.Netcode
     /// </summary>
     public class NetworkTimeSystem
     {
-        /// <summary>
-        /// TODO 2023-Q2: Not sure if this just needs to go away, but there is nothing that ever replaces this
-        /// </summary>
         /// <remarks>
         /// This was the original comment when it lived in NetworkManager:
         /// todo talk with UX/Product, find good default value for this

com.unity.netcode.gameobjects/Tests/Runtime/NetworkObject/NetworkObjectOwnershipTests.cs

@@ -294,5 +294,69 @@ bool WaitForClientsToSpawnNetworkObject()
             }
             serverComponent.ResetFlags();
         }
+
+        private const int k_NumberOfSpawnedObjects = 5;
+
+        private bool AllClientsHaveCorrectObjectCount()
+        {
+
+            foreach (var clientNetworkManager in m_ClientNetworkManagers)
+            {
+                if (clientNetworkManager.LocalClient.OwnedObjects.Count < k_NumberOfSpawnedObjects)
+                {
+                    return false;
+                }
+            }
+
+            return true;
+        }
+
+        private bool ServerHasCorrectClientOwnedObjectCount()
+        {
+            // Only check when we are the host
+            if (m_ServerNetworkManager.IsHost)
+            {
+                if (m_ServerNetworkManager.LocalClient.OwnedObjects.Count < k_NumberOfSpawnedObjects)
+                {
+                    return false;
+                }
+            }
+
+            foreach (var connectedClient in m_ServerNetworkManager.ConnectedClients)
+            {
+                if (connectedClient.Value.OwnedObjects.Count < k_NumberOfSpawnedObjects)
+                {
+                    return false;
+                }
+            }
+            return true;
+        }
+
+        [UnityTest]
+        public IEnumerator TestOwnedObjectCounts()
+        {
+            if (m_ServerNetworkManager.IsHost)
+            {
+                for (int i = 0; i < 5; i++)
+                {
+                    SpawnObject(m_OwnershipPrefab, m_ServerNetworkManager);
+                }
+            }
+
+            foreach (var clientNetworkManager in m_ClientNetworkManagers)
+            {
+                for (int i = 0; i < 5; i++)
+                {
+                    SpawnObject(m_OwnershipPrefab, clientNetworkManager);
+                }
+            }
+
+            yield return WaitForConditionOrTimeOut(AllClientsHaveCorrectObjectCount);
+            AssertOnTimeout($"Not all clients spawned {k_NumberOfSpawnedObjects} {nameof(NetworkObject)}s!");
+
+            yield return WaitForConditionOrTimeOut(ServerHasCorrectClientOwnedObjectCount);
+            AssertOnTimeout($"Server does not have the correct count for all clients spawned {k_NumberOfSpawnedObjects} {nameof(NetworkObject)}s!");
+
+        }
     }
 }