Fix drag drift during viewport transformations#4012
Fix drag drift during viewport transformations#4012afrdbaig7 wants to merge 1 commit intoGraphiteEditor:masterfrom
Conversation
This shifts all dragging offset calculations in TransformLayer, SelectTool, and PathTool to document space, removing any dependency on cached viewport projections for state logic. Both BoundingBoxManager's interactions and custom Path selection behaviors now properly invalidate and lazily reload viewport layouts on CanvasTransformed updates, resolving issues with geometry moving opposite the viewport during pan/tilt/zoom.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the Path, Select, and Transform tools to utilize document-space coordinates for drag operations, ensuring that viewport transformations like panning and zooming do not disrupt active interactions. It introduces a centralized DragState utility and a CanvasTransformed message to re-trigger tool logic when the viewport changes. The review feedback identifies critical logic errors where document-space coordinates were incorrectly updated during auto-panning, missing initialization for new document-space fields in the bounding box manager, and potential issues with hardcoded modifier keys in the state machine transitions.
| let (resize, rotate, skew) = transforming_transform_cage(document, &mut tool_data.bounding_box_manager, input, responses, &mut tool_data.layers_dragging, Some(position)); | ||
|
|
||
| tool_data.drag_start_center = position; | ||
| tool_data.drag_start_center = document.metadata().document_to_viewport.inverse().transform_point2(position); |
There was a problem hiding this comment.
The new fields original_bounds_to_document and center_of_transformation_doc in BoundingBoxManager are used to recompute viewport positions during PTZ, but they are never initialized when the transformation starts. This will cause incorrect behavior (e.g., jumping to the origin) when the viewport is transformed during a resize/rotate/skew operation.
let (resize, rotate, skew) = transforming_transform_cage(document, &mut tool_data.bounding_box_manager, input, responses, &mut tool_data.layers_dragging, Some(position));
if let Some(bounds) = &mut tool_data.bounding_box_manager {
let viewport_to_doc = document.metadata().document_to_viewport.inverse();
bounds.original_bounds_to_document = viewport_to_doc * bounds.original_bound_transform;
bounds.center_of_transformation_doc = viewport_to_doc.transform_point2(bounds.center_of_transformation);
}
tool_data.drag_start_center = document.metadata().document_to_viewport.inverse().transform_point2(position);| if let Some(offset) = tool_data.auto_panning.shift_viewport(input, viewport, responses) { | ||
| tool_data.drag_start_pos += offset; | ||
| tool_data.drag_start_doc += document.metadata().document_to_viewport.inverse().transform_vector2(offset); | ||
| } |
There was a problem hiding this comment.
Updating drag_start_doc during auto-panning is incorrect. Since drag_start_doc is in document space, it should remain invariant to viewport transformations. Updating it here effectively cancels out the panning movement for the drag delta, which defeats the purpose of auto-panning (where the object should move with the mouse relative to the document).
let _ = tool_data.auto_panning.shift_viewport(input, viewport, responses);| if let Some(offset) = tool_data.auto_panning.shift_viewport(input, viewport, responses) { | ||
| tool_data.drag_start_pos += offset; | ||
| tool_data.drag_start_doc += document.metadata().document_to_viewport.inverse().transform_vector2(offset); | ||
| } |
There was a problem hiding this comment.
Similar to the drawing state, drag_start_doc should not be updated during auto-panning while dragging. Document-space coordinates are independent of viewport shifts.
let _ = tool_data.auto_panning.shift_viewport(input, viewport, responses);| if tool_data.non_duplicated_layers.is_some() { | ||
| tool_data.stop_duplicates(document, responses); | ||
| tool_data.duplicate_paused_by_ptz = true; | ||
| } |
There was a problem hiding this comment.
Stopping duplicates during viewport transformations seems unnecessary and results in a poor user experience where the duplicated layers disappear and the original layers jump back if the user pans or zooms while dragging with Alt. Since PointerMove is re-emitted to handle PTZ, the duplication state should be able to persist correctly.
| let modifier_keys = PathToolMessage::PointerMove { | ||
| equidistant: Key::Alt, | ||
| toggle_colinear: Key::KeyC, | ||
| move_anchor_with_handles: Key::Space, | ||
| snap_angle: Key::Control, | ||
| lock_angle: Key::Shift, | ||
| delete_segment: Key::Backspace, | ||
| break_colinear_molding: Key::Tab, | ||
| segment_editing_modifier: Key::Alt, | ||
| }; |
There was a problem hiding this comment.
Hardcoding modifier keys like Key::Alt and Key::Control here bypasses the input mapper's configuration. If a user has remapped these actions, the tool will check the wrong physical keys during viewport transformations. It would be better to store the keys received in the last PointerMove message and reuse them here.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 6 files
Confidence score: 3/5
- There is a concrete interaction regression risk in
editor/src/messages/tool/tool_messages/path_tool.rs: during SlidingPoint, viewport pan/zoom via CanvasTransformed can leave the sliding anchor stale until pointer movement, which may cause visible desync. editor/src/messages/tool/transform_layer/transform_layer_message_handler.rschanges ghost-outline behavior so it follows the live transform each frame, which can break the intended pre-transform visual reference during edits.- Given two moderate-confidence, user-visible behavior issues in core editing feedback paths, this looks like some merge risk rather than a blocker.
- Pay close attention to
editor/src/messages/tool/tool_messages/path_tool.rsandeditor/src/messages/tool/transform_layer/transform_layer_message_handler.rs- viewport/transform updates may produce incorrect on-canvas reference positions.
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/tool_messages/path_tool.rs">
<violation number="1" location="editor/src/messages/tool/tool_messages/path_tool.rs:3163">
P2: During SlidingPoint, CanvasTransformed only redraws overlays and does not recompute the sliding anchor position. If the viewport is panned/zoomed without a pointer move, slide_point isn’t called, so the anchor can desync until the next mouse move. Dragging/Drawing explicitly re-emit PointerMove on CanvasTransformed to avoid this drift.</violation>
</file>
<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:210">
P2: Ghost outline now uses the live layer transform every frame, so it will move with the in-progress transform instead of remaining a pre-transform reference as intended.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| responses.add(OverlaysMessage::Draw); | ||
| PathToolFsmState::Drawing { selection_shape } | ||
| } | ||
| (PathToolFsmState::SlidingPoint, PathToolMessage::CanvasTransformed) => { |
There was a problem hiding this comment.
P2: During SlidingPoint, CanvasTransformed only redraws overlays and does not recompute the sliding anchor position. If the viewport is panned/zoomed without a pointer move, slide_point isn’t called, so the anchor can desync until the next mouse move. Dragging/Drawing explicitly re-emit PointerMove on CanvasTransformed to avoid this drift.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At editor/src/messages/tool/tool_messages/path_tool.rs, line 3163:
<comment>During SlidingPoint, CanvasTransformed only redraws overlays and does not recompute the sliding anchor position. If the viewport is panned/zoomed without a pointer move, slide_point isn’t called, so the anchor can desync until the next mouse move. Dragging/Drawing explicitly re-emit PointerMove on CanvasTransformed to avoid this drift.</comment>
<file context>
@@ -3119,6 +3129,42 @@ impl Fsm for PathToolFsmState {
+ responses.add(OverlaysMessage::Draw);
+ PathToolFsmState::Drawing { selection_shape }
+ }
+ (PathToolFsmState::SlidingPoint, PathToolMessage::CanvasTransformed) => {
+ responses.add(OverlaysMessage::Draw);
+ PathToolFsmState::SlidingPoint
</file context>
| for (outline, transform) in &self.ghost_outline { | ||
| overlay_context.outline(outline.iter(), *transform, Some(COLOR_OVERLAY_GRAY)); | ||
| for (outline, layer) in &self.ghost_outline { | ||
| overlay_context.outline(outline.iter(), document.metadata().transform_to_viewport(*layer), Some(COLOR_OVERLAY_GRAY)); |
There was a problem hiding this comment.
P2: Ghost outline now uses the live layer transform every frame, so it will move with the in-progress transform instead of remaining a pre-transform reference as intended.
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 210:
<comment>Ghost outline now uses the live layer transform every frame, so it will move with the in-progress transform instead of remaining a pre-transform reference as intended.</comment>
<file context>
@@ -207,8 +206,8 @@ impl MessageHandler<TransformLayerMessage, TransformLayerMessageContext<'_>> for
- for (outline, transform) in &self.ghost_outline {
- overlay_context.outline(outline.iter(), *transform, Some(COLOR_OVERLAY_GRAY));
+ for (outline, layer) in &self.ghost_outline {
+ overlay_context.outline(outline.iter(), document.metadata().transform_to_viewport(*layer), Some(COLOR_OVERLAY_GRAY));
}
}
</file context>
1 participant
Fixes #3500
The dragging mechanism would float away whenever the viewport got transformed (panned, tilted, zoomed) while interacting since the computation logic was based on the cached version of the transforms. This pull request ensures that the dragging computations are always done in document space and get projected to document_to_viewport each frame. TransformLayer no longer requires any cached version of transforms. In addition, both SelectTool and PathTool ensure that their interaction is recomputed on every PointerMove event through CanvasTransformed, while the BoundingBoxManager stores the bounding box cache in document space.