From 06e11b6f465d0c31c7291fe0fab1ea6d64c3f109 Mon Sep 17 00:00:00 2001 From: James Lindsay <78500760+0HyperCube@users.noreply.github.com> Date: Thu, 5 Sep 2024 19:45:14 +0100 Subject: [PATCH] Fix invalid segment crash when disolving point loop (#1959) * Fix invalid segment crash * Further improve robustness --- .../tool/common_functionality/shape_editor.rs | 14 +++++- .../messages/tool/tool_messages/pen_tool.rs | 4 +- .../src/vector/vector_data/attributes.rs | 43 ++++++++++++++++--- .../src/vector/vector_data/modification.rs | 14 ++++-- 4 files changed, 64 insertions(+), 11 deletions(-) diff --git a/editor/src/messages/tool/common_functionality/shape_editor.rs b/editor/src/messages/tool/common_functionality/shape_editor.rs index 24f45a057..1c48e3e2c 100644 --- a/editor/src/messages/tool/common_functionality/shape_editor.rs +++ b/editor/src/messages/tool/common_functionality/shape_editor.rs @@ -739,6 +739,7 @@ impl ShapeState { pub fn delete_selected_points(&mut self, document: &DocumentMessageHandler, responses: &mut VecDeque) { for (&layer, state) in &mut self.selected_shape_state { let mut missing_anchors = HashMap::new(); + let mut deleted_anchors = HashSet::new(); let Some(vector_data) = document.network_interface.compute_modified_vector(layer) else { continue; }; @@ -749,6 +750,7 @@ impl ShapeState { if let Some(handles) = Self::dissolve_anchor(anchor, responses, layer, &vector_data) { missing_anchors.insert(anchor, handles); } + deleted_anchors.insert(anchor); } ManipulatorPointId::PrimaryHandle(_) | ManipulatorPointId::EndHandle(_) => { let Some(handle) = point.as_handle() else { continue }; @@ -772,7 +774,10 @@ impl ShapeState { while let Some((anchor, handles)) = missing_anchors.keys().next().copied().and_then(|id| missing_anchors.remove_entry(&id)) { visited.push(anchor); - let mut handles = handles.map(Some); + // If the adgacent point is just this point then skip + let mut handles = handles.map(|handle| (handle.1 != anchor).then_some(handle)); + + // If the adjacent points are themselves being deleted, then repeatedly visit the newest agacent points. for handle in &mut handles { while let Some((point, connected)) = (*handle).and_then(|(_, point)| missing_anchors.remove_entry(&point)) { visited.push(point); @@ -782,6 +787,13 @@ impl ShapeState { } let [Some(start), Some(end)] = handles else { continue }; + + // Avoid reconnecting to points that are being deleted (this can happen if a whole loop is deleted) + if deleted_anchors.contains(&start.1) || deleted_anchors.contains(&end.1) { + continue; + } + + // Grab the handles from the opposite side of the segment(s) being deleted and make it relative to the anchor let [handle_start, handle_end] = [start, end].map(|(handle, _)| { let handle = handle.opposite(); let handle_position = handle.to_manipulator_point().get_position(&vector_data); diff --git a/editor/src/messages/tool/tool_messages/pen_tool.rs b/editor/src/messages/tool/tool_messages/pen_tool.rs index e35d68bee..2821cd7f2 100644 --- a/editor/src/messages/tool/tool_messages/pen_tool.rs +++ b/editor/src/messages/tool/tool_messages/pen_tool.rs @@ -306,7 +306,9 @@ impl PenToolData { }, }); } - if !close_subpath { + if close_subpath { + responses.add(DocumentMessage::EndTransaction); + } else { self.add_point(LastPoint { id: end, pos: next_point, diff --git a/node-graph/gcore/src/vector/vector_data/attributes.rs b/node-graph/gcore/src/vector/vector_data/attributes.rs index b376566d9..0ed6ba082 100644 --- a/node-graph/gcore/src/vector/vector_data/attributes.rs +++ b/node-graph/gcore/src/vector/vector_data/attributes.rs @@ -198,16 +198,47 @@ impl SegmentDomain { self.stroke.clear(); } - pub fn retain(&mut self, f: impl Fn(&SegmentId) -> bool) { - let mut keep = self.ids.iter().map(&f); + pub fn retain(&mut self, f: impl Fn(&SegmentId) -> bool, points_length: usize) { + let additional_delete_ids = self + .ids + .iter() + .zip(&self.start_point) + .zip(&self.end_point) + .filter(|((_, start), end)| **start >= points_length || **end >= points_length) + .map(|x| *x.0 .0) + .collect::>(); + + let can_delete = || { + let f = &f; + let mut delete_iter = additional_delete_ids.iter().peekable(); + move |id| { + if delete_iter.peek() == Some(&id) { + delete_iter.next(); + false + } else { + f(id) + } + } + }; + + let mut keep = self.ids.iter().map(can_delete()); self.start_point.retain(|_| keep.next().unwrap_or_default()); - let mut keep = self.ids.iter().map(&f); + let mut keep = self.ids.iter().map(can_delete()); self.end_point.retain(|_| keep.next().unwrap_or_default()); - let mut keep = self.ids.iter().map(&f); + let mut keep = self.ids.iter().map(can_delete()); self.handles.retain(|_| keep.next().unwrap_or_default()); - let mut keep = self.ids.iter().map(&f); + let mut keep = self.ids.iter().map(can_delete()); self.stroke.retain(|_| keep.next().unwrap_or_default()); - self.ids.retain(f); + + let mut delete_iter = additional_delete_ids.iter().peekable(); + self.ids.retain(move |id| { + if delete_iter.peek() == Some(&id) { + delete_iter.next(); + false + } else { + f(id) + } + }); } pub fn ids(&self) -> &[SegmentId] { diff --git a/node-graph/gcore/src/vector/vector_data/modification.rs b/node-graph/gcore/src/vector/vector_data/modification.rs index b3eb23edf..4be9a4e07 100644 --- a/node-graph/gcore/src/vector/vector_data/modification.rs +++ b/node-graph/gcore/src/vector/vector_data/modification.rs @@ -101,7 +101,7 @@ pub struct SegmentModification { impl SegmentModification { /// Apply this modification to the specified [`SegmentDomain`]. pub fn apply(&self, segment_domain: &mut SegmentDomain, point_domain: &PointDomain) { - segment_domain.retain(|id| !self.remove.contains(id)); + segment_domain.retain(|id| !self.remove.contains(id), point_domain.ids().len()); for (id, point) in segment_domain.start_point_mut() { let Some(&new) = self.start_point.get(&id) else { continue }; @@ -206,8 +206,16 @@ impl SegmentModification { segment_domain.push(add_id, start_index, end_index, handles, stroke); } - assert!(segment_domain.start_point().iter().all(|&index| index < point_domain.ids().len()), "index should be in range"); - assert!(segment_domain.end_point().iter().all(|&index| index < point_domain.ids().len()), "index should be in range"); + assert!( + segment_domain.start_point().iter().all(|&index| index < point_domain.ids().len()), + "index should be in range {:#?}", + segment_domain + ); + assert!( + segment_domain.end_point().iter().all(|&index| index < point_domain.ids().len()), + "index should be in range {:#?}", + segment_domain + ); } /// Create a new modification that will convert an empty [`VectorData`] into the target [`VectorData`].