From 454e51988365bcb739bb2e88c4611b04b6dddd7a Mon Sep 17 00:00:00 2001 From: Emma Date: Thu, 20 Feb 2025 14:44:50 -0500 Subject: [PATCH 1/2] fix: Incorrect clientId in OnClientConnectedCallback --- .../Messages/ConnectionApprovedMessage.cs | 2 +- .../Tests/Runtime/Connection.meta | 3 + .../Connection/ClientConnectionTests.cs | 82 +++++++++++++++++++ .../Connection/ClientConnectionTests.cs.meta | 3 + 4 files changed, 89 insertions(+), 1 deletion(-) create mode 100644 com.unity.netcode.gameobjects/Tests/Runtime/Connection.meta create mode 100644 com.unity.netcode.gameobjects/Tests/Runtime/Connection/ClientConnectionTests.cs create mode 100644 com.unity.netcode.gameobjects/Tests/Runtime/Connection/ClientConnectionTests.cs.meta diff --git a/com.unity.netcode.gameobjects/Runtime/Messaging/Messages/ConnectionApprovedMessage.cs b/com.unity.netcode.gameobjects/Runtime/Messaging/Messages/ConnectionApprovedMessage.cs index 5005c1987c..4f9e0f5453 100644 --- a/com.unity.netcode.gameobjects/Runtime/Messaging/Messages/ConnectionApprovedMessage.cs +++ b/com.unity.netcode.gameobjects/Runtime/Messaging/Messages/ConnectionApprovedMessage.cs @@ -305,7 +305,7 @@ public void Handle(ref NetworkContext context) NetworkLog.LogInfo($"[Client-{OwnerClientId}][Scene Management Disabled] Synchronization complete!"); } // When scene management is disabled we notify after everything is synchronized - networkManager.ConnectionManager.InvokeOnClientConnectedCallback(context.SenderId); + networkManager.ConnectionManager.InvokeOnClientConnectedCallback(OwnerClientId); // For convenience, notify all NetworkBehaviours that synchronization is complete. networkManager.SpawnManager.NotifyNetworkObjectsSynchronized(); diff --git a/com.unity.netcode.gameobjects/Tests/Runtime/Connection.meta b/com.unity.netcode.gameobjects/Tests/Runtime/Connection.meta new file mode 100644 index 0000000000..e8a0528d8a --- /dev/null +++ b/com.unity.netcode.gameobjects/Tests/Runtime/Connection.meta @@ -0,0 +1,3 @@ +fileFormatVersion: 2 +guid: 6a87f9e5685b4895bd5a7936e6ac567f +timeCreated: 1740072310 \ No newline at end of file diff --git a/com.unity.netcode.gameobjects/Tests/Runtime/Connection/ClientConnectionTests.cs b/com.unity.netcode.gameobjects/Tests/Runtime/Connection/ClientConnectionTests.cs new file mode 100644 index 0000000000..d4c1550b8d --- /dev/null +++ b/com.unity.netcode.gameobjects/Tests/Runtime/Connection/ClientConnectionTests.cs @@ -0,0 +1,82 @@ +using System.Collections; +using System.Collections.Generic; +using NUnit.Framework; +using Unity.Netcode.TestHelpers.Runtime; +using UnityEngine.TestTools; + +namespace Unity.Netcode.RuntimeTests +{ + [TestFixture(SceneManagementState.SceneManagementEnabled, NetworkTopologyTypes.DistributedAuthority)] + [TestFixture(SceneManagementState.SceneManagementDisabled, NetworkTopologyTypes.DistributedAuthority)] + [TestFixture(SceneManagementState.SceneManagementEnabled, NetworkTopologyTypes.ClientServer)] + [TestFixture(SceneManagementState.SceneManagementDisabled, NetworkTopologyTypes.ClientServer)] + internal class ClientConnectionTests : IntegrationTestWithApproximation + { + protected override int NumberOfClients => 3; + private readonly bool m_SceneManagementEnabled; + private HashSet m_ServerCallbackCalled = new HashSet(); + private HashSet m_ClientCallbackCalled = new HashSet(); + + public ClientConnectionTests(SceneManagementState sceneManagementState, NetworkTopologyTypes networkTopologyType) : base(networkTopologyType) + { + m_SceneManagementEnabled = sceneManagementState == SceneManagementState.SceneManagementEnabled; + } + + protected override void OnServerAndClientsCreated() + { + m_ServerNetworkManager.NetworkConfig.EnableSceneManagement = m_SceneManagementEnabled; + m_ServerNetworkManager.OnClientConnectedCallback += Server_OnClientConnectedCallback; + + foreach (var client in m_ClientNetworkManagers) + { + client.NetworkConfig.EnableSceneManagement = m_SceneManagementEnabled; + client.OnClientConnectedCallback += Client_OnClientConnectedCallback; + } + + base.OnServerAndClientsCreated(); + } + + [UnityTest] + public IEnumerator VerifyOnClientConnectedCallback() + { + yield return WaitForConditionOrTimeOut(AllCallbacksCalled); + AssertOnTimeout("Timed out waiting for all clients to be connected!"); + + // The client callbacks should have been called once per client (called once on self) + Assert.True(m_ClientCallbackCalled.Count == NumberOfClients); + + // The server callback should be called for self, and then once per client + Assert.True(m_ServerCallbackCalled.Count == 1 + NumberOfClients); + } + + private void Server_OnClientConnectedCallback(ulong clientId) + { + if (!m_ServerCallbackCalled.Add(clientId)) + { + Assert.Fail($"Client already connected: {clientId}"); + } + } + + private void Client_OnClientConnectedCallback(ulong clientId) + { + if (!m_ClientCallbackCalled.Add(clientId)) + { + Assert.Fail($"Client already connected: {clientId}"); + } + } + + private bool AllCallbacksCalled() + { + foreach (var client in m_ClientNetworkManagers) + { + if (!m_ClientCallbackCalled.Contains(client.LocalClientId) || !m_ServerCallbackCalled.Contains(client.LocalClientId)) + { + return false; + } + } + + return m_ServerCallbackCalled.Contains(m_ServerNetworkManager.LocalClientId); + } + } +} + diff --git a/com.unity.netcode.gameobjects/Tests/Runtime/Connection/ClientConnectionTests.cs.meta b/com.unity.netcode.gameobjects/Tests/Runtime/Connection/ClientConnectionTests.cs.meta new file mode 100644 index 0000000000..a52e7c6ccd --- /dev/null +++ b/com.unity.netcode.gameobjects/Tests/Runtime/Connection/ClientConnectionTests.cs.meta @@ -0,0 +1,3 @@ +fileFormatVersion: 2 +guid: c5c3b4494e2946dc967cc70ffea4d850 +timeCreated: 1740072340 \ No newline at end of file From a99382474cda44580a8061045ea7960a708a5560 Mon Sep 17 00:00:00 2001 From: Emma Date: Thu, 20 Feb 2025 14:49:07 -0500 Subject: [PATCH 2/2] CHANGELOG.md --- com.unity.netcode.gameobjects/CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/com.unity.netcode.gameobjects/CHANGELOG.md b/com.unity.netcode.gameobjects/CHANGELOG.md index cc82759db7..75f3dd780a 100644 --- a/com.unity.netcode.gameobjects/CHANGELOG.md +++ b/com.unity.netcode.gameobjects/CHANGELOG.md @@ -12,6 +12,7 @@ Additional documentation and release notes are available at [Multiplayer Documen ### Fixed +- Fixed `OnClientConnectedCallback` passing incorrect `clientId` when scene management is disabled. (#3312) - Fixed DestroyObject flow on non-authority game clients. (#3291) - Fixed exception being thrown when a `GameObject` with an associated `NetworkTransform` is disabled. (#3243) - Fixed issue where the scene migration synchronization table was not cleaned up if the `GameObject` of a `NetworkObject` is destroyed before it should have been. (#3230)