Wrap cursor around viewport during G/R/S (#3255) - #4486
Conversation
|
Cool! Could you post a video of it in action? Ideally on both desktop and web. That'd help to prioritize the review. |
There was a problem hiding this comment.
4 issues found and verified against the latest diff
Confidence score: 2/5
editor/src/messages/app_window/app_window_message_handler.rsdelivers each web lockedpointermovethrough both absolute and relative paths, allowing the absolute update to reset relative motion and make G/R/S transforms behave incorrectly; ensure only one path updates the pointer or reconcile their ordering.editor/src/messages/input_preprocessor/input_preprocessor_message_handler.rsadds raw physical-device deltas to viewport-logical coordinates, which can produce incorrectly scaled pointer movement on platforms with differing device and viewport scales; convert the delta into logical coordinates before applying it.editor/src/messages/app_window/app_window_message_handler.rstreats the sharedPointerLockMoveprotocol as canvas-tool input even during native number-field drags, so dragging a number input can also trigger the active canvas tool; distinguish number-input and transform-tool sources.frontend/wrapper/src/editor_commands.rsexposes an unusedapp_window_pointer_unlockbinding while unlock is handled directly by the transform layer, leaving dead integration code that should be removed or wired to the actual unlock 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">
P2: `AppWindowMessage::PointerLockMove` is also the native NumberInput drag protocol, not only G/R/S. This unconditional relative message therefore makes every native number-field drag invoke the active canvas tool's generic `PointerMove` mappings in addition to updating the field; gate the relative forwarding to G/R/S or introduce a separate transform-only pointer-lock message.</violation>
<violation number="2" 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>
| // 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.
P2: AppWindowMessage::PointerLockMove is also the native NumberInput drag protocol, not only G/R/S. This unconditional relative message therefore makes every native number-field drag invoke the active canvas tool's generic PointerMove mappings in addition to updating the field; gate the relative forwarding to G/R/S or introduce a separate transform-only pointer-lock message.
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>`AppWindowMessage::PointerLockMove` is also the native NumberInput drag protocol, not only G/R/S. This unconditional relative message therefore makes every native number-field drag invoke the active canvas tool's generic `PointerMove` mappings in addition to updating the field; gate the relative forwarding to G/R/S or introduce a separate transform-only pointer-lock message.</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>
| 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
2026-08-30.17-30-49.mp4If u want me to use the os cursor icon instead of the triangle |
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
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.
2 existing issues remain and 1 new issue found across 14 files (changes from recent commits).
Confidence score: 4/5
editor/src/messages/app_window/app_window_message_handler.rsmay treat every pointer-lock delta as a regular relative pointer move, updatingself.mouse.positionand firingPointerMove; verify the intended pointer-lock behavior and filter or route these deltas appropriately.editor/src/messages/tool/transform_layer/transform_layer_message_handler.rsresets a non-default tool cursor toDefaultwhen a G/R/S transform ends, which can leave the tool showing the wrong cursor; restore the previously active cursor or refresh it through the owning tool.frontend/wrapper/src/editor_commands.rsaddsappWindowPointerUnlockwithout any frontend callers, so the unlock path may remain unused and the wrapper adds dead API surface; add the required caller or remove the unused export.
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:701">
P3: When a G/R/S transform ends in a tool with a non-default cursor, this line overwrites that tool cursor with `Default`. Restore the cursor that was active before hiding it, or ask the owning tool to refresh its cursor instead of hardcoding `Default`.</violation>
</file>
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
| } | ||
| self.software_cursor_active = false; | ||
| responses.add(FrontendMessage::UpdateSoftwareCursor { visible: false, x: 0., y: 0. }); | ||
| responses.add(FrontendMessage::UpdateMouseCursor { cursor: MouseCursorIcon::Default }); |
There was a problem hiding this comment.
P3: When a G/R/S transform ends in a tool with a non-default cursor, this line overwrites that tool cursor with Default. Restore the cursor that was active before hiding it, or ask the owning tool to refresh its cursor instead of hardcoding Default.
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 701:
<comment>When a G/R/S transform ends in a tool with a non-default cursor, this line overwrites that tool cursor with `Default`. Restore the cursor that was active before hiding it, or ask the owning tool to refresh its cursor instead of hardcoding `Default`.</comment>
<file context>
@@ -651,6 +681,27 @@ impl TransformLayerMessageHandler {
+ }
+ self.software_cursor_active = false;
+ responses.add(FrontendMessage::UpdateSoftwareCursor { visible: false, x: 0., y: 0. });
+ responses.add(FrontendMessage::UpdateMouseCursor { cursor: MouseCursorIcon::Default });
+ responses.add(AppWindowMessage::PointerUnlock);
+ }
</file context>
Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
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
Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
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.
Like Blender GHOST_kGrabWrap.