From b564579362a23d3fe2ce5e8bd76cce088ab10df8 Mon Sep 17 00:00:00 2001 From: Adesh Gupta <148623820+4adex@users.noreply.github.com> Date: Wed, 28 May 2025 14:54:23 +0530 Subject: [PATCH] Make the Path tool only allow selecting points that are visible (#2668) * Fix only visible points selection in point selection * Fix comments * Remove bug from box selection and lasso * Code review * Fix comment --------- Co-authored-by: Keavon Chambers --- .../document/overlays/utility_functions.rs | 7 +- .../tool/common_functionality/shape_editor.rs | 98 ++++++++++++++++--- .../common_functionality/utility_functions.rs | 44 +++++++++ .../messages/tool/tool_messages/path_tool.rs | 95 +++++++++++++++--- 4 files changed, 216 insertions(+), 28 deletions(-) diff --git a/editor/src/messages/portfolio/document/overlays/utility_functions.rs b/editor/src/messages/portfolio/document/overlays/utility_functions.rs index 6f2b1aef..d2199a03 100644 --- a/editor/src/messages/portfolio/document/overlays/utility_functions.rs +++ b/editor/src/messages/portfolio/document/overlays/utility_functions.rs @@ -1,5 +1,6 @@ use super::utility_types::{DrawHandles, OverlayContext}; use crate::consts::HIDE_HANDLE_DISTANCE; +use crate::messages::portfolio::document::utility_types::network_interface::NodeNetworkInterface; use crate::messages::tool::common_functionality::shape_editor::{SelectedLayerState, ShapeState}; use crate::messages::tool::tool_messages::tool_prelude::{DocumentMessageHandler, PreferencesMessageHandler}; use bezier_rs::{Bezier, BezierHandles}; @@ -23,7 +24,7 @@ pub fn overlay_canvas_context() -> web_sys::CanvasRenderingContext2d { create_context().expect("Failed to get canvas context") } -pub fn selected_segments(document: &DocumentMessageHandler, shape_editor: &mut ShapeState) -> Vec { +pub fn selected_segments(network_interface: &NodeNetworkInterface, shape_editor: &ShapeState) -> Vec { let selected_points = shape_editor.selected_points(); let selected_anchors = selected_points .filter_map(|point_id| if let ManipulatorPointId::Anchor(p) = point_id { Some(*p) } else { None }) @@ -40,8 +41,8 @@ pub fn selected_segments(document: &DocumentMessageHandler, shape_editor: &mut S // TODO: Currently if there are two duplicate layers, both of their segments get overlays // Adding segments which are are connected to selected anchors - for layer in document.network_interface.selected_nodes().selected_layers(document.metadata()) { - let Some(vector_data) = document.network_interface.compute_modified_vector(layer) else { continue }; + for layer in network_interface.selected_nodes().selected_layers(network_interface.document_metadata()) { + let Some(vector_data) = network_interface.compute_modified_vector(layer) else { continue }; for (segment_id, _bezier, start, end) in vector_data.segment_bezier_iter() { if selected_anchors.contains(&start) || selected_anchors.contains(&end) { diff --git a/editor/src/messages/tool/common_functionality/shape_editor.rs b/editor/src/messages/tool/common_functionality/shape_editor.rs index 53cb4794..72287326 100644 --- a/editor/src/messages/tool/common_functionality/shape_editor.rs +++ b/editor/src/messages/tool/common_functionality/shape_editor.rs @@ -2,12 +2,14 @@ use super::graph_modification_utils::{self, merge_layers}; use super::snapping::{SnapCache, SnapCandidatePoint, SnapData, SnapManager, SnappedPoint}; use super::utility_functions::calculate_segment_angle; use crate::consts::HANDLE_LENGTH_FACTOR; +use crate::messages::portfolio::document::overlays::utility_functions::selected_segments; use crate::messages::portfolio::document::utility_types::document_metadata::{DocumentMetadata, LayerNodeIdentifier}; use crate::messages::portfolio::document::utility_types::misc::{PathSnapSource, SnapSource}; use crate::messages::portfolio::document::utility_types::network_interface::NodeNetworkInterface; use crate::messages::prelude::*; use crate::messages::tool::common_functionality::snapping::SnapTypeConfiguration; -use crate::messages::tool::tool_messages::path_tool::PointSelectState; +use crate::messages::tool::common_functionality::utility_functions::is_visible_point; +use crate::messages::tool::tool_messages::path_tool::{PathOverlayMode, PointSelectState}; use bezier_rs::{Bezier, BezierHandles, Subpath, TValue}; use glam::{DAffine2, DVec2}; use graphene_core::transform::Transform; @@ -421,12 +423,20 @@ impl ShapeState { /// Select/deselect the first point within the selection threshold. /// Returns a tuple of the points if found and the offset, or `None` otherwise. - pub fn change_point_selection(&mut self, network_interface: &NodeNetworkInterface, mouse_position: DVec2, select_threshold: f64, extend_selection: bool) -> Option> { + pub fn change_point_selection( + &mut self, + network_interface: &NodeNetworkInterface, + mouse_position: DVec2, + select_threshold: f64, + extend_selection: bool, + path_overlay_mode: PathOverlayMode, + frontier_handles_info: Option>>, + ) -> Option> { if self.selected_shape_state.is_empty() { return None; } - if let Some((layer, manipulator_point_id)) = self.find_nearest_point_indices(network_interface, mouse_position, select_threshold) { + if let Some((layer, manipulator_point_id)) = self.find_nearest_visible_point_indices(network_interface, mouse_position, select_threshold, path_overlay_mode, frontier_handles_info) { let vector_data = network_interface.compute_modified_vector(layer)?; let point_position = manipulator_point_id.get_position(&vector_data)?; @@ -467,7 +477,14 @@ impl ShapeState { None } - pub fn get_point_selection_state(&mut self, network_interface: &NodeNetworkInterface, mouse_position: DVec2, select_threshold: f64) -> Option<(bool, Option)> { + pub fn get_point_selection_state( + &mut self, + network_interface: &NodeNetworkInterface, + mouse_position: DVec2, + select_threshold: f64, + path_overlay_mode: PathOverlayMode, + frontier_handles_info: Option>>, + ) -> Option<(bool, Option)> { if self.selected_shape_state.is_empty() { return None; } @@ -476,6 +493,13 @@ impl ShapeState { let vector_data = network_interface.compute_modified_vector(layer)?; let point_position = manipulator_point_id.get_position(&vector_data)?; + // Check if point is visible under current overlay mode or not + let selected_segments = selected_segments(network_interface, self); + let selected_points = self.selected_points().cloned().collect::>(); + if !is_visible_point(manipulator_point_id, &vector_data, path_overlay_mode, frontier_handles_info, selected_segments, &selected_points) { + return None; + } + let selected_shape_state = self.selected_shape_state.get(&layer)?; let already_selected = selected_shape_state.is_selected(manipulator_point_id); @@ -1320,6 +1344,42 @@ impl ShapeState { None } + pub fn find_nearest_visible_point_indices( + &mut self, + network_interface: &NodeNetworkInterface, + mouse_position: DVec2, + select_threshold: f64, + path_overlay_mode: PathOverlayMode, + frontier_handles_info: Option>>, + ) -> Option<(LayerNodeIdentifier, ManipulatorPointId)> { + if self.selected_shape_state.is_empty() { + return None; + } + + let select_threshold_squared = select_threshold.powi(2); + + // Find the closest control point among all elements of shapes_to_modify + for &layer in self.selected_shape_state.keys() { + if let Some((manipulator_point_id, distance_squared)) = Self::closest_point_in_layer(network_interface, layer, mouse_position) { + // Choose the first point under the threshold + if distance_squared < select_threshold_squared { + // Check if point is visible in current PathOverlayMode + let vector_data = network_interface.compute_modified_vector(layer)?; + let selected_segments = selected_segments(network_interface, self); + let selected_points = self.selected_points().cloned().collect::>(); + + if !is_visible_point(manipulator_point_id, &vector_data, path_overlay_mode, frontier_handles_info, selected_segments, &selected_points) { + return None; + } + + return Some((layer, manipulator_point_id)); + } + } + } + + None + } + // TODO Use quadtree or some equivalent spatial acceleration structure to improve this to O(log(n)) /// Find the closest manipulator, manipulator point, and distance so we can select path elements. /// Brute force comparison to determine which manipulator (handle or anchor) we want to select taking O(n) time. @@ -1623,7 +1683,17 @@ impl ShapeState { false } - pub fn select_all_in_shape(&mut self, network_interface: &NodeNetworkInterface, selection_shape: SelectionShape, selection_change: SelectionChange) { + pub fn select_all_in_shape( + &mut self, + network_interface: &NodeNetworkInterface, + selection_shape: SelectionShape, + selection_change: SelectionChange, + path_overlay_mode: PathOverlayMode, + frontier_handles_info: Option>>, + ) { + let selected_points = self.selected_points().cloned().collect::>(); + 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() @@ -1666,13 +1736,17 @@ impl ShapeState { }; if select { - 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) + let is_visible_handle = is_visible_point(id, &vector_data, path_overlay_mode, frontier_handles_info.clone(), 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) + } } } } diff --git a/editor/src/messages/tool/common_functionality/utility_functions.rs b/editor/src/messages/tool/common_functionality/utility_functions.rs index 4794ec73..61b300a1 100644 --- a/editor/src/messages/tool/common_functionality/utility_functions.rs +++ b/editor/src/messages/tool/common_functionality/utility_functions.rs @@ -1,6 +1,7 @@ use crate::messages::portfolio::document::utility_types::document_metadata::LayerNodeIdentifier; use crate::messages::prelude::*; use crate::messages::tool::common_functionality::graph_modification_utils::get_text; +use crate::messages::tool::tool_messages::path_tool::PathOverlayMode; use glam::DVec2; use graphene_core::renderer::Quad; use graphene_core::text::{FontCache, load_face}; @@ -93,3 +94,46 @@ pub fn calculate_segment_angle(anchor: PointId, segment: SegmentId, vector_data: required_handle.map(|handle| -(handle - anchor_position).angle_to(DVec2::X)) } + +/// Check whether a point is visible in the current overlay mode. +pub fn is_visible_point( + manipulator_point_id: ManipulatorPointId, + vector_data: &VectorData, + path_overlay_mode: PathOverlayMode, + frontier_handles_info: Option>>, + selected_segments: Vec, + selected_points: &HashSet, +) -> bool { + match manipulator_point_id { + ManipulatorPointId::Anchor(_) => true, + ManipulatorPointId::EndHandle(segment_id) | ManipulatorPointId::PrimaryHandle(segment_id) => { + match (path_overlay_mode, selected_points.len() == 1) { + (PathOverlayMode::AllHandles, _) => true, + (PathOverlayMode::SelectedPointHandles, _) | (PathOverlayMode::FrontierHandles, true) => { + if selected_segments.contains(&segment_id) { + return true; + } + + // Either the segment is a part of selected segments or the opposite handle is a part of existing selection + let Some(handle_pair) = manipulator_point_id.get_handle_pair(vector_data) else { return false }; + let other_handle = handle_pair[1].to_manipulator_point(); + + // Return whether the list of selected points contain the other handle + selected_points.contains(&other_handle) + } + (PathOverlayMode::FrontierHandles, false) => { + let Some(anchor) = manipulator_point_id.get_anchor(vector_data) else { + warn!("No anchor for selected handle"); + return false; + }; + let Some(frontier_handles) = &frontier_handles_info else { + warn!("No frontier handles info provided"); + return false; + }; + + frontier_handles.get(&segment_id).map(|anchors| anchors.contains(&anchor)).unwrap_or_default() + } + } + } + } +} diff --git a/editor/src/messages/tool/tool_messages/path_tool.rs b/editor/src/messages/tool/tool_messages/path_tool.rs index 7769e740..1dfd7f83 100644 --- a/editor/src/messages/tool/tool_messages/path_tool.rs +++ b/editor/src/messages/tool/tool_messages/path_tool.rs @@ -379,6 +379,7 @@ struct PathToolData { alt_dragging_from_anchor: bool, angle_locked: bool, temporary_colinear_handles: bool, + frontier_handles_info: Option>>, adjacent_anchor_offset: Option, } @@ -451,6 +452,7 @@ impl PathToolData { lasso_select: bool, handle_drag_from_anchor: bool, drag_zero_handle: bool, + path_overlay_mode: PathOverlayMode, ) -> PathToolFsmState { self.double_click_handled = false; self.opposing_handle_lengths = None; @@ -466,8 +468,14 @@ impl PathToolData { let old_selection = shape_editor.selected_points().cloned().collect::>(); // Check if the point is already selected; if not, select the first point within the threshold (in pixels) - if let Some((already_selected, mut selection_info)) = shape_editor.get_point_selection_state(&document.network_interface, input.mouse.position, SELECTION_THRESHOLD) { - log::info!("entered the part where tool identifies a near point"); + // Don't select the points which are not shown currently in PathOverlayMode + if let Some((already_selected, mut selection_info)) = shape_editor.get_point_selection_state( + &document.network_interface, + input.mouse.position, + SELECTION_THRESHOLD, + path_overlay_mode, + self.frontier_handles_info.clone(), + ) { responses.add(DocumentMessage::StartTransaction); self.last_clicked_point_was_selected = already_selected; @@ -475,7 +483,14 @@ impl PathToolData { // If the point is already selected and shift (`extend_selection`) is used, keep the selection unchanged. // Otherwise, select the first point within the threshold. if !(already_selected && extend_selection) { - if let Some(updated_selection_info) = shape_editor.change_point_selection(&document.network_interface, input.mouse.position, SELECTION_THRESHOLD, extend_selection) { + if let Some(updated_selection_info) = shape_editor.change_point_selection( + &document.network_interface, + input.mouse.position, + SELECTION_THRESHOLD, + extend_selection, + path_overlay_mode, + self.frontier_handles_info.clone(), + ) { selection_info = updated_selection_info; } } @@ -1055,14 +1070,16 @@ impl Fsm for PathToolFsmState { match tool_options.path_overlay_mode { PathOverlayMode::AllHandles => { path_overlays(document, DrawHandles::All, shape_editor, &mut overlay_context); + tool_data.frontier_handles_info = None; } PathOverlayMode::SelectedPointHandles => { - let selected_segments = selected_segments(document, shape_editor); + let selected_segments = selected_segments(&document.network_interface, shape_editor); path_overlays(document, DrawHandles::SelectedAnchors(selected_segments), shape_editor, &mut overlay_context); + tool_data.frontier_handles_info = None; } PathOverlayMode::FrontierHandles => { - let selected_segments = selected_segments(document, shape_editor); + let selected_segments = selected_segments(&document.network_interface, shape_editor); let selected_points = shape_editor.selected_points(); let selected_anchors = selected_points .filter_map(|point_id| if let ManipulatorPointId::Anchor(p) = point_id { Some(*p) } else { None }) @@ -1090,13 +1107,18 @@ impl Fsm for PathToolFsmState { for (point, attached_segments) in selected_segments_by_point { if attached_segments.len() == 1 { segment_endpoints.entry(attached_segments[0]).or_default().push(point); - } else if !selected_anchors.contains(&point) { + } + // Handle the edge case where a point, although not explicitly selected, is shared by two segments. + else if !selected_anchors.contains(&point) { segment_endpoints.entry(attached_segments[0]).or_default().push(point); segment_endpoints.entry(attached_segments[1]).or_default().push(point); } } } + // Caching segment endpoints for use in point selection logic + tool_data.frontier_handles_info = Some(segment_endpoints.clone()); + // Now frontier anchors can be sent for rendering overlays path_overlays(document, DrawHandles::FrontierHandles(segment_endpoints), shape_editor, &mut overlay_context); } @@ -1198,7 +1220,17 @@ impl Fsm for PathToolFsmState { tool_data.selection_mode = None; tool_data.lasso_polygon.clear(); - tool_data.mouse_down(shape_editor, document, input, responses, extend_selection, lasso_select, handle_drag_from_anchor, drag_zero_handle) + tool_data.mouse_down( + shape_editor, + document, + input, + responses, + extend_selection, + lasso_select, + handle_drag_from_anchor, + drag_zero_handle, + tool_options.path_overlay_mode, + ) } ( PathToolFsmState::Drawing { selection_shape }, @@ -1353,7 +1385,13 @@ impl Fsm for PathToolFsmState { // If there is a point nearby, then remove the overlay if shape_editor - .find_nearest_point_indices(&document.network_interface, input.mouse.position, SELECTION_THRESHOLD) + .find_nearest_visible_point_indices( + &document.network_interface, + input.mouse.position, + SELECTION_THRESHOLD, + tool_options.path_overlay_mode, + tool_data.frontier_handles_info.clone(), + ) .is_some() { tool_data.segment = None; @@ -1447,9 +1485,21 @@ impl Fsm for PathToolFsmState { match selection_shape { SelectionShapeType::Box => { let bbox = [tool_data.drag_start_pos, previous_mouse]; - shape_editor.select_all_in_shape(&document.network_interface, SelectionShape::Box(bbox), selection_change); + shape_editor.select_all_in_shape( + &document.network_interface, + SelectionShape::Box(bbox), + selection_change, + tool_options.path_overlay_mode, + tool_data.frontier_handles_info.clone(), + ); } - SelectionShapeType::Lasso => shape_editor.select_all_in_shape(&document.network_interface, SelectionShape::Lasso(&tool_data.lasso_polygon), selection_change), + SelectionShapeType::Lasso => shape_editor.select_all_in_shape( + &document.network_interface, + SelectionShape::Lasso(&tool_data.lasso_polygon), + selection_change, + tool_options.path_overlay_mode, + tool_data.frontier_handles_info.clone(), + ), } } @@ -1495,9 +1545,21 @@ impl Fsm for PathToolFsmState { match selection_shape { SelectionShapeType::Box => { let bbox = [tool_data.drag_start_pos, previous_mouse]; - shape_editor.select_all_in_shape(&document.network_interface, SelectionShape::Box(bbox), select_kind); + shape_editor.select_all_in_shape( + &document.network_interface, + SelectionShape::Box(bbox), + select_kind, + tool_options.path_overlay_mode, + tool_data.frontier_handles_info.clone(), + ); } - SelectionShapeType::Lasso => shape_editor.select_all_in_shape(&document.network_interface, SelectionShape::Lasso(&tool_data.lasso_polygon), select_kind), + SelectionShapeType::Lasso => shape_editor.select_all_in_shape( + &document.network_interface, + SelectionShape::Lasso(&tool_data.lasso_polygon), + select_kind, + tool_options.path_overlay_mode, + tool_data.frontier_handles_info.clone(), + ), } } responses.add(OverlaysMessage::Draw); @@ -1508,7 +1570,14 @@ impl Fsm for PathToolFsmState { (_, PathToolMessage::DragStop { extend_selection, .. }) => { let extend_selection = input.keyboard.get(extend_selection as usize); let drag_occurred = tool_data.drag_start_pos.distance(input.mouse.position) > DRAG_THRESHOLD; - let nearest_point = shape_editor.find_nearest_point_indices(&document.network_interface, input.mouse.position, SELECTION_THRESHOLD); + // TODO: Here we want only visible points to be considered + let nearest_point = shape_editor.find_nearest_visible_point_indices( + &document.network_interface, + input.mouse.position, + SELECTION_THRESHOLD, + tool_options.path_overlay_mode, + tool_data.frontier_handles_info.clone(), + ); if let Some((layer, nearest_point)) = nearest_point { if !drag_occurred && extend_selection {