Improve the Path tool's segment editing mode and make hovering manipulators react contextually (#2860)

* Improve path editing mode

* Code review

* Tidy up UI

* Update path selection behaviour

* Fix linting

* Remove frozen flag

* Code review

* Fix segment split

* Fix deleting segments

* Add requred methods in vello overlay context

---------

Co-authored-by: Keavon Chambers <[email protected]>

Author: 4adex

editor/src/consts.rs

@@ -106,6 +106,7 @@ pub const HIDE_HANDLE_DISTANCE: f64 = 3.;
 pub const HANDLE_ROTATE_SNAP_ANGLE: f64 = 15.;
 pub const SEGMENT_INSERTION_DISTANCE: f64 = 5.;
 pub const SEGMENT_OVERLAY_SIZE: f64 = 10.;
+pub const SEGMENT_SELECTED_THICKNESS: f64 = 3.;
 pub const HANDLE_LENGTH_FACTOR: f64 = 0.5;
 
 // PEN TOOL

editor/src/messages/input_mapper/input_mappings.rs

@@ -212,13 +212,13 @@ pub fn input_mappings() -> Mapping {
 		entry!(KeyDown(Delete); modifiers=[Shift], action_dispatch=PathToolMessage::BreakPath),
 		entry!(KeyDown(Backspace); modifiers=[Shift], action_dispatch=PathToolMessage::BreakPath),
 		entry!(KeyDownNoRepeat(Tab); action_dispatch=PathToolMessage::SwapSelectedHandles),
-		entry!(KeyDown(MouseLeft); action_dispatch=PathToolMessage::MouseDown { extend_selection: Shift, lasso_select: Control, handle_drag_from_anchor: Alt, drag_restore_handle: Control, molding_in_segment_edit: KeyA }),
+		entry!(KeyDown(MouseLeft); action_dispatch=PathToolMessage::MouseDown { extend_selection: Shift, lasso_select: Control, handle_drag_from_anchor: Alt, drag_restore_handle: Control, segment_editing_modifier: Control }),
 		entry!(KeyDown(MouseRight); action_dispatch=PathToolMessage::RightClick),
 		entry!(KeyDown(Escape); action_dispatch=PathToolMessage::Escape),
 		entry!(KeyDown(KeyG); action_dispatch=PathToolMessage::GRS { key: KeyG }),
 		entry!(KeyDown(KeyR); action_dispatch=PathToolMessage::GRS { key: KeyR }),
 		entry!(KeyDown(KeyS); action_dispatch=PathToolMessage::GRS { key: KeyS }),
-		entry!(PointerMove; refresh_keys=[KeyC, Space, Control, Shift, Alt], action_dispatch=PathToolMessage::PointerMove { toggle_colinear: KeyC, equidistant: Alt, move_anchor_with_handles: Space, snap_angle: Shift, lock_angle: Control, delete_segment: Alt, break_colinear_molding: Alt }),
+		entry!(PointerMove; refresh_keys=[KeyC, Space, Control, Shift, Alt], action_dispatch=PathToolMessage::PointerMove { toggle_colinear: KeyC, equidistant: Alt, move_anchor_with_handles: Space, snap_angle: Shift, lock_angle: Control, delete_segment: Alt, break_colinear_molding: Alt, segment_editing_modifier: Control }),
 		entry!(KeyDown(Delete); action_dispatch=PathToolMessage::Delete),
 		entry!(KeyDown(KeyA); modifiers=[Accel], action_dispatch=PathToolMessage::SelectAllAnchors),
 		entry!(KeyDown(KeyA); modifiers=[Accel, Shift], canonical, action_dispatch=PathToolMessage::DeselectAllPoints),

editor/src/messages/portfolio/document/overlays/utility_types.rs

@@ -2,7 +2,7 @@ use super::utility_functions::overlay_canvas_context;
 use crate::consts::{
 	ARC_SWEEP_GIZMO_RADIUS, COLOR_OVERLAY_BLUE, COLOR_OVERLAY_BLUE_50, COLOR_OVERLAY_GREEN, COLOR_OVERLAY_RED, COLOR_OVERLAY_WHITE, COLOR_OVERLAY_YELLOW, COLOR_OVERLAY_YELLOW_DULL,
 	COMPASS_ROSE_ARROW_SIZE, COMPASS_ROSE_HOVER_RING_DIAMETER, COMPASS_ROSE_MAIN_RING_DIAMETER, COMPASS_ROSE_RING_INNER_DIAMETER, DOWEL_PIN_RADIUS, MANIPULATOR_GROUP_MARKER_SIZE,
-	PIVOT_CROSSHAIR_LENGTH, PIVOT_CROSSHAIR_THICKNESS, PIVOT_DIAMETER,
+	PIVOT_CROSSHAIR_LENGTH, PIVOT_CROSSHAIR_THICKNESS, PIVOT_DIAMETER, SEGMENT_SELECTED_THICKNESS,
 };
 use crate::messages::prelude::Message;
 use bezier_rs::{Bezier, Subpath};
@@ -460,6 +460,42 @@ impl OverlayContext {
 		self.square(position, None, Some(color_fill), Some(color_stroke));
 	}
 
+	pub fn hover_manipulator_handle(&mut self, position: DVec2, selected: bool) {
+		self.start_dpi_aware_transform();
+
+		let position = position.round() - DVec2::splat(0.5);
+
+		self.render_context.begin_path();
+		self.render_context
+			.arc(position.x, position.y, (MANIPULATOR_GROUP_MARKER_SIZE + 2.) / 2., 0., TAU)
+			.expect("Failed to draw the circle");
+
+		self.render_context.set_fill_style_str(COLOR_OVERLAY_BLUE_50);
+		self.render_context.set_stroke_style_str(COLOR_OVERLAY_BLUE_50);
+		self.render_context.fill();
+		self.render_context.stroke();
+
+		self.render_context.begin_path();
+		self.render_context
+			.arc(position.x, position.y, MANIPULATOR_GROUP_MARKER_SIZE / 2., 0., TAU)
+			.expect("Failed to draw the circle");
+
+		let color_fill = if selected { COLOR_OVERLAY_BLUE } else { COLOR_OVERLAY_WHITE };
+
+		self.render_context.set_fill_style_str(color_fill);
+		self.render_context.set_stroke_style_str(COLOR_OVERLAY_BLUE);
+		self.render_context.fill();
+		self.render_context.stroke();
+
+		self.end_dpi_aware_transform();
+	}
+
+	pub fn hover_manipulator_anchor(&mut self, position: DVec2, selected: bool) {
+		self.square(position, Some(MANIPULATOR_GROUP_MARKER_SIZE + 2.), Some(COLOR_OVERLAY_BLUE_50), Some(COLOR_OVERLAY_BLUE_50));
+		let color_fill = if selected { COLOR_OVERLAY_BLUE } else { COLOR_OVERLAY_WHITE };
+		self.square(position, None, Some(color_fill), Some(COLOR_OVERLAY_BLUE));
+	}
+
 	/// Transforms the canvas context to adjust for DPI scaling
 	///
 	/// Overwrites all existing tranforms. This operation can be reversed with [`Self::reset_transform`].
@@ -758,7 +794,7 @@ impl OverlayContext {
 		self.render_context.begin_path();
 		self.bezier_command(bezier, transform, true);
 		self.render_context.set_stroke_style_str(COLOR_OVERLAY_BLUE);
-		self.render_context.set_line_width(4.);
+		self.render_context.set_line_width(SEGMENT_SELECTED_THICKNESS);
 		self.render_context.stroke();
 
 		self.render_context.set_line_width(1.);
@@ -772,7 +808,7 @@ impl OverlayContext {
 		self.render_context.begin_path();
 		self.bezier_command(bezier, transform, true);
 		self.render_context.set_stroke_style_str(COLOR_OVERLAY_BLUE_50);
-		self.render_context.set_line_width(4.);
+		self.render_context.set_line_width(SEGMENT_SELECTED_THICKNESS);
 		self.render_context.stroke();
 
 		self.render_context.set_line_width(1.);

editor/src/messages/portfolio/document/overlays/utility_types_vello.rs

@@ -293,12 +293,36 @@ impl OverlayContext {
 			.stroke(&kurbo::Stroke::new(1.0), transform, Self::parse_color(color.unwrap_or(COLOR_OVERLAY_BLUE)), None, &circle);
 	}
 
+	pub fn hover_manipulator_handle(&mut self, position: DVec2, selected: bool) {
+		let transform = self.get_transform();
+
+		let position = position.round() - DVec2::splat(0.5);
+
+		let circle = kurbo::Circle::new((position.x, position.y), (MANIPULATOR_GROUP_MARKER_SIZE + 2.) / 2.);
+
+		let fill = COLOR_OVERLAY_BLUE_50;
+		self.scene.fill(peniko::Fill::NonZero, transform, Self::parse_color(fill), None, &circle);
+		self.scene.stroke(&kurbo::Stroke::new(1.0), transform, Self::parse_color(COLOR_OVERLAY_BLUE_50), None, &circle);
+
+		let inner_circle = kurbo::Circle::new((position.x, position.y), MANIPULATOR_GROUP_MARKER_SIZE / 2.);
+
+		let color_fill = if selected { COLOR_OVERLAY_BLUE } else { COLOR_OVERLAY_WHITE };
+		self.scene.fill(peniko::Fill::NonZero, transform, Self::parse_color(color_fill), None, &circle);
+		self.scene.stroke(&kurbo::Stroke::new(1.0), transform, Self::parse_color(COLOR_OVERLAY_BLUE), None, &inner_circle);
+	}
+
 	pub fn manipulator_anchor(&mut self, position: DVec2, selected: bool, color: Option<&str>) {
 		let color_stroke = color.unwrap_or(COLOR_OVERLAY_BLUE);
 		let color_fill = if selected { color_stroke } else { COLOR_OVERLAY_WHITE };
 		self.square(position, None, Some(color_fill), Some(color_stroke));
 	}
 
+	pub fn hover_manipulator_anchor(&mut self, position: DVec2, selected: bool) {
+		self.square(position, Some(MANIPULATOR_GROUP_MARKER_SIZE + 2.), Some(COLOR_OVERLAY_BLUE_50), Some(COLOR_OVERLAY_BLUE_50));
+		let color_fill = if selected { COLOR_OVERLAY_BLUE } else { COLOR_OVERLAY_WHITE };
+		self.square(position, None, Some(color_fill), Some(COLOR_OVERLAY_BLUE));
+	}
+
 	fn get_transform(&self) -> kurbo::Affine {
 		kurbo::Affine::scale(self.device_pixel_ratio)
 	}

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

@@ -302,9 +302,16 @@ impl ClosestSegment {
 		(midpoint, segment_ids)
 	}
 
-	pub fn adjusted_insert_and_select(&self, shape_editor: &mut ShapeState, responses: &mut VecDeque<Message>, extend_selection: bool) {
-		let (id, _) = self.adjusted_insert(responses);
-		shape_editor.select_anchor_point_by_id(self.layer, id, extend_selection)
+	pub fn adjusted_insert_and_select(&self, shape_editor: &mut ShapeState, responses: &mut VecDeque<Message>, extend_selection: bool, point_mode: bool, is_segment_selected: bool) {
+		let (id, segments) = self.adjusted_insert(responses);
+		if point_mode || is_segment_selected {
+			shape_editor.select_anchor_point_by_id(self.layer, id, extend_selection);
+		}
+
+		if is_segment_selected {
+			let Some(state) = shape_editor.selected_shape_state.get_mut(&self.layer) else { return };
+			segments.iter().for_each(|segment| state.select_segment(*segment));
+		}
 	}
 
 	pub fn calculate_perp(&self, document: &DocumentMessageHandler) -> DVec2 {
@@ -551,7 +558,7 @@ impl ShapeState {
 		select_threshold: f64,
 		extend_selection: bool,
 		path_overlay_mode: PathOverlayMode,
-		frontier_handles_info: Option<HashMap<SegmentId, Vec<PointId>>>,
+		frontier_handles_info: &Option<HashMap<SegmentId, Vec<PointId>>>,
 	) -> Option<Option<SelectedPointsInfo>> {
 		if self.selected_shape_state.is_empty() {
 			return None;
@@ -600,18 +607,18 @@ impl ShapeState {
 		mouse_position: DVec2,
 		select_threshold: f64,
 		path_overlay_mode: PathOverlayMode,
-		frontier_handles_info: Option<HashMap<SegmentId, Vec<PointId>>>,
+		frontier_handles_info: &Option<HashMap<SegmentId, Vec<PointId>>>,
 		point_editing_mode: bool,
 	) -> Option<(bool, Option<SelectedPointsInfo>)> {
 		if self.selected_shape_state.is_empty() {
 			return None;
 		}
 
-		if !point_editing_mode {
-			return None;
-		}
-
 		if let Some((layer, manipulator_point_id)) = self.find_nearest_point_indices(network_interface, mouse_position, select_threshold) {
+			// If not point editing mode then only handles are allowed to be dragged
+			if !point_editing_mode && matches!(manipulator_point_id, ManipulatorPointId::Anchor(_)) {
+				return None;
+			}
 			let vector_data = network_interface.compute_modified_vector(layer)?;
 			let point_position = manipulator_point_id.get_position(&vector_data)?;
 
@@ -1483,6 +1490,23 @@ impl ShapeState {
 		}
 	}
 
+	pub fn delete_hanging_selected_anchors(&mut self, document: &DocumentMessageHandler, responses: &mut VecDeque<Message>) {
+		for (&layer, state) in &self.selected_shape_state {
+			let Some(vector_data) = document.network_interface.compute_modified_vector(layer) else {
+				continue;
+			};
+
+			for point in &state.selected_points {
+				if let ManipulatorPointId::Anchor(anchor) = point {
+					if vector_data.all_connected(*anchor).all(|segment| state.is_segment_selected(segment.segment)) {
+						let modification_type = VectorModificationType::RemovePoint { id: *anchor };
+						responses.add(GraphOperationMessage::Vector { layer, modification_type });
+					}
+				}
+			}
+		}
+	}
+
 	pub fn break_path_at_selected_point(&self, document: &DocumentMessageHandler, responses: &mut VecDeque<Message>) {
 		for (&layer, state) in &self.selected_shape_state {
 			let Some(vector_data) = document.network_interface.compute_modified_vector(layer) else { continue };
@@ -1600,7 +1624,7 @@ impl ShapeState {
 		mouse_position: DVec2,
 		select_threshold: f64,
 		path_overlay_mode: PathOverlayMode,
-		frontier_handles_info: Option<HashMap<SegmentId, Vec<PointId>>>,
+		frontier_handles_info: &Option<HashMap<SegmentId, Vec<PointId>>>,
 	) -> Option<(LayerNodeIdentifier, ManipulatorPointId)> {
 		if self.selected_shape_state.is_empty() {
 			return None;
@@ -1968,20 +1992,91 @@ impl ShapeState {
 		selection_shape: SelectionShape,
 		selection_change: SelectionChange,
 		path_overlay_mode: PathOverlayMode,
-		frontier_handles_info: Option<HashMap<SegmentId, Vec<PointId>>>,
+		frontier_handles_info: &Option<HashMap<SegmentId, Vec<PointId>>>,
 		select_segments: bool,
+		select_points: bool,
 		// Here, "selection mode" represents touched or enclosed, not to be confused with editing modes
 		selection_mode: SelectionMode,
 	) {
+		let (points_inside, segments_inside) = self.get_inside_points_and_segments(
+			network_interface,
+			selection_shape,
+			path_overlay_mode,
+			frontier_handles_info,
+			select_segments,
+			select_points,
+			selection_mode,
+		);
+
+		if selection_change == SelectionChange::Clear {
+			self.deselect_all_points();
+			self.deselect_all_segments();
+		}
+
+		for (layer, points) in points_inside {
+			let Some(state) = self.selected_shape_state.get_mut(&layer) else { continue };
+			let Some(vector_data) = network_interface.compute_modified_vector(layer) else { continue };
+
+			for point in points {
+				match (point, selection_change) {
+					(_, SelectionChange::Shrink) => state.deselect_point(point),
+					(ManipulatorPointId::EndHandle(_) | ManipulatorPointId::PrimaryHandle(_), _) => {
+						let handle = point.as_handle().expect("Handle cannot be converted");
+						if handle.length(&vector_data) > 0. {
+							state.select_point(point);
+						}
+					}
+					(_, _) => state.select_point(point),
+				}
+			}
+		}
+
+		for (layer, segments) in segments_inside {
+			let Some(state) = self.selected_shape_state.get_mut(&layer) else { continue };
+			match selection_change {
+				SelectionChange::Shrink => segments.iter().for_each(|segment| state.deselect_segment(*segment)),
+				_ => segments.iter().for_each(|segment| state.select_segment(*segment)),
+			}
+
+			// Also select/deselect the endpoints of respective segments
+			let Some(vector_data) = network_interface.compute_modified_vector(layer) else { continue };
+			if !select_points && select_segments {
+				vector_data
+					.segment_bezier_iter()
+					.filter(|(segment, _, _, _)| segments.contains(segment))
+					.for_each(|(_, _, start, end)| match selection_change {
+						SelectionChange::Shrink => {
+							state.deselect_point(ManipulatorPointId::Anchor(start));
+							state.deselect_point(ManipulatorPointId::Anchor(end));
+						}
+						_ => {
+							state.select_point(ManipulatorPointId::Anchor(start));
+							state.select_point(ManipulatorPointId::Anchor(end));
+						}
+					});
+			}
+		}
+	}
+
+	#[allow(clippy::too_many_arguments)]
+	pub fn get_inside_points_and_segments(
+		&mut self,
+		network_interface: &NodeNetworkInterface,
+		selection_shape: SelectionShape,
+		path_overlay_mode: PathOverlayMode,
+		frontier_handles_info: &Option<HashMap<SegmentId, Vec<PointId>>>,
+		select_segments: bool,
+		select_points: bool,
+		// Represents if the box/lasso selection touches or encloses the targets (not to be confused with editing modes).
+		selection_mode: SelectionMode,
+	) -> (HashMap<LayerNodeIdentifier, HashSet<ManipulatorPointId>>, HashMap<LayerNodeIdentifier, HashSet<SegmentId>>) {
 		let selected_points = self.selected_points().cloned().collect::<HashSet<_>>();
 		let selected_segments = selected_segments(network_interface, self);
 
-		for (&layer, state) in &mut self.selected_shape_state {
-			if selection_change == SelectionChange::Clear {
-				state.clear_points();
-				state.clear_segments();
-			}
+		let mut points_inside: HashMap<LayerNodeIdentifier, HashSet<ManipulatorPointId>> = HashMap::new();
+		let mut segments_inside: HashMap<LayerNodeIdentifier, HashSet<SegmentId>> = HashMap::new();
 
+		for &layer in self.selected_shape_state.keys() {
 			let vector_data = network_interface.compute_modified_vector(layer);
 			let Some(vector_data) = vector_data else { continue };
 			let transform = network_interface.document_metadata().transform_to_viewport_if_feeds(layer, network_interface);
@@ -1997,7 +2092,7 @@ impl ShapeState {
 
 			let polygon_subpath = if let SelectionShape::Lasso(polygon) = selection_shape {
 				if polygon.len() < 2 {
-					return;
+					return (points_inside, segments_inside);
 				}
 				let polygon: Subpath<PointId> = Subpath::from_anchors_linear(polygon.to_vec(), true);
 				Some(polygon)
@@ -2037,10 +2132,7 @@ impl ShapeState {
 					};
 
 					if select {
-						match selection_change {
-							SelectionChange::Shrink => state.deselect_segment(id),
-							_ => state.select_segment(id),
-						}
+						segments_inside.entry(layer).or_default().insert(id);
 					}
 				}
 
@@ -2057,21 +2149,11 @@ impl ShapeState {
 							.contains_point(transformed_position),
 					};
 
-					if select {
-						let is_visible_handle = is_visible_point(id, &vector_data, path_overlay_mode, frontier_handles_info.clone(), selected_segments.clone(), &selected_points);
+					if select && select_points {
+						let is_visible_handle = is_visible_point(id, &vector_data, path_overlay_mode, frontier_handles_info, selected_segments.clone(), &selected_points);
 
 						if is_visible_handle {
-							match selection_change {
-								SelectionChange::Shrink => state.deselect_point(id),
-								_ => {
-									// Select only the handles which are of nonzero length
-									if let Some(handle) = id.as_handle() {
-										if handle.length(&vector_data) > 0. {
-											state.select_point(id)
-										}
-									}
-								}
-							}
+							points_inside.entry(layer).or_default().insert(id);
 						}
 					}
 				}
@@ -2089,13 +2171,12 @@ impl ShapeState {
 						.contains_point(transformed_position),
 				};
 
-				if select {
-					match selection_change {
-						SelectionChange::Shrink => state.deselect_point(ManipulatorPointId::Anchor(id)),
-						_ => state.select_point(ManipulatorPointId::Anchor(id)),
-					}
+				if select && select_points {
+					points_inside.entry(layer).or_default().insert(ManipulatorPointId::Anchor(id));
 				}
 			}
 		}
+
+		(points_inside, segments_inside)
 	}
 }

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

@@ -175,7 +175,7 @@ pub fn is_visible_point(
 	manipulator_point_id: ManipulatorPointId,
 	vector_data: &VectorData,
 	path_overlay_mode: PathOverlayMode,
-	frontier_handles_info: Option<HashMap<SegmentId, Vec<PointId>>>,
+	frontier_handles_info: &Option<HashMap<SegmentId, Vec<PointId>>>,
 	selected_segments: Vec<SegmentId>,
 	selected_points: &HashSet<ManipulatorPointId>,
 ) -> bool {
@@ -201,7 +201,7 @@ pub fn is_visible_point(
 						warn!("No anchor for selected handle");
 						return false;
 					};
-					let Some(frontier_handles) = &frontier_handles_info else {
+					let Some(frontier_handles) = frontier_handles_info else {
 						warn!("No frontier handles info provided");
 						return false;
 					};