Fix vector mesh editing behavior in various edge cases (#2943)

* Fix colinear switch behaviour

* Fix Tab to switch handles

* Code review

---------

Co-authored-by: Keavon Chambers <keavon@keavon.com>

editor/src/messages/tool/common_functionality/shape_editor.rs

@@ -15,6 +15,7 @@ use bezier_rs::{Bezier, BezierHandles, Subpath, TValue};
 use glam::{DAffine2, DVec2};
 use graphene_std::vector::{HandleExt, HandleId, SegmentId};
 use graphene_std::vector::{ManipulatorPointId, PointId, VectorData, VectorModificationType};
+use std::f64::consts::TAU;
 
 #[derive(Debug, Copy, Clone, PartialEq, Eq)]
 pub enum SelectionChange {
@@ -892,16 +893,20 @@ impl ShapeState {
 			ManipulatorPointId::Anchor(point) => self.move_anchor(point, &vector_data, delta, layer, None, responses),
 			ManipulatorPointId::PrimaryHandle(segment) => {
 				self.move_primary(segment, delta, layer, responses);
-				if let Some(handles) = point.get_handle_pair(&vector_data) {
+				if let Some(handle) = point.as_handle() {
-					let modification_type = VectorModificationType::SetG1Continuous { handles, enabled: false };
+					if let Some(handles) = vector_data.colinear_manipulators.iter().find(|handles| handles[0] == handle || handles[1] == handle) {
-					responses.add(GraphOperationMessage::Vector { layer, modification_type });
+						let modification_type = VectorModificationType::SetG1Continuous { handles: *handles, enabled: false };
+						responses.add(GraphOperationMessage::Vector { layer, modification_type });
+					}
 				}
 			}
 			ManipulatorPointId::EndHandle(segment) => {
 				self.move_end(segment, delta, layer, responses);
-				if let Some(handles) = point.get_handle_pair(&vector_data) {
+				if let Some(handle) = point.as_handle() {
-					let modification_type = VectorModificationType::SetG1Continuous { handles, enabled: false };
+					if let Some(handles) = vector_data.colinear_manipulators.iter().find(|handles| handles[0] == handle || handles[1] == handle) {
-					responses.add(GraphOperationMessage::Vector { layer, modification_type });
+						let modification_type = VectorModificationType::SetG1Continuous { handles: *handles, enabled: false };
+						responses.add(GraphOperationMessage::Vector { layer, modification_type });
+					}
 				}
 			}
 		}
@@ -1027,6 +1032,9 @@ impl ShapeState {
 	/// If only one handle is selected, the other handle will be moved to match the angle of the selected handle.
 	/// If both or neither handles are selected, the angle of both handles will be averaged from their current angles, weighted by their lengths.
 	/// Assumes all selected manipulators have handles that are already not colinear.
+	///
+	/// For vector meshes, the non-colinear handle which is nearest in the direction of 180° angle separation becomes colinear with current handle.
+	/// If there is no such handle, nothing happens.
 	pub fn convert_selected_manipulators_to_colinear_handles(&self, responses: &mut VecDeque<Message>, document: &DocumentMessageHandler) {
 		let mut skip_set = HashSet::new();
 
@@ -1037,7 +1045,55 @@ impl ShapeState {
 			let transform = document.metadata().transform_to_document_if_feeds(layer, &document.network_interface);
 
 			for &point in layer_state.selected_points.iter() {
-				let Some(handles) = point.get_handle_pair(&vector_data) else { continue };
+				// Skip a point which has more than 2 segments connected (vector meshes)
+				if let ManipulatorPointId::Anchor(anchor) = point {
+					if vector_data.all_connected(anchor).count() > 2 {
+						continue;
+					}
+				}
+
+				// Here we take handles as the current handle and the most opposite non-colinear-handle
+
+				let is_handle_colinear = |handle: HandleId| -> bool { vector_data.colinear_manipulators.iter().any(|&handles| handles[0] == handle || handles[1] == handle) };
+
+				let other_handles = if matches!(point, ManipulatorPointId::Anchor(_)) {
+					point.get_handle_pair(&vector_data)
+				} else {
+					point.get_all_connected_handles(&vector_data).and_then(|handles| {
+						let mut non_colinear_handles = handles.iter().filter(|&handle| !is_handle_colinear(*handle)).clone().collect::<Vec<_>>();
+
+						// Sort these by angle from the current handle
+						non_colinear_handles.sort_by(|&handle_a, &handle_b| {
+							let anchor = point.get_anchor_position(&vector_data).expect("No anchor position for handle");
+							let orig_handle_pos = point.get_position(&vector_data).expect("No handle position");
+
+							let a_pos = handle_a.to_manipulator_point().get_position(&vector_data).expect("No handle position");
+							let b_pos = handle_b.to_manipulator_point().get_position(&vector_data).expect("No handle position");
+
+							let v_orig = (orig_handle_pos - anchor).normalize_or_zero();
+
+							let v_a = (a_pos - anchor).normalize_or_zero();
+							let v_b = (b_pos - anchor).normalize_or_zero();
+
+							let angle_a = v_orig.angle_to(v_a).abs();
+							let angle_b = v_orig.angle_to(v_b).abs();
+
+							// Sort by descending angle (180° is furthest)
+							angle_b.partial_cmp(&angle_a).unwrap_or(std::cmp::Ordering::Equal)
+						});
+
+						let current = match point {
+							ManipulatorPointId::EndHandle(segment) => HandleId::end(segment),
+							ManipulatorPointId::PrimaryHandle(segment) => HandleId::primary(segment),
+							ManipulatorPointId::Anchor(_) => unreachable!(),
+						};
+
+						non_colinear_handles.first().map(|other| [current, **other])
+					})
+				};
+
+				let Some(handles) = other_handles else { continue };
+
 				if skip_set.contains(&handles) || skip_set.contains(&[handles[1], handles[0]]) {
 					continue;
 				};
@@ -1317,7 +1373,7 @@ impl ShapeState {
 				match point {
 					ManipulatorPointId::Anchor(anchor) => {
 						if let Some(handles) = Self::dissolve_anchor(anchor, responses, layer, &vector_data) {
-							if !vector_data.all_connected(anchor).any(|a| selected_segments.contains(&a.segment)) {
+							if !vector_data.all_connected(anchor).any(|a| selected_segments.contains(&a.segment)) && vector_data.all_connected(anchor).count() <= 2 {
 								missing_anchors.insert(anchor, handles);
 							}
 						}
@@ -1496,21 +1552,21 @@ impl ShapeState {
 	/// Disable colinear handles colinear.
 	pub fn disable_colinear_handles_state_on_selected(&self, network_interface: &NodeNetworkInterface, responses: &mut VecDeque<Message>) {
 		for (&layer, state) in &self.selected_shape_state {
-			let Some(vector_data) = network_interface.compute_modified_vector(layer) else {
+			let Some(vector_data) = network_interface.compute_modified_vector(layer) else { continue };
-				continue;
-			};
 
 			for &point in &state.selected_points {
 				if let ManipulatorPointId::Anchor(point) = point {
 					for connected in vector_data.all_connected(point) {
-						if let Some(&handles) = vector_data.colinear_manipulators.iter().find(|target| target.iter().any(|&target| target == connected)) {
+						if let Some(&handles) = vector_data.colinear_manipulators.iter().find(|target| target.contains(&connected)) {
 							let modification_type = VectorModificationType::SetG1Continuous { handles, enabled: false };
 							responses.add(GraphOperationMessage::Vector { layer, modification_type });
 						}
 					}
-				} else if let Some(handles) = point.get_handle_pair(&vector_data) {
+				} else if let Some(handle) = point.as_handle() {
-					let modification_type = VectorModificationType::SetG1Continuous { handles, enabled: false };
+					if let Some(handles) = vector_data.colinear_manipulators.iter().find(|handles| handles[0] == handle || handles[1] == handle) {
-					responses.add(GraphOperationMessage::Vector { layer, modification_type });
+						let modification_type = VectorModificationType::SetG1Continuous { handles: *handles, enabled: false };
+						responses.add(GraphOperationMessage::Vector { layer, modification_type });
+					}
 				}
 			}
 		}
@@ -1720,9 +1776,40 @@ impl ShapeState {
 				if point.as_anchor().is_some() {
 					continue;
 				}
-				if let Some(handles) = point.get_handle_pair(&vector_data) {
+
-					// handle[0] is selected, handle[1] is opposite / mirror handle
+				if let Some(other_handles) = point.get_all_connected_handles(&vector_data) {
-					handles_to_update.push((layer, handles[0].to_manipulator_point(), handles[1].to_manipulator_point()));
+					// Find the next closest handle in the clockwise sense
+					let mut candidates = other_handles.clone();
+					candidates.sort_by(|&handle_a, &handle_b| {
+						let anchor = point.get_anchor_position(&vector_data).expect("No anchor position for handle");
+						let orig_handle_pos = point.get_position(&vector_data).expect("No handle position");
+
+						let a_pos = handle_a.to_manipulator_point().get_position(&vector_data).expect("No handle position");
+						let b_pos = handle_b.to_manipulator_point().get_position(&vector_data).expect("No handle position");
+
+						let v_orig = (orig_handle_pos - anchor).normalize_or_zero();
+
+						let v_a = (a_pos - anchor).normalize_or_zero();
+						let v_b = (b_pos - anchor).normalize_or_zero();
+
+						let signed_angle = |base: DVec2, to: DVec2| -> f64 {
+							let angle = base.angle_to(to);
+							let cross = base.perp_dot(to);
+
+							if cross < 0. { TAU - angle } else { angle }
+						};
+
+						let angle_a = signed_angle(v_orig, v_a);
+						let angle_b = signed_angle(v_orig, v_b);
+
+						angle_a.partial_cmp(&angle_b).unwrap_or(std::cmp::Ordering::Equal)
+					});
+
+					if candidates.is_empty() {
+						continue;
+					}
+
+					handles_to_update.push((layer, *point, candidates[0].to_manipulator_point()));
 				}
 			}
 		}

editor/src/messages/tool/tool_messages/path_tool.rs

@@ -583,7 +583,12 @@ impl PathToolData {
 			SelectionStatus::None => false,
 			SelectionStatus::One(single_selected_point) => {
 				let vector_data = document.network_interface.compute_modified_vector(single_selected_point.layer).unwrap();
-				single_selected_point.id.get_handle_pair(&vector_data).is_some()
+				if single_selected_point.id.get_handle_pair(&vector_data).is_some() {
+					let anchor = single_selected_point.id.get_anchor(&vector_data).expect("Cannot find connected anchor");
+					vector_data.all_connected(anchor).count() <= 2
+				} else {
+					false
+				}
 			}
 			SelectionStatus::Multiple(_) => true,
 		};
@@ -600,7 +605,7 @@ impl PathToolData {
 	}
 
 	fn next_drill_through_cycle(&mut self, position: DVec2) -> usize {
-		if self.last_drill_through_click_position.map_or(true, |last_pos| last_pos.distance(position) > DRILL_THROUGH_THRESHOLD) {
+		if self.last_drill_through_click_position.is_none_or(|last_pos| last_pos.distance(position) > DRILL_THROUGH_THRESHOLD) {
 			// New position, reset cycle
 			self.drill_through_cycle_index = 0;
 		} else {
@@ -620,7 +625,7 @@ impl PathToolData {
 	}
 
 	fn has_drill_through_mouse_moved(&self, position: DVec2) -> bool {
-		self.last_drill_through_click_position.map_or(true, |last_pos| last_pos.distance(position) > DRILL_THROUGH_THRESHOLD)
+		self.last_drill_through_click_position.is_none_or(|last_pos| last_pos.distance(position) > DRILL_THROUGH_THRESHOLD)
 	}
 
 	fn set_ghost_outline(&mut self, shape_editor: &ShapeState, document: &DocumentMessageHandler) {
@@ -2406,15 +2411,15 @@ impl Fsm for PathToolFsmState {
 
 					let compatible_type = first_layer.and_then(|layer| {
 						let graph_layer = graph_modification_utils::NodeGraphLayer::new(layer, &document.network_interface);
-						graph_layer.horizontal_layer_flow().nth(1).and_then(|node_id| {
+						graph_layer.horizontal_layer_flow().nth(1).map(|node_id| {
 							let (output_type, _) = document.network_interface.output_type(&node_id, 0, &[]);
-							Some(format!("type:{}", output_type.nested_type()))
+							format!("type:{}", output_type.nested_type())
 						})
 					});
 
 					let is_compatible = compatible_type.as_deref() == Some("type:Instances<VectorData>");
 
-					let is_modifiable = first_layer.map_or(false, |layer| {
+					let is_modifiable = first_layer.is_some_and(|layer| {
 						let graph_layer = graph_modification_utils::NodeGraphLayer::new(layer, &document.network_interface);
 						matches!(graph_layer.find_input("Path", 1), Some(TaggedValue::VectorModification(_)))
 					});

node-graph/gcore/src/vector/vector_data.rs

@@ -561,6 +561,30 @@ impl ManipulatorPointId {
 		}
 	}
 
+	/// Finds all the connected handles of a point.
+	/// For an anchor it is all the connected handles.
+	/// For a handle it is all the handles connected to its corresponding anchor other than the current handle.
+	pub fn get_all_connected_handles(self, vector_data: &VectorData) -> Option<Vec<HandleId>> {
+		match self {
+			ManipulatorPointId::Anchor(point) => {
+				let connected = vector_data.all_connected(point).collect::<Vec<_>>();
+				Some(connected)
+			}
+			ManipulatorPointId::PrimaryHandle(segment) => {
+				let point = vector_data.segment_domain.segment_start_from_id(segment)?;
+				let current = HandleId::primary(segment);
+				let connected = vector_data.segment_domain.all_connected(point).filter(|&value| value != current).collect::<Vec<_>>();
+				Some(connected)
+			}
+			ManipulatorPointId::EndHandle(segment) => {
+				let point = vector_data.segment_domain.segment_end_from_id(segment)?;
+				let current = HandleId::end(segment);
+				let connected = vector_data.segment_domain.all_connected(point).filter(|&value| value != current).collect::<Vec<_>>();
+				Some(connected)
+			}
+		}
+	}
+
 	/// Attempt to find the closest anchor. If self is already an anchor then it is just self. If it is a start or end handle, then the start or end point is chosen.
 	#[must_use]
 	pub fn get_anchor(self, vector_data: &VectorData) -> Option<PointId> {