fix: provide post synchronization scene unloading and validation [MTT-5743] (#2461)

NoelStephensUnity · web-flow · commit 6ddef4d26bf1 · 2023-03-23T00:40:08.000Z
* fix

Providing a way for users to not always unload all scenes when running in client synchronization mode additive.
Replaced &lt;br/&gt; with &lt;br /&gt; for docusaurus compatibility.

* update

adding warning if a client tries to change client synchronization mode and exit early.
adding warning if a server or host tries to change client synchronization mode after clients are connected and synchronized.

* test 

Generic testing of the fix with a more specific test coming.
tests for the warning messages when setting client synchronization mode.
diff --git a/com.unity.netcode.gameobjects/Runtime/SceneManagement/DefaultSceneManagerHandler.cs b/com.unity.netcode.gameobjects/Runtime/SceneManagement/DefaultSceneManagerHandler.cs
@@ -203,12 +203,11 @@ public void PopulateLoadedScenes(ref Dictionary<int, Scene> scenesLoaded, Networ
 
         /// <summary>
         /// Unloads any scenes that have not been assigned.
-        /// TODO: There needs to be a way to validate if a scene should be unloaded
-        /// or not (i.e. local client-side UI loaded additively)
         /// </summary>
         /// <param name="networkManager"></param>
         public void UnloadUnassignedScenes(NetworkManager networkManager = null)
         {
+            var sceneManager = networkManager.SceneManager;
             SceneManager.sceneUnloaded += SceneManager_SceneUnloaded;
             foreach (var sceneEntry in SceneNameToSceneHandles)
             {
@@ -217,7 +216,10 @@ public void UnloadUnassignedScenes(NetworkManager networkManager = null)
                 {
                     if (!sceneHandleEntry.Value.IsAssigned)
                     {
-                        m_ScenesToUnload.Add(sceneHandleEntry.Value.Scene);
+                        if (sceneManager.VerifySceneBeforeUnloading == null || sceneManager.VerifySceneBeforeUnloading.Invoke(sceneHandleEntry.Value.Scene))
+                        {
+                            m_ScenesToUnload.Add(sceneHandleEntry.Value.Scene);
+                        }
                     }
                 }
             }
@@ -325,6 +327,23 @@ public void MoveObjectsFromSceneToDontDestroyOnLoad(ref NetworkManager networkMa
         public void SetClientSynchronizationMode(ref NetworkManager networkManager, LoadSceneMode mode)
         {
             var sceneManager = networkManager.SceneManager;
+            // Don't let client's set this value
+            if (!networkManager.IsServer)
+            {
+                if (NetworkLog.CurrentLogLevel <= LogLevel.Normal)
+                {
+                    NetworkLog.LogWarning("Clients should not set this value as it is automatically synchronized with the server's setting!");
+                }
+                return;
+            }
+            else // Warn users if they are changing this after there are clients already connected and synchronized
+            if (networkManager.ConnectedClientsIds.Count > (networkManager.IsServer ? 0 : 1) && sceneManager.ClientSynchronizationMode != mode)
+            {
+                if (NetworkLog.CurrentLogLevel <= LogLevel.Normal)
+                {
+                    NetworkLog.LogWarning("Server is changing client synchronization mode after clients have been synchronized! It is recommended to do this before clients are connected!");
+                }
+            }
 
             // For additive client synchronization, we take into consideration scenes
             // already loaded.
diff --git a/com.unity.netcode.gameobjects/Runtime/SceneManagement/NetworkSceneManager.cs b/com.unity.netcode.gameobjects/Runtime/SceneManagement/NetworkSceneManager.cs
diff --git a/com.unity.netcode.gameobjects/TestHelpers/Runtime/IntegrationTestSceneHandler.cs b/com.unity.netcode.gameobjects/TestHelpers/Runtime/IntegrationTestSceneHandler.cs
@@ -625,26 +625,20 @@ public void PopulateLoadedScenes(ref Dictionary<int, Scene> scenesLoaded, Networ
 
         private Dictionary<Scene, NetworkManager> m_ScenesToUnload = new Dictionary<Scene, NetworkManager>();
 
-        // When true, any remaining scenes loaded will be unloaded
-        // TODO: There needs to be a way to validate if a scene should be unloaded
-        // or not (i.e. local client-side UI loaded additively)
-        public bool AllowUnassignedScenesToBeUnloaded = false;
-
         /// <summary>
         /// Handles unloading any scenes that might remain on a client that
         /// need to be unloaded.
         /// </summary>
         /// <param name="networkManager"></param>
         public void UnloadUnassignedScenes(NetworkManager networkManager = null)
         {
-            // Only if we are specifically testing this functionality
-            if (!AllowUnassignedScenesToBeUnloaded)
+            if (!SceneNameToSceneHandles.ContainsKey(networkManager))
             {
                 return;
             }
-
-            SceneManager.sceneUnloaded += SceneManager_SceneUnloaded;
             var relativeSceneNameToSceneHandles = SceneNameToSceneHandles[networkManager];
+            var sceneManager = networkManager.SceneManager;
+            SceneManager.sceneUnloaded += SceneManager_SceneUnloaded;
 
             foreach (var sceneEntry in relativeSceneNameToSceneHandles)
             {
@@ -653,10 +647,14 @@ public void UnloadUnassignedScenes(NetworkManager networkManager = null)
                 {
                     if (!sceneHandleEntry.Value.IsAssigned)
                     {
-                        m_ScenesToUnload.Add(sceneHandleEntry.Value.Scene, networkManager);
+                        if (sceneManager.VerifySceneBeforeUnloading == null || sceneManager.VerifySceneBeforeUnloading.Invoke(sceneHandleEntry.Value.Scene))
+                        {
+                            m_ScenesToUnload.Add(sceneHandleEntry.Value.Scene, networkManager);
+                        }
                     }
                 }
             }
+
             foreach (var sceneToUnload in m_ScenesToUnload)
             {
                 SceneManager.UnloadSceneAsync(sceneToUnload.Key);
@@ -775,8 +773,27 @@ public void MoveObjectsFromSceneToDontDestroyOnLoad(ref NetworkManager networkMa
         /// <param name="mode"><see cref="LoadSceneMode.Single"/> or <see cref="LoadSceneMode.Additive"/></param>
         public void SetClientSynchronizationMode(ref NetworkManager networkManager, LoadSceneMode mode)
         {
+
             var sceneManager = networkManager.SceneManager;
 
+            // Don't let client's set this value
+            if (!networkManager.IsServer)
+            {
+                if (NetworkLog.CurrentLogLevel <= LogLevel.Normal)
+                {
+                    NetworkLog.LogWarning("Clients should not set this value as it is automatically synchronized with the server's setting!");
+                }
+                return;
+            }
+            else if (networkManager.ConnectedClientsIds.Count > (networkManager.IsHost ? 1 : 0) && sceneManager.ClientSynchronizationMode != mode)
+            {
+                if (NetworkLog.CurrentLogLevel <= LogLevel.Normal)
+                {
+                    NetworkLog.LogWarning("Server is changing client synchronization mode after clients have been synchronized! It is recommended to do this before clients are connected!");
+                }
+            }
+
+
 
             // For additive client synchronization, we take into consideration scenes
             // already loaded.
diff --git a/com.unity.netcode.gameobjects/TestHelpers/Runtime/NetcodeIntegrationTestHelpers.cs b/com.unity.netcode.gameobjects/TestHelpers/Runtime/NetcodeIntegrationTestHelpers.cs
@@ -339,6 +339,12 @@ private static bool VerifySceneIsValidForClientsToLoad(int sceneIndex, string sc
             return true;
         }
 
+        private static bool VerifySceneIsValidForClientsToUnload(Scene scene)
+        {
+            // Unless specifically set, we always return false
+            return false;
+        }
+
         /// <summary>
         /// This registers scene validation callback for the server to prevent it from telling connecting
         /// clients to synchronize (i.e. load) the test runner scene.  This will also register the test runner
@@ -351,10 +357,21 @@ private static void SceneManagerValidationAndTestRunnerInitialization(NetworkMan
             if (networkManager.IsServer && networkManager.SceneManager.VerifySceneBeforeLoading == null)
             {
                 networkManager.SceneManager.VerifySceneBeforeLoading = VerifySceneIsValidForClientsToLoad;
+
                 // If a unit/integration test does not handle this on their own, then Ignore the validation warning
                 networkManager.SceneManager.DisableValidationWarnings(true);
             }
 
+            // For testing purposes, all clients always set the VerifySceneBeforeUnloading callback and enabled
+            // PostSynchronizationSceneUnloading. Where tests that expect clients to unload scenes should override
+            // the callback and return true for the scenes the client(s) is/are allowed to unload.
+            if (!networkManager.IsServer && networkManager.SceneManager.VerifySceneBeforeUnloading == null)
+            {
+                networkManager.SceneManager.VerifySceneBeforeUnloading = VerifySceneIsValidForClientsToUnload;
+                networkManager.SceneManager.PostSynchronizationSceneUnloading = true;
+            }
+
+
             // Register the test runner scene so it will be able to synchronize NetworkObjects without logging a
             // warning about using the currently active scene
             var scene = SceneManager.GetActiveScene();
diff --git a/testproject/Assets/Tests/Runtime/NetworkSceneManager/NetworkSceneManagerEventDataPoolTest.cs b/testproject/Assets/Tests/Runtime/NetworkSceneManager/NetworkSceneManagerEventDataPoolTest.cs
@@ -10,12 +10,18 @@
 
 namespace TestProject.RuntimeTests
 {
-    [TestFixture(HostOrServer.Host)]
-    [TestFixture(HostOrServer.Server)]
+    [TestFixture(HostOrServer.Host, LoadSceneMode.Single)]
+    [TestFixture(HostOrServer.Host, LoadSceneMode.Additive)]
+    [TestFixture(HostOrServer.Server, LoadSceneMode.Single)]
+    [TestFixture(HostOrServer.Server, LoadSceneMode.Additive)]
     public class NetworkSceneManagerEventDataPoolTest : NetcodeIntegrationTest
     {
         protected override int NumberOfClients => 4;
-        public NetworkSceneManagerEventDataPoolTest(HostOrServer hostOrServer) : base(hostOrServer) { }
+
+        public NetworkSceneManagerEventDataPoolTest(HostOrServer hostOrServer, LoadSceneMode loadSceneMode) : base(hostOrServer)
+        {
+            m_LoadSceneMode = loadSceneMode;
+        }
 
         private const string k_BaseUnitTestSceneName = "UnitTestBaseScene";
         private const string k_MultiInstanceTestScenename = "AdditiveSceneMultiInstance";
@@ -56,6 +62,7 @@ protected override IEnumerator OnStartedServerAndClients()
             m_ServerVerificationAction = DataPoolVerifySceneServer;
             m_ServerNetworkManager.SceneManager.OnSceneEvent += ServerSceneManager_OnSceneEvent;
             m_ServerNetworkManager.SceneManager.DisableValidationWarnings(true);
+            m_ServerNetworkManager.SceneManager.SetClientSynchronizationMode(m_LoadSceneMode);
             foreach (var client in m_ClientNetworkManagers)
             {
                 client.SceneManager.ClientSynchronizationMode = m_LoadSceneMode;
@@ -142,17 +149,15 @@ private void ResetWait()
         /// <summary>
         /// Initializes the m_ShouldWaitList
         /// </summary>
-        private void InitializeSceneTestInfo(LoadSceneMode clientSynchronizationMode, bool enableSceneVerification = false)
+        private void InitializeSceneTestInfo(bool enableSceneVerification = false)
         {
             m_ShouldWaitList.Add(new SceneTestInfo() { ClientId = NetworkManager.ServerClientId, ShouldWait = false });
             m_ServerNetworkManager.SceneManager.VerifySceneBeforeLoading = m_ServerVerificationAction;
-            m_ServerNetworkManager.SceneManager.SetClientSynchronizationMode(clientSynchronizationMode);
             m_ScenesLoaded.Clear();
             foreach (var manager in m_ClientNetworkManagers)
             {
                 m_ShouldWaitList.Add(new SceneTestInfo() { ClientId = manager.LocalClientId, ShouldWait = false });
                 manager.SceneManager.VerifySceneBeforeLoading = m_ClientVerificationAction;
-                manager.SceneManager.SetClientSynchronizationMode(clientSynchronizationMode);
             }
         }
 
@@ -307,15 +312,14 @@ private bool DataPoolVerifySceneClient(int sceneIndex, string sceneName, LoadSce
         /// Will load from 1 to 32 scenes in both single and additive ClientSynchronizationMode
         /// </summary>
         [UnityTest]
-        public IEnumerator SceneEventDataPoolSceneLoadingTest([Values(LoadSceneMode.Single, LoadSceneMode.Additive)] LoadSceneMode clientSynchronizationMode, [Values(1, 2, 4, 6)] int numberOfScenesToLoad)
+        public IEnumerator SceneEventDataPoolSceneLoadingTest([Values(1, 2, 4, 6)] int numberOfScenesToLoad)
         {
-            m_LoadSceneMode = clientSynchronizationMode;
             m_CanStartServerOrClients = true;
 
             yield return StartServerAndClients();
 
             // Now prepare for the loading and unloading additive scene testing
-            InitializeSceneTestInfo(clientSynchronizationMode, true);
+            InitializeSceneTestInfo(true);
 
             yield return WaitForConditionOrTimeOut(() => m_ClientsReceivedSynchronize.Count == (m_ClientNetworkManagers.Length));
             Assert.False(s_GlobalTimeoutHelper.TimedOut, $"Timed out waiting for all clients to receive synchronization event! Received: {m_ClientsReceivedSynchronize.Count} | Expected: {m_ClientNetworkManagers.Length}");
diff --git a/testproject/Assets/Tests/Runtime/NetworkSceneManager/NetworkSceneManagerSeneVerification.cs b/testproject/Assets/Tests/Runtime/NetworkSceneManager/NetworkSceneManagerSeneVerification.cs
@@ -9,12 +9,17 @@
 
 namespace TestProject.RuntimeTests
 {
-    [TestFixture(HostOrServer.Host)]
-    [TestFixture(HostOrServer.Server)]
+    [TestFixture(HostOrServer.Host, LoadSceneMode.Single)]
+    [TestFixture(HostOrServer.Host, LoadSceneMode.Additive)]
+    [TestFixture(HostOrServer.Server, LoadSceneMode.Single)]
+    [TestFixture(HostOrServer.Server, LoadSceneMode.Additive)]
     public class NetworkSceneManagerSceneVerification : NetcodeIntegrationTest
     {
         protected override int NumberOfClients => 4;
-        public NetworkSceneManagerSceneVerification(HostOrServer hostOrServer) : base(hostOrServer) { }
+        public NetworkSceneManagerSceneVerification(HostOrServer hostOrServer, LoadSceneMode loadSceneMode) : base(hostOrServer)
+        {
+            m_LoadSceneMode = loadSceneMode;
+        }
 
         private const string k_AdditiveScene1 = "InSceneNetworkObject";
         private const string k_AdditiveScene2 = "AdditiveSceneMultiInstance";
@@ -65,9 +70,9 @@ protected override IEnumerator OnStartedServerAndClients()
             m_ServerVerificationAction = ServerVerifySceneBeforeLoading;
             m_ServerNetworkManager.SceneManager.OnSceneEvent += ServerSceneManager_OnSceneEvent;
             m_ServerNetworkManager.SceneManager.DisableValidationWarnings(true);
+            m_ServerNetworkManager.SceneManager.SetClientSynchronizationMode(m_LoadSceneMode);
             foreach (var client in m_ClientNetworkManagers)
             {
-                client.SceneManager.ClientSynchronizationMode = m_LoadSceneMode;
                 client.SceneManager.DisableValidationWarnings(true);
             }
 
@@ -153,11 +158,10 @@ private void ResetWait()
         /// <summary>
         /// Initializes the m_ShouldWaitList
         /// </summary>
-        private void InitializeSceneTestInfo(LoadSceneMode clientSynchronizationMode, bool enableSceneVerification = false)
+        private void InitializeSceneTestInfo(bool enableSceneVerification = false)
         {
             m_ShouldWaitList.Add(new SceneTestInfo() { ClientId = NetworkManager.ServerClientId, ShouldWait = false });
             m_ServerNetworkManager.SceneManager.VerifySceneBeforeLoading = m_ServerVerificationAction;
-            m_ServerNetworkManager.SceneManager.SetClientSynchronizationMode(clientSynchronizationMode);
             m_ScenesLoaded.Clear();
             foreach (var manager in m_ClientNetworkManagers)
             {
@@ -287,15 +291,15 @@ private bool ClientVerifySceneBeforeLoading(int sceneIndex, string sceneName, Lo
         /// </summary>
         /// <returns></returns>
         [UnityTest]
-        public IEnumerator SceneVerifyBeforeLoadTest([Values(LoadSceneMode.Single, LoadSceneMode.Additive)] LoadSceneMode clientSynchronizationMode)
+        public IEnumerator SceneVerifyBeforeLoadTest()
         {
             m_CurrentSceneName = k_AdditiveScene1;
             m_CanStartServerOrClients = true;
             m_IsTestingVerifyScene = false;
             yield return StartServerAndClients();
 
             // Now prepare for the loading and unloading additive scene testing
-            InitializeSceneTestInfo(clientSynchronizationMode);
+            InitializeSceneTestInfo();
 
             // Test VerifySceneBeforeLoading with both server and client set to true
             ResetWait();
@@ -311,6 +315,7 @@ public IEnumerator SceneVerifyBeforeLoadTest([Values(LoadSceneMode.Single, LoadS
 
             // Unload the scene
             ResetWait();
+
             Assert.AreEqual(m_ServerNetworkManager.SceneManager.UnloadScene(m_CurrentScene), SceneEventProgressStatus.Started);
 
             yield return WaitForConditionOrTimeOut(ConditionPassed);
diff --git a/testproject/Assets/Tests/Runtime/NetworkSceneManager/NetworkSceneManagerUsageTests.cs b/testproject/Assets/Tests/Runtime/NetworkSceneManager/NetworkSceneManagerUsageTests.cs
@@ -47,6 +47,31 @@ public void SceneManagementDisabledException([Values(LoadSceneMode.Single, LoadS
             Assert.IsTrue(threwException);
         }
 
+        /// <summary>
+        /// Validate warning message generation for setting client synchronization mode on the client side.
+        /// </summary>
+        [Test]
+        public void ClientSetClientSynchronizationMode()
+        {
+            LogAssert.Expect(UnityEngine.LogType.Warning, "[Netcode] Clients should not set this value as it is automatically synchronized with the server's setting!");
+            m_ClientNetworkManagers[0].SceneManager.SetClientSynchronizationMode(LoadSceneMode.Single);
+        }
+
+        /// <summary>
+        /// Validate warning message generation for setting client synchronization mode on the server side.
+        /// </summary>
+        [UnityTest]
+        public IEnumerator ServerSetClientSynchronizationModeAfterClientsConnected()
+        {
+            // Verify that changing this setting when additional clients are connect will generate the warning
+            LogAssert.Expect(UnityEngine.LogType.Warning, "[Netcode] Server is changing client synchronization mode after clients have been synchronized! It is recommended to do this before clients are connected!");
+            m_ServerNetworkManager.SceneManager.SetClientSynchronizationMode(LoadSceneMode.Additive);
+            // Verify that changing this setting when no additional clients are connected will not generate a warning
+            yield return StopOneClient(m_ClientNetworkManagers[0]);
+            m_ServerNetworkManager.SceneManager.SetClientSynchronizationMode(LoadSceneMode.Additive);
+            LogAssert.NoUnexpectedReceived();
+        }
+
         /// <summary>
         /// Checks that a client cannot call LoadScene
         /// </summary>
@@ -97,6 +122,10 @@ public IEnumerator ClientCannotUseException([Values(LoadSceneMode.Single, LoadSc
             }
             Assert.IsTrue(threwException);
 
+            foreach (var clientNetworkManager in m_ClientNetworkManagers)
+            {
+                clientNetworkManager.SceneManager.VerifySceneBeforeUnloading = OnClientVerifySceneBeforeUnloading;
+            }
             // Loading additive only because we don't want to unload the
             // Test Runner's scene using LoadSceneMode.Single
             retStatus = m_ServerNetworkManager.SceneManager.UnloadScene(m_CurrentScene);
@@ -105,6 +134,11 @@ public IEnumerator ClientCannotUseException([Values(LoadSceneMode.Single, LoadSc
             Assert.IsFalse(s_GlobalTimeoutHelper.TimedOut, $"Timed out waiting for {m_CurrentSceneName} {nameof(SceneEventType.UnloadComplete)} event from client!");
         }
 
+        private bool OnClientVerifySceneBeforeUnloading(Scene scene)
+        {
+            return m_CurrentSceneName == scene.name;
+        }
+
         private void ServerSceneManager_OnSceneEvent(SceneEvent sceneEvent)
         {
             if (sceneEvent.ClientId != m_ServerNetworkManager.LocalClientId)
