fix: network clock adjustment overshoot and per-frame NetworkManager scene search (#4195)

* fix: stop the network clock adjustment on its target instead of stepping past it

Once a client's clock reached the offset it adjusts towards, each frame
stepped the full AdjustmentRatio past it and back, so LocalTime and
ServerTime alternated between running fast and slow every frame.

* fix: keep a list of NetworkManagers instead of searching the scene for them every frame

UnifiedUpdateConnections called Object.FindObjectsByType<NetworkManager>() on every update, whose cost
grows with the number of MonoBehaviours in the scene: about 1.5 ms a call with 10,000 and 14 ms with
50,000. NetworkManager.Instances is filled in Awake and emptied in OnDestroy, and the system visits that.

* docs: describe the clock adjustment fix by what users saw

* fix: bind connection updates to the hybrid world's own NetworkManager

UnifiedUpdateConnections now runs only in client and server worlds that hold
the NetworkManager they were created for, and updates that NetworkManager
alone. This replaces the NetworkManager.Instances list, which a derived
NetworkManager declaring its own Awake never entered.

* fix: create a server only world for hybrid mode in dedicated server builds

Netcode for Entities cannot create a single world host in a dedicated server
build, which every hybrid server used. A build for only one side now logs an
error when asked to start the side it does not support.

* fix: treat a destroyed NetworkManager as shutting down in connection updates

The hybrid world outlives its NetworkManager if ShutdownInternal throws before
disposing it, so UnifiedUpdateConnections now disconnects every connection and
announces none instead of throwing each frame. HybridNetworkManager.Value is
renamed to NetworkManager, the clock adjustment comment says what the clamp
does, and the unused Unity.Entities using that failed the Standards check is
removed.

* ci: TEMPORARY run the unified job on b1.xlarge to test the memory pressure theory

Revert before merge.

* fix: return no RTT for a hybrid connection that has already been removed

A client drains its incoming queue while it disconnects, after the unified transport has removed the
server connection, so a queued TimeSyncMessage made GetCurrentRtt throw KeyNotFoundException. It now
returns 0 for a connection it no longer has, as UnityTransport does.

* ci: TEMPORARY run the unified job on b1.large with 6000.7.0b4 to test whether 6000.7.0b6 raised memory use

Replaces the b1.xlarge test. Revert before merge.

* ci: compile testproject in a separate editor session before the unified test pass

The IL post-processor and compiler processes the editor starts stay resident through the test pass
and, on a b1.large agent, leave too little memory for the hybrid tests. The project is now imported
and compiled in its own batchmode session that exits first, any build process that outlives it is
stopped, and the job log lists the largest processes left before the tests start.

Also drops the temporary 6000.7.0b4 editor pin.

* ci: pin the unified warm-up to the Release code configuration

The test runner compiles in Release. An editor whose code optimization preference is Debug builds the
other configuration in the warm-up, and the test session then recompiles every assembly and restarts
the IL post-processor. CI's agents already default to Release, so this is a guard rather than a fix
there; it removed the recompile on a machine set to Debug.

Author: NoelStephensUnity

.yamato/unified-tests.yml

@@ -35,11 +35,24 @@ unified_test_{{ project.name }}_{{ platform.name }}_{{ editor }}:
 
   commands:
     - unity-downloader-cli --fast --wait -u {{ editor }} -c Editor --arch {{ platform.architecture }}
+    # Import and compile testproject in its own editor session first. The IL post-processor and compiler processes it starts
+    # otherwise stay resident for the whole test pass and, on this agent, leave too little memory for the tests.
+    - mkdir -p warmup
+    # -releaseCodeOptimization pins the Release configuration the test runner compiles. An editor whose code optimization
+    # preference is Debug would otherwise build the other configuration, and the test session would recompile everything.
+    - .Editor/Unity -batchmode -nographics -releaseCodeOptimization -logFile warmup/editor.log -projectPath {{ project.path }} -quit
+    # Stop any build processes that outlive the warm-up editor, then list what is left so the job log shows it.
+    - dotnet build-server shutdown || true
+    - pkill -f Unity.ILPP.Runner || true
+    - pkill -f VBCSCompiler || true
+    - pkill -f bee_backend || true
+    - ps -eo rss,comm --sort=-rss | head -15
     - UnifiedTestRunner --testproject={{ project.path }} --suite=playmode --artifacts-path=test-results --editor-location=.Editor --rerun-strategy=Test --retry={{ num_test_retries }} --clean-library-on-rerun --timeout={{ test_timeout }}
   artifacts:
     logs:
       paths:
         - "test-results/**/*"
+        - "warmup/**/*"
   dependencies:
     - .yamato/_run-all.yml#run_quick_checks
 {% endfor -%}

com.unity.netcode.gameobjects/CHANGELOG.md

@@ -23,6 +23,7 @@ Additional documentation and release notes are available at [Multiplayer Documen
 
 ### Fixed
 
+- 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.
 - 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.
 - Fixed issue where a prefab added to a `NetworkPrefabsList` while a session was running was registered more than once and logged a duplicate `GlobalObjectIdHash` error. (#4184)
 - Issue where moving a `NetworkObject` into another scene made the clients that did not observe it log "Trying to synchronize NetworkObjectId but it was not spawned". The scene migration is now only sent to the clients that observe the `NetworkObject`. (#4176)

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

@@ -71,17 +71,17 @@ public override bool Initialize(string defaultWorldName)
                 RegisterCreatedWorlds();
                 Debug.Log($"Starting a world for {(networkManager.IsServer ? "Host" : "Client")}");
                 s_WorldCounter++;
-                LastCreatedWorld = networkManager.IsServer ? CreateSingleWorldHost($"HostSingleWorld-{s_WorldCounter}")
-                    : CreateClientWorld($"ClientWorld-{s_WorldCounter}");
+                var world = CreateWorld(networkManager);
+                LastCreatedWorld = world;
 
-                if (LastCreatedWorld == null)
+                if (world == null)
                 {
                     s_WorldCounter--;
                     Debug.LogError($"[{nameof(UnifiedBootstrap)}] World is null!");
                     return false;
                 }
 
-                if (!LastCreatedWorld.IsCreated)
+                if (!world.IsCreated)
                 {
                     s_WorldCounter--;
                     Debug.LogError($"[{nameof(UnifiedBootstrap)}] World was not created!");
@@ -90,10 +90,10 @@ public override bool Initialize(string defaultWorldName)
 
                 if (networkManager.LogLevel <= LogLevel.Developer)
                 {
-                    NetworkLog.LogInfo($"[{nameof(UnifiedBootstrap)}] Created world: {LastCreatedWorld.Name} / {LastCreatedWorld.SequenceNumber}");
+                    NetworkLog.LogInfo($"[{nameof(UnifiedBootstrap)}] Created world: {world.Name} / {world.SequenceNumber}");
                 }
 
-                networkManager.NetcodeWorld = (NetcodeWorld)LastCreatedWorld;
+                networkManager.NetcodeWorld = (NetcodeWorld)world;
                 s_CreatedWorlds.Add(networkManager.NetcodeWorld);
             }
             else
@@ -106,10 +106,33 @@ public override bool Initialize(string defaultWorldName)
             return true;
         }
 
-        ~UnifiedBootstrap()
+        /// <summary>
+        /// Creates the world used by a <see cref="NetworkManager"/>'s hybrid session.
+        /// </summary>
+        /// <remarks>
+        /// A server or host uses a single world host. A dedicated server build uses a server-only world instead, because
+        /// Netcode for Entities cannot create a client world there. A build that only targets the client or the server
+        /// cannot start a host.
+        /// </remarks>
+        private static World CreateWorld(NetworkManager networkManager)
         {
-            LastCreatedWorld = null;
-            Instance = null;
+#if UNITY_SERVER && !UNITY_EDITOR
+            if (networkManager.IsClient)
+            {
+                Debug.LogError($"[{nameof(UnifiedBootstrap)}] A dedicated server build cannot start a {(networkManager.IsServer ? "host" : "client")} in hybrid mode.");
+                return null;
+            }
+            return CreateServerWorld($"ServerWorld-{s_WorldCounter}");
+#elif UNITY_CLIENT && !UNITY_EDITOR
+            if (networkManager.IsServer)
+            {
+                Debug.LogError($"[{nameof(UnifiedBootstrap)}] A client only build cannot start a {(networkManager.IsClient ? "host" : "server")} in hybrid mode. Set the Netcode for Entities client build target to ClientAndServer.");
+                return null;
+            }
+            return CreateClientWorld($"ClientWorld-{s_WorldCounter}");
+#else
+            return networkManager.IsServer ? CreateSingleWorldHost($"HostSingleWorld-{s_WorldCounter}") : CreateClientWorld($"ClientWorld-{s_WorldCounter}");
+#endif
         }
     }
 }

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

@@ -2,7 +2,6 @@
 using System.Collections.Generic;
 using Unity.Collections;
 using Unity.Entities;
-using UnityEngine;
 
 namespace Unity.Netcode.Components
 {
@@ -26,37 +25,57 @@ public void SendMessage<T>(T message) where T : unmanaged, IRpcCommand
         }
     }
 
+    /// <summary>
+    /// This is used to track the <see cref="NetworkManager"/>, as a singleton, when running a hybrid session.
+    /// </summary>
+    internal struct HybridNetworkManager : IComponentData
+    {
+        public UnityObjectRef<NetworkManager> NetworkManager;
+    }
+
+    // Only exists in client and server worlds, and only updates in a world created for a hybrid session.
+    [WorldSystemFilter(WorldSystemFilterFlags.ServerSimulation | WorldSystemFilterFlags.ClientSimulation)]
     internal partial class UnifiedUpdateConnections : SystemBase
     {
         private List<NetcodeConnection> m_TempConnections = new List<NetcodeConnection>();
 
         private Dictionary<int, NetcodeConnection> m_NewConnections = new Dictionary<int, NetcodeConnection>();
 
+        private EntityQuery m_HybridNetworkManagerQuery;
+
+        protected override void OnCreate()
+        {
+            m_HybridNetworkManagerQuery = GetEntityQuery(ComponentType.ReadOnly<HybridNetworkManager>());
+            RequireForUpdate(m_HybridNetworkManagerQuery);
+        }
+
         protected override void OnUpdate()
         {
-            var isServer = World.IsServer();
             var commandBuffer = new EntityCommandBuffer(Allocator.Temp);
-            foreach (var networkManager in Object.FindObjectsByType<NetworkManager>())
+            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())
             {
-                foreach (var (networkId, connectionState, entity) in SystemAPI.Query<NetworkId, ConnectionState>().WithNone<NetworkStreamConnection>().WithEntityAccess())
+                commandBuffer.RemoveComponent<ConnectionState>(entity);
+                m_TempConnections.Add(new NetcodeConnection
                 {
-                    commandBuffer.RemoveComponent<ConnectionState>(entity);
-                    m_TempConnections.Add(new NetcodeConnection
-                    {
-                        World = World,
-                        Entity = entity,
-                        NetworkId = networkId.Value
-                    });
-                }
+                    World = World,
+                    Entity = entity,
+                    NetworkId = networkId.Value
+                });
+            }
 
-                foreach (var con in m_TempConnections)
-                {
-                    NetworkManager.OnNetCodeDisconnect?.Invoke(con);
-                }
+            foreach (var con in m_TempConnections)
+            {
+                NetworkManager.OnNetCodeDisconnect?.Invoke(con);
+            }
 
-                m_TempConnections.Clear();
+            m_TempConnections.Clear();
 
-                // TODO: We should figure out how to associate the N4E NetworkId with the NGO ClientId
+            // TODO: We should figure out how to associate the N4E NetworkId with the NGO ClientId
+            if (!isShuttingDown)
+            {
                 foreach (var (networkId, entity) in SystemAPI.Query<NetworkId>().WithAll<NetworkStreamConnection>()
                              .WithNone<NetworkStreamInGame>().WithEntityAccess())
                 {
@@ -67,43 +86,44 @@ protected override void OnUpdate()
                         m_NewConnections.Add(networkId.Value, newConnection);
                     }
                 }
+            }
 
-                // If we have any pending connections
-                if (m_NewConnections.Count > 0)
+            // If we have any pending connections
+            if (!isShuttingDown && m_NewConnections.Count > 0)
+            {
+                foreach (var entry in m_NewConnections)
                 {
-                    foreach (var entry in m_NewConnections)
+                    // Server: always connect
+                    // Client: wait until we have synchronized before announcing we are ready to receive snapshots
+                    if (networkManager.IsServer || (!networkManager.IsServer && networkManager.IsConnectedClient))
                     {
-                        // Server: always connect
-                        // Client: wait until we have synchronized before announcing we are ready to receive snapshots
-                        if (networkManager.IsServer || (!networkManager.IsServer && networkManager.IsConnectedClient))
-                        {
-                            // Set the connection in-game
-                            commandBuffer.AddComponent<NetworkStreamInGame>(entry.Value.Entity);
-                            commandBuffer.AddComponent(entry.Value.Entity, default(ConnectionState));
-                            NetworkManager.OnNetCodeConnect?.Invoke(entry.Value);
-                            m_TempConnections.Add(entry.Value);
-                        }
+                        // Set the connection in-game
+                        commandBuffer.AddComponent<NetworkStreamInGame>(entry.Value.Entity);
+                        commandBuffer.AddComponent(entry.Value.Entity, default(ConnectionState));
+                        NetworkManager.OnNetCodeConnect?.Invoke(entry.Value);
+                        m_TempConnections.Add(entry.Value);
                     }
+                }
 
-                    // Remove any connections that have "gone in-game".
-                    foreach (var connection in m_TempConnections)
-                    {
-                        m_NewConnections.Remove(connection.NetworkId);
-                    }
+                // Remove any connections that have "gone in-game".
+                foreach (var connection in m_TempConnections)
+                {
+                    m_NewConnections.Remove(connection.NetworkId);
                 }
+            }
 
-                m_TempConnections.Clear();
+            m_TempConnections.Clear();
 
-                // If the local NetworkManager is shutting down or no longer connected, then
-                // make sure we have disconnected all known connections.
-                if (networkManager.ShutdownInProgress || !networkManager.IsListening)
+            // If the local NetworkManager is shutting down or no longer connected, then
+            // make sure we have disconnected all known connections.
+            if (isShuttingDown)
+            {
+                // Only in-game connections have ConnectionState, and removing it here means each one is only reported once.
+                foreach (var (networkId, entity) in SystemAPI.Query<NetworkId>().WithAll<ConnectionState>().WithEntityAccess())
                 {
-                    foreach (var (networkId, entity) in SystemAPI.Query<NetworkId>().WithEntityAccess())
-                    {
-                        commandBuffer.RemoveComponent<ConnectionState>(entity);
-                        NetworkManager.OnNetCodeDisconnect?.Invoke(new NetcodeConnection
-                        { World = World, Entity = entity, NetworkId = networkId.Value });
-                    }
+                    commandBuffer.RemoveComponent<ConnectionState>(entity);
+                    NetworkManager.OnNetCodeDisconnect?.Invoke(new NetcodeConnection
+                    { World = World, Entity = entity, NetworkId = networkId.Value });
                 }
             }
             commandBuffer.Playback(EntityManager);

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

@@ -1395,6 +1395,12 @@ internal void InitializeNetcodeWorld()
             TypeManager.Initialize();
             UnifiedBootstrap.CurrentNetworkManagerForInitialization = this;
             new UnifiedBootstrap().Initialize("Default World");
+
+            if (NetcodeWorld != null && NetcodeWorld.IsCreated)
+            {
+                var entityManager = NetcodeWorld.EntityManager;
+                entityManager.AddComponentData(entityManager.CreateEntity(), new HybridNetworkManager { NetworkManager = this });
+            }
         }
 
         /// <summary>

com.unity.netcode.gameobjects/Runtime/Timing/NetworkTimeSystem.cs

@@ -260,8 +260,13 @@ public bool Advance(double deltaTimeSec)
                 return true;
             }
 
-            m_CurrentLocalTimeOffset += deltaTimeSec * (m_DesiredLocalTimeOffset > m_CurrentLocalTimeOffset ? AdjustmentRatio : -AdjustmentRatio);
-            m_CurrentServerTimeOffset += deltaTimeSec * (m_DesiredServerTimeOffset > m_CurrentServerTimeOffset ? AdjustmentRatio : -AdjustmentRatio);
+            // Move each offset toward its target by at most AdjustmentRatio of this frame's delta, stopping once it gets there.
+            // Stepping the full amount every frame overshot the target, so a synced clock alternated between running fast and
+            // slow, and anything driven by it juddered.
+            // Math.Clamp throws on a negative step, so a negative delta or AdjustmentRatio applies no adjustment.
+            var maximumStep = Math.Max(0.0, deltaTimeSec * AdjustmentRatio);
+            m_CurrentLocalTimeOffset += Math.Clamp(m_DesiredLocalTimeOffset - m_CurrentLocalTimeOffset, -maximumStep, maximumStep);
+            m_CurrentServerTimeOffset += Math.Clamp(m_DesiredServerTimeOffset - m_CurrentServerTimeOffset, -maximumStep, maximumStep);
 
             return false;
         }

com.unity.netcode.gameobjects/Runtime/Transports/Unified/UnifiedNetcodeTransport.cs

@@ -476,7 +476,8 @@ public override void DisconnectLocalClient()
         public override ulong GetCurrentRtt(ulong clientId)
         {
             var (transportId, _) = m_NetworkManager.ConnectionManager.ClientIdToTransportId(clientId);
-            return (ulong)m_Connections[(int)transportId].Connection.RTT;
+            // Returns 0 when the connection was already removed (for example while shutting down), the same as UnityTransport.
+            return m_Connections.TryGetValue((int)transportId, out ConnectionInfo connectionInfo) ? (ulong)connectionInfo.Connection.RTT : 0;
         }
 
         public override void Initialize(NetworkManager networkManager = null)

com.unity.netcode.gameobjects/Tests/Editor/Timing/ClientNetworkTimeSystemTests.cs

@@ -138,6 +138,36 @@ public void RttCatchupSlowdownTest()
             Assert.True(Math.Abs(totalServerSpeedUpTime) < k_AcceptableRttOffset, $"Expected server speed up time to be less than {k_AcceptableRttOffset}ms but it was {expectedServerSpeedUpTime}!");
         }
 
+        /// <summary>
+        /// Validates that once the clock has caught up with a time sync, its local and server time both advance by
+        /// exactly the frame's delta time on every frame, instead of alternating between slightly fast and slightly slow.
+        /// </summary>
+        [Test]
+        public void ConvergedClockAdvancesAtRealRate([Values(0.01d, 0.1d)] double adjustmentRatio)
+        {
+            const double deltaTime = 1.0d / 60.0d;
+            double receivedServerTime = 2;
+            var timeSystem = new NetworkTimeSystem(0.05d, 0.05d, 0.1d, adjustmentRatio);
+            timeSystem.Reset(receivedServerTime, 0.1d);
+            // A sync 10 ms ahead, below the hard reset threshold, makes the clock catch up gradually instead of resetting.
+            receivedServerTime += 0.01d;
+            timeSystem.Sync(receivedServerTime, 0.1d);
+
+            for (int i = 0; i < 600; i++)
+            {
+                timeSystem.Advance(deltaTime);
+            }
+
+            for (int i = 0; i < 10; i++)
+            {
+                var localTime = timeSystem.LocalTime;
+                var serverTime = timeSystem.ServerTime;
+                timeSystem.Advance(deltaTime);
+                Assert.AreEqual(deltaTime, timeSystem.LocalTime - localTime, 1e-9d, $"Local time advanced at the wrong rate on frame {i}.");
+                Assert.AreEqual(deltaTime, timeSystem.ServerTime - serverTime, 1e-9d, $"Server time advanced at the wrong rate on frame {i}.");
+            }
+        }
+
         /// <summary>
         /// Tests whether time resets when there is a huge spike in RTT and is able to stabilize again.
         /// </summary>

com.unity.netcode.gameobjects/Tests/Runtime/NetworkTransform/UnifiedNetworkTransformTest.cs

@@ -100,6 +100,24 @@ public IEnumerator BasicMovementTest()
             AssertOnTimeout($"A client failed to synchronize the changed position: {GetVector3Values(newPos)}!");
             VerboseDebug("Test Passed!");
         }
+
+        /// <summary>
+        /// Validates that each <see cref="NetworkManager"/>'s world only updates that <see cref="NetworkManager"/>'s connections.
+        /// </summary>
+        [Test]
+        public void EachWorldUpdatesConnectionsForItsOwnNetworkManager()
+        {
+            foreach (var networkManager in m_NetworkManagers)
+            {
+                var world = networkManager.NetcodeWorld;
+                Assert.IsNotNull(world, $"[Client-{networkManager.LocalClientId}] Has no world!");
+                Assert.IsNotNull(world.GetExistingSystemManaged<UnifiedUpdateConnections>(), $"[{world.Name}] Has no {nameof(UnifiedUpdateConnections)} system!");
+                using var query = world.EntityManager.CreateEntityQuery(typeof(HybridNetworkManager));
+                Assert.AreEqual(1, query.CalculateEntityCount(), $"[{world.Name}] Does not have exactly one {nameof(HybridNetworkManager)}!");
+                var bound = query.GetSingleton<HybridNetworkManager>().NetworkManager.Value;
+                Assert.AreSame(networkManager, bound, $"[{world.Name}] Is bound to another {nameof(NetworkManager)}!");
+            }
+        }
     }
 }
 #endif