Skip to content

Fix snapping edge cases #4511

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
devviktoria wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix snapping edge cases #4511

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
215 changes: 213 additions & 2 deletions editor/src/messages/tool/tool_messages/path_tool.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ use crate::messages::tool::common_functionality::pivot::{PivotGizmo, PivotGizmoT
use crate::messages::tool::common_functionality::shape_editor::{
ClosestSegment, ManipulatorAngle, OpposingHandleLengths, SelectedLayerState, SelectedPointsInfo, SelectionChange, SelectionShape, SelectionShapeType, ShapeState,
};
use crate::messages::tool::common_functionality::snapping::{SnapCache, SnapCandidatePoint, SnapConstraint, SnapData, SnapManager};
use crate::messages::tool::common_functionality::snapping::{SnapCache, SnapCandidatePoint, SnapConstraint, SnapData, SnapManager, SnappedPoint};
use crate::messages::tool::common_functionality::utility_functions::{calculate_segment_angle, find_two_param_best_approximate, make_path_editable_is_allowed};
use graphene_std::Color;
use graphene_std::renderer::Quad;
Expand Down Expand Up @@ -1066,6 +1066,10 @@ impl PathToolData {
snap_angle: bool,
tangent_to_neighboring_tangents: bool,
) -> f64 {
if handle_vector.length_squared() < f64::EPSILON {
return self.angle;
}

let current_angle = -handle_vector.angle_to(DVec2::X);

if let Some((vector, layer)) = shape_editor
Expand Down Expand Up @@ -1142,7 +1146,7 @@ impl PathToolData {
let snap_data = SnapData::new(document, input, viewport);
let snap_point = SnapCandidatePoint::handle_neighbors(new_handle_position, [anchor_position]);

let snap_result = match using_angle_constraints {
let mut snap_result = match using_angle_constraints {
true => {
let snap_constraint = SnapConstraint::Line {
origin: anchor_position,
Expand All @@ -1154,6 +1158,14 @@ impl PathToolData {
false => self.snap_manager.free_snap(&snap_data, &snap_point, Default::default()),
};

//If the snapping result is a non-finite position
if snap_result.distance == f64::INFINITY {

@cubic-dev-ai cubic-dev-ai Bot Sep 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: In the buggy path this guard is meant to fix, the fallback still yields a non-finite point. free_snap/constrained_snap return SnappedPoint::infinite_snap(DVec2::ZERO) (distance = INFINITY) precisely when point.document_point.is_finite() is false, i.e. when new_handle_position itself is non-finite. So replacing the snapped point with snapped_point_document: new_handle_position substitutes NaN/Infinity for the old 0,0 point instead of a usable finite position. Use a known-finite fallback (e.g. handle_position, which produces a zero delta) for the non-finite case, while keeping new_handle_position for the normal no-snap case.

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 1162:

<comment>In the buggy path this guard is meant to fix, the fallback still yields a non-finite point. `free_snap`/`constrained_snap` return `SnappedPoint::infinite_snap(DVec2::ZERO)` (distance = INFINITY) precisely when `point.document_point.is_finite()` is false, i.e. when `new_handle_position` itself is non-finite. So replacing the snapped point with `snapped_point_document: new_handle_position` substitutes NaN/Infinity for the old 0,0 point instead of a usable finite position. Use a known-finite fallback (e.g. `handle_position`, which produces a zero delta) for the non-finite case, while keeping `new_handle_position` for the normal no-snap case.</comment>

<file context>
@@ -1154,6 +1158,14 @@ impl PathToolData {
 		};
 
+		//If the snapping result is a non-finite position
+		if snap_result.distance == f64::INFINITY {
+			snap_result = SnappedPoint {
+				snapped_point_document: new_handle_position,
</file context>

snap_result = SnappedPoint {
snapped_point_document: new_handle_position,
..Default::default()
};
}

self.snap_manager.update_indicator(snap_result.clone());

document.metadata().document_to_viewport.transform_vector2(snap_result.snapped_point_document - handle_position)
Expand Down Expand Up @@ -3661,3 +3673,202 @@ fn update_dynamic_hints(
hint_data.send_layout(responses);
responses.add(ToolMessage::UpdateHints);
}

#[cfg(test)]
mod test_path {
use crate::messages::input_mapper::utility_types::keyboard::ModifierKeys;
use crate::messages::input_mapper::utility_types::pointer::MouseKeys;
use crate::messages::portfolio::document::utility_types::misc::{SNAP_FUNCTIONS_FOR_BOUNDING_BOXES, SNAP_FUNCTIONS_FOR_PATHS};
pub use crate::test_utils::test_prelude::*;
use glam::DAffine2;
use graphene_std::vector::Vector;
use graphene_std::vector::misc::{BezierHandles, segment_to_handles};

async fn prepare_document_for_path_snap_weight(anchor_positions: &[DVec2]) -> EditorTestUtils {
let mut editor = EditorTestUtils::create();
editor.new_document().await;
editor.set_viewport_size(DVec2::splat(-1000.), DVec2::splat(1000.)).await; // Necessary for doing snapping since snaps outside of the viewport are discarded

editor.drag_tool(ToolType::Artboard, 0., 0., 1000., 600., ModifierKeys::empty()).await; // Necessary for doing path snapping without it that path snapping does not work
editor.select_tool(ToolType::Select).await;

// Disable all bounding box snapping
for (_, closure, _) in SNAP_FUNCTIONS_FOR_BOUNDING_BOXES {
editor
.handle_message(DocumentMessage::SetSnapping {
closure: Some(closure),
snapping_state: false,
})
.await;
}

// Disable all path snapping EXCEPT along_path
for (name, closure, _) in SNAP_FUNCTIONS_FOR_PATHS {
let enabled = name == "Along Paths";

editor
.handle_message(DocumentMessage::SetSnapping {
closure: Some(closure),
snapping_state: enabled,
})
.await;
}

//Create Bezier path for testing
editor.drag_tool(ToolType::Pen, anchor_positions[0].x, anchor_positions[0].y, 650., 30., ModifierKeys::empty()).await;
editor.drag_tool(ToolType::Pen, anchor_positions[1].x, anchor_positions[1].y, 370., 420., ModifierKeys::empty()).await;
editor.drag_tool(ToolType::Pen, anchor_positions[2].x, anchor_positions[2].y, 20., 330., ModifierKeys::empty()).await;
editor.press(Key::Enter, ModifierKeys::empty()).await;

assert_eq!(
editor.active_document().metadata().all_layers().count(),
2,
"The document should contain one artboard and one path layers"
);

let (modified_path, layer_to_viewport) = get_path_data(&editor);
assert_anchor_positions(&modified_path, layer_to_viewport, anchor_positions, 1e-10);

let expected_handles: Vec<BezierHandles> = vec![
BezierHandles::Cubic {
handle_start: DVec2::new(700.0, 60.0),
handle_end: DVec2::new(430.0, 180.0),
},
BezierHandles::Cubic {
handle_start: DVec2::new(370.0, 420.0),
handle_end: DVec2::new(80.0, 70.0),
},
];
assert_handle_positions(&modified_path, &expected_handles, layer_to_viewport, 1e-10);

editor
}

fn get_path_data(editor: &EditorTestUtils) -> (Vector, DAffine2) {
let document = editor.active_document();

let path_layer = document.metadata().all_layers().nth(1).expect("Expected path layer");

let modified_path = document.network_interface.compute_modified_vector(path_layer).expect("Vector not found in the path layer");

let layer_to_document = document.metadata().transform_to_document(path_layer);

(modified_path, layer_to_document)
}

fn assert_anchor_positions(vector: &Vector, transform: DAffine2, expected_anchors: &[DVec2], epsilon: f64) {
let anchors_in_viewport: Vec<DVec2> = vector
.point_domain
.ids()
.iter()
.filter_map(|&point_id| {
let pos = vector.point_domain.position_from_id(point_id)?;
Some(transform.transform_point2(pos))
})
.collect();

assert_eq!(anchors_in_viewport.len(), expected_anchors.len(), "Anchor count mismatch");

for (i, expected) in expected_anchors.iter().enumerate() {
let actual = anchors_in_viewport[i];
let distance = (actual - *expected).length();

assert!(distance < epsilon, "Anchor {i} mismatch: expected {expected:?}, got {actual:?}, distance {distance}");
}
}

fn assert_handle_positions(vector: &Vector, expected_handles: &[BezierHandles], transform: DAffine2, epsilon: f64) {
let segment_ids = vector.segment_domain.ids();

assert_eq!(segment_ids.len(), expected_handles.len(), "Segment count mismatch for handles");

for (i, segment_id) in segment_ids.iter().enumerate() {
let segment = vector.segment_from_id(*segment_id).expect("Segment not found");
let segment_handles = segment_to_handles(&segment);
let expected = &expected_handles[i];

match (&segment_handles, expected) {
(BezierHandles::Linear, BezierHandles::Linear) => {
// OK
}

(BezierHandles::Quadratic { handle: actual }, BezierHandles::Quadratic { handle: expected }) => {
let actual_viewport = transform.transform_point2(*actual);

let dist = (actual_viewport - expected).length();

assert!(
dist < epsilon,
"Segment {i} quadratic handle mismatch: expected {:?}, got {:?}, dist {}",
expected,
actual_viewport,
dist
);
}

(
BezierHandles::Cubic {
handle_start: actual_start,
handle_end: actual_end,
},
BezierHandles::Cubic {
handle_start: expected_start,
handle_end: expected_end,
},
) => {
let actual_start_viewport = transform.transform_point2(*actual_start);
let actual_end_viewport = transform.transform_point2(*actual_end);

let dist_start = (actual_start_viewport - expected_start).length();
let dist_end = (actual_end_viewport - expected_end).length();

assert!(
dist_start < epsilon,
"Segment {i} cubic start handle mismatch: expected {:?}, got {:?}, dist {}",
expected_start,
actual_start_viewport,
dist_start
);

assert!(
dist_end < epsilon,
"Segment {i} cubic end handle mismatch: expected {:?}, got {:?}, dist {}",
expected_end,
actual_end_viewport,
dist_end
);
}
// Mismatch case
(actual, expected) => {
panic!("Segment {i} handle type mismatch: actual = {:?}, expected = {:?}", actual, expected);
}
}
}
}

#[tokio::test]
async fn path_move_handle_to_anchor() {
let anchor_positions = [DVec2::new(50., 30.), DVec2::new(400., 300.), DVec2::new(50., 200.)];
let mut editor = prepare_document_for_path_snap_weight(&anchor_positions).await;

editor.click_tool(ToolType::Path, MouseKeys::LEFT, anchor_positions[1], ModifierKeys::empty()).await;
editor.drag_tool(ToolType::Path, 370., 420., anchor_positions[1].x, anchor_positions[1].y, ModifierKeys::empty()).await;
editor.press(Key::Enter, ModifierKeys::empty()).await;

let (modified_path, layer_to_document) = get_path_data(&editor);

assert_anchor_positions(&modified_path, layer_to_document, &anchor_positions, 1e-10);

let expected_handles: Vec<BezierHandles> = vec![
BezierHandles::Cubic {
handle_start: DVec2::new(700.0, 60.0),
handle_end: DVec2::new(430.0, 180.0),
},
BezierHandles::Cubic {
handle_start: DVec2::new(400.0, 300.0),
handle_end: DVec2::new(80.0, 70.0),
},
];
assert_handle_positions(&modified_path, &expected_handles, layer_to_document, 1e-10);
}
}