Repository navigation
Re-arm the replicate start delay when inputs resume - #1094
Open
jimdroberts wants to merge 1 commit into
Open
jimdroberts wants to merge 1 commit into
jimdroberts wants to merge 1 commit into
Conversation
The only assignment of _replicateCurrentStartTick was guarded by _replicateCurrentStartTick != UNSET_TICK, but the field starts unset and is reset to unset once reached, so the StateInterpolation start delay never armed after 4.6.19. Remove the guard, restoring the 4.6.18 behaviour of delaying inputs that resume into an empty queue.
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
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.
Problem
PredictionManager.StateInterpolationis documented as "How many states to try and hold in a buffer before running them". For the server running a client's inputs (and for clients using Appended state order), that buffer is built by a start delay. When inputs start arriving into an empty queue, they are held forStateInterpolationticks before the first one runs. Since 4.6.19R that delay never starts. The server runs each input on the tick it arrives, so the queue holds no buffer when an owner starts sending after being idle. A packet that arrives a tick late then finds the queue empty, and the server runs default data for that tick.Cause
NetworkBehaviour.Prediction.cson main:_replicateCurrentStartTickis initialised toTimeManager.UNSET_TICK(0).Replicate_NonAuthoritative): it is set back toUNSET_TICKwheneverlocalTick >= _replicateCurrentStartTick.Replicate_EnqueueReceivedReplicate) is the only place it is armed:As a result,
localTick >= _replicateCurrentStartTickat L641 is alwayslocalTick >= 0. The start-delay adjustment inReplicate_SendNonAuthoritative(L1074-1075) is also never applied.The guard arrived in 4.6.19R (e005324), which renamed
_replicateStartTickto_replicateCurrentStartTick. Up to and including 4.6.18 the line wasif ((isServer || isAppendedOrder) && startQueueCount == 0 && replicatesQueue.Count > 0). The 4.6.19R changelog does not mention removing the delay. The field, its three uses, the comment above L1177 ("start the queued inputs delay") and theStateInterpolationtooltip all still describe the delay, so the guard looks unintentional.Fix
Remove the
_replicateCurrentStartTick != TimeManager.UNSET_TICK &&condition. This restores the ≤4.6.18 behaviour: whenever the queue goes from empty to non-empty, the start tick is set toLocalTick + StateInterpolation.I chose removal over flipping the guard to
== UNSET_TICKbecause the field is not reset inResetState_Prediction/ClearReplicateCache. If a replicate cache is cleared while a delay is pending, a stale start tick survives. The unconditional re-arm overwrites it on the next input. With==the stale value would stay, and L1075 could then computestart - LocalTickon a start tick that is already in the past, which wraps theuint.If disabling the delay was intentional, I'm happy to change this PR so it removes the field, its uses and the stale comments instead.
Evidence
A throwaway model of the two methods (
Replicate_EnqueueReceivedReplicatearming, plusReplicate_NonAuthoritativeconsuming one entry per tick withDropExcessiveReplicateson) was also run. The owner sends one input per tick in bursts of 30-120 ticks with 10-60 idle ticks between them, latency of 5 ticks + 0-3 ticks jitter, 2% loss, redundancy = StateInterpolation + 1 = 3, and StateInterpolation = 2. Totals over 5 seeds × 200k ticks:On main the delay is never armed, and the server hits an empty queue mid-stream about 10× as often. The fix trades that for the documented delay:
StateInterpolationticks each time a stream starts. This is a model of the queue logic, not a FishNet runtime measurement.Behavior change
The documented one. When an owner's inputs resume into an empty queue, the server (and Appended-order clients for spectated objects) waits
StateInterpolationticks before running them, as in 4.6.18 and earlier. The "run tick of last entry" sent to spectators includes that delay again.