fix: hidden objects from newly promoted session owner still synchronize with newly joining clients (#3051)

NoelStephensUnity · web-flow · commit 91bb80eaff8b · 2024-09-07T13:45:28.000-05:00
* fix

This fixes the issue where a NetworkObject hidden from a client that is promoted to session owner will still be synchronized with newly joining clients.

* test

The test to validate the fix

* update

adding changelog entry

* style

Minor typo
diff --git a/com.unity.netcode.gameobjects/CHANGELOG.md b/com.unity.netcode.gameobjects/CHANGELOG.md
@@ -20,6 +20,7 @@ Additional documentation and release notes are available at [Multiplayer Documen
 
 ### Fixed
 
+- Fixed issue where a NetworkObject hidden from a client that is then promoted to be session owner was not being synchronized with newly joining clients.(#3051)
 - Fixed issue where setting a prefab hash value during connection approval but not having a player prefab assigned could cause an exception when spawning a player. (#3042)
 - Fixed issue where the `NetworkSpawnManager.HandleNetworkObjectShow` could throw an exception if one of the `NetworkObject` components to show was destroyed during the same frame. (#3030)
 - Fixed issue where the `NetworkManagerHelper` was continuing to check for hierarchy changes when in play mode. (#3026)
diff --git a/com.unity.netcode.gameobjects/Runtime/Connection/NetworkConnectionManager.cs b/com.unity.netcode.gameobjects/Runtime/Connection/NetworkConnectionManager.cs
@@ -985,10 +985,18 @@ internal NetworkClient AddClient(ulong clientId)
                 ConnectedClientIds.Add(clientId);
             }
 
+            var distributedAuthority = NetworkManager.DistributedAuthorityMode;
+            var sessionOwnerId = NetworkManager.CurrentSessionOwner;
+            var isSessionOwner = NetworkManager.LocalClient.IsSessionOwner;
             foreach (var networkObject in NetworkManager.SpawnManager.SpawnedObjectsList)
             {
                 if (networkObject.SpawnWithObservers)
                 {
+                    // Don't add the client to the observers if hidden from the session owner
+                    if (networkObject.IsOwner && distributedAuthority && !isSessionOwner && !networkObject.Observers.Contains(sessionOwnerId))
+                    {
+                        continue;
+                    }
                     networkObject.Observers.Add(clientId);
                 }
             }
diff --git a/com.unity.netcode.gameobjects/Runtime/Core/NetworkManager.cs b/com.unity.netcode.gameobjects/Runtime/Core/NetworkManager.cs
@@ -195,7 +195,6 @@ internal void SetSessionOwner(ulong sessionOwner)
             OnSessionOwnerPromoted?.Invoke(sessionOwner);
         }
 
-        // TODO: Make this internal after testing
         internal void PromoteSessionOwner(ulong clientId)
         {
             if (!DistributedAuthorityMode)
diff --git a/com.unity.netcode.gameobjects/Runtime/Messaging/Messages/ClientConnectedMessage.cs b/com.unity.netcode.gameobjects/Runtime/Messaging/Messages/ClientConnectedMessage.cs
@@ -55,6 +55,12 @@ public void Handle(ref NetworkContext context)
                 // Don't redistribute for the local instance
                 if (ClientId != networkManager.LocalClientId)
                 {
+                    // Show any NetworkObjects that are:
+                    // - Hidden from the session owner
+                    // - Owned by this client
+                    // - Has NetworkObject.SpawnWithObservers set to true (the default)
+                    networkManager.SpawnManager.ShowHiddenObjectsToNewlyJoinedClient(ClientId);
+
                     // We defer redistribution to the end of the NetworkUpdateStage.PostLateUpdate
                     networkManager.RedistributeToClient = true;
                     networkManager.ClientToRedistribute = ClientId;
diff --git a/com.unity.netcode.gameobjects/Runtime/SceneManagement/NetworkSceneManager.cs b/com.unity.netcode.gameobjects/Runtime/SceneManagement/NetworkSceneManager.cs
@@ -2550,17 +2550,6 @@ private void HandleSessionOwnerEvent(uint sceneEventId, ulong clientId)
                         // At this point the client is considered fully "connected"
                         if ((NetworkManager.DistributedAuthorityMode && NetworkManager.LocalClient.IsSessionOwner) || !NetworkManager.DistributedAuthorityMode)
                         {
-                            if (NetworkManager.DistributedAuthorityMode && !NetworkManager.DAHost)
-                            {
-                                // DANGO-EXP TODO: Remove this once service is sending the synchronization message to all clients
-                                if (NetworkManager.ConnectedClients.ContainsKey(clientId) && NetworkManager.ConnectionManager.ConnectedClientIds.Contains(clientId) && NetworkManager.ConnectedClientsList.Contains(NetworkManager.ConnectedClients[clientId]))
-                                {
-                                    EndSceneEvent(sceneEventId);
-                                    return;
-                                }
-                                NetworkManager.ConnectionManager.AddClient(clientId);
-                            }
-
                             // Notify the local server that a client has finished synchronizing
                             OnSceneEvent?.Invoke(new SceneEvent()
                             {
@@ -2575,6 +2564,20 @@ private void HandleSessionOwnerEvent(uint sceneEventId, ulong clientId)
                         }
                         else
                         {
+                            // Notify the local server that a client has finished synchronizing
+                            OnSceneEvent?.Invoke(new SceneEvent()
+                            {
+                                SceneEventType = sceneEventData.SceneEventType,
+                                SceneName = string.Empty,
+                                ClientId = clientId
+                            });
+
+                            // Show any NetworkObjects that are:
+                            // - Hidden from the session owner
+                            // - Owned by this client
+                            // - Has NetworkObject.SpawnWithObservers set to true (the default)
+                            NetworkManager.SpawnManager.ShowHiddenObjectsToNewlyJoinedClient(clientId);
+
                             // DANGO-EXP TODO: Remove this once service distributes objects
                             // Non-session owners receive this notification from newly connected clients and upon receiving
                             // the event they will redistribute their NetworkObjects
@@ -2589,9 +2592,6 @@ private void HandleSessionOwnerEvent(uint sceneEventId, ulong clientId)
                         // At this time the client is fully synchronized with all loaded scenes and
                         // NetworkObjects and should be considered "fully connected". Send the
                         // notification that the client is connected.
-                        // TODO 2023: We should have a better name for this or have multiple states the
-                        // client progresses through (the name and associated legacy behavior/expected state
-                        // of the client was persisted since MLAPI)
                         NetworkManager.ConnectionManager.InvokeOnClientConnectedCallback(clientId);
 
                         if (NetworkManager.IsHost)
@@ -2664,9 +2664,14 @@ internal void HandleSceneEvent(ulong clientId, FastBufferReader reader)
                                 EventData = sceneEventData,
                             };
                             // Forward synchronization to client then exit early because DAHost is not the current session owner
-                            NetworkManager.MessageManager.SendMessage(ref message, NetworkDelivery.ReliableFragmentedSequenced, NetworkManager.CurrentSessionOwner);
-                            EndSceneEvent(sceneEventData.SceneEventId);
-                            return;
+                            foreach (var client in NetworkManager.ConnectedClientsIds)
+                            {
+                                if (client == NetworkManager.LocalClientId)
+                                {
+                                    continue;
+                                }
+                                NetworkManager.MessageManager.SendMessage(ref message, NetworkDelivery.ReliableFragmentedSequenced, client);
+                            }
                         }
                     }
                     else
diff --git a/com.unity.netcode.gameobjects/Runtime/Spawning/NetworkSpawnManager.cs b/com.unity.netcode.gameobjects/Runtime/Spawning/NetworkSpawnManager.cs
@@ -1909,5 +1909,55 @@ internal void NotifyNetworkObjectsSynchronized()
                 networkObject.InternalNetworkSessionSynchronized();
             }
         }
+
+        /// <summary>
+        /// Distributed Authority Only
+        /// Should be invoked on non-session owner clients when a newly joined client is finished
+        /// synchronizing in order to "show" (spawn) anything that might be currently hidden from
+        /// the session owner.
+        /// </summary>
+        internal void ShowHiddenObjectsToNewlyJoinedClient(ulong newClientId)
+        {
+            if (!NetworkManager.DistributedAuthorityMode)
+            {
+                if (NetworkManager == null || !NetworkManager.ShutdownInProgress && NetworkManager.LogLevel <= LogLevel.Developer)
+                {
+                    Debug.LogWarning($"[Internal Error] {nameof(ShowHiddenObjectsToNewlyJoinedClient)} invoked while !");
+                }
+                return;
+            }
+
+            if (!NetworkManager.DistributedAuthorityMode)
+            {
+                Debug.LogError($"[Internal Error] {nameof(ShowHiddenObjectsToNewlyJoinedClient)} should only be invoked when using a distributed authority network topology!");
+                return;
+            }
+
+            if (NetworkManager.LocalClient.IsSessionOwner)
+            {
+                Debug.LogError($"[Internal Error] {nameof(ShowHiddenObjectsToNewlyJoinedClient)} should only be invoked on a non-session owner client!");
+                return;
+            }
+            var localClientId = NetworkManager.LocalClient.ClientId;
+            var sessionOwnerId = NetworkManager.CurrentSessionOwner;
+            foreach (var networkObject in SpawnedObjectsList)
+            {
+                if (networkObject.SpawnWithObservers && networkObject.OwnerClientId == localClientId && !networkObject.Observers.Contains(sessionOwnerId))
+                {
+                    if (networkObject.Observers.Contains(newClientId))
+                    {
+                        if (NetworkManager.LogLevel <= LogLevel.Developer)
+                        {
+                            // Track if there is some other location where the client is being added to the observers list when the object is hidden from the session owner
+                            Debug.LogWarning($"[{networkObject.name}] Has new client as an observer but it is hidden from the session owner!");
+                        }
+                        // For now, remove the client (impossible for the new client to have an instance since the session owner doesn't) to make sure newly added
+                        // code to handle this edge case works.
+                        networkObject.Observers.Remove(newClientId);
+                    }
+                    networkObject.NetworkShow(newClientId);
+                }
+            }
+        }
     }
 }
diff --git a/com.unity.netcode.gameobjects/Tests/Runtime/DistributedAuthority/ExtendedNetworkShowAndHideTests.cs b/com.unity.netcode.gameobjects/Tests/Runtime/DistributedAuthority/ExtendedNetworkShowAndHideTests.cs
@@ -0,0 +1,139 @@
+using System.Collections;
+using NUnit.Framework;
+using Unity.Netcode.TestHelpers.Runtime;
+using UnityEngine;
+using UnityEngine.TestTools;
+
+namespace Unity.Netcode.RuntimeTests
+{
+    [TestFixture(HostOrServer.DAHost)]
+    public class ExtendedNetworkShowAndHideTests : NetcodeIntegrationTest
+    {
+        protected override int NumberOfClients => 3;
+
+        private GameObject m_ObjectToSpawn;
+        private NetworkObject m_SpawnedObject;
+        private NetworkManager m_ClientToHideFrom;
+        private NetworkManager m_LateJoinClient;
+        private NetworkManager m_SpawnOwner;
+
+        public ExtendedNetworkShowAndHideTests(HostOrServer hostOrServer) : base(hostOrServer) { }
+
+        protected override void OnServerAndClientsCreated()
+        {
+            m_ObjectToSpawn = CreateNetworkObjectPrefab("TestObject");
+            m_ObjectToSpawn.SetActive(false);
+            base.OnServerAndClientsCreated();
+        }
+
+        private bool AllClientsSpawnedObject()
+        {
+            if (!UseCMBService())
+            {
+                if (!s_GlobalNetworkObjects.ContainsKey(m_ServerNetworkManager.LocalClientId))
+                {
+                    return false;
+                }
+                if (!s_GlobalNetworkObjects[m_ServerNetworkManager.LocalClientId].ContainsKey(m_SpawnedObject.NetworkObjectId))
+                {
+                    return false;
+                }
+            }
+
+            foreach (var client in m_ClientNetworkManagers)
+            {
+                if (!s_GlobalNetworkObjects.ContainsKey(client.LocalClientId))
+                {
+                    return false;
+                }
+                if (!s_GlobalNetworkObjects[client.LocalClientId].ContainsKey(m_SpawnedObject.NetworkObjectId))
+                {
+                    return false;
+                }
+            }
+            return true;
+        }
+
+        private bool IsClientPromotedToSessionOwner()
+        {
+            if (!UseCMBService())
+            {
+                if (m_ServerNetworkManager.CurrentSessionOwner != m_ClientToHideFrom.LocalClientId)
+                {
+                    return false;
+                }
+            }
+
+            foreach (var client in m_ClientNetworkManagers)
+            {
+                if (!client.IsConnectedClient)
+                {
+                    continue;
+                }
+                if (client.CurrentSessionOwner != m_ClientToHideFrom.LocalClientId)
+                {
+                    return false;
+                }
+            }
+            return true;
+        }
+
+        protected override void OnNewClientCreated(NetworkManager networkManager)
+        {
+            m_LateJoinClient = networkManager;
+
+            networkManager.NetworkConfig.Prefabs = m_SpawnOwner.NetworkConfig.Prefabs;
+            base.OnNewClientCreated(networkManager);
+        }
+
+        /// <summary>
+        /// This test validates the following NetworkShow - NetworkHide issue:
+        /// - During a session, a spawned object is hidden from a client.
+        /// - The current session owner disconnects and the client the object is hidden from is prommoted to the session owner.
+        /// - A new client joins and the newly promoted session owner synchronizes the newly joined client with only objects visible to it.
+        /// - Any already connected non-session owner client should "NetworkShow" the object to the newly connected client
+        /// (but only if the hidden object has SpawnWithObservers enabled)
+        /// </summary>
+        [UnityTest]
+        public IEnumerator HiddenObjectPromotedSessionOwnerNewClientSynchronizes()
+        {
+            // Get the test relative session owner
+            var sessionOwner = UseCMBService() ? m_ClientNetworkManagers[0] : m_ServerNetworkManager;
+            m_SpawnOwner = UseCMBService() ? m_ClientNetworkManagers[1] : m_ClientNetworkManagers[0];
+            m_ClientToHideFrom = UseCMBService() ? m_ClientNetworkManagers[NumberOfClients - 1] : m_ClientNetworkManagers[1];
+            m_ObjectToSpawn.SetActive(true);
+
+            // Spawn the object with a non-session owner client
+            m_SpawnedObject = SpawnObject(m_ObjectToSpawn, m_SpawnOwner).GetComponent<NetworkObject>();
+            yield return WaitForConditionOrTimeOut(AllClientsSpawnedObject);
+            AssertOnTimeout($"Not all clients spawned and instance of {m_SpawnedObject.name}");
+
+            // Hide the spawned object from the to be promoted session owner
+            m_SpawnedObject.NetworkHide(m_ClientToHideFrom.LocalClientId);
+
+            yield return WaitForConditionOrTimeOut(() => !m_ClientToHideFrom.SpawnManager.SpawnedObjects.ContainsKey(m_SpawnedObject.NetworkObjectId));
+            AssertOnTimeout($"{m_SpawnedObject.name} was not hidden from Client-{m_ClientToHideFrom.LocalClientId}!");
+
+            // Promoted a new session owner (DAHost promotes while CMB Session we disconnect the current session owner)
+            if (!UseCMBService())
+            {
+                m_ServerNetworkManager.PromoteSessionOwner(m_ClientToHideFrom.LocalClientId);
+            }
+            else
+            {
+                sessionOwner.Shutdown();
+            }
+
+            // Wait for the new session owner to be promoted and for all clients to acknowledge the promotion
+            yield return WaitForConditionOrTimeOut(IsClientPromotedToSessionOwner);
+            AssertOnTimeout($"Client-{m_ClientToHideFrom.LocalClientId} was not promoted as session owner on all client instances!");
+
+            // Connect a new client instance
+            yield return CreateAndStartNewClient();
+
+            // Assure the newly connected client is synchronized with the NetworkObject hidden from the newly promoted session owner
+            yield return WaitForConditionOrTimeOut(() => m_LateJoinClient.SpawnManager.SpawnedObjects.ContainsKey(m_SpawnedObject.NetworkObjectId));
+            AssertOnTimeout($"Client-{m_LateJoinClient.LocalClientId} never spawned {nameof(NetworkObject)} {m_SpawnedObject.name}!");
+        }
+    }
+}
diff --git a/com.unity.netcode.gameobjects/Tests/Runtime/DistributedAuthority/ExtendedNetworkShowAndHideTests.cs.meta b/com.unity.netcode.gameobjects/Tests/Runtime/DistributedAuthority/ExtendedNetworkShowAndHideTests.cs.meta

Original file line number	Diff line number	Diff line change
`@@ -985,10 +985,18 @@ internal NetworkClient AddClient(ulong clientId)`
`985`	`985`	`ConnectedClientIds.Add(clientId);`
`986`	`986`	`}`
`987`	`987`
	`988`	`+ var distributedAuthority = NetworkManager.DistributedAuthorityMode;`
	`989`	`+ var sessionOwnerId = NetworkManager.CurrentSessionOwner;`
	`990`	`+ var isSessionOwner = NetworkManager.LocalClient.IsSessionOwner;`
`988`	`991`	`foreach (var networkObject in NetworkManager.SpawnManager.SpawnedObjectsList)`
`989`	`992`	`{`
`990`	`993`	`if (networkObject.SpawnWithObservers)`
`991`	`994`	`{`
	`995`	`+ // Don't add the client to the observers if hidden from the session owner`
	`996`	`+ if (networkObject.IsOwner && distributedAuthority && !isSessionOwner && !networkObject.Observers.Contains(sessionOwnerId))`
	`997`	`+ {`
	`998`	`+ continue;`
	`999`	`+ }`
`992`	`1000`	`networkObject.Observers.Add(clientId);`
`993`	`1001`	`}`
`994`	`1002`	`}`
Original file line number	Diff line number	Diff line change
`@@ -195,7 +195,6 @@ internal void SetSessionOwner(ulong sessionOwner)`
`195`	`195`	`OnSessionOwnerPromoted?.Invoke(sessionOwner);`
`196`	`196`	`}`
`197`	`197`
`198`		`- // TODO: Make this internal after testing`
`199`	`198`	`internal void PromoteSessionOwner(ulong clientId)`
`200`	`199`	`{`
`201`	`200`	`if (!DistributedAuthorityMode)`