diff --git a/com.unity.netcode.gameobjects/CHANGELOG.md b/com.unity.netcode.gameobjects/CHANGELOG.md index 65a8fda1f8..5d9b4a9e6a 100644 --- a/com.unity.netcode.gameobjects/CHANGELOG.md +++ b/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) diff --git a/com.unity.netcode.gameobjects/Runtime/SceneManagement/SceneEventProgress.cs b/com.unity.netcode.gameobjects/Runtime/SceneManagement/SceneEventProgress.cs index bce5389b1c..6ab998c0d7 100644 --- a/com.unity.netcode.gameobjects/Runtime/SceneManagement/SceneEventProgress.cs +++ b/com.unity.netcode.gameobjects/Runtime/SceneManagement/SceneEventProgress.cs @@ -108,19 +108,37 @@ internal bool HasTimedOut() internal List GetClientsWithStatus(bool completedSceneEvent) { var clients = new List(); - 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; } /// diff --git a/com.unity.netcode.gameobjects/TestHelpers/Runtime/NetcodeIntegrationTest.cs b/com.unity.netcode.gameobjects/TestHelpers/Runtime/NetcodeIntegrationTest.cs index c2f3dfe557..aad8529813 100644 --- a/com.unity.netcode.gameobjects/TestHelpers/Runtime/NetcodeIntegrationTest.cs +++ b/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(m_ClientNetworkManagers); @@ -299,6 +289,37 @@ private void AddRemoveNetworkManager(NetworkManager networkManager, bool addNetw m_NumberOfClients = clientNetworkManagersList.Count; } + /// + /// CreateAndStartNewClient Only + /// Invoked when the newly created client has been created + /// + protected virtual void OnNewClientCreated(NetworkManager networkManager) + { + + } + + /// + /// CreateAndStartNewClient Only + /// Invoked when the newly created client has been created and started + /// + protected virtual void OnNewClientStarted(NetworkManager networkManager) + { + } + + /// + /// CreateAndStartNewClient Only + /// Invoked when the newly created client has been created, started, and connected + /// to the server-host. + /// + protected virtual void OnNewClientStartedAndConnected(NetworkManager networkManager) + { + + } + + /// + /// This will create, start, and connect a new client while in the middle of an + /// integration test. + /// 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!"); } + /// + /// This will stop a client while in the middle of an integration test + /// protected IEnumerator StopOneClient(NetworkManager networkManager, bool destroy = false) { NetcodeIntegrationTestHelpers.StopOneClient(networkManager, destroy); diff --git a/testproject/Assets/Tests/Runtime/NetworkSceneManager/ClientSynchronizationValidationTest.cs b/testproject/Assets/Tests/Runtime/NetworkSceneManager/ClientSynchronizationValidationTest.cs index 91564ac13f..5e5eba1106 100644 --- a/testproject/Assets/Tests/Runtime/NetworkSceneManager/ClientSynchronizationValidationTest.cs +++ b/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 m_ClientSceneVerifiers = new List(); + private const string k_SecondSceneToLoad = "InSceneNetworkObject"; + private const string k_ThirdSceneToSkip = "EmptyScene"; - protected override bool CanStartServerAndClients() - { - return m_CanStartServerAndClients; - } + private List m_ClientSceneVerifiers = new List(); - 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() { k_FirstSceneToLoad, k_SecondSceneToSkip, k_ThirdSceneToLoad }; + var scenesToLoad = new List() { 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; } diff --git a/testproject/Assets/Tests/Runtime/NetworkSceneManager/NetworkSceneManagerUsageTests.cs b/testproject/Assets/Tests/Runtime/NetworkSceneManager/NetworkSceneManagerUsageTests.cs index 2077b573a8..693992ce17 100644 --- a/testproject/Assets/Tests/Runtime/NetworkSceneManager/NetworkSceneManagerUsageTests.cs +++ b/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; + + /// + /// 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 + /// + [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 clientsCompleted, System.Collections.Generic.List clientsTimedOut) + { + m_LoadEventCompletedInvocationCount++; + } } }