diff --git a/com.unity.netcode.gameobjects/CHANGELOG.md b/com.unity.netcode.gameobjects/CHANGELOG.md index 9646852ed7..b325481e81 100644 --- a/com.unity.netcode.gameobjects/CHANGELOG.md +++ b/com.unity.netcode.gameobjects/CHANGELOG.md @@ -22,6 +22,7 @@ Additional documentation and release notes are available at [Multiplayer Documen ### Fixed +- Issue where a client promoted to session owner could throw an exception when synchronizing a newly joining client. (#4217) - Issue where a growable `FastBufferWriter` created with a size of 0 never returned from its first write or `TryBeginWrite` call that needed the buffer to grow. (#4223) - Issue where a `FastBufferReader` created with an offset and the default length of -1 read past the end of its source. The `ArraySegment` overload that takes an offset now applies it from the start of the segment. (#4221) - Issue where a client synchronized while a `NetworkList` still had unsent changes applied those changes twice, resulting in duplicate entries and index-based events landing on the wrong elements on that client. (#4215) diff --git a/com.unity.netcode.gameobjects/Runtime/Connection/NetworkConnectionManager.cs b/com.unity.netcode.gameobjects/Runtime/Connection/NetworkConnectionManager.cs index 3ff131131f..25704c8a35 100644 --- a/com.unity.netcode.gameobjects/Runtime/Connection/NetworkConnectionManager.cs +++ b/com.unity.netcode.gameobjects/Runtime/Connection/NetworkConnectionManager.cs @@ -1086,7 +1086,6 @@ internal void HandleConnectionApproval(ulong ownerClientId, bool createPlayerObj if (NetworkManager.DistributedAuthorityMode && NetworkManager.DAHost) { NetworkManager.SetSessionOwner(NetworkManager.LocalClientId); - NetworkManager.SceneManager.InitializeScenesLoaded(); } if (NetworkManager.DistributedAuthorityMode && NetworkManager.AutoSpawnPlayerPrefabClientSide) diff --git a/com.unity.netcode.gameobjects/Runtime/Core/NetworkManager.cs b/com.unity.netcode.gameobjects/Runtime/Core/NetworkManager.cs index 453f10b88f..25b86e3da0 100644 --- a/com.unity.netcode.gameobjects/Runtime/Core/NetworkManager.cs +++ b/com.unity.netcode.gameobjects/Runtime/Core/NetworkManager.cs @@ -233,6 +233,11 @@ internal void SetSessionOwner(ulong sessionOwner) var isSessionOwner = LocalClientId == sessionOwner; LocalClient.IsSessionOwner = isSessionOwner; + if (isSessionOwner && NetworkConfig.EnableSceneManagement) + { + SceneManager.InitializeScenesLoaded(); + } + foreach (var networkObject in SpawnManager.SpawnedObjects.Values) { if (isSessionOwner) diff --git a/com.unity.netcode.gameobjects/Runtime/Messaging/Messages/ConnectionApprovedMessage.cs b/com.unity.netcode.gameobjects/Runtime/Messaging/Messages/ConnectionApprovedMessage.cs index 2a7eee8be7..19b706e802 100644 --- a/com.unity.netcode.gameobjects/Runtime/Messaging/Messages/ConnectionApprovedMessage.cs +++ b/com.unity.netcode.gameobjects/Runtime/Messaging/Messages/ConnectionApprovedMessage.cs @@ -280,10 +280,6 @@ public void Handle(ref NetworkContext context) networkManager.SetSessionOwner(GetSessionOwner()); if (networkManager.LocalClient.IsSessionOwner) { - if (networkManager.NetworkConfig.EnableSceneManagement) - { - networkManager.SceneManager.InitializeScenesLoaded(); - } if (networkManager.NetworkConfig.ConnectionApproval && networkManager.LogLevel <= LogLevel.Developer) { NetworkLog.LogWarning($"{nameof(NetworkConfig.ConnectionApproval)} is enabled but is not supported when using a distributed authority topology. The {nameof(NetworkManager.ConnectionApprovalCallback)} will not be invoked."); diff --git a/com.unity.netcode.gameobjects/Runtime/SceneManagement/NetworkSceneManager.cs b/com.unity.netcode.gameobjects/Runtime/SceneManagement/NetworkSceneManager.cs index 74d8e4b8f0..1b83bca39c 100644 --- a/com.unity.netcode.gameobjects/Runtime/SceneManagement/NetworkSceneManager.cs +++ b/com.unity.netcode.gameobjects/Runtime/SceneManagement/NetworkSceneManager.cs @@ -3,6 +3,7 @@ using System.Linq; using System.Runtime.CompilerServices; using Unity.Collections; +using Unity.Netcode.Logging; using UnityEngine; using UnityEngine.SceneManagement; using Debug = UnityEngine.Debug; @@ -1951,6 +1952,37 @@ private void OnClientLoadedScene(uint sceneEventId, Scene scene) /// internal List ClientConnectionQueue = new List(); + [MethodImpl(MethodImplOptions.AggressiveInlining)] + private void AddActiveSceneToClientSynchronization(ref SceneEventData sceneEventData, ref Scene activeScene, bool shouldRunValidation) + { + // It is possible a user might not want to synchronize the active scene, so we will check to see if it is valid before adding it to the synchronization list. + if (shouldRunValidation && !ValidateSceneBeforeLoading(activeScene.buildIndex, activeScene.name, sceneEventData.LoadSceneMode)) + { + return; + } + + if (NetworkManager.DistributedAuthorityMode) + { + if (!ClientSceneHandleToServerSceneHandle.TryGetValue(activeScene.handle, out var localHandle)) + { + NetworkManager.Log.Warning(new Context(LogLevel.Developer, $"Failed to synchronize active scene. Use {nameof(ValidateSceneBeforeLoading)} to exclude this scene from validation").AddInfo("SceneName", activeScene.name).AddInfo("SceneHandle", activeScene.handle)); + return; + } + + // For DA session owners we need to get the scene handle reference that is shared between all clients + // This ensures any late joiners get the same scene handle references after a session owner migration. + sceneEventData.SceneHandle = localHandle; + sceneEventData.SenderClientId = NetworkManager.LocalClientId; + } + // If we're a normal server, then always use the server scene handle + else + { + sceneEventData.SceneHandle = activeScene.handle; + } + + sceneEventData.SceneHash = SceneHashFromNameOrPath(activeScene.path); + AddSceneToClientSynchronization(ref sceneEventData, ref activeScene); + } [MethodImpl(MethodImplOptions.AggressiveInlining)] private void AddSceneToClientSynchronization(ref SceneEventData sceneEventData, ref Scene scene) @@ -2009,37 +2041,17 @@ internal void SynchronizeNetworkObjects(ulong clientId, bool synchronizingServic sceneEventData.LoadSceneMode = ClientSynchronizationMode; var activeScene = SceneManager.GetActiveScene(); sceneEventData.SceneEventType = SceneEventType.Synchronize; - if (BuildIndexToHash.ContainsKey(activeScene.buildIndex)) + if (BuildIndexToHash.TryGetValue(activeScene.buildIndex, out var activeSceneHash)) { - sceneEventData.ActiveSceneHash = BuildIndexToHash[activeScene.buildIndex]; + sceneEventData.ActiveSceneHash = activeSceneHash; } // Organize how (and when) we serialize our NetworkObjects - var hasSynchronizedActive = false; - - // It is possible a user might not want to synchronize the active scene, so we will check to see if it is valid before adding it to the synchronization list. // !! Important !! // The active scene MUST always be the first scene in the synchronization list. - if (ValidateSceneBeforeLoading(activeScene.buildIndex, activeScene.name, sceneEventData.LoadSceneMode)) - { - sceneEventData.SceneHash = SceneHashFromNameOrPath(activeScene.path); - if (sceneEventData.SceneHash == sceneEventData.ActiveSceneHash) - { - hasSynchronizedActive = true; - } - // If we are just a normal client, then always use the server scene handle - if (NetworkManager.DistributedAuthorityMode) - { - sceneEventData.SenderClientId = NetworkManager.LocalClientId; - sceneEventData.SceneHandle = ClientSceneHandleToServerSceneHandle[activeScene.handle]; - } - else - { - sceneEventData.SceneHandle = activeScene.handle; - } - AddSceneToClientSynchronization(ref sceneEventData, ref activeScene); - } + // Don't run user-validation when we're synchronizing the CMB service. CMB always needs to know about the active scene + AddActiveSceneToClientSynchronization(ref sceneEventData, ref activeScene, !synchronizingService); for (int i = 0; i < SceneManager.sceneCount; i++) { @@ -2065,11 +2077,6 @@ internal void SynchronizeNetworkObjects(ulong clientId, bool synchronizingServic AddSceneToClientSynchronization(ref sceneEventData, ref scene); } - if (!hasSynchronizedActive && NetworkManager.CMBServiceConnection && synchronizingService) - { - sceneEventData.AddSceneToSynchronize(BuildIndexToHash[activeScene.buildIndex], ClientSceneHandleToServerSceneHandle[activeScene.handle]); - } - sceneEventData.AddSpawnedNetworkObjects(); sceneEventData.AddDespawnedInSceneNetworkObjects(); var message = new SceneEventMessage diff --git a/testproject/Assets/Tests/Runtime/NetworkSceneManager/SessionOwnerPromotionSceneTests.cs b/testproject/Assets/Tests/Runtime/NetworkSceneManager/SessionOwnerPromotionSceneTests.cs new file mode 100644 index 0000000000..95a3a15ff0 --- /dev/null +++ b/testproject/Assets/Tests/Runtime/NetworkSceneManager/SessionOwnerPromotionSceneTests.cs @@ -0,0 +1,130 @@ +using System.Collections; +using System.Text; +using NUnit.Framework; +using Unity.Netcode; +using Unity.Netcode.TestHelpers.Runtime; +using UnityEngine.SceneManagement; +using UnityEngine.TestTools; + +namespace TestProject.RuntimeTests +{ + /// + /// Validates that a client promoted to session owner registers the scenes it loaded outside of + /// so it can synchronize newly joining clients. + /// Runs against the DAHost by default and against the live CMB service when USE_CMB_SERVICE is set. + /// + internal class SessionOwnerPromotionSceneTests : NetcodeIntegrationTest + { + private const string k_LocallyLoadedScene = "EmptyScene"; + protected override int NumberOfClients => 2; + + private Scene m_OriginalActiveScene; + private Scene m_LocallyLoadedScene; + + public SessionOwnerPromotionSceneTests() : base(HostOrServer.DAHost) { } + + protected override void OnOneTimeSetup() + { + m_OriginalActiveScene = SceneManager.GetActiveScene(); + base.OnOneTimeSetup(); + } + + protected override IEnumerator OnTearDown() + { + SceneManager.SetActiveScene(m_OriginalActiveScene); + if (m_LocallyLoadedScene.IsValid() && m_LocallyLoadedScene.isLoaded) + { + var scene = m_LocallyLoadedScene; + SceneManager.UnloadSceneAsync(scene); + yield return WaitForConditionOrTimeOut(() => !scene.isLoaded); + } + yield return base.OnTearDown(); + } + + private NetworkManager m_InitialSessionOwner; + + /// + /// Avoids since it fails the test when + /// invoked before a new session owner has been promoted. + /// + private bool IsNewSessionOwnerPromoted(StringBuilder errorLog) + { + // Have to do our own loop looking for a session owner as GetAuthorityNetworkManager will fail the test if no session owner is found. + // It is expected that no session owner will be found for a frame or two during session owner promotion. + NetworkManager sessionOwner = null; + foreach (var networkManager in m_NetworkManagers) + { + if (networkManager.LocalClient.IsSessionOwner) + { + sessionOwner = networkManager; + break; + } + } + + if (sessionOwner == null || sessionOwner == m_InitialSessionOwner) + { + errorLog.AppendLine("No new session owner has been promoted."); + return false; + } + + var allClientsSynchronized = true; + foreach (var networkManager in m_NetworkManagers) + { + if (networkManager.CurrentSessionOwner != sessionOwner.LocalClientId) + { + errorLog.AppendLine($"[Client-{networkManager.LocalClientId}] Incorrect {nameof(NetworkManager.CurrentSessionOwner)}! Expected: {sessionOwner.LocalClientId}. Has: {networkManager.CurrentSessionOwner}."); + allClientsSynchronized = false; + } + } + return allClientsSynchronized; + } + + /// + /// Loads and activates a scene without using , promotes a new session owner, + /// and then verifies the promoted session owner can synchronize late joining clients. + /// + [UnityTest] + public IEnumerator PromotedSessionOwnerSynchronizesLocallyLoadedActiveScene() + { + // Load and activate a scene outside of NetworkSceneManager after the session has started, + // so no NetworkManager instance has a network scene handle registered for it. + var loadOperation = SceneManager.LoadSceneAsync(k_LocallyLoadedScene, LoadSceneMode.Additive); + Assert.NotNull(loadOperation, "Failed to load test scene!"); + + yield return WaitForConditionOrTimeOut(() => loadOperation.isDone); + AssertOnTimeout($"Timed out waiting for {k_LocallyLoadedScene} to load!"); + + // Set the newly loaded scene as the active scene + // Now all clients have this unregistered scene as their active scene. + m_LocallyLoadedScene = SceneManager.GetSceneByName(k_LocallyLoadedScene); + SceneManager.SetActiveScene(m_LocallyLoadedScene); + + // Now all clients have this unregistered scene as their active scene. + m_InitialSessionOwner = GetAuthorityNetworkManager(); + + // Sanity check: Double-check the newly loaded scene is unregistered on all clients + foreach (var networkManager in m_NetworkManagers) + { + var isSceneRegistered = networkManager.SceneManager.ClientSceneHandleToServerSceneHandle.ContainsKey(m_LocallyLoadedScene.handle); + Assert.That(isSceneRegistered, Is.False, $"[Client-{networkManager.LocalClientId}] Scene loaded by the UnityEngine SceneManager should not have been registered!"); + } + + if (m_UseCmbService) + { + // the CMB service promotes a client when the current session owner disconnects + yield return StopOneClient(m_InitialSessionOwner); + } + else + { + // The DAHost can manually promote another client as the session owner. + m_InitialSessionOwner.PromoteSessionOwner(GetNonAuthorityNetworkManager().LocalClientId); + } + + yield return WaitForConditionOrTimeOut(IsNewSessionOwnerPromoted); + AssertOnTimeout("Session owner promotion failed!"); + + // Connect a late-joining client to verify the new session owner handles the previously unregistered scene. + yield return CreateAndStartNewClient(); + } + } +} diff --git a/testproject/Assets/Tests/Runtime/NetworkSceneManager/SessionOwnerPromotionSceneTests.cs.meta b/testproject/Assets/Tests/Runtime/NetworkSceneManager/SessionOwnerPromotionSceneTests.cs.meta new file mode 100644 index 0000000000..cd4693033b --- /dev/null +++ b/testproject/Assets/Tests/Runtime/NetworkSceneManager/SessionOwnerPromotionSceneTests.cs.meta @@ -0,0 +1,11 @@ +fileFormatVersion: 2 +guid: ae555b54a49d4f209c7029abe02c3358 +MonoImporter: + externalObjects: {} + serializedVersion: 2 + defaultReferences: [] + executionOrder: 0 + icon: {instanceID: 0} + userData: + assetBundleName: + assetBundleVariant: