fix: SceneEventProgress was triggering twice when running a host and no clients were connected (#2292)

* fix
Exclude the host-client from being processed as one of the clients to test for when the scene event has completed.
GetClientsWithStatus needed to add the host-client to the list of clients that completed once the scene event was completed. Also fixed a bug in GetClientsWithStatus where, when checking for clients that did  not complete, it would include the clients that completed and the clients that did not complete in the list of client identifiers it returned.
Fixing issue where integration tests could lag by more than 1 network tick and the AsyncOperation could not yet be assigned.

* test
Added HostReceivesOneLoadEventCompletedNotification integration test to validate the fix for this PR.
Updated NetcodeIntegrationTest virtual methods to be able to gain access to a newly created client (via NetcodeIntegrationTest.CreateAndStartNewClient) when it is first created, started, and when it has been connected.

The updates to SceneEventProgress exposed an issue with ClientSynchronizationValidationTest. It was written before many NetcodeIntegrationTest helpers, so there are adjustments here to leverage from those which greatly simplifies the test.
Switched the scene that was not validated by the client-side to one that did not contain an in-scene placed NetworkObject.

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

Author: NoelStephensUnity

com.unity.netcode.gameobjects/CHANGELOG.md

@@ -14,6 +14,7 @@ Additional documentation and release notes are available at [Multiplayer Documen
 
 ### Fixed
 
+- Fixed issue where the host would receive more than one event completed notification when loading or unloading a scene only when no clients were connected. (#2292)
 - Fixed issue where in-scene placed `NetworkObjects` were not honoring the `AutoObjectParentSync` property. (#2281)
 - Fixed the issue where `NetworkManager.OnClientConnectedCallback` was being invoked before in-scene placed `NetworkObject`s had been spawned when starting `NetworkManager` as a host. (#2277)
 - Creating a `FastBufferReader` with `Allocator.None` will not result in extra memory being allocated for the buffer (since it's owned externally in that scenario). (#2265)

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

@@ -108,19 +108,37 @@ internal bool HasTimedOut()
         internal List<ulong> GetClientsWithStatus(bool completedSceneEvent)
         {
             var clients = new List<ulong>();
-            foreach (var clientStatus in ClientsProcessingSceneEvent)
+            if (completedSceneEvent)
             {
-                if (clientStatus.Value == completedSceneEvent)
+                // If we are the host, then add the host-client to the list
+                // of clients that completed if the AsyncOperation is done.
+                if (m_NetworkManager.IsHost && m_AsyncOperation.isDone)
                 {
-                    clients.Add(clientStatus.Key);
+                    clients.Add(m_NetworkManager.LocalClientId);
                 }
-            }
 
-            // If we are getting the list of clients that have not completed the
-            // scene event, then add any clients that disconnected during this
-            // scene event.
-            if (!completedSceneEvent)
+                // Add all clients that completed the scene event
+                foreach (var clientStatus in ClientsProcessingSceneEvent)
+                {
+                    if (clientStatus.Value == completedSceneEvent)
+                    {
+                        clients.Add(clientStatus.Key);
+                    }
+                }
+            }
+            else
             {
+                // If we are the host, then add the host-client to the list
+                // of clients that did not complete if the AsyncOperation is
+                // not done.
+                if (m_NetworkManager.IsHost && !m_AsyncOperation.isDone)
+                {
+                    clients.Add(m_NetworkManager.LocalClientId);
+                }
+
+                // If we are getting the list of clients that have not completed the
+                // scene event, then add any clients that disconnected during this
+                // scene event.
                 clients.AddRange(ClientsThatDisconnected);
             }
             return clients;
@@ -138,6 +156,11 @@ internal SceneEventProgress(NetworkManager networkManager, SceneEventProgressSta
                     // Track the clients that were connected when we started this event
                     foreach (var connectedClientId in networkManager.ConnectedClientsIds)
                     {
+                        // Ignore the host client
+                        if (NetworkManager.ServerClientId == connectedClientId)
+                        {
+                            continue;
+                        }
                         ClientsProcessingSceneEvent.Add(connectedClientId, false);
                     }
 
@@ -218,7 +241,10 @@ private bool HasFinished()
             }
 
             // Return the local scene event's AsyncOperation status
-            return m_AsyncOperation.isDone;
+            // Note: Integration tests process scene loading through a queue
+            // and the AsyncOperation could not be assigned for several
+            // network tick periods. Return false if that is the case.
+            return m_AsyncOperation == null ? false : m_AsyncOperation.isDone;
         }
 
         /// <summary>

com.unity.netcode.gameobjects/TestHelpers/Runtime/NetcodeIntegrationTest.cs

@@ -274,16 +274,6 @@ protected void CreateServerAndClients()
             CreateServerAndClients(NumberOfClients);
         }
 
-        protected virtual void OnNewClientCreated(NetworkManager networkManager)
-        {
-
-        }
-
-        protected virtual void OnNewClientStartedAndConnected(NetworkManager networkManager)
-        {
-
-        }
-
         private void AddRemoveNetworkManager(NetworkManager networkManager, bool addNetworkManager)
         {
             var clientNetworkManagersList = new List<NetworkManager>(m_ClientNetworkManagers);
@@ -299,6 +289,37 @@ private void AddRemoveNetworkManager(NetworkManager networkManager, bool addNetw
             m_NumberOfClients = clientNetworkManagersList.Count;
         }
 
+        /// <summary>
+        /// CreateAndStartNewClient Only
+        /// Invoked when the newly created client has been created
+        /// </summary>
+        protected virtual void OnNewClientCreated(NetworkManager networkManager)
+        {
+
+        }
+
+        /// <summary>
+        /// CreateAndStartNewClient Only
+        /// Invoked when the newly created client has been created and started
+        /// </summary>
+        protected virtual void OnNewClientStarted(NetworkManager networkManager)
+        {
+        }
+
+        /// <summary>
+        /// CreateAndStartNewClient Only
+        /// Invoked when the newly created client has been created, started, and connected
+        /// to the server-host.
+        /// </summary>
+        protected virtual void OnNewClientStartedAndConnected(NetworkManager networkManager)
+        {
+
+        }
+
+        /// <summary>
+        /// This will create, start, and connect a new client while in the middle of an
+        /// integration test.
+        /// </summary>
         protected IEnumerator CreateAndStartNewClient()
         {
             var networkManager = NetcodeIntegrationTestHelpers.CreateNewClient(m_ClientNetworkManagers.Length);
@@ -309,9 +330,15 @@ protected IEnumerator CreateAndStartNewClient()
             OnNewClientCreated(networkManager);
 
             NetcodeIntegrationTestHelpers.StartOneClient(networkManager);
+
             AddRemoveNetworkManager(networkManager, true);
+
+            OnNewClientStarted(networkManager);
+
             // Wait for the new client to connect
             yield return WaitForClientsConnectedOrTimeOut();
+
+            OnNewClientStartedAndConnected(networkManager);
             if (s_GlobalTimeoutHelper.TimedOut)
             {
                 AddRemoveNetworkManager(networkManager, false);
@@ -322,6 +349,9 @@ protected IEnumerator CreateAndStartNewClient()
             VerboseDebug($"[{networkManager.name}] Created and connected!");
         }
 
+        /// <summary>
+        /// This will stop a client while in the middle of an integration test
+        /// </summary>
         protected IEnumerator StopOneClient(NetworkManager networkManager, bool destroy = false)
         {
             NetcodeIntegrationTestHelpers.StopOneClient(networkManager, destroy);

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

@@ -1,6 +1,5 @@
 using System.Collections;
 using System.Collections.Generic;
-using System.Linq;
 using NUnit.Framework;
 using UnityEngine;
 using UnityEngine.SceneManagement;
@@ -12,48 +11,23 @@ namespace TestProject.RuntimeTests
 {
     public class ClientSynchronizationValidationTest : NetcodeIntegrationTest
     {
-        protected override int NumberOfClients => 1;
+        protected override int NumberOfClients => 0;
         private const string k_FirstSceneToLoad = "UnitTestBaseScene";
-        private const string k_SecondSceneToSkip = "InSceneNetworkObject";
-        private const string k_ThirdSceneToLoad = "EmptyScene";
-        private bool m_CanStartServerAndClients;
-        private List<ClientSceneVerificationHandler> m_ClientSceneVerifiers = new List<ClientSceneVerificationHandler>();
+        private const string k_SecondSceneToLoad = "InSceneNetworkObject";
+        private const string k_ThirdSceneToSkip = "EmptyScene";
 
-        protected override bool CanStartServerAndClients()
-        {
-            return m_CanStartServerAndClients;
-        }
+        private List<ClientSceneVerificationHandler> m_ClientSceneVerifiers = new List<ClientSceneVerificationHandler>();
 
-        protected override IEnumerator OnStartedServerAndClients()
+        protected override void OnNewClientStarted(NetworkManager networkManager)
         {
-            // Create ClientSceneVerificationHandlers for each client
-            foreach (var client in m_ClientNetworkManagers)
-            {
-                m_ClientSceneVerifiers.Add(new ClientSceneVerificationHandler(client));
-            }
-            return base.OnStartedServerAndClients();
+            m_ClientSceneVerifiers.Add(new ClientSceneVerificationHandler(networkManager));
+            base.OnNewClientStarted(networkManager);
         }
 
         [UnityTest]
         public IEnumerator ClientVerifySceneBeforeLoading()
         {
-            // Because despawning a client will cause it to shutdown and clean everything in the
-            // scene hierarchy, we have to prevent one of the clients from spawning initially before
-            // we test synchronizing late joining clients.
-            // So, we prevent the automatic starting of the server and clients, remove the client we
-            // will be targeting to join late from the m_ClientNetworkManagers array, start the server
-            // and the remaining client, despawn the in-scene NetworkObject, and then start and synchronize
-            // the clientToTest.
-            var clientToTest = m_ClientNetworkManagers[0];
-            var clients = m_ClientNetworkManagers.ToList();
-            clients.Remove(clientToTest);
-            m_ClientNetworkManagers = clients.ToArray();
-            m_CanStartServerAndClients = true;
-            yield return StartServerAndClients();
-            clients.Add(clientToTest);
-            m_ClientNetworkManagers = clients.ToArray();
-
-            var scenesToLoad = new List<string>() { k_FirstSceneToLoad, k_SecondSceneToSkip, k_ThirdSceneToLoad };
+            var scenesToLoad = new List<string>() { k_FirstSceneToLoad, k_SecondSceneToLoad, k_ThirdSceneToSkip };
             m_ServerNetworkManager.SceneManager.OnLoadComplete += OnLoadComplete;
             foreach (var sceneToLoad in scenesToLoad)
             {
@@ -65,15 +39,10 @@ public IEnumerator ClientVerifySceneBeforeLoading()
                 AssertOnTimeout($"Timed out waiting for scene {m_SceneBeingLoaded} to finish loading!");
             }
 
-            // Now late join a client to make sure the client synchronizes to 2 of the 3 scenes loaded
-            NetcodeIntegrationTestHelpers.StartOneClient(clientToTest);
-            yield return WaitForConditionOrTimeOut(() => (clientToTest.IsConnectedClient && clientToTest.IsListening));
-            AssertOnTimeout($"Timed out waiting for {clientToTest.name} to reconnect!");
-
-            yield return s_DefaultWaitForTick;
+            yield return CreateAndStartNewClient();
 
-            // Update the newly joined client information
-            ClientNetworkManagerPostStartInit();
+            yield return WaitForConditionOrTimeOut(m_ClientSceneVerifiers[0].HasLoadedExpectedScenes);
+            AssertOnTimeout($"Timed out waiting for the client to have loaded the expected scenes");
 
             // Check to make sure only the two scenes were loaded and one
             // completely skipped.
@@ -110,15 +79,20 @@ public ClientSceneVerificationHandler(NetworkManager networkManager)
                 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);
+                m_ValidSceneEventCount.Add(k_SecondSceneToLoad, 0);
+                m_ValidSceneEventCount.Add(k_ThirdSceneToSkip, 0);
+            }
+
+            public bool HasLoadedExpectedScenes()
+            {
+                return m_ValidSceneEventCount[k_FirstSceneToLoad] == 2 && m_ValidSceneEventCount[k_SecondSceneToLoad] == 2;
             }
 
             public void ValidateScenesLoaded()
             {
-                Assert.IsFalse(m_ValidSceneEventCount[k_SecondSceneToSkip] > 0, $"Client still loaded the invalidated scene {k_SecondSceneToSkip}!");
-                Assert.IsTrue(m_ValidSceneEventCount[k_FirstSceneToLoad] == 1, $"Client did not load and process the validated scene {k_FirstSceneToLoad}! Expected (1) but was ({m_ValidSceneEventCount[k_FirstSceneToLoad]})");
-                Assert.IsTrue(m_ValidSceneEventCount[k_ThirdSceneToLoad] == 1, $"Client did not load and process the validated scene {k_ThirdSceneToLoad}! Expected (1) but was ({m_ValidSceneEventCount[k_ThirdSceneToLoad]})");
+                Assert.IsTrue(m_ValidSceneEventCount[k_ThirdSceneToSkip] == 0, $"Client still loaded the invalidated scene {k_ThirdSceneToSkip}!");
+                Assert.IsTrue(m_ValidSceneEventCount[k_FirstSceneToLoad] == 2, $"Client did not load and process the validated scene {k_FirstSceneToLoad}! Expected (1) but was ({m_ValidSceneEventCount[k_FirstSceneToLoad]})");
+                Assert.IsTrue(m_ValidSceneEventCount[k_SecondSceneToLoad] == 2, $"Client did not load and process the validated scene {k_SecondSceneToLoad}! Expected (1) but was ({m_ValidSceneEventCount[k_SecondSceneToLoad]})");
             }
 
             private void ClientSceneManager_OnLoadComplete(ulong clientId, string sceneName, LoadSceneMode loadSceneMode)
@@ -139,7 +113,7 @@ private void ClientSceneManager_OnLoad(ulong clientId, string sceneName, LoadSce
 
             private bool VerifySceneBeforeLoading(int sceneIndex, string sceneName, LoadSceneMode loadSceneMode)
             {
-                if (sceneName == k_SecondSceneToSkip)
+                if (sceneName == k_ThirdSceneToSkip)
                 {
                     return false;
                 }

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

@@ -134,5 +134,41 @@ private void ClientSceneManager_OnLoadComplete(ulong clientId, string sceneName,
                 m_ClientLoadedScene = true;
             }
         }
+
+        private int m_LoadEventCompletedInvocationCount;
+
+        /// <summary>
+        /// This test validates that a host only receives one OnLoadEventCompleted callback per scene loading event when
+        /// no clients are connected.
+        /// Note: the fix was within SceneEventProgress but is associated with NetworkSceneManager
+        /// </summary>
+        [UnityTest]
+        public IEnumerator HostReceivesOneLoadEventCompletedNotification()
+        {
+            // Host only test
+            if (!m_UseHost)
+            {
+                yield break;
+            }
+
+            yield return StopOneClient(m_ClientNetworkManagers[0]);
+            m_LoadEventCompletedInvocationCount = 0;
+            m_ServerNetworkManager.SceneManager.OnLoadEventCompleted += SceneManager_OnLoadEventCompleted;
+
+            var retStatus = m_ServerNetworkManager.SceneManager.LoadScene(k_AdditiveScene1, LoadSceneMode.Additive);
+            Assert.AreEqual(retStatus, SceneEventProgressStatus.Started);
+            yield return WaitForConditionOrTimeOut(() => m_LoadEventCompletedInvocationCount > 0);
+            AssertOnTimeout($"Host timed out loading scene {k_AdditiveScene1} additively!");
+
+            // Wait one tick to make sure any other notifications are not triggered.
+            yield return s_DefaultWaitForTick;
+
+            Assert.IsTrue(m_LoadEventCompletedInvocationCount == 1, $"Expected OnLoadEventCompleted to be triggered once but was triggered {m_LoadEventCompletedInvocationCount} times!");
+        }
+
+        private void SceneManager_OnLoadEventCompleted(string sceneName, LoadSceneMode loadSceneMode, System.Collections.Generic.List<ulong> clientsCompleted, System.Collections.Generic.List<ulong> clientsTimedOut)
+        {
+            m_LoadEventCompletedInvocationCount++;
+        }
     }
 }