fix: mouse coords to scene - #9754
Conversation
Opening the chat puts it into the focused state, which disables the entire `Camera` input action map (intended to stop camera-look/zoom while typing). `PrimaryPointerInfoSystem` reads the raw pointer position through that same map's `Point` action; a disabled `InputAction` returns `default(Vector2)`, so the client writes `PBPrimaryPointerInfo.ScreenCoordinates = (0,0)` to the scene on every tick while chat is focused. The fishing-pond scene normalizes that value against the canvas size and renders the tooltip at `left:14, bottom:10` — exactly the bottom-left corner, where the chat input sits. Fixes #9496 Includes a regression test that fails without this fix. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
🚦 CI StatusWindows and Mac build successful in Unity Cloud! You can find a link to the downloadable artifact below. Warnings not reduced: 13116 => 13126 — remove at least 11 warnings to merge. Warnings/errors in files changed by this PR (12)All Unity tests passed ✅
|
This comment has been minimized.
This comment has been minimized.
decentraland-bot
left a comment
There was a problem hiding this comment.
STEP 1 — Context & Scope
PR: #9754 — fix: mouse coords to scene
Branch: bugsweep/toggle-hints-tooltip-position → dev
Files changed: 3 (+133 −1)
PrimaryPointerInfoSystem.cs— 6-line production fix inUpdatePointerInfo()PrimaryPointerInfoSystemCameraMapDisabledShould.cs— new 116-line regression test.cs.meta— Unity asset metadata
Subsystem docs loaded: CLAUDE.md, docs/README.md, review-instructions.md. Neighbouring files read: full PrimaryPointerInfoSystem.cs, existing PrimaryPointerInfoSystemShould.cs test fixture.
STEP 2 — Root-cause check: PASS ✅
Problem: ApplyInputMapsSystem disables the entire Camera action map when UI holds input focus (e.g. chat). PrimaryPointerInfoSystem reads pointer position from DCLInput.Instance.Camera.Point — an action in that map. A disabled InputAction.ReadValue<Vector2>() returns default(Vector2) = (0, 0). The fishing-pond scene normalises these coordinates and pins the "Toggle Hints" tooltip to the bottom-left corner.
Fix: Falls back to the raw pointer device (Pointer.current?.position.ReadValue()) when the input action is disabled. This is a correct root-cause fix:
PrimaryPointerInfoSystemis an observation system (scene-facing CRDT data), not a control system (camera/movement). The zero-on-disable pattern used byUpdateCameraInputSystem/UpdateInputMovementSystem/UpdateInputJumpSystemis intentionally wrong here.- The broader alternative (moving
Pointto an always-on action map) would be architecturally purer but disproportionate for this bug severity.
STEP 3 — Design & integration: PASS ✅
No new long-lived unit introduced — the change modifies one private method in an existing system.
Lifecycle check:
Pointer.currentis a static point-in-time read, not a subscription — no leak, no disposal needed.- The cached
inputPointreference remains valid when disabled (Disable()stops processing but doesn't destroy the action). previousPositionwas already tracked by the system; the fallback to it is natural.
Teardown trace: No new subscriptions, events, callbacks, connections, or resources added. Nothing to unsubscribe.
STEP 4 — Member audit: N/A
No new public properties, accessors, or exported members.
STEP 5 — Line-level review
A. Blocking issues: None
- Allocation-free ✅ —
inputPoint.enabled(property read),Pointer.current(static property),.position.ReadValue()(struct read),??(value-type coalescing). No heap allocations, no LINQ, no closures, no boxing. - Null safety ✅ —
Pointer.current?.handles absent device;?? previousPositionprovides final fallback. AfterInitialize()callsUpdatePointerInfo(),previousPositionalways reflects a real position. - Transition correctness ✅ —
previousPosition = rawPositionexecutes regardless of source, so lock/unlock and enable/disable transitions remain spike-free. - No resource leaks ✅ — No subscriptions, events, or connections added.
- No detached async ✅.
- Security ✅ — No auth changes, no secrets, no injection vectors.
Pointer.currentreads the same physical device asCamera.Point— not a new input channel. (Scenes now receive real coordinates during UI focus, which is the intended fix, not a vulnerability.)
B. Design smells
[P2] Comment enumerates specific UI panels that will go stale (PrimaryPointerInfoSystem.cs lines 68–70):
The comment lists "chat, passport, explore panel" — these specific panels can change as the UI evolves, silently making the comment inaccurate. Additionally, the raw-device fallback bypasses any action-level InputProcessors on Camera.Point; today there are none, but noting the assumption aids future maintainers. See inline suggestion.
[P2] Test fixture duplication (PrimaryPointerInfoSystemCameraMapDisabledShould.cs):
The new fixture duplicates ~90 lines of setup/teardown from the existing PrimaryPointerInfoSystemShould: identical fields (sceneWorld, globalWorld, mouse, camera, ecsToCRDTWriter, sceneStateProvider, exposedCameraData, system, putCalls), near-identical SetUp(), identical Cleanup(). The single test method needs only two unique lines in its Arrange section:
Set(mouse.position, simulatedPosition);
DCLInput.Instance.Camera.Disable();Suggestion: Add NotReportZeroScreenCoordinatesWhenCameraMapDisabledByChatFocus() to the existing PrimaryPointerInfoSystemShould fixture, eliminating the new file entirely. If more disabled-map tests are planned, extract a shared base class later.
STEP 6 — Complexity: COMPLEX
Touches input handling (InputAction.enabled check, raw Pointer.current device read) and scene-facing CRDT data flow (PBPrimaryPointerInfo.ScreenCoordinates).
STEP 7 — QA: YES
Modifies runtime input behaviour visible to users and scenes. Manual verification needed: follow repro steps from #9496.
STEP 8 — Non-blocking warnings
None. Main.unity is not modified.
STEP 9 — Verdict
REVIEW_RESULT: PASS ✅
COMPLEXITY: COMPLEX
COMPLEXITY_REASON: Modifies input handling (InputAction/Pointer device read) and scene-facing CRDT data flow
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 #9754, run #32243052524 Builds: Windows change, Windows baseline, macOS change, macOS baseline How to read this table
Intel Core i5
Exception breakdown
Apple M1
|
anicalbano
left a comment
There was a problem hiding this comment.
✔️ Tested and working as expected in both Windows and macOS systems
✔️ No regressions were found
Pull Request Description
Fixes #9496
What does this PR change?
Reads the inputsystem pointer data when the pointer action is disabled (e.g. chat is focused) so it reads the actual coords instead of
default(Vector2).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.