From a9e19bd1985d370538c10aaac5899a8ff33f5cdb Mon Sep 17 00:00:00 2001 From: NoelStephensUnity <73188597+NoelStephensUnity@users.noreply.github.com> Date: Tue, 31 Aug 2021 18:43:22 -0500 Subject: [PATCH 1/7] fix Fix for the client connected message not being invoked on the client if scene management is disabled. This includes an integration test, MultiClientConnectionApproval.ClientConnectedCallbackTest, to verify client connected is being invoked when scene management is both enabled and disabled. --- .../Runtime/Core/NetworkManager.cs | 7 +++++-- .../Runtime/Messaging/InternalMessageHandler.cs | 5 +++++ .../Runtime/SceneManagement/NetworkSceneManager.cs | 8 +++++++- .../Tests/Editor/NetworkManagerMessageHandlerTests.cs | 1 + 4 files changed, 18 insertions(+), 3 deletions(-) diff --git a/com.unity.netcode.gameobjects/Runtime/Core/NetworkManager.cs b/com.unity.netcode.gameobjects/Runtime/Core/NetworkManager.cs index 1af4e45b31..89be713d52 100644 --- a/com.unity.netcode.gameobjects/Runtime/Core/NetworkManager.cs +++ b/com.unity.netcode.gameobjects/Runtime/Core/NetworkManager.cs @@ -1495,14 +1495,17 @@ internal void HandleApproval(ulong ownerClientId, bool createPlayerObject, uint? { SceneManager.SynchronizeNetworkObjects(ownerClientId); } + else + { + InvokeOnClientConnectedCallback(ownerClientId); + } } else // Server just adds itself as an observer to all spawned NetworkObjects { SpawnManager.UpdateObservedNetworkObjects(ownerClientId); + InvokeOnClientConnectedCallback(ownerClientId); } - OnClientConnectedCallback?.Invoke(ownerClientId); - if (!createPlayerObject || (playerPrefabHash == null && NetworkConfig.PlayerPrefab == null)) { return; diff --git a/com.unity.netcode.gameobjects/Runtime/Messaging/InternalMessageHandler.cs b/com.unity.netcode.gameobjects/Runtime/Messaging/InternalMessageHandler.cs index c4af496fc7..52cb61f30a 100644 --- a/com.unity.netcode.gameobjects/Runtime/Messaging/InternalMessageHandler.cs +++ b/com.unity.netcode.gameobjects/Runtime/Messaging/InternalMessageHandler.cs @@ -76,6 +76,11 @@ public void HandleConnectionApproved(ulong clientId, Stream stream, float receiv { NetworkObject.DeserializeSceneObject(reader.GetStream() as NetworkBuffer, reader, m_NetworkManager); } + + // Mark the client being connected + m_NetworkManager.IsConnectedClient = true; + // When scene management is disabled we notify after everything is synchronized + m_NetworkManager.InvokeOnClientConnectedCallback(clientId); } } diff --git a/com.unity.netcode.gameobjects/Runtime/SceneManagement/NetworkSceneManager.cs b/com.unity.netcode.gameobjects/Runtime/SceneManagement/NetworkSceneManager.cs index 04a0760bca..9cff4e0a14 100644 --- a/com.unity.netcode.gameobjects/Runtime/SceneManagement/NetworkSceneManager.cs +++ b/com.unity.netcode.gameobjects/Runtime/SceneManagement/NetworkSceneManager.cs @@ -1377,7 +1377,6 @@ private void HandleClientSceneEvent(Stream stream) // All scenes are synchronized, let the server know we are done synchronizing m_NetworkManager.IsConnectedClient = true; - m_NetworkManager.InvokeOnClientConnectedCallback(m_NetworkManager.LocalClientId); // Notify the client that they have finished synchronizing OnSceneEvent?.Invoke(new SceneEvent() @@ -1385,6 +1384,9 @@ private void HandleClientSceneEvent(Stream stream) SceneEventType = SceneEventData.SceneEventType, ClientId = m_NetworkManager.LocalClientId, // Client sent this to the server }); + + // Client is now synchronized and fully "connected". This also means the client can send "RPCs" at this time + m_NetworkManager.InvokeOnClientConnectedCallback(m_NetworkManager.LocalClientId); } break; } @@ -1477,6 +1479,10 @@ private void HandleServerSceneEvent(ulong clientId, Stream stream) ClientId = clientId }); + // While we did invoke the C2S_SyncComplete event notification, we will also call the traditional client connected callback on the server + // which assures the client is "ready to receive RPCs" as well. + m_NetworkManager.InvokeOnClientConnectedCallback(clientId); + if (SceneEventData.ClientNeedsReSynchronization() && !DisableReSynchronization) { SceneEventData.SceneEventType = SceneEventData.SceneEventTypes.S2C_ReSync; diff --git a/com.unity.netcode.gameobjects/Tests/Editor/NetworkManagerMessageHandlerTests.cs b/com.unity.netcode.gameobjects/Tests/Editor/NetworkManagerMessageHandlerTests.cs index 2b01929ccc..58816d0af2 100644 --- a/com.unity.netcode.gameobjects/Tests/Editor/NetworkManagerMessageHandlerTests.cs +++ b/com.unity.netcode.gameobjects/Tests/Editor/NetworkManagerMessageHandlerTests.cs @@ -140,6 +140,7 @@ public void MessageHandlerReceivedMessageServerClient() using var messageStream13 = MessagePacker.WrapMessage(MessageQueueContainer.MessageType.CreateObject, inputBuffer, networkManager.MessageQueueContainer.IsUsingBatching()); networkManager.HandleIncomingData(0, NetworkChannel.Internal, new ArraySegment(messageStream13.GetBuffer(), 0, (int)messageStream13.Length), 0); + // Should cause log (client only) // Everything should log MessageReceiveQueueItem even if ignored LogAssert.Expect(LogType.Log, nameof(DummyMessageHandler.MessageReceiveQueueItem)); From 8de82c03f14a8b2dc4dcf76610a7820c02ffc40e Mon Sep 17 00:00:00 2001 From: NoelStephensUnity <73188597+NoelStephensUnity@users.noreply.github.com> Date: Tue, 31 Aug 2021 18:44:27 -0500 Subject: [PATCH 2/7] fix This is a fix to reduce the amount of console spew due to network buffer pools exceeding 1024. This reduces unit test processing time and console log sizes, while also providing a more realistic buffer pool size. --- .../Runtime/Serialization/Pooled/NetworkBufferPool.cs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/com.unity.netcode.gameobjects/Runtime/Serialization/Pooled/NetworkBufferPool.cs b/com.unity.netcode.gameobjects/Runtime/Serialization/Pooled/NetworkBufferPool.cs index 1e35b991b6..884ecb0177 100644 --- a/com.unity.netcode.gameobjects/Runtime/Serialization/Pooled/NetworkBufferPool.cs +++ b/com.unity.netcode.gameobjects/Runtime/Serialization/Pooled/NetworkBufferPool.cs @@ -12,8 +12,11 @@ public static class NetworkBufferPool private static Queue s_OverflowBuffers = new Queue(); private static Queue s_Buffers = new Queue(); - private const uint k_MaxBitPoolBuffers = 1024; - private const uint k_MaxCreatedDelta = 768; + // Increasing this value for various reasons: + // Some tests (i.e. MessageHandlerReceivedMessageServerClient) expect certain log messages and if we happen to hit the threshold it will cause those tests to fail + // This slows down tests having to log warnings repeatedly. + private const uint k_MaxBitPoolBuffers = 2048; + private const uint k_MaxCreatedDelta = 1512; /// From 9ce55595deb06c9d8a13276a314767346ac25758 Mon Sep 17 00:00:00 2001 From: NoelStephensUnity <73188597+NoelStephensUnity@users.noreply.github.com> Date: Tue, 31 Aug 2021 18:55:41 -0500 Subject: [PATCH 3/7] test The unit test. --- .../Runtime/MultiClientConnectionApproval.cs | 56 +++++++++++++++++++ 1 file changed, 56 insertions(+) diff --git a/testproject/Assets/Tests/Runtime/MultiClientConnectionApproval.cs b/testproject/Assets/Tests/Runtime/MultiClientConnectionApproval.cs index 04b624fed3..ca8288b248 100644 --- a/testproject/Assets/Tests/Runtime/MultiClientConnectionApproval.cs +++ b/testproject/Assets/Tests/Runtime/MultiClientConnectionApproval.cs @@ -43,6 +43,8 @@ public IEnumerator ConnectionApprovalPrefabOverride() } + + /// /// Allows for several connection approval related configurations /// @@ -187,6 +189,60 @@ private void ConnectionApprovalCallback(byte[] connectionData, ulong clientId, N } } + + + private int m_ServerClientConnectedInvocations; + + private int m_ClientConnectedInvocations; + + /// + /// Tests that the OnClientConnectedCallback is invoked when scene management is enabled and disabled + /// + /// + [UnityTest] + public IEnumerator ClientConnectedCallbackTest([Values(true, false)] bool enableSceneManagement) + { + m_ServerClientConnectedInvocations = 0; + m_ClientConnectedInvocations = 0; + + // Create Host and (numClients) clients + Assert.True(MultiInstanceHelpers.Create(3, out NetworkManager server, out NetworkManager[] clients)); + + server.NetworkConfig.EnableSceneManagement = enableSceneManagement; + server.OnClientConnectedCallback += Server_OnClientConnectedCallback; + + foreach (var client in clients) + { + client.NetworkConfig.EnableSceneManagement = enableSceneManagement; + client.OnClientConnectedCallback += Client_OnClientConnectedCallback; + } + + // Start the instances + if (!MultiInstanceHelpers.Start(true, server, clients)) + { + Assert.Fail("Failed to start instances"); + } + + // [Client-Side] Wait for a connection to the server + yield return MultiInstanceHelpers.Run(MultiInstanceHelpers.WaitForClientsConnected(clients, null, 512)); + + // [Host-Side] Check to make sure all clients are connected + yield return MultiInstanceHelpers.Run(MultiInstanceHelpers.WaitForClientsConnectedToServer(server, clients.Length + 1, null, 512)); + + Assert.True(m_ClientConnectedInvocations == 3); + Assert.True(m_ServerClientConnectedInvocations == 4); + } + + private void Client_OnClientConnectedCallback(ulong clientId) + { + m_ClientConnectedInvocations++; + } + + private void Server_OnClientConnectedCallback(ulong clientId) + { + m_ServerClientConnectedInvocations++; + } + [TearDown] public void TearDown() { From 495e38ae498d68d8e9085469eea14985d4ef3ae6 Mon Sep 17 00:00:00 2001 From: NoelStephensUnity <73188597+NoelStephensUnity@users.noreply.github.com> Date: Tue, 31 Aug 2021 19:39:28 -0500 Subject: [PATCH 4/7] refactor Removing this minor LF as the file has nothing to do with this PR. --- .../Tests/Editor/NetworkManagerMessageHandlerTests.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/com.unity.netcode.gameobjects/Tests/Editor/NetworkManagerMessageHandlerTests.cs b/com.unity.netcode.gameobjects/Tests/Editor/NetworkManagerMessageHandlerTests.cs index 58816d0af2..2b01929ccc 100644 --- a/com.unity.netcode.gameobjects/Tests/Editor/NetworkManagerMessageHandlerTests.cs +++ b/com.unity.netcode.gameobjects/Tests/Editor/NetworkManagerMessageHandlerTests.cs @@ -140,7 +140,6 @@ public void MessageHandlerReceivedMessageServerClient() using var messageStream13 = MessagePacker.WrapMessage(MessageQueueContainer.MessageType.CreateObject, inputBuffer, networkManager.MessageQueueContainer.IsUsingBatching()); networkManager.HandleIncomingData(0, NetworkChannel.Internal, new ArraySegment(messageStream13.GetBuffer(), 0, (int)messageStream13.Length), 0); - // Should cause log (client only) // Everything should log MessageReceiveQueueItem even if ignored LogAssert.Expect(LogType.Log, nameof(DummyMessageHandler.MessageReceiveQueueItem)); From fa13e2efec03d8bf24b67f4a3ced27ed743da114 Mon Sep 17 00:00:00 2001 From: NoelStephensUnity <73188597+NoelStephensUnity@users.noreply.github.com> Date: Tue, 31 Aug 2021 19:41:26 -0500 Subject: [PATCH 5/7] style removing 2 LF that were not intended --- .../Assets/Tests/Runtime/MultiClientConnectionApproval.cs | 2 -- 1 file changed, 2 deletions(-) diff --git a/testproject/Assets/Tests/Runtime/MultiClientConnectionApproval.cs b/testproject/Assets/Tests/Runtime/MultiClientConnectionApproval.cs index ca8288b248..c898b189be 100644 --- a/testproject/Assets/Tests/Runtime/MultiClientConnectionApproval.cs +++ b/testproject/Assets/Tests/Runtime/MultiClientConnectionApproval.cs @@ -43,8 +43,6 @@ public IEnumerator ConnectionApprovalPrefabOverride() } - - /// /// Allows for several connection approval related configurations /// From 4fe30c9fc61688357e4d3a38a064eaf4013b95a5 Mon Sep 17 00:00:00 2001 From: NoelStephensUnity <73188597+NoelStephensUnity@users.noreply.github.com> Date: Tue, 31 Aug 2021 20:06:04 -0500 Subject: [PATCH 6/7] refactor style Some straggler style requests from PR-1080 that are harmless and piggy-backing onto this PR. --- .../Runtime/SceneManagement/NetworkSceneManager.cs | 8 ++++---- .../Runtime/SceneManagement/SceneEventProgress.cs | 2 +- .../ObjectParenting/NetworkObjectParentingTests.cs | 2 -- 3 files changed, 5 insertions(+), 7 deletions(-) diff --git a/com.unity.netcode.gameobjects/Runtime/SceneManagement/NetworkSceneManager.cs b/com.unity.netcode.gameobjects/Runtime/SceneManagement/NetworkSceneManager.cs index 9cff4e0a14..a54bba17be 100644 --- a/com.unity.netcode.gameobjects/Runtime/SceneManagement/NetworkSceneManager.cs +++ b/com.unity.netcode.gameobjects/Runtime/SceneManagement/NetworkSceneManager.cs @@ -547,7 +547,7 @@ private SceneEventProgress ValidateSceneEvent(string sceneName, bool isUnloading } var sceneEventProgress = new SceneEventProgress(m_NetworkManager); - sceneEventProgress.SceneIndex = GetBuildIndexFromSceneName(sceneName); + sceneEventProgress.SceneBuildIndex = GetBuildIndexFromSceneName(sceneName); SceneEventProgressTracking.Add(sceneEventProgress.Guid, sceneEventProgress); if (!isUnloading) @@ -582,7 +582,7 @@ private bool OnSceneEventProgressCompleted(SceneEventProgress sceneEventProgress { using var nonNullContext = (InternalCommandContext)context; ClientSynchEventData.SceneEventGuid = sceneEventProgress.Guid; - ClientSynchEventData.SceneIndex = sceneEventProgress.SceneIndex; + ClientSynchEventData.SceneIndex = sceneEventProgress.SceneBuildIndex; ClientSynchEventData.SceneEventType = sceneEventProgress.SceneEventType; ClientSynchEventData.ClientsCompleted = sceneEventProgress.DoneClients; ClientSynchEventData.ClientsTimedOut = m_NetworkManager.ConnectedClients.Keys.Except(sceneEventProgress.DoneClients).ToList(); @@ -596,7 +596,7 @@ private bool OnSceneEventProgressCompleted(SceneEventProgress sceneEventProgress m_NetworkManager.NetworkMetrics.TrackSceneEventSent( m_NetworkManager.ConnectedClientsIds, (uint)sceneEventProgress.SceneEventType, - ScenesInBuild[(int)sceneEventProgress.SceneIndex], + ScenesInBuild[(int)sceneEventProgress.SceneBuildIndex], size); } @@ -604,7 +604,7 @@ private bool OnSceneEventProgressCompleted(SceneEventProgress sceneEventProgress OnSceneEvent?.Invoke(new SceneEvent() { SceneEventType = sceneEventProgress.SceneEventType, - SceneName = ScenesInBuild[(int)sceneEventProgress.SceneIndex], + SceneName = ScenesInBuild[(int)sceneEventProgress.SceneBuildIndex], ClientId = m_NetworkManager.ServerClientId, LoadSceneMode = sceneEventProgress.LoadSceneMode, ClientsThatCompleted = sceneEventProgress.DoneClients, diff --git a/com.unity.netcode.gameobjects/Runtime/SceneManagement/SceneEventProgress.cs b/com.unity.netcode.gameobjects/Runtime/SceneManagement/SceneEventProgress.cs index d38df4ddfb..f9f2010217 100644 --- a/com.unity.netcode.gameobjects/Runtime/SceneManagement/SceneEventProgress.cs +++ b/com.unity.netcode.gameobjects/Runtime/SceneManagement/SceneEventProgress.cs @@ -88,7 +88,7 @@ internal class SceneEventProgress /// internal bool AreAllClientsDoneLoading { get; private set; } - internal uint SceneIndex { get; set; } + internal uint SceneBuildIndex { get; set; } internal Guid Guid { get; } = Guid.NewGuid(); diff --git a/testproject/Assets/Tests/Runtime/ObjectParenting/NetworkObjectParentingTests.cs b/testproject/Assets/Tests/Runtime/ObjectParenting/NetworkObjectParentingTests.cs index 47120d4e5c..8ce7a0eeb5 100644 --- a/testproject/Assets/Tests/Runtime/ObjectParenting/NetworkObjectParentingTests.cs +++ b/testproject/Assets/Tests/Runtime/ObjectParenting/NetworkObjectParentingTests.cs @@ -51,8 +51,6 @@ public IEnumerator Setup() { SceneManager.sceneLoaded += OnSceneLoaded; - // We need NetworkManager to be instantiated first before you load scenes externally in order to be able to determine if - // we are running a unit test or not. (it is this or manually setting a property) Assert.That(MultiInstanceHelpers.Create(k_ClientInstanceCount, out m_ServerNetworkManager, out m_ClientNetworkManagers)); const string scenePath = "Assets/Tests/Runtime/ObjectParenting/" + nameof(NetworkObjectParentingTests) + ".unity"; From 17c2eab25da891c3ec064fac2941ac509395d163 Mon Sep 17 00:00:00 2001 From: NoelStephensUnity <73188597+NoelStephensUnity@users.noreply.github.com> Date: Wed, 1 Sep 2021 09:10:34 -0500 Subject: [PATCH 7/7] refactor Reverting the NetworkBufferPool changes. This will make MessageHandlerReceivedMessageServerClient fail because of the warning message that we have exceeded 1024 buffers. --- .../Runtime/Serialization/Pooled/NetworkBufferPool.cs | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/com.unity.netcode.gameobjects/Runtime/Serialization/Pooled/NetworkBufferPool.cs b/com.unity.netcode.gameobjects/Runtime/Serialization/Pooled/NetworkBufferPool.cs index 884ecb0177..b1cc8186f2 100644 --- a/com.unity.netcode.gameobjects/Runtime/Serialization/Pooled/NetworkBufferPool.cs +++ b/com.unity.netcode.gameobjects/Runtime/Serialization/Pooled/NetworkBufferPool.cs @@ -12,11 +12,8 @@ public static class NetworkBufferPool private static Queue s_OverflowBuffers = new Queue(); private static Queue s_Buffers = new Queue(); - // Increasing this value for various reasons: - // Some tests (i.e. MessageHandlerReceivedMessageServerClient) expect certain log messages and if we happen to hit the threshold it will cause those tests to fail - // This slows down tests having to log warnings repeatedly. - private const uint k_MaxBitPoolBuffers = 2048; - private const uint k_MaxCreatedDelta = 1512; + private const uint k_MaxBitPoolBuffers = 1024; + private const uint k_MaxCreatedDelta = 512; ///