Reduce along-path snap influence near colinear anchor point#4078
Reduce along-path snap influence near colinear anchor point#4078devviktoria wants to merge 2 commits intoGraphiteEditor:masterfrom
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a snap dampening mechanism for the path tool to improve handle movement precision when snapping to paths. The review suggests updating the snap indicator after the weight reduction to ensure visual consistency and recommends changing the method signature of 'reduce_snap_weight' to take '&self' instead of '&mut self' to improve code immutability.
| self.snap_manager.update_indicator(snap_result.clone()); | ||
|
|
||
| let snap_result = self.reduce_snap_weight(snap_result, new_handle_position, anchor_position); |
There was a problem hiding this comment.
The snap indicator should be updated after the snap weight reduction. This ensures that the visual feedback (the blue square and snap lines) correctly reflects the dampened position of the handle rather than the raw snap point on the path. This provides a more accurate representation of the actual handle movement to the user.
| self.snap_manager.update_indicator(snap_result.clone()); | |
| let snap_result = self.reduce_snap_weight(snap_result, new_handle_position, anchor_position); | |
| let snap_result = self.reduce_snap_weight(snap_result, new_handle_position, anchor_position); | |
| self.snap_manager.update_indicator(snap_result.clone()); |
| document.metadata().document_to_viewport.transform_vector2(snap_result.snapped_point_document - handle_position) | ||
| } | ||
|
|
||
| fn reduce_snap_weight(&mut self, mut snap_result: SnappedPoint, new_handle_position: DVec2, anchor_position: DVec2) -> SnappedPoint { |
There was a problem hiding this comment.
This method only reads from self.selection_status and does not perform any mutations on the PathToolData struct. It should take &self instead of &mut self to follow Rust best practices for immutability and to clarify the function's intent.
| fn reduce_snap_weight(&mut self, mut snap_result: SnappedPoint, new_handle_position: DVec2, anchor_position: DVec2) -> SnappedPoint { | |
| fn reduce_snap_weight(&self, mut snap_result: SnappedPoint, new_handle_position: DVec2, anchor_position: DVec2) -> SnappedPoint { |
|
Please note that this does not contain test code, I will look into if I can create some tests for this next. |
f07c79b to
76938eb
Compare
9387f23 to
44abd76
Compare
|
I force pushed to this, so please pull/reset to the version here on GitHub before you do any further commits or pushes. (Do NOT force push.) |
There was a problem hiding this comment.
1 issue found across 2 files
Confidence score: 3/5
- There is a concrete behavior risk in
editor/src/messages/tool/tool_messages/path_tool.rs: along-path snap dampening uses the snapped point instead of the dragged handle position, which can undermine the intended anti-jitter logic near anchors. - Because this is a medium-severity (6/10) issue with fairly strong confidence (7/10) and directly affects interaction behavior, this carries some user-visible regression risk.
- Pay close attention to
editor/src/messages/tool/tool_messages/path_tool.rs- near-anchor drags may still receive full snap strength, bypassing dampening.
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:1182">
P2: Along-path snap dampening is based on the snapped point instead of the dragged handle position, so near-anchor drags can still apply full snap strength and bypass the new anti-jitter behavior.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| return snap_result; | ||
| } | ||
|
|
||
| let anchor_distance = snap_result.snapped_point_document.distance(anchor_position); |
There was a problem hiding this comment.
P2: Along-path snap dampening is based on the snapped point instead of the dragged handle position, so near-anchor drags can still apply full snap strength and bypass the new anti-jitter behavior.
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 1182:
<comment>Along-path snap dampening is based on the snapped point instead of the dragged handle position, so near-anchor drags can still apply full snap strength and bypass the new anti-jitter behavior.</comment>
<file context>
@@ -1150,11 +1154,47 @@ impl PathToolData {
+ return snap_result;
+ }
+
+ let anchor_distance = snap_result.snapped_point_document.distance(anchor_position);
+ let dampening_start = snap_result.tolerance * SNAP_DAMPENING_START_MULTIPLIER;
+ if anchor_distance >= dampening_start {
</file context>
| let anchor_distance = snap_result.snapped_point_document.distance(anchor_position); | |
| let anchor_distance = new_handle_position.distance(anchor_position); |

If I was editing a path and dragging a handle when the handle moves close to the anchor point then there were some weird jittering issues with the other handle. This occurred only when the anchor is in collinear mode nad the Alpng Path snapping is tuned on. You can see my reproduction video here:
iss2451reproduction.webm
I corrected this by reducing the snap weight when the handle is close to the anchor point (less then 30% of tolerance distance) (and of course we are in collinear mode and the snapping is Along Path) in this way the behavior is similar when there is no snapping.
Here is the video with my fix:
iss2451withmysolution.webm
Closes #2451