fix: load event completed fires early (#1379) (#1464)

* fix

Only allow a host to include itself in the clients completed list of a SceneEventProgress (i.e. don't include dedicate-server only).

* style

adding adjusted comments.

* test

Updated SceneLoadingAndNotifications to run as a host and a server in order to test that when running as a server the SceneEventType.LoadEventCompleted and SceneEventType.UnloadEventCompleted events are triggered without the server being added to the ClientsThatCompleted list within the SceneEvent.

* update changelog

Updating the changelog file with this fix information.

Co-authored-by: Unity Netcode CI <74025435+netcode-ci-service@users.noreply.github.com>

Author: andrews-unity

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

@@ -1028,8 +1028,8 @@ private void OnSceneUnloaded(uint sceneEventId)
                 // despawned that no longer exists
                 SendSceneEventData(sceneEventId, m_NetworkManager.ConnectedClientsIds.Where(c => c != m_NetworkManager.ServerClientId).ToArray());
 
-                //Second, server sets itself as having finished unloading
-                if (SceneEventProgressTracking.ContainsKey(sceneEventData.SceneEventProgressId))
+                //Only if we are a host do we want register having loaded for the associated SceneEventProgress
+                if (SceneEventProgressTracking.ContainsKey(sceneEventData.SceneEventProgressId) && m_NetworkManager.IsHost)
                 {
                     SceneEventProgressTracking[sceneEventData.SceneEventProgressId].AddClientAsDone(m_NetworkManager.ServerClientId);
                 }
@@ -1344,8 +1344,8 @@ private void OnServerLoadedScene(uint sceneEventId, Scene scene)
 
             OnLoadComplete?.Invoke(m_NetworkManager.ServerClientId, SceneNameFromHash(sceneEventData.SceneHash), sceneEventData.LoadSceneMode);
 
-            //Second, set the server as having loaded for the associated SceneEventProgress
-            if (SceneEventProgressTracking.ContainsKey(sceneEventData.SceneEventProgressId))
+            //Second, only if we are a host do we want register having loaded for the associated SceneEventProgress
+            if (SceneEventProgressTracking.ContainsKey(sceneEventData.SceneEventProgressId) && m_NetworkManager.IsHost)
             {
                 SceneEventProgressTracking[sceneEventData.SceneEventProgressId].AddClientAsDone(m_NetworkManager.ServerClientId);
             }

testproject/Assets/Tests/Runtime/NetworkSceneManagerTests.cs

@@ -52,13 +52,49 @@ private class SceneTestInfo
         private NetworkSceneManager.VerifySceneBeforeLoadingDelegateHandler m_ClientVerificationAction;
         private NetworkSceneManager.VerifySceneBeforeLoadingDelegateHandler m_ServerVerificationAction;
 
+        public enum ServerType
+        {
+            Host,
+            Server
+        }
+
         /// <summary>
         /// Tests the different types of NetworkSceneManager notifications (including exceptions) generated
         /// Also tests invalid loading scenarios (i.e. client trying to load a scene)
         /// </summary>
         [UnityTest]
-        public IEnumerator SceneLoadingAndNotifications([Values(LoadSceneMode.Single, LoadSceneMode.Additive)] LoadSceneMode clientSynchronizationMode)
+        public IEnumerator SceneLoadingAndNotifications([Values(LoadSceneMode.Single, LoadSceneMode.Additive)] LoadSceneMode clientSynchronizationMode, [Values(ServerType.Host, ServerType.Server)] ServerType serverType)
         {
+            // First we disconnect and shutdown because we want to verify the synchronize events
+            yield return Teardown();
+
+            // Give a little time for handling clean-up and the like
+            yield return new WaitForSeconds(0.01f);
+
+            // We set this to true in order to bypass the automatic starting of the host and clients
+            m_BypassStartAndWaitForClients = true;
+
+            // Now just create the instances (server and client) without starting anything
+            yield return Setup();
+
+            // This provides both host and server coverage, when a server we should still get SceneEventType.LoadEventCompleted and SceneEventType.UnloadEventCompleted events
+            // but the client count as a server should be 1 less than when a host
+            var isHost = serverType == ServerType.Host ? true : false;
+
+            // Start the host and  clients
+            if (!MultiInstanceHelpers.Start(isHost, m_ServerNetworkManager, m_ClientNetworkManagers, SceneManagerValidationAndTestRunnerInitialization))
+            {
+                Debug.LogError("Failed to start instances");
+                Assert.Fail("Failed to start instances");
+            }
+
+            // Wait for connection on client side
+            yield return MultiInstanceHelpers.Run(MultiInstanceHelpers.WaitForClientsConnected(m_ClientNetworkManagers));
+
+            var numberOfClients = isHost ? NbClients + 1 : NbClients;
+            // Wait for connection on server side
+            yield return MultiInstanceHelpers.Run(MultiInstanceHelpers.WaitForClientsConnectedToServer(m_ServerNetworkManager, numberOfClients));
+
             m_ServerNetworkManager.SceneManager.OnSceneEvent += SceneManager_OnSceneEvent;
             m_CurrentSceneName = k_AdditiveScene1;
 
@@ -327,8 +363,17 @@ private void SceneManager_OnSceneEvent(SceneEvent sceneEvent)
                     {
                         Assert.AreEqual(sceneEvent.SceneName, m_CurrentSceneName);
                         Assert.IsTrue(ContainsClient(sceneEvent.ClientId));
+
+                        // If we are a server and this is being processed by the server, then add the server to the completed list
+                        // to validate that the event completed on all clients (and the server).
+                        if (!m_ServerNetworkManager.IsHost && sceneEvent.ClientId == m_ServerNetworkManager.LocalClientId &&
+                            !sceneEvent.ClientsThatCompleted.Contains(m_ServerNetworkManager.LocalClientId))
+                        {
+                            sceneEvent.ClientsThatCompleted.Add(m_ServerNetworkManager.LocalClientId);
+                        }
                         Assert.IsTrue(ContainsAllClients(sceneEvent.ClientsThatCompleted));
                         SetClientWaitDone(sceneEvent.ClientsThatCompleted);
+
                         break;
                     }
             }