fix: ValidatesSceneBeforeLoading halts client synchronization (#1883)

* fix MTT-3285

MTT-3285
This fixes the issue with ValidateSceneBeforeLoading not continuing to synchronize if one scene is skipped over.

* update

adding some debug information (LogLevel.Developer) so users can track when a client declines to load a scene

* fix

When a scene is not validated on the client side it should invoke HandleClientSceneEvent not ClientLoadedSynchronization

* test

MTT-3285
Integration test

* style

* Update CHANGELOG.md

* Update CHANGELOG.md

* style

removing space in assert message

* test fix

unload the loaded scenes.

* Update CHANGELOG.md

Author: NoelStephensUnity

com.unity.netcode.gameobjects/CHANGELOG.md

@@ -14,7 +14,8 @@ Additional documentation and release notes are available at [Multiplayer Documen
 ### Removed
 
 ### Fixed
-- Fixed client throwing an exception if it has messages in the outbound queue when processing the NetworkEvent.Disconnect event and is using UTP. (#1884)
+- Fixed client throwing an exception if it has messages in the outbound queue when processing the `NetworkEvent.Disconnect` event and is using UTP. (#1884)
+- Fixed issue during client synchronization if 'ValidateSceneBeforeLoading' returned false it would halt the client synchronization process resulting in a client that was approved but not synchronized or fully connected with the server. (#1883)
 
 ## [1.0.0-pre.7] - 2022-04-06
 
@@ -39,6 +40,7 @@ Additional documentation and release notes are available at [Multiplayer Documen
 - Removed `com.unity.collections` dependency from the package (#1849)
 
 ### Fixed
+
 - Fixed in-scene placed NetworkObjects not being found/ignored after a client disconnects and then reconnects. (#1850)
 - Fixed issue where `UnityTransport` send queues were not flushed when calling `DisconnectLocalClient` or `DisconnectRemoteClient`. (#1847)
 - Fixed NetworkBehaviour dependency verification check for an existing NetworkObject not searching from root parent transform relative GameObject. (#1841)

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

@@ -1446,12 +1446,9 @@ private void OnClientBeginSync(uint sceneEventId)
 
             var loadSceneMode = sceneHash == sceneEventData.SceneHash ? sceneEventData.LoadSceneMode : LoadSceneMode.Additive;
 
-            // Always check to see if the scene needs to be validated
-            if (!ValidateSceneBeforeLoading(sceneHash, loadSceneMode))
-            {
-                EndSceneEvent(sceneEventId);
-                return;
-            }
+            // Store the sceneHandle and hash
+            sceneEventData.ClientSceneHandle = sceneHandle;
+            sceneEventData.ClientSceneHash = sceneHash;
 
             // If this is the beginning of the synchronization event, then send client a notification that synchronization has begun
             if (sceneHash == sceneEventData.SceneHash)
@@ -1468,9 +1465,16 @@ private void OnClientBeginSync(uint sceneEventId)
                 ScenePlacedObjects.Clear();
             }
 
-            // Store the sceneHandle and hash
-            sceneEventData.ClientSceneHandle = sceneHandle;
-            sceneEventData.ClientSceneHash = sceneHash;
+            // Always check to see if the scene needs to be validated
+            if (!ValidateSceneBeforeLoading(sceneHash, loadSceneMode))
+            {
+                HandleClientSceneEvent(sceneEventId);
+                if (m_NetworkManager.LogLevel == LogLevel.Developer)
+                {
+                    NetworkLog.LogInfo($"Client declined to load the scene {sceneName}, continuing with synchronization.");
+                }
+                return;
+            }
 
             var shouldPassThrough = false;
             var sceneLoad = (AsyncOperation)null;

testproject/Assets/Tests/Runtime/NetworkSceneManager/ClientSynchronizationValidationTest.cs

@@ -0,0 +1,123 @@
+using System.Collections;
+using System.Collections.Generic;
+using NUnit.Framework;
+using UnityEngine;
+using UnityEngine.SceneManagement;
+using UnityEngine.TestTools;
+using Unity.Netcode;
+using Unity.Netcode.TestHelpers.Runtime;
+
+namespace TestProject.RuntimeTests
+{
+    public class ClientSynchronizationValidationTest : NetcodeIntegrationTest
+    {
+        protected override int NumberOfClients => 1;
+        private const string k_FirstSceneToLoad = "UnitTestBaseScene";
+        private const string k_SecondSceneToSkip = "InSceneNetworkObject";
+        private const string k_ThirdSceneToLoad = "EmptyScene";
+        private List<ClientSceneVerificationHandler> m_ClientSceneVerifiers = new List<ClientSceneVerificationHandler>();
+
+        protected override void OnOneTimeSetup()
+        {
+            SceneManager.sceneLoaded += SceneManager_sceneLoaded;
+            // Pre-load some scenes (i.e. server will tell clients to synchronize to these scenes)
+            SceneManager.LoadSceneAsync(k_FirstSceneToLoad, LoadSceneMode.Additive);
+            SceneManager.LoadSceneAsync(k_SecondSceneToSkip, LoadSceneMode.Additive);
+            SceneManager.LoadSceneAsync(k_ThirdSceneToLoad, LoadSceneMode.Additive);
+            base.OnOneTimeSetup();
+        }
+        private List<Scene> m_ScenesLoaded = new List<Scene>();
+        private void SceneManager_sceneLoaded(Scene scene, LoadSceneMode mode)
+        {
+            m_ScenesLoaded.Add(scene);
+        }
+
+        protected override IEnumerator OnStartedServerAndClients()
+        {
+            // Create ClientSceneVerificationHandlers for each client
+            foreach (var client in m_ClientNetworkManagers)
+            {
+                m_ClientSceneVerifiers.Add(new ClientSceneVerificationHandler(client));
+            }
+            return base.OnStartedServerAndClients();
+        }
+
+        [UnityTest]
+        public IEnumerator ClientVerifySceneBeforeLoading()
+        {
+            // If we made it here it means that all clients finished synchronizing
+            // Now check to make sure only the two scenes were loaded and one
+            // completely skipped.
+            foreach (var clientSceneVerifier in m_ClientSceneVerifiers)
+            {
+                clientSceneVerifier.ValidateScenesLoaded();
+            }
+            yield return null;
+        }
+
+        protected override IEnumerator OnTearDown()
+        {
+            foreach (var scene in m_ScenesLoaded)
+            {
+                if (scene.isLoaded)
+                {
+                    SceneManager.UnloadSceneAsync(scene);
+                }
+            }
+            m_ScenesLoaded.Clear();
+            return base.OnTearDown();
+        }
+
+        /// <summary>
+        /// Determines if all clients loaded only two of the 3 scenes
+        /// </summary>
+        private class ClientSceneVerificationHandler
+        {
+            private NetworkManager m_NetworkManager;
+            private Dictionary<string, int> m_ValidSceneEventCount = new Dictionary<string, int>();
+            public ClientSceneVerificationHandler(NetworkManager networkManager)
+            {
+                m_NetworkManager = networkManager;
+                m_NetworkManager.SceneManager.DisableValidationWarnings(true);
+                m_NetworkManager.SceneManager.VerifySceneBeforeLoading = VerifySceneBeforeLoading;
+                m_NetworkManager.SceneManager.OnLoad += ClientSceneManager_OnLoad;
+                m_NetworkManager.SceneManager.OnLoadComplete += ClientSceneManager_OnLoadComplete;
+                m_ValidSceneEventCount.Add(k_FirstSceneToLoad, 0);
+                m_ValidSceneEventCount.Add(k_SecondSceneToSkip, 0);
+                m_ValidSceneEventCount.Add(k_ThirdSceneToLoad, 0);
+            }
+
+            public void ValidateScenesLoaded()
+            {
+                Assert.IsFalse(m_ValidSceneEventCount[k_SecondSceneToSkip] > 0, $"Client still loaded the invalidated scene {k_SecondSceneToSkip}!");
+                Assert.IsTrue(m_ValidSceneEventCount[k_FirstSceneToLoad] == 2, $"Client did not load and process the validated scene {k_FirstSceneToLoad}!");
+                Assert.IsTrue(m_ValidSceneEventCount[k_ThirdSceneToLoad] == 2, $"Client did not load and process the validated scene {k_ThirdSceneToLoad}!");
+            }
+
+            private void ClientSceneManager_OnLoadComplete(ulong clientId, string sceneName, LoadSceneMode loadSceneMode)
+            {
+                if (m_ValidSceneEventCount.ContainsKey(sceneName))
+                {
+                    m_ValidSceneEventCount[sceneName]++;
+                }
+            }
+
+            private void ClientSceneManager_OnLoad(ulong clientId, string sceneName, LoadSceneMode loadSceneMode, AsyncOperation asyncOperation)
+            {
+                if (m_ValidSceneEventCount.ContainsKey(sceneName))
+                {
+                    m_ValidSceneEventCount[sceneName]++;
+                }
+            }
+
+            private bool VerifySceneBeforeLoading(int sceneIndex, string sceneName, LoadSceneMode loadSceneMode)
+            {
+                if (sceneName == k_SecondSceneToSkip)
+                {
+                    return false;
+                }
+                return true;
+            }
+        }
+    }
+}