fix: invoke OnNetCodeDisconnect when a hybrid connection closes

com.unity.netcode.gameobjects/CHANGELOG.md

@@ -15,6 +15,7 @@ Additional documentation and release notes are available at [Multiplayer Documen
 
 ### Changed
 
+- A hybrid mode session now disables `NetcodeConfig.AutomaticStateReplication` at runtime, as Netcode for GameObjects puts each connection in game once the client has synchronized. (#4209)
 - Renamed the Editor assembly definition files to match their assembly names. Assembly names and GUIDs are unchanged. (#4187)
 
 ### Deprecated
@@ -24,6 +25,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. (#4214)
+- Issue where `NetworkManager.OnNetCodeDisconnect` was not invoked when a connection closed during a hybrid mode session, only when the session shut down. (#4209)
 - Issue where a rejected parent change, for example one made by a client without parenting authority, restored the `NetworkObject`'s parent but left it at the moved position when the change was made with `worldPositionStays` disabled. The original position, rotation and scale are now restored. (#4202)
 - Issue where a client's clock kept alternating between running slightly fast and slightly slow on every frame, even after it had synchronized with the server. Interpolated objects juddered (a periodic motion artifact caused by mismatched frame rates): they moved in uneven steps instead of smoothly. Barely noticeable at the default `NetworkTimeSystem.AdjustmentRatio`, and clearly visible at higher values. (#4195)
 - Issue where the IL post-processor could not resolve core library types, such as `Guid` and `List<T>`, in assemblies compiled against .NET reference assemblies, for example when targeting .NET 10. Those split the core library across `System.Runtime`, `System.Collections` and other assemblies. (#4188)

com.unity.netcode.gameobjects/Runtime/Components/Helpers/UnifiedUpdateConnections.cs

@@ -55,14 +55,15 @@
             var networkManager = m_HybridNetworkManagerQuery.GetSingleton<HybridNetworkManager>().NetworkManager.Value;
             // A missing NetworkManager counts as shutting down: the world outlives it if ShutdownInternal throws before disposing the world.
             var isShuttingDown = !networkManager || networkManager.ShutdownInProgress || !networkManager.IsListening;
-            foreach (var (networkId, connectionState, entity) in SystemAPI.Query<NetworkId, ConnectionState>().WithNone<NetworkStreamConnection>().WithEntityAccess())
+            // A destroyed connection keeps only its cleanup components, so the NetworkId comes from ConnectionState.
+            foreach (var (connectionState, entity) in SystemAPI.Query<ConnectionState>().WithNone<NetworkStreamConnection>().WithEntityAccess())
             {
                 commandBuffer.RemoveComponent<ConnectionState>(entity);
                 m_TempConnections.Add(new NetcodeConnection
                 {
                     World = World,
                     Entity = entity,
-                    NetworkId = networkId.Value
+                    NetworkId = connectionState.NetworkId
                 });
             }
 
@@ -99,7 +100,7 @@
                     {
                         // Set the connection in-game
                         commandBuffer.AddComponent<NetworkStreamInGame>(entry.Value.Entity);
-                        commandBuffer.AddComponent(entry.Value.Entity, default(ConnectionState));
+                        commandBuffer.AddComponent(entry.Value.Entity, new ConnectionState { NetworkId = entry.Value.NetworkId });
                         NetworkManager.OnNetCodeConnect?.Invoke(entry.Value);
                         m_TempConnections.Add(entry.Value);
                     }

com.unity.netcode.gameobjects/Runtime/Configuration/HybridNetcodeDefaults.cs

@@ -3,7 +3,7 @@
 namespace Unity.Netcode
 {
     /// <summary>
-    /// This handles applying two <see cref="NetcodeConfig"/> settings NGO requires and an optional recommended snapshot size when running in hybrid mode.
+    /// This handles applying the <see cref="NetcodeConfig"/> settings NGO requires and an optional recommended snapshot size when running in hybrid mode.
     /// </summary>
     /// <remarks>
     /// This intentionally does not modify the <see cref="NetcodeConfig"/> asset to preserve the default settings and/or any user adjustments other than the ones required.
@@ -55,6 +55,26 @@ internal static bool ApplySnapshotPacketSize(NetcodeConfig config)
             return true;
         }
 
+        /// <summary>
+        /// Assures N4E leaves putting connections in game to NGO.
+        /// </summary>
+        /// <remarks>
+        /// <see cref="Components.UnifiedUpdateConnections"/> raises the connect events for connections not yet in game,
+        /// and a client only goes in game once NGO has synchronized it, so it receives no ghosts prior to that.
+        /// </remarks>
+        /// <param name="config">The config to correct.</param>
+        /// <returns>True if anything changed.</returns>
+        internal static bool ApplyManualStateReplication(NetcodeConfig config)
+        {
+            if (!config.AutomaticStateReplication)
+            {
+                return false;
+            }
+
+            config.AutomaticStateReplication = false;
+            return true;
+        }
+
         /// <summary>
         /// Determines if the NetcodeConfig about to be used to start a session meets the required settings.
         /// </summary>

com.unity.netcode.gameobjects/Runtime/Configuration/NetworkPrefabs.cs

@@ -429,7 +429,7 @@
             if (networkPrefab.HasGhost)
             {
                 // Registering a hybrid prefab mid-session would switch NetworkManager into hybrid mode and stop its send queue.
-                if (m_GhostPrefabRejection != null)
+                if (!string.IsNullOrEmpty(m_GhostPrefabRejection))
                 {
                     Debug.LogError($"{m_GhostPrefabRejection} {networkPrefab.GetDebugName()} was not added.");
                     return false;

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

@@ -1447,6 +1447,11 @@
                 Log.Info(new Context(LogLevel.Developer, $"The {nameof(NetcodeConfig)} tick rates have been set to {nameof(NetworkConfig)}.{nameof(NetworkConfig.TickRate)} ({NetworkConfig.TickRate}).").AddTag("Unified"));
             }
 
+            if (HybridNetcodeDefaults.ApplyManualStateReplication(NetcodeConfig.Global))
+            {
+                Log.Info(new Context(LogLevel.Developer, $"The {nameof(NetcodeConfig)}.{nameof(NetcodeConfig.AutomaticStateReplication)} has been disabled, as NGO puts connections in game once synchronized.").AddTag("Unified"));
+            }
+
             if (NetworkConfig.AutoConfigureSnapshotSize && HybridNetcodeDefaults.ApplySnapshotPacketSize(NetcodeConfig.Global))
             {
                 Log.Info(new Context(LogLevel.Developer, $"The {nameof(NetcodeConfig)} snapshot packet size has been set to {HybridNetcodeDefaults.SnapshotPacketSize}. Disable {nameof(NetworkConfig)}.{nameof(NetworkConfig.AutoConfigureSnapshotSize)} to manage it yourself.").AddTag("Unified"));

com.unity.netcode.gameobjects/Tests/Editor/HybridNetcodeDefaultsTests.cs

@@ -63,6 +63,17 @@ public void ApplySnapshotPacketSizeLeavesTheTickRatesAlone()
             Assert.AreEqual(60, m_Config.ClientServerTickRate.NetworkTickRate, "The snapshot size pass should leave NetworkTickRate alone.");
         }
 
+        [Test]
+        public void ApplyManualStateReplicationDisablesAutomaticStateReplication()
+        {
+            m_Config.AutomaticStateReplication = true;
+
+            Assert.IsTrue(HybridNetcodeDefaults.ApplyManualStateReplication(m_Config), "The first apply should report a change.");
+            Assert.IsFalse(m_Config.AutomaticStateReplication, "NGO should own putting connections in game.");
+
+            Assert.IsFalse(HybridNetcodeDefaults.ApplyManualStateReplication(m_Config), "Re-applying an unchanged config should report no change.");
+        }
+
         [Test]
         public void IsMissingRequiredDetectsBinaryWorlds()
         {

com.unity.netcode.gameobjects/Tests/Runtime/Unified/HybridConnectionEventsTests.cs

@@ -0,0 +1,97 @@
+#if UNIFIED_NETCODE
+using System.Collections;
+using System.Collections.Generic;
+using NUnit.Framework;
+using Unity.Netcode.Components;
+using Unity.Netcode.TestHelpers.Runtime;
+using UnityEngine.TestTools;
+
+namespace Unity.Netcode.RuntimeTests
+{
+    /// <summary>
+    /// Validates <see cref="NetworkManager.OnNetCodeConnect"/> and <see cref="NetworkManager.OnNetCodeDisconnect"/> fire
+    /// for each connection, and that a client only goes in game once NGO has synchronized it.
+    /// </summary>
+    [TestFixture(HostOrServer.UnifiedHost)]
+    [TestFixture(HostOrServer.UnifiedServer)]
+    internal class HybridConnectionEventsTests : NetcodeIntegrationTest
+    {
+        protected override int NumberOfClients => 1;
+
+        private readonly List<int> m_ServerConnects = new List<int>();
+        private readonly List<int> m_ServerDisconnects = new List<int>();
+        private int m_ClientConnects;
+        private bool m_ClientSynchronizedOnConnect;
+
+        public HybridConnectionEventsTests(HostOrServer hostOrServer) : base(hostOrServer) { }
+
+        protected override bool UseUnifiedTests()
+        {
+            return true;
+        }
+
+        protected override void OnServerAndClientsCreated()
+        {
+            // No world is created for a session without ghost prefabs.
+            CreateHybridPrefab("ConnectionEventsPrefab");
+            m_ServerConnects.Clear();
+            m_ServerDisconnects.Clear();
+            m_ClientConnects = 0;
+            m_ClientSynchronizedOnConnect = false;
+            NetworkManager.OnNetCodeConnect += OnNetCodeConnect;
+            NetworkManager.OnNetCodeDisconnect += OnNetCodeDisconnect;
+            base.OnServerAndClientsCreated();
+        }
+
+        protected override IEnumerator OnTearDown()
+        {
+            NetworkManager.OnNetCodeConnect -= OnNetCodeConnect;
+            NetworkManager.OnNetCodeDisconnect -= OnNetCodeDisconnect;
+            return base.OnTearDown();
+        }
+
+        private void OnNetCodeConnect(NetcodeConnection connection)
+        {
+            if (connection.IsServer)
+            {
+                m_ServerConnects.Add(connection.NetworkId);
+                return;
+            }
+            m_ClientConnects++;
+            m_ClientSynchronizedOnConnect = m_ClientNetworkManagers[0].IsConnectedClient;
+        }
+
+        private void OnNetCodeDisconnect(NetcodeConnection connection)
+        {
+            if (connection.IsServer)
+            {
+                m_ServerDisconnects.Add(connection.NetworkId);
+            }
+        }
+
+        [UnityTest]
+        public IEnumerator ConnectFiresOnceSynchronized()
+        {
+            var clientNetworkId = m_ClientNetworkManagers[0].NetcodeWorld.LocalConnection.NetworkId.Value;
+            yield return WaitForConditionOrTimeOut(() => m_ClientConnects > 0 && m_ServerConnects.Contains(clientNetworkId));
+            AssertOnTimeout($"Connect events missing! Client connects: {m_ClientConnects}, server saw the client: {m_ServerConnects.Contains(clientNetworkId)}");
+
+            Assert.AreEqual(1, m_ClientConnects, "The client connection should only be reported once.");
+            Assert.IsTrue(m_ClientSynchronizedOnConnect, "The client went in game prior to NGO synchronizing it.");
+        }
+
+        [UnityTest]
+        public IEnumerator ServerReportsClientDisconnect()
+        {
+            var clientNetworkId = m_ClientNetworkManagers[0].NetcodeWorld.LocalConnection.NetworkId.Value;
+            yield return WaitForConditionOrTimeOut(() => m_ServerConnects.Contains(clientNetworkId));
+            AssertOnTimeout("The server never reported the client connection!");
+
+            yield return StopOneClient(m_ClientNetworkManagers[0]);
+            yield return WaitForConditionOrTimeOut(() => m_ServerDisconnects.Contains(clientNetworkId));
+            AssertOnTimeout("The server never reported the client disconnect!");
+            Assert.IsTrue(m_ServerNetworkManager.IsListening, "The disconnect should be reported while the session is still running.");
+        }
+    }
+}
+#endif