fix: Corrected wrong clientId on receive by keeping SDK internal mapping of client ID to transport ID so we can ensure no overlap (#1368) (#1374)

Author: andrews-unity

com.unity.netcode.gameobjects/CHANGELOG.md

@@ -16,6 +16,8 @@ Additional documentation and release notes are available at [Multiplayer Documen
 
 - Overflow exception when syncing Animator state. (#1327)
 - Added `try`/`catch` around RPC calls, preventing exception from causing further RPC calls to fail (#1329)
+- Fixed an issue where ServerClientId and LocalClientId could have the same value, causing potential confusion, and also fixed an issue with the UNet where the server could be identified with two different values, one of which might be the same as LocalClientId, and the other of which would not.(#1368)
+- IL2CPP would not properly compile (#1359)
 
 ## [1.0.0-pre.2] - 2021-10-19
 

com.unity.netcode.gameobjects/Runtime/Core/NetworkManager.cs

@@ -151,7 +151,7 @@ public void Send(ulong clientId, NetworkDelivery delivery, FastBufferWriter batc
             {
                 var sendBuffer = batchData.ToTempByteArray();
 
-                m_NetworkManager.NetworkConfig.NetworkTransport.Send(clientId, sendBuffer, delivery);
+                m_NetworkManager.NetworkConfig.NetworkTransport.Send(m_NetworkManager.ClientIdToTransportId(clientId), sendBuffer, delivery);
             }
         }
 
@@ -223,10 +223,12 @@ public GameObject GetNetworkPrefabOverride(GameObject gameObject)
 
         public NetworkSceneManager SceneManager { get; private set; }
 
+        public readonly ulong ServerClientId = 0;
+
         /// <summary>
         /// Gets the networkId of the server
         /// </summary>
-        public ulong ServerClientId => NetworkConfig.NetworkTransport?.ServerClientId ??
+        private ulong m_ServerTransportId => NetworkConfig.NetworkTransport?.ServerClientId ??
                                        throw new NullReferenceException(
                                            $"The transport in the active {nameof(NetworkConfig)} is null");
 
@@ -243,6 +245,10 @@ public ulong LocalClientId
 
         private Dictionary<ulong, NetworkClient> m_ConnectedClients = new Dictionary<ulong, NetworkClient>();
 
+        private ulong m_NextClientId = 1;
+        private Dictionary<ulong, ulong> m_ClientIdToTransportIdMap = new Dictionary<ulong, ulong>();
+        private Dictionary<ulong, ulong> m_TransportIdToClientIdMap = new Dictionary<ulong, ulong>();
+
         private List<NetworkClient> m_ConnectedClientsList = new List<NetworkClient>();
 
         private List<ulong> m_ConnectedClientIds = new List<ulong>();
@@ -980,6 +986,12 @@ private void OnDestroy()
             }
         }
 
+        private void DisconnectRemoteClient(ulong clientId)
+        {
+            var transportId = ClientIdToTransportId(clientId);
+            NetworkConfig.NetworkTransport.DisconnectRemoteClient(transportId);
+        }
+
         /// <summary>
         /// Globally shuts down the library.
         /// Disconnects clients if connected and stops server if running.
@@ -1014,7 +1026,7 @@ public void Shutdown()
                             continue;
                         }
 
-                        NetworkConfig.NetworkTransport.DisconnectRemoteClient(pair.Key);
+                        DisconnectRemoteClient(pair.Key);
                     }
                 }
 
@@ -1028,7 +1040,7 @@ public void Shutdown()
                             continue;
                         }
 
-                        NetworkConfig.NetworkTransport.DisconnectRemoteClient(pair.Key);
+                        DisconnectRemoteClient(pair.Key);
                     }
                 }
             }
@@ -1098,6 +1110,9 @@ public void Shutdown()
                 NetworkConfig?.NetworkTransport?.Shutdown();
             }
 
+            m_ClientIdToTransportIdMap.Clear();
+            m_TransportIdToClientIdMap.Clear();
+
             IsListening = false;
         }
 
@@ -1224,14 +1239,30 @@ private IEnumerator ApprovalTimeout(ulong clientId)
             }
         }
 
+        private ulong TransportIdToClientId(ulong transportId)
+        {
+            return transportId == m_ServerTransportId ? ServerClientId : m_TransportIdToClientIdMap[transportId];
+        }
+
+        private ulong ClientIdToTransportId(ulong clientId)
+        {
+            return clientId == ServerClientId ? m_ServerTransportId : m_ClientIdToTransportIdMap[clientId];
+        }
+
         private void HandleRawTransportPoll(NetworkEvent networkEvent, ulong clientId, ArraySegment<byte> payload, float receiveTime)
         {
+            var transportId = clientId;
             switch (networkEvent)
             {
                 case NetworkEvent.Connect:
 #if DEVELOPMENT_BUILD || UNITY_EDITOR
                     s_TransportConnect.Begin();
 #endif
+
+                    clientId = m_NextClientId++;
+                    m_ClientIdToTransportIdMap[clientId] = transportId;
+                    m_TransportIdToClientIdMap[transportId] = clientId;
+
                     MessagingSystem.ClientConnected(clientId);
                     if (IsServer)
                     {
@@ -1270,13 +1301,19 @@ private void HandleRawTransportPoll(NetworkEvent networkEvent, ulong clientId, A
                             NetworkLog.LogInfo($"Incoming Data From {clientId}: {payload.Count} bytes");
                         }
 
+                        clientId = TransportIdToClientId(clientId);
+
                         HandleIncomingData(clientId, payload, receiveTime);
                         break;
                     }
                 case NetworkEvent.Disconnect:
 #if DEVELOPMENT_BUILD || UNITY_EDITOR
                     s_TransportDisconnect.Begin();
 #endif
+                    clientId = TransportIdToClientId(clientId);
+
+                    m_TransportIdToClientIdMap.Remove(transportId);
+                    m_ClientIdToTransportIdMap.Remove(clientId);
 
                     if (NetworkLog.CurrentLogLevel <= LogLevel.Developer)
                     {
@@ -1400,8 +1437,7 @@ public void DisconnectClient(ulong clientId)
             }
 
             OnClientDisconnectFromServer(clientId);
-
-            NetworkConfig.NetworkTransport.DisconnectRemoteClient(clientId);
+            DisconnectRemoteClient(clientId);
         }
 
         private void OnClientDisconnectFromServer(ulong clientId)
@@ -1575,7 +1611,7 @@ internal void HandleApproval(ulong ownerClientId, bool createPlayerObject, uint?
             else
             {
                 PendingClients.Remove(ownerClientId);
-                NetworkConfig.NetworkTransport.DisconnectRemoteClient(ownerClientId);
+                DisconnectRemoteClient(ownerClientId);
             }
         }
 

testproject/Assets/Tests/Runtime/SenderIdTests.cs

@@ -0,0 +1,93 @@
+using System;
+using System.Collections;
+using Unity.Netcode;
+using Unity.Netcode.RuntimeTests;
+using NUnit.Framework;
+using Unity.Collections;
+using UnityEngine;
+using UnityEngine.TestTools;
+
+namespace TestProject.RuntimeTests
+{
+    public class SenderIdTests : BaseMultiInstanceTest
+    {
+        protected override int NbClients => 2;
+
+        private NetworkManager FirstClient => m_ClientNetworkManagers[0];
+        private NetworkManager SecondClient => m_ClientNetworkManagers[1];
+
+        [UnityTest]
+        public IEnumerator WhenSendingMessageFromServerToClient_SenderIdIsCorrect()
+        {
+            var messageContent = Guid.NewGuid();
+            var writer = new FastBufferWriter(1300, Allocator.Temp);
+            using (writer)
+            {
+                writer.WriteValueSafe(messageContent);
+                m_ServerNetworkManager.CustomMessagingManager.SendUnnamedMessageToAll(writer);
+            }
+
+            bool firstReceived = false;
+            FirstClient.CustomMessagingManager.OnUnnamedMessage +=
+                (sender, reader) =>
+                {
+                    firstReceived = true;
+                    Assert.AreEqual(sender, FirstClient.ServerClientId);
+                    Assert.AreNotEqual(sender, FirstClient.LocalClientId);
+                };
+
+            bool secondReceived = false;
+            SecondClient.CustomMessagingManager.OnUnnamedMessage +=
+                (sender, reader) =>
+                {
+                    secondReceived = true;
+                    Assert.AreEqual(sender, FirstClient.ServerClientId);
+                    Assert.AreNotEqual(sender, FirstClient.LocalClientId);
+                };
+
+            yield return new WaitForSeconds(0.2f);
+
+            Assert.IsTrue(firstReceived);
+            Assert.IsTrue(secondReceived);
+        }
+        [UnityTest]
+        public IEnumerator WhenSendingMessageFromClientToServer_SenderIdIsCorrect()
+        {
+            var messageContent = Guid.NewGuid();
+            var writer = new FastBufferWriter(1300, Allocator.Temp);
+            using (writer)
+            {
+                FirstClient.CustomMessagingManager.SendNamedMessage("FirstClient", FirstClient.ServerClientId, writer);
+                SecondClient.CustomMessagingManager.SendNamedMessage("SecondClient", SecondClient.ServerClientId, writer);
+
+            }
+
+            bool firstReceived = false;
+            m_ServerNetworkManager.CustomMessagingManager.RegisterNamedMessageHandler(
+                "FirstClient",
+                (sender, reader) =>
+                {
+                    firstReceived = true;
+                    Assert.AreEqual(sender, FirstClient.LocalClientId);
+                    Assert.AreNotEqual(sender, SecondClient.LocalClientId);
+                    Assert.AreNotEqual(sender, m_ServerNetworkManager.LocalClientId);
+                });
+
+            bool secondReceived = false;
+            m_ServerNetworkManager.CustomMessagingManager.RegisterNamedMessageHandler(
+                "SecondClient",
+                (sender, reader) =>
+                {
+                    secondReceived = true;
+                    Assert.AreNotEqual(sender, FirstClient.LocalClientId);
+                    Assert.AreEqual(sender, SecondClient.LocalClientId);
+                    Assert.AreNotEqual(sender, m_ServerNetworkManager.LocalClientId);
+                });
+
+            yield return new WaitForSeconds(0.2f);
+
+            Assert.IsTrue(firstReceived);
+            Assert.IsTrue(secondReceived);
+        }
+    }
+}