fix: expire stale point-at state from replayed remote snapshots - #9780
fix: expire stale point-at state from replayed remote snapshots#9780alejandro-jimenez-dcl wants to merge 1 commit into
Conversation
Server-cached movement snapshots delivered on join/teleport are applied by HandleFirstMessage with no freshness bound, and the remote path never expires IsPointing — one replayed pointing=true snapshot pins a remote avatar's arm at a stale absolute WorldHitPoint indefinitely. Bound the remote point-at lifetime the same way HandPointAtSystem bounds the local gesture. Includes a regression test that fails without this fix. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
Step 2 — Root-cause check: PASS
Problem: Server-cached movement snapshots delivered on join/teleport are applied by HandleFirstMessage with no freshness bound. The remote point-at path in RemotePlayersMovementSystem sets HandPointAtComponent.IsPointing and RemotePlayerMovementComponent.IsPointingAt from the replayed message but never ticks the duration timer — unlike HandPointAtSystem, which calls TickDuration every frame for the local player. One stale isPointingAt=true snapshot pins the arm at an absolute WorldHitPoint indefinitely.
Fix: The diff mirrors the local gesture's expiry mechanism: RefreshDuration on every pointing message, TickDuration per-frame, and sync-back to remotePlayerMovement.IsPointingAt on expiry. This addresses the root cause directly — not a workaround or symptom suppression.
Step 3 — Design & integration: PASS
Owner search: The lifecycle of remote point-at state is managed by RemotePlayersMovementSystem, which:
- Receives
NetworkMovementMessages from the player inbox - Updates
RemotePlayerMovementComponent.IsPointingAtandPointAtWorldHitPoint(viaUpdatePointAtIK) - Calls
ApplyPointAtIKto propagate state intoHandPointAtComponent
The downstream RemoteHandPointAtSystem (CharacterMotion/Systems/RemoteHandPointAtSystem.cs) runs [UpdateAfter(typeof(RemotePlayersMovementSystem))] and is purely a visual consumer — it reads HandPointAtComponent.IsPointing for animation weights and IK, never writes to RemotePlayerMovementComponent. The local player's HandPointAtSystem (CharacterMotion/Systems/HandPointAtSystem.cs) owns both input/duration management and IK application in a single system.
The fix correctly places the expiry logic in RemotePlayersMovementSystem — the existing owner of remote point-at state. Moving it to RemoteHandPointAtSystem was considered but rejected: that system uses RemotePlayerMovementComponent only as a query filter ([All(typeof(...))]) and never accesses it — adding a write-back there would introduce a reverse dependency that doesn't exist today.
Teardown trace: No new subscriptions, callbacks, event hookups, connections, or persistent collections are introduced. The only additions are per-frame arithmetic on existing struct fields (HandPointAtComponent.duration).
Step 4 — Member audit
UpdatePointAtIK (private method, new): 3 consumers — HandleFirstMessage (line 49), HandleNewMessage (line 137), and the inner loop of HandleNewMessage (line 154). Each call site previously had the bare remotePlayerMovement.UpdatePointAtIK(remote) call; the extraction adds the RefreshDuration side-effect consistently across all three. Not single-use; appropriate extraction.
No new public properties or accessors are introduced.
Step 5 — Line-level review
No blocking issues found across two passes:
A. Blocking-issue categories: No code quality violations, bugs, security vulnerabilities, performance issues, resource leaks, nullability violations, or false-intent conditions.
B. Design, encapsulation & resource smells: No construction issues, naming concerns, encapsulation violations, magic values, or resource lifecycle problems.
Execution order verified:
ApplyPointAtIKsetshandPointAt.IsPointingfromremotePlayerMovement.IsPointingAtTickDurationdecrements the timer; if expired, setshandPointAt.IsPointing = false- Sync-back clears
remotePlayerMovement.IsPointingAtwhenhandPointAtexpires HandleNewMessagemay re-assert pointing viaRefreshDuration
The set-then-clear of handPointAt.IsPointing on the expiry frame is harmless since RemoteHandPointAtSystem runs after and reads the final value.
Struct ref safety verified: Both handPointAt and remotePlayerMovement are received by ref in the query. No structural changes (Add/Remove) occur after the refs are obtained. TickDuration mutates the struct through the ref correctly.
Per-frame allocations: None introduced. UpdatePointAtIK operates on struct refs, TickDuration/RefreshDuration are struct methods. No LINQ, closures, or boxing.
Security review
No security concerns: no user input handling changes, no auth/permissions changes, no serialization of external data, no file operations, no sensitive data exposure, no new dependencies.
Test coverage
The new RemotePlayerPointAtExpiryShould.ExpirePointingAfterPointAtDurationWithNoReassertion test is well-structured:
- Follows AAA pattern with clear Arrange/Act/Assert phases
- Uses
UnitySystemTestBase<RemotePlayersMovementSystem>correctly - Uses NUnit + NSubstitute per project conventions
- Pre-clears the interpolation cooldown gate to isolate the expiry behavior
- Verifies both
HandPointAtComponent.IsPointingandRemotePlayerMovementComponent.IsPointingAtclear on expiry - Documents the regression scenario inline
CI status
- ✅
semantic / title-matches-convention— PASS - ⏳
Test (editmode),Test (playmode)— IN PROGRESS - ❌
rsp files match generator— FAIL (may need.rspregeneration after adding the new test file; typically resolved by opening the project in Unity Editor) - ❌
enforce-approvals— FAIL (expected; awaiting QA and DEV approvals) - ⏳
Prebuild— IN PROGRESS
REVIEW_RESULT: PASS ✅
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Modifies multiplayer movement synchronization and point-at IK state management in RemotePlayersMovementSystem
QA_REQUIRED: YES
Reviewed by Jarvis 🤖 · Requested by decentraland-bot via GitHub
|
🔍 Jarvis reviewed this PR and found no blocking issues, but assessed it as complex — human DEV review is still required before merging. |
|
PR #9780, run #32152545601 Builds: Windows change, Windows baseline, macOS change, macOS baseline How to read this table
Intel Core i5
Exception breakdown
Apple M1
|
|
Not valid: see #9606 |
3 participants
Server-cached movement snapshots delivered on join/teleport are applied by HandleFirstMessage with no freshness bound, and the remote path never expires IsPointing — one replayed pointing=true snapshot pins a remote avatar's arm at a stale absolute WorldHitPoint indefinitely. Bound the remote point-at lifetime the same way HandPointAtSystem bounds the local gesture.
Includes a regression test that fails without this fix.
Pull Request Description
What does this PR change?
Test Instructions
Steps (standard run):
metaforge explorer run XXXX # ← replace with this PR numberExpected result:
Steps (fresh account):
metaforge account create --clear metaforge explorer run XXXX # ← replace with this PR numberExpected result:
Automation (if applicable):
metaforge explorer test XXXXPrerequisites
Test Steps
Additional Testing Notes
Quality Checklist
Code Review Reference
Please review our Branch & PR Standards before submitting. It explains the automated review flow, QA/DEV approval requirements, and what each label does — especially useful for first-time contributors.