Fix invalid segment crash when disolving point loop (#1959)

* Fix invalid segment crash

* Further improve robustness

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<Message>) {
 		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);

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,

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) {
+	pub fn retain(&mut self, f: impl Fn(&SegmentId) -> bool, points_length: usize) {
-		let mut keep = self.ids.iter().map(&f);
+		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::<Vec<_>>();
+
+		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] {

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!(
-		assert!(segment_domain.end_point().iter().all(|&index| index < point_domain.ids().len()), "index should be in range");
+			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`].