fix: reset stale hand point-at on teleport and scene reload (#9578) - #9693
fix: reset stale hand point-at on teleport and scene reload (#9578)#9693lorenzo-ranciaffi wants to merge 3 commits into
Conversation
A cached absolute point-at world hit-point survived a teleport or same-realm scene reload, leaving an avatar's arm pinned at a stale position. Reset the point-at target from both the PlayerTeleportIntent path and the scene-reload path (ECSReloadScene, both overloads). Verified on real Unity 6000.4.0f1 (compile + EditMode/PlayMode regression tests).
🚦 CI StatusWindows and Mac build successful in Unity Cloud! You can find a link to the downloadable artifact below. Warnings not reduced: 13292 => 13296 — remove at least 5 warnings to merge. Warnings/errors in files changed by this PR (13)All Unity tests passed ✅
|
decentraland-bot
left a comment
There was a problem hiding this comment.
Review — PR #9693: fix: reset stale hand point-at on teleport and scene reload (#9578)
STEP 1 — Context
Subsystem: Character Motion (hand point-at IK system)
Files: HandPointAtComponent.cs, HandPointAtSystem.cs, HandPointAtStaleResetShould.cs (new), ECSReloadScene.cs, CharacterMotionPlugin.cs
Diff size: +157 −3 across 6 files (including test + meta)
Reviewed against CLAUDE.md, docs/README.md, and surrounding files: TeleportCharacterSystem.cs, UpdateCurrentSceneSystem.cs, IScenesCache.cs, ReactiveProperty.cs, SmartWearableSystem.cs, BaseUnityLoopSystem.cs, CharacterMotionPlugin.cs.
STEP 2 — Root-cause check: PASS ✅
Problem: A cached absolute WorldHitPoint survived a teleport or same-realm scene reload, leaving the avatar's arm pinned at a stale world-space position.
Fix: Resets IsPointing and duration via two distinct ECS queries:
- Teleport —
ResetPointAtOnTeleportmatches entities withPlayerTeleportIntent, clearing point-at while the teleport is in flight. - Scene teardown —
ResetPointAtOnCurrentSceneLostfires whenCurrentScenetransitions tonull(scene reload, ban, realm change), bridged via a one-frame latch set in theOnUpdatecallback.
This addresses the root cause — the hit-point was never invalidated when the player's spatial context changed. The fix is not a symptom workaround.
STEP 3 — Design & integration: PASS ✅
Owner search:
HandPointAtComponentis created byCharacterMotionPlugin.InjectToWorld(line ~147) on the player entity.HandPointAtComponentstate is mutated exclusively byHandPointAtSystem— confirmed viarg "ref HandPointAtComponent"(onlyHandPointAtSystemqueries, plusRemoteHandPointAtSystemfor remote avatars).- The teleport lifecycle is owned by
TeleportCharacterSystem(inChangeCharacterPositionGroup). It manages position, collisions, and readiness reports — it has no knowledge of hand IK and should not. - The scene lifecycle is owned by
UpdateCurrentSceneSystem(inRealmGroup), which callsscenesCache.SetCurrentScene(). TheCurrentScene.OnUpdatecallback pattern is the established bridge for downstream consumers.
Verdict: HandPointAtSystem is the correct owner for this reset logic. Moving it to TeleportCharacterSystem or scene lifecycle code would create cross-cutting concerns in systems that don't own HandPointAtComponent.
Callback+latch pattern: The currentSceneLost bool latch (set in OnCurrentSceneChanged, consumed in Update) is the identical pattern used by SmartWearableSystem.currentSceneDirty (line 313-314) and DonationsService. It correctly bridges a synchronous event callback into ECS frame processing without mutating components outside Update(). Acceptable precedented deviation from the "no system state" rule — this is a transient one-frame flag, not persistent membership.
Teardown trace:
scenesCache.CurrentScene.OnUpdate += OnCurrentSceneChanged— constructor (line 67)scenesCache.CurrentScene.OnUpdate -= OnCurrentSceneChanged—OnDispose()(line 77)- ✅ Properly paired.
BaseUnityLoopSystem.Dispose()guaranteesOnDispose()runs exactly once.
STEP 4 — Member audit
StopPointing() (new public method on HandPointAtComponent):
- Consumers: 2 —
ResetPointAtOnCurrentSceneLostandResetPointAtOnTeleport. - Justification: Combines
IsPointing = false+RefreshDuration(0f)— a meaningful grouping. Differs fromForceStopAction()(which also setsForceStop = trueto prevent re-trigger while button held). Not single-use-to-merge — it has two independent callers. - ✅ Well-scoped, intent-named.
WorldHitPoint not cleared by StopPointing():
- Every reader gates on
IsPointingfirst:ApplyPointAtIK(line 305),TorsoIKSystem(line 49/59),PointAtMarkerSystem(lines 90/132/149),HeadIKSystem(line 192),HandsIKSystem(line 91/99). - Network path (
PlayerMovementNetSendSystem) also gates onisPointingAt. - ✅ Stale
WorldHitPointis never observable whenIsPointingis false.
STEP 5 — Line-level review
No blocking issues found. All code follows project conventions:
- Queries correctly filter
[None(typeof(DeleteEntityIntention))] - Constructor is
internal(CLAUDE.md §1/§10) - No LINQ, no allocations in Update path
refaccess for component mutation- Event subscribe/unsubscribe properly paired
_ = Resources.UnloadUnusedAssets()is a correct discard ofAsyncOperationreturn value (suppresses CS4014)
Coverage note (non-blocking): For instant teleports (loadReport == null), TeleportCharacterSystem (in ChangeCharacterPositionGroup) resolves and removes PlayerTeleportIntent before HandPointAtSystem (in PresentationSystemGroup, UpdateAfter(ChangeCharacterPositionGroup)) runs. The teleport reset query would not observe the intent in that frame. This is not a regression — existing movement guards (IsGrounded set to false on teleport, velocity checks) naturally suppress pointing within 1-2 frames. The PR targets the async teleport path (realm/portal) where the intent persists across multiple pending frames.
STEP 6 — Complexity
COMPLEX — Adds a new dependency (IScenesCache) to an ECS system, introduces an event subscription with lifecycle management, adds two new ECS queries with archetype filters, and modifies system execution order in Update().
STEP 7 — QA assessment
QA_REQUIRED: YES — Changes affect user-visible pointing behavior during teleport and scene reload. Manual verification needed per the PR's test steps.
STEP 8 — Non-blocking warnings
No warnings. Main.unity is not modified.
STEP 9 — Verdict
REVIEW_RESULT: PASS ✅
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Adds IScenesCache dependency and CurrentScene event subscription to HandPointAtSystem, introduces two new ECS queries modifying point-at component state on teleport and scene teardown.
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 #9693, run #31610131253 Builds: Windows change, Windows baseline, macOS change, macOS baseline How to read this table
Apple M1
Intel Core i5
Exception breakdown
|
…9705 #9706) (#9707) * opti(avatar-rendering): bound bone-matrix job by each avatar's actual bone count BoneMatrixCalculationJob recomputed all MAX_BONE_COUNT matrices per avatar every frame regardless of how many ComputeSkinning actually uploads; refresh and pass through the authoritative per-avatar count so the job only computes the uploaded range. * opti(transforms): assign world cache directly in SetWorldTransform Transform.SetPositionAndRotation is world-authoritative, so the resulting world pose equals the arguments passed in; skip the native position/rotation readback and assign the cache from them directly. * opti(landscape): reuse persistent native containers in RenderGroundSystem RenderGroundInternal allocated and disposed a NativeArray<int> and a NativeList<Matrix4x4> with Allocator.TempJob every frame the ground was visible; hold them as Allocator.Persistent fields instead, clearing them each frame and disposing once in OnDispose. * opti(web-requests): pre-size PartialDownloadHandler and grow geometrically ReceiveData rented exactly PartialData.Length + dataLength and copied the full old buffer on every regrow, causing O(N^2) rent+memcpy churn over a chunked download. Pre-size the buffer from the Content-Length header when available, and otherwise double the buffer on regrow, copying only the bytes written so far. Exercising the growth path requires driving DownloadHandlerScript's native callbacks, which has no stable unit-test seam in headless batch mode. * opti(comms-profiles): skip the remove-intentions lock on empty frames RemoteEntitiesExtensions.Remove ran every frame and unconditionally built an OwnedBunch<RemoveIntention>, whose ctor acquires MutexSync even when there is nothing to remove. Add a racy, lock-free NewBunchAvailable() pre-check (mirrors RemoteProfiles.NewBunchAvailable()) so the lock is skipped entirely on empty frames. * fix: reset stale hand point-at on teleport and scene reload (#9578) A cached absolute point-at world hit-point survived a teleport or same-realm scene reload, leaving an avatar's arm pinned at a stale position. Reset the point-at target from both the PlayerTeleportIntent path and the scene-reload path (ECSReloadScene, both overloads). Verified on real Unity 6000.4.0f1 (compile + EditMode/PlayMode regression tests). * opti(avatar-rendering): hoist frustum-plane extraction out of the outline query GetAvatarsVisibleWithOutline recalculated the camera's frustum planes and re-fetched the camera component for every avatar; both are identical across a frame, so compute them once in Update and pass the camera in via [Data]. * opti(avatar-animation): skip redundant point-at/rotation layer weight writes SetPointAtLayerWeight and SetRotationLayerWeight now shadow the last value written and skip the native Animator.SetLayerWeight call when it hasn't changed; ResetState clears the shadow so a rebind isn't skipped. * refactor(landscape): replace init flag with nullable tuple in RenderGroundSystem * docs(landscape): explain why persistent ground containers never need reallocation * Fix formatting (no code changes) * changed how reload/teleport reset is managed * opti(avatar-animation): finish indexed animator-layer API and drop string-based lookups Completes the layer-index scaffolding introduced with the weight guard: - AnimatorEmoteLayers gains the BASE_LAYER_INDEX constant the scaffolding referenced (fixes compilation) and loses GetFromEmoteMask plus the unused ALL_LAYERS / NON_BASE_LAYERS arrays. - GetAnimatorCurrentStateTag(string) and SetLayerWeight(string) resolved the layer via a native Animator.GetLayerIndex call on every invocation, per frame in HeadIKSystem and HandsIKSystem. All callers now use the cached index API (UpperBodyLayerIndex, GetEmoteLayerIndex, int overloads) and the string overloads are removed. - Removes the unused DclAvatarMask using alias. * Update Explorer/Assets/DCL/Multiplayer/Profiles/RemoveIntentions/IRemoveIntentions.cs Co-authored-by: Muna <44584806+decentraland-bot@users.noreply.github.com> Signed-off-by: Nick Khalow <71646502+NickKhalow@users.noreply.github.com> * docs(transforms): explain direct world cache write in SetWorldTransform * expected length "no double calculation" and comments * refactor(avatar-rendering): drop camera param from IsVisibleInCamera, pass CameraComponent as query data --------- Signed-off-by: Nick Khalow <71646502+NickKhalow@users.noreply.github.com> Co-authored-by: Esteban Ordano <esteban@decentraland.org> Co-authored-by: lorenzo-ranciaffi <lorenzo.ranciaffi@decentraland.org> Co-authored-by: lorenzo-ranciaffi <41125365+lorenzo-ranciaffi@users.noreply.github.com> Co-authored-by: Muna <44584806+decentraland-bot@users.noreply.github.com>
|
introduced in #9707 |
Pull Request Description
What does this PR change?
A cached absolute point-at world hit-point survived a teleport or same-realm scene reload, leaving an avatar's arm pinned at a stale position. Reset the point-at target from both the PlayerTeleportIntent path and the scene-reload path by using the CurrentScene update in the scene cache..
Test Steps
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.