fix: NetworkTransform interpolation render time (#4133)

* test: Add NetworkTransform interpolation render time regression test

Adds an integration test that measures how far behind the server clock the
state a non-authority NetworkTransform is interpolating towards was sent.

Only states sent at or before the render time are eligible to be interpolated
towards, and the render time is the server clock minus the tick latency, so
that measurement can never be less than the tick latency. It currently is,
and goes negative, meaning the interpolator is chasing a state that the server
clock says has not happened yet.

An in-process integration test has effectively no round trip time, so the test
first widens the client's local time buffer to separate LocalTime and
ServerTime by a known amount and waits for that separation to take hold.
Without it the two clocks sit close enough together that the test would pass
regardless of which one the render time is derived from.

This commit contains the test only, so it can be run against an unfixed tree.

* fix: Derive NetworkTransform interpolation time from the server clock

A NetworkTransform state's SentTime comes from its NetworkTick, which is a
server tick, but the render time the interpolators were given was derived from
LocalTime. That mixes two clocks. LocalTime leads ServerTime, so subtracting
the tick latency from it lands the render time back at approximately ServerTime
rather than a whole tick latency behind it, and a state's SentTime is floored to
a tick boundary on top of that. The render time therefore sat at or ahead of the
newest state that could exist and the interpolator had nothing to interpolate
towards.

Measuring from ServerTime makes the offset the whole tick latency instead of
whatever is left of it, and is self correcting: as the round trip time grows the
tick latency grows and the render time moves further back with it. This also
matches the rest of the component, which already resets the interpolators using
ServerTime.

This is a no-op on a host or server, where the two clocks are the same, so it
only affects clients.

GetTickLatencyInSeconds returns an absolute time rather than a duration and had
the same defect, so it now derives from ServerTime as well. GetTickLatency is
left alone because it returns a tick count rather than a point in time.

* docs: Condense interpolation render time comments and changelog

Comment and changelog wording only, no behavioral or test logic changes.

Trims the explanation in UpdateInterpolation from twenty one lines to six and
drops the measurement anecdote and the unfilled Jira placeholder, keeping the
reason the server clock is the correct one to measure from. Shortens the test's
remarks and constant comments to match the density of the surrounding tests.

The removed detail, the measurements behind the fix, and the metrics that were
tried and rejected while building the test are recorded outside the repository.

* update

Adding PR number to changelog entries.

* fix: Return a duration from NetworkTransform.GetTickLatencyInSeconds

GetTickLatencyInSeconds returned TimeTicksAgo(...).Time, which is an absolute
network timestamp rather than a duration, so the value grew for as long as the
session ran. It is documented as returning the tick latency in seconds, and
NetworkTimeSystem.TickLatency points at it as a way to inspect that latency, so
the contract was misleading regardless of which clock it was measured from. It
now returns the tick count multiplied by the tick interval.

This also takes the clock question out of this method entirely, since a duration
does not reference LocalTime or ServerTime. The change to derive interpolation
render time from ServerTime now applies only to UpdateInterpolation.

Adds integration tests covering the documented contract: the value tracks the
tick latency rather than elapsed time, and lengthens by exactly the tick
interval for each tick of additional buffering. Both fail against the previous
implementation, the second regardless of how long the session has run, since
buffering more ticks used to make the reported latency smaller.

* style

Removing using directive for UnityEngine as it is an unused namespace.

* test: Tolerate an adaptive tick latency in the tick latency tests

NetworkTimeSystem.TickLatency is recomputed from the averaged round trip time
and can legitimately change mid-run. Both tests assumed it would not, and one
failed on macOS when it moved from two ticks to three, reporting the value as
having gone from 0.0666s to 0.1s.

The duration is now only held to being unchanged across samples where the tick
latency itself did not change, and the buffer offset test accounts for any tick
latency movement between its two samples so that only the buffering is held to
an exact figure.

Both still fail against the previous absolute timestamp implementation.

* test: Address review feedback on the render time and tick latency tests

- Drop the redundant HostOrServer fixture argument and the UseCMBService
  override; Host is the default and a client-server fixture never runs
  under the CMB service.
- Use WaitForSpawnedOnAllOrTimeOut, GetNonAuthorityNetworkManager and
  WaitForTicks instead of hand rolled equivalents.
- There is only ever one connected client, so drop the collections and
  refer to the single non-authority instance directly.
- Fold the two tick latency tests into one and drop the assertion that
  recomputed the implementation's own formula. What is left is what can
  actually regress: the value does not drift with session time, and it
  grows by exactly the ticks added to the interpolation buffer.

Author: NoelStephensUnity

com.unity.netcode.gameobjects/CHANGELOG.md

@@ -22,6 +22,8 @@ Additional documentation and release notes are available at [Multiplayer Documen
 
 ### Fixed
 
+- Issue where `NetworkTransform` interpolated towards a point in time taken from the local clock rather than the server clock that state updates are stamped on, which starved the interpolator on clients and reduced interpolation to snapping between state updates. (#4133)
+- Issue where `NetworkTransform.GetTickLatencyInSeconds` returned an absolute network timestamp that grew for as long as the session ran, rather than the tick latency as a duration in seconds that it is documented to return. (#4133)
 - Issue where lerp smoothing was applied per frame instead of over time, which caused the `Lerp` and `SmoothDampening` interpolation types to smooth by different amounts at different frame rates. Results at 60fps are unchanged. (#4130)
 - Issue where setting a maximum interpolation time of 1.0 would stop a `NetworkTransform` from interpolating at all when using the `Lerp` or `SmoothDampening` interpolation types. (#4130)
 

com.unity.netcode.gameobjects/Runtime/Components/NetworkTransform.cs

@@ -4242,14 +4242,13 @@ internal BufferedLinearInterpolatorQuaternion GetRotationInterpolator()
         // Non-Authority
         private void UpdateInterpolation()
         {
-            // Use the local time because:
-            // Client-Server:
-            // Local time is server time on a host or server.
-            // Local time on clients takes latency into consideration.
-            // Distributed authority:
-            // Local time is used by the authority.
-            // Local time on non-authority takes latency into consid]eration.
-            var timeSystem = m_CachedNetworkManager.LocalTime;
+            // Use the server time, since that is the clock the states being interpolated between are stamped on
+            // (a state's SentTime is derived from its NetworkTick). Deriving the render time from LocalTime
+            // subtracts the tick latency from a clock that already leads ServerTime by roughly that much, which
+            // leaves the render time at or ahead of the newest state that can exist and starves the interpolator.
+            // Measuring from ServerTime is also self correcting, as the tick latency grows with the round trip
+            // time. This is a no-op on a host or server, where both clocks are the same.
+            var timeSystem = m_CachedNetworkManager.ServerTime;
             var currentTime = timeSystem.Time;
 #if COM_UNITY_MODULES_PHYSICS || COM_UNITY_MODULES_PHYSICS2D
             var cachedDeltaTime = m_UseRigidbodyForMotion ? m_CachedNetworkManager.RealTimeProvider.FixedDeltaTime : m_CachedNetworkManager.RealTimeProvider.DeltaTime;
@@ -4713,7 +4712,10 @@ internal static float GetTickLatencyInSeconds(NetworkManager networkManager)
         {
             if (networkManager.IsListening)
             {
-                return (float)networkManager.LocalTime.TimeTicksAgo(networkManager.NetworkTimeSystem.TickLatency + InterpolationBufferTickOffset).Time;
+                // The number of ticks the interpolators run behind, as a duration. This is not a point in time:
+                // it does not grow as the session runs.
+                var ticksBehind = networkManager.NetworkTimeSystem.TickLatency + InterpolationBufferTickOffset;
+                return (float)(ticksBehind * networkManager.ServerTime.FixedDeltaTimeAsDouble);
             }
             return 0f;
         }

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

@@ -0,0 +1,147 @@
+using System.Collections;
+using NUnit.Framework;
+using Unity.Netcode.Components;
+using Unity.Netcode.TestHelpers.Runtime;
+using UnityEngine;
+using UnityEngine.TestTools;
+
+namespace Unity.Netcode.RuntimeTests
+{
+    /// <summary>
+    /// Validates that the render time a non-authority instance interpolates towards is derived from the same
+    /// clock that the state updates it is interpolating between are stamped on.
+    /// </summary>
+    /// <remarks>
+    /// Measures how far behind ServerTime the state being interpolated towards was sent. The render time is
+    /// ServerTime minus the tick latency and only states sent at or before it are eligible, so that measurement
+    /// can never be less than the tick latency. Deriving the render time from LocalTime eats into that margin by
+    /// however far the two clocks are apart, and can push the target past ServerTime entirely.
+    /// </remarks>
+    [TestFixture(NetworkTransform.InterpolationTypes.Lerp)]
+    [TestFixture(NetworkTransform.InterpolationTypes.SmoothDampening)]
+    internal class NetworkTransformInterpolationRenderTimeTests : IntegrationTestWithApproximation
+    {
+        protected override int NumberOfClients => 1;
+
+        // How far LocalTime is pushed ahead of ServerTime, in ticks. An in-process test has no round trip time
+        // to separate the two clocks, and this is large enough to exceed NetworkTimeSystem's hard reset
+        // threshold so the offset snaps instead of converging at its default adjustment ratio.
+        private const int k_LocalBufferTicks = 12;
+
+        // The separation the clocks must actually reach before any measurement is taken.
+        private const double k_RequiredLeadTicks = 8.0d;
+
+        // Ticks of authority motion after the clocks have separated, so the interpolator reaches steady state.
+        private const int k_WarmUpTicks = 20;
+
+        private const int k_SampledFrames = 90;
+
+        // Far enough each tick that every tick produces a state update rather than being filtered out by the
+        // position threshold.
+        private const float k_DistancePerTick = 1.37f;
+
+        private readonly NetworkTransform.InterpolationTypes m_InterpolationType;
+
+        private GameObject m_TestPrefab;
+        private NetworkManager m_AuthorityNetworkManager;
+        private NetworkTransform m_AuthorityInstance;
+        private Vector3 m_Direction;
+
+        public NetworkTransformInterpolationRenderTimeTests(NetworkTransform.InterpolationTypes interpolationType)
+        {
+            m_InterpolationType = interpolationType;
+        }
+
+        protected override void OnServerAndClientsCreated()
+        {
+            m_TestPrefab = CreateNetworkObjectPrefab("RenderTimeTestObj");
+            var networkTransform = m_TestPrefab.AddComponent<NetworkTransform>();
+            networkTransform.PositionInterpolationType = m_InterpolationType;
+            base.OnServerAndClientsCreated();
+        }
+
+        private static double GetTickInterval(NetworkManager networkManager)
+        {
+            return 1.0d / networkManager.NetworkTickSystem.TickRate;
+        }
+
+        /// <summary>
+        /// How far LocalTime currently leads ServerTime, expressed in ticks.
+        /// </summary>
+        private static double GetClockLeadInTicks(NetworkManager networkManager)
+        {
+            return (networkManager.LocalTime.Time - networkManager.ServerTime.Time) / GetTickInterval(networkManager);
+        }
+
+        /// <summary>
+        /// Moves the authority instance once per tick so that a state update is generated every tick.
+        /// </summary>
+        private void OnNetworkTick()
+        {
+            m_AuthorityInstance.transform.position += m_Direction * k_DistancePerTick;
+        }
+
+        [UnityTest]
+        public IEnumerator RenderTimeTrailsTheServerClock()
+        {
+            m_AuthorityNetworkManager = GetAuthorityNetworkManager();
+            m_AuthorityInstance = SpawnObject(m_TestPrefab, m_AuthorityNetworkManager).GetComponent<NetworkTransform>();
+
+            yield return WaitForSpawnedOnAllOrTimeOut(m_AuthorityInstance.NetworkObject);
+            AssertOnTimeout($"Not all clients spawned {m_AuthorityInstance.name}!");
+
+            var nonAuthority = GetNonAuthorityNetworkManager();
+            var nonAuthorityInstance = nonAuthority.SpawnManager.SpawnedObjects[m_AuthorityInstance.NetworkObject.NetworkObjectId].GetComponent<NetworkTransform>();
+
+            // Separate the two clocks by a known amount so that which one the render time is derived from is
+            // actually distinguishable.
+            nonAuthority.NetworkTimeSystem.LocalBufferSec = k_LocalBufferTicks * GetTickInterval(nonAuthority);
+
+            // Start continuous motion on the authority.
+            m_Direction = GetRandomVector3(-10, 10).normalized;
+            m_AuthorityNetworkManager.NetworkTickSystem.Tick += OnNetworkTick;
+
+            // The offset only moves when the client next receives a time sync, so wait for the separation to
+            // actually take hold rather than assuming it has.
+            yield return WaitForConditionOrTimeOut(() => GetClockLeadInTicks(nonAuthority) >= k_RequiredLeadTicks);
+            AssertOnTimeout($"The nonAuthority clock never fell {k_RequiredLeadTicks} ticks behind, so this test " +
+                "cannot tell the two clocks apart and would pass regardless of which one is used.");
+
+            // Let the interpolator settle at the new separation before measuring.
+            yield return WaitForTicks(m_AuthorityNetworkManager, k_WarmUpTicks);
+
+            // Sample how far behind ServerTime the state being interpolated towards was sent.
+            var interpolator = nonAuthorityInstance.GetPositionInterpolator();
+            var totalTargetLagTicks = 0.0d;
+            var totalBuffered = 0;
+            var samples = 0;
+            for (int frame = 0; frame < k_SampledFrames; frame++)
+            {
+                if (interpolator.InterpolateState.Target.HasValue)
+                {
+                    var targetLag = nonAuthority.ServerTime.Time - interpolator.InterpolateState.Target.Value.TimeSent;
+                    totalTargetLagTicks += targetLag / GetTickInterval(nonAuthority);
+                    totalBuffered += interpolator.m_BufferQueue.Count;
+                    samples++;
+                }
+                yield return null;
+            }
+
+            m_AuthorityNetworkManager.NetworkTickSystem.Tick -= OnNetworkTick;
+
+            Assert.Greater(samples, 0, $"{nonAuthorityInstance.name} never had a state to interpolate towards!");
+
+            var meanTargetLagTicks = totalTargetLagTicks / samples;
+            var meanBuffered = totalBuffered / (float)samples;
+            var tickLatency = nonAuthority.NetworkTimeSystem.TickLatency;
+
+            // Anything less than the tick latency means the render time came from a clock that leads the one
+            // the states are stamped on.
+            Assert.GreaterOrEqual(meanTargetLagTicks, tickLatency,
+                $"[{m_InterpolationType}] {nonAuthorityInstance.name} was interpolating towards a state sent " +
+                $"{meanTargetLagTicks:F3} ticks behind the server clock, but the render time is the server " +
+                $"clock minus a tick latency of {tickLatency}, so it should never be less than that. " +
+                $"(clock lead {GetClockLeadInTicks(nonAuthority):F3} ticks, mean buffered {meanBuffered:F3})");
+        }
+    }
+}

com.unity.netcode.gameobjects/Tests/Runtime/NetworkTransform/NetworkTransformInterpolationRenderTimeTests.cs.meta

@@ -0,0 +1,93 @@
+using System.Collections;
+using NUnit.Framework;
+using Unity.Netcode.Components;
+using Unity.Netcode.TestHelpers.Runtime;
+using UnityEngine.TestTools;
+
+namespace Unity.Netcode.RuntimeTests
+{
+    /// <summary>
+    /// Validates that <see cref="NetworkTransform.GetTickLatencyInSeconds()"/> returns what it is documented to
+    /// return: the tick latency as a duration in seconds.
+    /// </summary>
+    /// <remarks>
+    /// It previously returned <c>TimeTicksAgo(...).Time</c>, which is an absolute network timestamp rather than a
+    /// duration, so the value grew for as long as the session ran.
+    /// </remarks>
+    internal class NetworkTransformTickLatencyTests : NetcodeIntegrationTest
+    {
+        protected override int NumberOfClients => 1;
+
+        // Ticks of additional buffering applied part way through the test to confirm the returned duration
+        // tracks the tick latency it is derived from.
+        private const int k_AddedBufferTicks = 3;
+
+        // Seconds of tolerance when comparing against the expected duration.
+        private const float k_Tolerance = 0.0005f;
+
+        // The number of samples taken while the session runs, to confirm the value does not drift with time.
+        private const int k_Samples = 30;
+
+        private int m_OriginalBufferTickOffset;
+
+        protected override IEnumerator OnSetup()
+        {
+            m_OriginalBufferTickOffset = NetworkTransform.InterpolationBufferTickOffset;
+            return base.OnSetup();
+        }
+
+        protected override IEnumerator OnTearDown()
+        {
+            // This is static, so leaving it modified would leak into every test that runs afterwards.
+            NetworkTransform.InterpolationBufferTickOffset = m_OriginalBufferTickOffset;
+            return base.OnTearDown();
+        }
+
+        [UnityTest]
+        public IEnumerator GetTickLatencyInSecondsReturnsADurationNotATimestamp()
+        {
+            var client = GetNonAuthorityNetworkManager();
+            var tickInterval = (float)client.ServerTime.FixedDeltaTimeAsDouble;
+
+            // A timestamp climbs by roughly a second per second, so sample while the session clock advances
+            // and hold the value to only moving when the tick latency it is derived from moves. That latency
+            // is adaptive and can legitimately change mid-run.
+            var previousTicksBehind = -1;
+            var previousValue = 0f;
+            for (int i = 0; i < k_Samples; i++)
+            {
+                var ticksBehind = client.NetworkTimeSystem.TickLatency + NetworkTransform.InterpolationBufferTickOffset;
+                var latency = NetworkTransform.GetTickLatencyInSeconds(client);
+
+                Assert.Greater(latency, 0f, "A latency of zero or less is not a duration this can be measured against.");
+                if (ticksBehind == previousTicksBehind)
+                {
+                    Assert.AreEqual(previousValue, latency, k_Tolerance,
+                        $"The reported latency moved from {previousValue}s to {latency}s while the tick latency " +
+                        $"stayed at {ticksBehind} ticks, so it is tracking elapsed time rather than latency.");
+                }
+
+                previousTicksBehind = ticksBehind;
+                previousValue = latency;
+                yield return null;
+            }
+
+            // Buffering more ticks has to lengthen the reported duration by exactly those ticks.
+            var latencyBefore = client.NetworkTimeSystem.TickLatency;
+            var before = NetworkTransform.GetTickLatencyInSeconds(client);
+            NetworkTransform.InterpolationBufferTickOffset = m_OriginalBufferTickOffset + k_AddedBufferTicks;
+            yield return null;
+
+            var latencyAfter = client.NetworkTimeSystem.TickLatency;
+            var after = NetworkTransform.GetTickLatencyInSeconds(client);
+
+            // The adaptive tick latency may also have moved in between, so only the buffering is held to an
+            // exact figure.
+            var expectedIncrease = (k_AddedBufferTicks + (latencyAfter - latencyBefore)) * tickInterval;
+            Assert.AreEqual(expectedIncrease, after - before, k_Tolerance,
+                $"Adding {k_AddedBufferTicks} ticks of buffering changed the reported latency by " +
+                $"{after - before}s when a tick is {tickInterval}s, so it should have changed by " +
+                $"{expectedIncrease}s (tick latency went from {latencyBefore} to {latencyAfter}).");
+        }
+    }
+}