fix: networktransform interpolation render time up port - #4135
Open
NoelStephensUnity wants to merge 9 commits into
Open
fix: networktransform interpolation render time up port#4135NoelStephensUnity wants to merge 9 commits into
NoelStephensUnity wants to merge 9 commits into
Conversation
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.
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.
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.
6 tasks
NoelStephensUnity
marked this pull request as ready for review
August 19, 2026 16:47
Codecov ReportAll modified and coverable lines are covered by tests ✅ @@ Coverage Diff @@
## develop-3.x.x #4135 +/- ##
=================================================
+ Coverage 78.00% 78.05% +0.05%
=================================================
Files 153 153
Lines 26260 26261 +1
=================================================
+ Hits 20483 20498 +15
+ Misses 5777 5763 -14
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 4 files with indirect coverage changes
|
…render-time-up-port-2
![u-pr[bot]](https://avatars.githubusercontent.com/in/1201017?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Harness Review
I examined the runtime time synchronization and state-stamping contracts, interpolation paths, helper behavior, integration-test lifecycle, and release-note update; the server-time change is consistent with the timeline used by received transform states.
Reviewed commit d9d6658
🤖 Helpful? 👍/👎
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.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose of this PR
This is an up-port of #4133
Jira ticket
TODO: add ticket
Changelog
NetworkTransforminterpolated 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.NetworkTransform.GetTickLatencyInSecondsreturned 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.Documentation
Testing & QA (How your changes can be verified during release Playtest)
New integration test measures how far behind
ServerTimethe state being interpolated towards was sent. That value can never be less than the tick latency, since the render time isServerTimeminus the tick latency and only states sent at or before it are eligible.Validated in both directions on
develop-2.0.0: without the fix both fixtures fail, reporting the target as −0.899 and −1.059 ticks — the interpolator chasing a state the server clock says has not happened yet. With the fix both pass. The fullNetworkTransformplaymode suite is green (3942/3942).The test widens the client's local time buffer before measuring, because an in-process test has no round trip time to separate the two clocks and would otherwise pass regardless of which one is used. It waits for that separation to take hold and fails if it never does, so it cannot silently become a no-op.
For release playtest, the thing to look at is client-side smoothness of moving networked objects, and whether the added latency is acceptable.
Functional Testing
Manual testing :
Manual testing doneAutomated tests:
Covered by existing automated testsCovered by new automated testsDoes the change require QA team to:
Review automated tests?Execute manual tests?Provide feedback about the PR?Up-port
Not needed. This is an up-port.
Backports
Not needed.