From e8356e6efa723815ea290969a3c5c77b4ae9acfa Mon Sep 17 00:00:00 2001 From: Shubh Pingale <135618936+shubhtrek@users.noreply.github.com> Date: Mon, 12 Oct 2026 18:53:09 +0530 Subject: [PATCH 1/5] Fix #4618: Prevent colinear handles when extending from points connected to multiple segments --- editor/src/messages/tool/tool_messages/pen_tool.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/editor/src/messages/tool/tool_messages/pen_tool.rs b/editor/src/messages/tool/tool_messages/pen_tool.rs index f7fe4ab7c84..16b6b9231b8 100644 --- a/editor/src/messages/tool/tool_messages/pen_tool.rs +++ b/editor/src/messages/tool/tool_messages/pen_tool.rs @@ -1368,7 +1368,9 @@ impl PenToolData { /// Perform extension of an existing path fn extend_existing_path(&mut self, document: &DocumentMessageHandler, layer: LayerNodeIdentifier, point: PointId, position: DVec2) { let vector: Option = document.network_interface.compute_modified_vector(layer); - let (handle_start, in_segment) = if let Some(vector) = &vector { + let (handle_start, in_segment) = if let Some(vector) = &vector + && vector.connected_count(point) <= 1 + { vector .segment_iter() .find_map(|(segment_id, bezier, start, end)| { From 9f444bc9640441399709bff9c15e11f9d731f0a6 Mon Sep 17 00:00:00 2001 From: Shubh Pingale <135618936+shubhtrek@users.noreply.github.com> Date: Wed, 14 Oct 2026 09:25:07 +0530 Subject: [PATCH 2/5] Add test for extending from points connected to multiple segments --- .../messages/tool/tool_messages/pen_tool.rs | 73 +++++++++++++++++++ 1 file changed, 73 insertions(+) diff --git a/editor/src/messages/tool/tool_messages/pen_tool.rs b/editor/src/messages/tool/tool_messages/pen_tool.rs index 16b6b9231b8..df501665bed 100644 --- a/editor/src/messages/tool/tool_messages/pen_tool.rs +++ b/editor/src/messages/tool/tool_messages/pen_tool.rs @@ -3004,4 +3004,77 @@ mod test_pen_tool { let incoming = editor.active_document().metadata().transform_to_viewport(layer).transform_point2(incoming); assert!(incoming.distance(corner + anchor - handle) < 1e-6, "the incoming handle is at {incoming}"); } + + #[tokio::test] + async fn extending_from_anchor_with_multiple_segments_does_not_create_colinear_handle() { + let mut editor = EditorTestUtils::create(); + editor.new_document().await; + + // 1. Draw a path through A, B, and C with curved handles dragged out at B + click_pen(&mut editor, A).await; + let handle = B + DVec2::new(50., 0.); + editor.drag_path(&[B, handle], ModifierKeys::empty()).await; + click_pen(&mut editor, C).await; + editor.press(Key::Enter, ModifierKeys::empty()).await; + assert_path_shape(&editor, 3, 2); + + // 2. Extend from anchor B to D by clicking on B and then D (without dragging) + click_pen(&mut editor, B).await; + click_pen(&mut editor, D).await; + editor.press(Key::Enter, ModifierKeys::empty()).await; + assert_path_shape(&editor, 4, 3); + + // 3. Verify the new branch leaving B does not create a mirrored/colinear handle + let (layer, vector) = drawn_path(&editor).expect("Expected a drawn path"); + let layer_to_viewport = editor.active_document().metadata().transform_to_viewport(layer); + + let b_id = vector + .point_domain + .ids() + .iter() + .copied() + .find(|&id| { + let pos = vector.point_domain.position_from_id(id).unwrap(); + layer_to_viewport.transform_point2(pos).distance(B) < 1e-6 + }) + .expect("Anchor B should exist"); + + let d_id = vector + .point_domain + .ids() + .iter() + .copied() + .find(|&id| { + let pos = vector.point_domain.position_from_id(id).unwrap(); + layer_to_viewport.transform_point2(pos).distance(D) < 1e-6 + }) + .expect("Anchor D should exist"); + + let bd_segment = vector + .segment_domain + .ids() + .iter() + .copied() + .find(|&segment| { + let start = vector.segment_start_from_id(segment); + let end = vector.segment_end_from_id(segment); + (start == Some(b_id) && end == Some(d_id)) || (start == Some(d_id) && end == Some(b_id)) + }) + .expect("Segment between B and D should exist"); + + let handle_at_b = if vector.segment_start_from_id(bd_segment) == Some(b_id) { + ManipulatorPointId::PrimaryHandle(bd_segment).get_position(&vector) + } else { + ManipulatorPointId::EndHandle(bd_segment).get_position(&vector) + }; + + let mirrored_handle = 2. * B - handle; + if let Some(handle_pos) = handle_at_b { + let handle_pos = layer_to_viewport.transform_point2(handle_pos); + assert!( + handle_pos.distance(mirrored_handle) > 1e-4, + "Handle at B for the new segment must not be mirrored into a colinear handle" + ); + } + } } From 9efed51d8bfd08f3680943783a21c113c9a27b9c Mon Sep 17 00:00:00 2001 From: Shubh Pingale <135618936+shubhtrek@users.noreply.github.com> Date: Wed, 14 Oct 2026 09:39:49 +0530 Subject: [PATCH 3/5] Assert new segment handle remains at the anchor --- editor/src/messages/tool/tool_messages/pen_tool.rs | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/editor/src/messages/tool/tool_messages/pen_tool.rs b/editor/src/messages/tool/tool_messages/pen_tool.rs index df501665bed..fa241c2df82 100644 --- a/editor/src/messages/tool/tool_messages/pen_tool.rs +++ b/editor/src/messages/tool/tool_messages/pen_tool.rs @@ -3068,13 +3068,9 @@ mod test_pen_tool { ManipulatorPointId::EndHandle(bd_segment).get_position(&vector) }; - let mirrored_handle = 2. * B - handle; if let Some(handle_pos) = handle_at_b { let handle_pos = layer_to_viewport.transform_point2(handle_pos); - assert!( - handle_pos.distance(mirrored_handle) > 1e-4, - "Handle at B for the new segment must not be mirrored into a colinear handle" - ); + assert!(handle_pos.distance(B) < 1e-4, "Handle at B for the new segment should remain at the anchor"); } } } From 49412afedfc5bab64b07359fcc78f4c1344d309f Mon Sep 17 00:00:00 2001 From: Shubh Pingale <135618936+shubhtrek@users.noreply.github.com> Date: Wed, 14 Oct 2026 14:10:07 +0530 Subject: [PATCH 4/5] Extract get_anchor_near and get_segment_between helpers; add single-segment colinear handle test Address review nits from 0HyperCube: - Extract repeated point-lookup into a reusable get_anchor_near helper - Extract repeated segment-lookup into a reusable get_segment_between helper - Add test verifying single-segment anchors preserve colinear handles --- .../messages/tool/tool_messages/pen_tool.rs | 110 ++++++++++++------ 1 file changed, 76 insertions(+), 34 deletions(-) diff --git a/editor/src/messages/tool/tool_messages/pen_tool.rs b/editor/src/messages/tool/tool_messages/pen_tool.rs index fa241c2df82..7ea9a561bb8 100644 --- a/editor/src/messages/tool/tool_messages/pen_tool.rs +++ b/editor/src/messages/tool/tool_messages/pen_tool.rs @@ -2397,7 +2397,7 @@ mod test_pen_tool { use crate::test_utils::test_prelude::*; use glam::DAffine2; use graphene_std::vector::misc::ManipulatorPointId; - use graphene_std::vector::{SegmentId, Vector}; + use graphene_std::vector::{PointId, SegmentId, Vector}; /// The single Pen-drawn path layer and its vector, or `None` once the layer has been undone away. fn drawn_path(editor: &EditorTestUtils) -> Option<(LayerNodeIdentifier, Vector)> { @@ -2972,6 +2972,35 @@ mod test_pen_tool { assert!(leaving.distance(handle) < 1e-6, "the joining segment's handle is at {leaving}"); } + /// Finds the point ID whose viewport position is within tolerance of `target`. + fn get_anchor_near(vector: &Vector, layer_to_viewport: DAffine2, target: DVec2) -> PointId { + vector + .point_domain + .ids() + .iter() + .copied() + .find(|&id| { + let pos = vector.point_domain.position_from_id(id).unwrap(); + layer_to_viewport.transform_point2(pos).distance(target) < 1e-6 + }) + .unwrap_or_else(|| panic!("No anchor found near {target}")) + } + + /// Finds the segment that connects two given point IDs (in either direction). + fn get_segment_between(vector: &Vector, point_a: PointId, point_b: PointId) -> SegmentId { + vector + .segment_domain + .ids() + .iter() + .copied() + .find(|&segment| { + let start = vector.segment_start_from_id(segment); + let end = vector.segment_end_from_id(segment); + (start == Some(point_a) && end == Some(point_b)) || (start == Some(point_b) && end == Some(point_a)) + }) + .unwrap_or_else(|| panic!("No segment found between the given anchors")) + } + #[tokio::test] async fn an_anchor_moved_with_space_lands_on_a_corner_and_carries_its_handles_along() { let mut editor = EditorTestUtils::create(); @@ -3028,39 +3057,9 @@ mod test_pen_tool { let (layer, vector) = drawn_path(&editor).expect("Expected a drawn path"); let layer_to_viewport = editor.active_document().metadata().transform_to_viewport(layer); - let b_id = vector - .point_domain - .ids() - .iter() - .copied() - .find(|&id| { - let pos = vector.point_domain.position_from_id(id).unwrap(); - layer_to_viewport.transform_point2(pos).distance(B) < 1e-6 - }) - .expect("Anchor B should exist"); - - let d_id = vector - .point_domain - .ids() - .iter() - .copied() - .find(|&id| { - let pos = vector.point_domain.position_from_id(id).unwrap(); - layer_to_viewport.transform_point2(pos).distance(D) < 1e-6 - }) - .expect("Anchor D should exist"); - - let bd_segment = vector - .segment_domain - .ids() - .iter() - .copied() - .find(|&segment| { - let start = vector.segment_start_from_id(segment); - let end = vector.segment_end_from_id(segment); - (start == Some(b_id) && end == Some(d_id)) || (start == Some(d_id) && end == Some(b_id)) - }) - .expect("Segment between B and D should exist"); + let b_id = get_anchor_near(&vector, layer_to_viewport, B); + let d_id = get_anchor_near(&vector, layer_to_viewport, D); + let bd_segment = get_segment_between(&vector, b_id, d_id); let handle_at_b = if vector.segment_start_from_id(bd_segment) == Some(b_id) { ManipulatorPointId::PrimaryHandle(bd_segment).get_position(&vector) @@ -3073,4 +3072,47 @@ mod test_pen_tool { assert!(handle_pos.distance(B) < 1e-4, "Handle at B for the new segment should remain at the anchor"); } } + + #[tokio::test] + async fn extending_from_anchor_with_single_segment_uses_colinear_handle() { + let mut editor = EditorTestUtils::create(); + editor.new_document().await; + + // 1. Draw a single segment A→B with a curved handle dragged out at B + click_pen(&mut editor, A).await; + let handle = B + DVec2::new(50., 0.); + editor.drag_path(&[B, handle], ModifierKeys::empty()).await; + editor.press(Key::Enter, ModifierKeys::empty()).await; + assert_path_shape(&editor, 2, 1); + + // 2. Extend from B to C by clicking B then C (only one segment is attached to B) + click_pen(&mut editor, B).await; + click_pen(&mut editor, C).await; + editor.press(Key::Enter, ModifierKeys::empty()).await; + assert_path_shape(&editor, 3, 2); + + // 3. Verify the new segment leaving B carries a mirrored/colinear handle + let (layer, vector) = drawn_path(&editor).expect("Expected a drawn path"); + let layer_to_viewport = editor.active_document().metadata().transform_to_viewport(layer); + + let b_id = get_anchor_near(&vector, layer_to_viewport, B); + let c_id = get_anchor_near(&vector, layer_to_viewport, C); + let bc_segment = get_segment_between(&vector, b_id, c_id); + + let handle_at_b = if vector.segment_start_from_id(bc_segment) == Some(b_id) { + ManipulatorPointId::PrimaryHandle(bc_segment).get_position(&vector) + } else { + ManipulatorPointId::EndHandle(bc_segment).get_position(&vector) + }; + + // When only one segment is connected, the handle should NOT collapse to the + // anchor — it should carry a meaningful colinear handle away from B. + let handle_pos = handle_at_b.expect("The new segment should have a handle at B"); + let handle_pos = layer_to_viewport.transform_point2(handle_pos); + assert!( + handle_pos.distance(B) > 1e-4, + "Handle at B should not collapse to the anchor when only one segment is connected (got {handle_pos})" + ); + } } + From 69d3b465d62e5c1a3d611ab48ffb62729cf6571f Mon Sep 17 00:00:00 2001 From: Shubh Pingale <135618936+shubhtrek@users.noreply.github.com> Date: Wed, 14 Oct 2026 14:29:53 +0530 Subject: [PATCH 5/5] Verify colinearity with is_almost_colinear and add get_handle_at helper --- .../messages/tool/tool_messages/pen_tool.rs | 45 +++++++++++-------- 1 file changed, 26 insertions(+), 19 deletions(-) diff --git a/editor/src/messages/tool/tool_messages/pen_tool.rs b/editor/src/messages/tool/tool_messages/pen_tool.rs index 7ea9a561bb8..418a07a2ef3 100644 --- a/editor/src/messages/tool/tool_messages/pen_tool.rs +++ b/editor/src/messages/tool/tool_messages/pen_tool.rs @@ -2391,6 +2391,7 @@ impl Fsm for PenToolFsmState { #[cfg(test)] mod test_pen_tool { + use super::is_almost_colinear; use crate::messages::input_mapper::utility_types::pointer::EditorPointerState; use crate::messages::portfolio::document::graph_operation::utility_types::TransformIn; use crate::messages::tool::common_functionality::snap_session::WAKE_DWELL_ON_APPROACH; @@ -3001,6 +3002,15 @@ mod test_pen_tool { .unwrap_or_else(|| panic!("No segment found between the given anchors")) } + /// Finds the handle position on `segment` that corresponds to `point`. + fn get_handle_at(vector: &Vector, segment: SegmentId, point: PointId) -> Option { + if vector.segment_start_from_id(segment) == Some(point) { + ManipulatorPointId::PrimaryHandle(segment).get_position(vector) + } else { + ManipulatorPointId::EndHandle(segment).get_position(vector) + } + } + #[tokio::test] async fn an_anchor_moved_with_space_lands_on_a_corner_and_carries_its_handles_along() { let mut editor = EditorTestUtils::create(); @@ -3061,13 +3071,7 @@ mod test_pen_tool { let d_id = get_anchor_near(&vector, layer_to_viewport, D); let bd_segment = get_segment_between(&vector, b_id, d_id); - let handle_at_b = if vector.segment_start_from_id(bd_segment) == Some(b_id) { - ManipulatorPointId::PrimaryHandle(bd_segment).get_position(&vector) - } else { - ManipulatorPointId::EndHandle(bd_segment).get_position(&vector) - }; - - if let Some(handle_pos) = handle_at_b { + if let Some(handle_pos) = get_handle_at(&vector, bd_segment, b_id) { let handle_pos = layer_to_viewport.transform_point2(handle_pos); assert!(handle_pos.distance(B) < 1e-4, "Handle at B for the new segment should remain at the anchor"); } @@ -3099,20 +3103,23 @@ mod test_pen_tool { let c_id = get_anchor_near(&vector, layer_to_viewport, C); let bc_segment = get_segment_between(&vector, b_id, c_id); - let handle_at_b = if vector.segment_start_from_id(bc_segment) == Some(b_id) { - ManipulatorPointId::PrimaryHandle(bc_segment).get_position(&vector) - } else { - ManipulatorPointId::EndHandle(bc_segment).get_position(&vector) - }; + let a_id = get_anchor_near(&vector, layer_to_viewport, A); + let ab_segment = get_segment_between(&vector, a_id, b_id); + let existing_handle_at_b = get_handle_at(&vector, ab_segment, b_id).expect("Segment AB should have a handle at B"); + let existing_handle_pos = layer_to_viewport.transform_point2(existing_handle_at_b); + + let new_handle_at_b = get_handle_at(&vector, bc_segment, b_id).expect("The new segment should have a handle at B"); + let new_handle_pos = layer_to_viewport.transform_point2(new_handle_at_b); - // When only one segment is connected, the handle should NOT collapse to the - // anchor — it should carry a meaningful colinear handle away from B. - let handle_pos = handle_at_b.expect("The new segment should have a handle at B"); - let handle_pos = layer_to_viewport.transform_point2(handle_pos); + // When only one segment is connected, the handle should not collapse to the + // anchor, and must be colinear with the existing handle at B. assert!( - handle_pos.distance(B) > 1e-4, - "Handle at B should not collapse to the anchor when only one segment is connected (got {handle_pos})" + new_handle_pos.distance(B) > 1e-4, + "Handle at B should not collapse to the anchor when only one segment is connected (got {new_handle_pos})" + ); + assert!( + is_almost_colinear(B, existing_handle_pos, new_handle_pos), + "New handle should be colinear with the existing handle at B (existing: {existing_handle_pos}, new: {new_handle_pos})" ); } } -