Fix Pen tool race condition when drawing handles fast, and stuck overlays (#1283)

* Fix pen tool with async

* Fix overlays staying too long

* Perhaps fix the transform issue

* Fix compile error

* Formatting

---------

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

editor/src/messages/portfolio/document/node_graph/graph_operation_message_handler/transform_utils.rs

@@ -276,6 +276,7 @@ impl<'a> VectorModificationState<'a> {
 	}
 
 	fn set_position(&mut self, point: ManipulatorPointId, position: DVec2) {
+		assert!(position.is_finite(), "Point position should be finite");
 		for subpath in self.subpaths.iter_mut() {
 			if let Some(manipulator) = subpath.manipulator_mut_from_id(point.group) {
 				match point.manipulator_type {

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

@@ -36,7 +36,7 @@ const POINT_STROKE_WEIGHT: f64 = 2.;
 #[derive(Clone, Debug, Default)]
 pub struct OverlayRenderer {
 	shape_overlay_cache: HashMap<LayerId, Vec<LayerId>>,
-	manipulator_group_overlay_cache: HashMap<(LayerId, ManipulatorGroupId), ManipulatorGroupOverlays>,
+	manipulator_group_overlay_cache: HashMap<LayerId, HashMap<ManipulatorGroupId, ManipulatorGroupOverlays>>,
 }
 
 impl OverlayRenderer {
@@ -68,7 +68,7 @@ impl OverlayRenderer {
 
 				// Create, place, and style the manipulator overlays
 				for manipulator_group in vector_data.manipulator_groups() {
-					let manipulator_group_cache = self.manipulator_group_overlay_cache.entry((*layer_id, manipulator_group.id)).or_default();
+					let manipulator_group_cache = self.manipulator_group_overlay_cache.entry(*layer_id).or_default().entry(manipulator_group.id).or_default();
 
 					// Only view in and out handles if they are not on top of the anchor
 					let [in_handle, out_handle] = {
@@ -103,6 +103,19 @@ impl OverlayRenderer {
 					Self::place_manipulator_group_overlays(manipulator_group, manipulator_group_cache, &transform, responses);
 					Self::style_overlays(selected_shape_state, &layer_path, manipulator_group, manipulator_group_cache, responses);
 				}
+
+				if let Some(layer_overlays) = self.manipulator_group_overlay_cache.get_mut(layer_id) {
+					if layer_overlays.len() > vector_data.manipulator_groups().count() {
+						layer_overlays.retain(|manipulator, manipulator_group_overlays| {
+							if vector_data.manipulator_groups().any(|current_manipulator| current_manipulator.id == *manipulator) {
+								true
+							} else {
+								Self::remove_manipulator_group_overlays(manipulator_group_overlays, responses);
+								false
+							}
+						});
+					}
+				}
 				// TODO Handle removing shapes from cache so we don't memory leak
 				// Eventually will get replaced with am immediate mode renderer for overlays
 			}
@@ -120,16 +133,10 @@ impl OverlayRenderer {
 		self.shape_overlay_cache.remove(layer_id);
 
 		// Remove the ManipulatorGroup overlays
-		if let Ok(layer) = document.layer(&layer_path) {
+		let Some(layer_cache) = self.manipulator_group_overlay_cache.remove(layer_id) else { return };
-			if let Some(vector_data) = layer.as_vector_data() {
+
-				for manipulator_group in vector_data.manipulator_groups() {
+		for manipulator_group_overlays in layer_cache.values() {
-					let id = manipulator_group.id;
+			Self::remove_manipulator_group_overlays(manipulator_group_overlays, responses);
-					if let Some(manipulator_group_overlays) = self.manipulator_group_overlay_cache.get(&(*layer_id, id)) {
-						Self::remove_manipulator_group_overlays(manipulator_group_overlays, responses);
-						self.manipulator_group_overlay_cache.remove(&(*layer_id, id));
-					}
-				}
-			}
 		}
 	}
 
@@ -142,15 +149,20 @@ impl OverlayRenderer {
 		}
 
 		// Hide the manipulator group overlays
-		if let Ok(layer) = document.layer(&layer_path) {
+		let Some(manipulator_groups) = self.manipulator_group_overlay_cache.get(layer_id) else { return };
-			if let Some(vector_data) = layer.as_vector_data() {
+		if visibility {
-				for manipulator_group in vector_data.manipulator_groups() {
+			let Ok(layer) = document.layer(&layer_path) else { return };
-					let id = manipulator_group.id;
+			let Some(vector_data) = layer.as_vector_data()  else { return };
-					if let Some(manipulator_group_overlays) = self.manipulator_group_overlay_cache.get(&(*layer_id, id)) {
+			for manipulator_group in vector_data.manipulator_groups() {
-						Self::set_manipulator_group_overlay_visibility(manipulator_group_overlays, visibility, responses);
+				let id = manipulator_group.id;
-					}
+				if let Some(manipulator_group_overlays) = manipulator_groups.get(&id) {
+					Self::set_manipulator_group_overlay_visibility(manipulator_group_overlays, visibility, responses);
 				}
 			}
+		} else {
+			for manipulator_group_overlays in manipulator_groups.values() {
+				Self::set_manipulator_group_overlay_visibility(manipulator_group_overlays, visibility, responses);
+			}
 		}
 	}
 

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

@@ -15,7 +15,10 @@ use crate::messages::tool::common_functionality::snapping::SnapManager;
 use crate::messages::tool::utility_types::{EventToMessageMap, Fsm, ToolActionHandlerData, ToolMetadata, ToolTransition, ToolType};
 use crate::messages::tool::utility_types::{HintData, HintGroup, HintInfo};
 
+use bezier_rs::Subpath;
 use document_legacy::LayerId;
+use graph_craft::document::value::TaggedValue;
+use graph_craft::document::NodeInput;
 use graphene_core::uuid::ManipulatorGroupId;
 use graphene_core::vector::style::{Fill, Stroke};
 use graphene_core::vector::{ManipulatorPointId, SelectedType};
@@ -227,8 +230,8 @@ impl PenToolData {
 		self.subpath_index = subpath_index;
 
 		// Stop the handles on the first point from mirroring
-		let Some(vector_data) = document.document_legacy.layer(layer).ok().and_then(|layer| layer.as_vector_data()) else { return };
+		let Some(subpaths) = get_subpaths(layer, document) else { return };
-		let manipulator_groups = vector_data.subpaths[subpath_index].manipulator_groups();
+		let manipulator_groups = subpaths[subpath_index].manipulator_groups();
 		let Some(last_handle) = (if from_start { manipulator_groups.first() } else { manipulator_groups.last() }) else { return };
 
 		responses.add(GraphOperationMessage::Vector {
@@ -284,8 +287,7 @@ impl PenToolData {
 	fn check_break(&mut self, document: &DocumentMessageHandler, transform: DAffine2, shape_overlay: &mut OverlayRenderer, responses: &mut VecDeque<Message>) -> Option<()> {
 		// Get subpath
 		let layer_path = self.path.as_ref()?;
-		let vector_data = document.document_legacy.layer(layer_path).ok().and_then(|layer| layer.as_vector_data())?;
+		let subpath = &get_subpaths(layer_path, document)?[self.subpath_index];
-		let subpath = &vector_data.subpaths[self.subpath_index];
 
 		// Get the last manipulator group and the one previous to that
 		let mut manipulator_groups = subpath.manipulator_groups().iter();
@@ -336,8 +338,7 @@ impl PenToolData {
 	fn finish_placing_handle(&mut self, document: &DocumentMessageHandler, transform: DAffine2, shape_overlay: &mut OverlayRenderer, responses: &mut VecDeque<Message>) -> Option<PenToolFsmState> {
 		// Get subpath
 		let layer_path = self.path.as_ref()?;
-		let vector_data = document.document_legacy.layer(layer_path).ok().and_then(|layer| layer.as_vector_data())?;
+		let subpath = &get_subpaths(layer_path, document)?[self.subpath_index];
-		let subpath = &vector_data.subpaths[self.subpath_index];
 
 		// Get the last manipulator group and the one previous to that
 		let mut manipulator_groups = subpath.manipulator_groups().iter();
@@ -415,8 +416,7 @@ impl PenToolData {
 	fn drag_handle(&mut self, document: &DocumentMessageHandler, transform: DAffine2, mouse: DVec2, modifiers: ModifierState, responses: &mut VecDeque<Message>) -> Option<PenToolFsmState> {
 		// Get subpath
 		let layer_path = self.path.as_ref()?;
-		let vector_data = document.document_legacy.layer(layer_path).ok().and_then(|layer| layer.as_vector_data())?;
+		let subpath = &get_subpaths(layer_path, document)?[self.subpath_index];
-		let subpath = &vector_data.subpaths[self.subpath_index];
 
 		// Get the last manipulator group
 		let manipulator_groups = subpath.manipulator_groups();
@@ -433,6 +433,9 @@ impl PenToolData {
 		let pos = transform.inverse().transform_point2(mouse);
 
 		let pos = compute_snapped_angle(&mut self.angle, modifiers.lock_angle, modifiers.snap_angle, pos, last_anchor);
+		if !pos.is_finite() {
+			return Some(PenToolFsmState::DraggingHandle);
+		}
 
 		// Update points on current segment (to show preview of new handle)
 		let point = ManipulatorPointId::new(last_manipulator_group.id, outwards_handle);
@@ -466,8 +469,7 @@ impl PenToolData {
 	fn place_anchor(&mut self, document: &DocumentMessageHandler, transform: DAffine2, mouse: DVec2, modifiers: ModifierState, responses: &mut VecDeque<Message>) -> Option<PenToolFsmState> {
 		// Get subpath
 		let layer_path = self.path.as_ref()?;
-		let vector_data = document.document_legacy.layer(layer_path).ok().and_then(|layer| layer.as_vector_data())?;
+		let subpath = &get_subpaths(layer_path, document)?[self.subpath_index];
-		let subpath = &vector_data.subpaths[self.subpath_index];
 
 		// Get the last manipulator group and the one previous to that
 		let mut manipulator_groups = subpath.manipulator_groups().iter();
@@ -512,8 +514,7 @@ impl PenToolData {
 	fn finish_transaction(&mut self, fsm: PenToolFsmState, document: &DocumentMessageHandler, responses: &mut VecDeque<Message>) -> Option<DocumentMessage> {
 		// Get subpath
 		let layer_path = self.path.as_ref()?;
-		let vector_data = document.document_legacy.layer(layer_path).ok().and_then(|layer| layer.as_vector_data())?;
+		let subpath = &get_subpaths(layer_path, document)?[self.subpath_index];
-		let subpath = &vector_data.subpaths[self.subpath_index];
 
 		// Abort if only one manipulator group has been placed
 		if fsm == PenToolFsmState::PlacingAnchor && subpath.len() < 3 {
@@ -576,7 +577,20 @@ impl Fsm for PenToolFsmState {
 		tool_options: &Self::ToolOptions,
 		responses: &mut VecDeque<Message>,
 	) -> Self {
-		let transform = tool_data.path.as_ref().and_then(|path| document.document_legacy.multiply_transforms(path).ok()).unwrap_or_default();
+		let mut transform = tool_data.path.as_ref().and_then(|path| document.document_legacy.multiply_transforms(path).ok()).unwrap_or_default();
+
+		if !transform.inverse().is_finite() {
+			let parent_transform = tool_data
+				.path
+				.as_ref()
+				.and_then(|layer_path| document.document_legacy.multiply_transforms(&layer_path[..layer_path.len() - 1]).ok());
+
+			transform = parent_transform.unwrap_or(DAffine2::IDENTITY);
+		}
+
+		if !transform.inverse().is_finite() {
+			transform = DAffine2::IDENTITY;
+		}
 
 		if let ToolMessage::Pen(event) = event {
 			match (self, event) {
@@ -753,8 +767,8 @@ fn should_extend(document: &DocumentMessageHandler, pos: DVec2, tolerance: f64)
 	for layer_path in document.selected_layers() {
 		let Ok(viewspace) = document.document_legacy.generate_transform_relative_to_viewport(layer_path) else { continue };
 
-		let Some(vector_data) = document.document_legacy.layer(layer_path).ok().and_then(|layer| layer.as_vector_data()) else { continue };
+		let subpaths = get_subpaths(layer_path, document)?;
-		for (subpath_index, subpath) in vector_data.subpaths.iter().enumerate() {
+		for (subpath_index, subpath) in subpaths.iter().enumerate() {
 			if subpath.closed() {
 				continue;
 			}
@@ -774,3 +788,19 @@ fn should_extend(document: &DocumentMessageHandler, pos: DVec2, tolerance: f64)
 
 	best
 }
+
+fn get_subpaths<'a>(layer_path: &[LayerId], document: &'a DocumentMessageHandler) -> Option<&'a Vec<Subpath<ManipulatorGroupId>>> {
+	let layer = document.document_legacy.layer(layer_path).ok().and_then(|layer| layer.as_layer().ok())?;
+	let network = &layer.network;
+	for (node, _node_id) in network.primary_flow() {
+		if node.name == "Path Generator" {
+			let subpaths_input = node.inputs.get(0)?;
+			let NodeInput::Value { tagged_value: TaggedValue::Subpaths(subpaths), .. } = subpaths_input else {
+				continue;
+			};
+
+			return Some(subpaths);
+		}
+	}
+	None
+}

libraries/bezier-rs/src/subpath/manipulators.rs

@@ -31,11 +31,13 @@ impl<ManipulatorGroupId: crate::Identifier> Subpath<ManipulatorGroupId> {
 
 	/// Insert a manipulator group at an index.
 	pub fn insert_manipulator_group(&mut self, index: usize, group: ManipulatorGroup<ManipulatorGroupId>) {
+		assert!(group.is_finite(), "Inserting non finite manipulator group");
 		self.manipulator_groups.insert(index, group)
 	}
 
 	/// Push a manipulator group to the end.
 	pub fn push_manipulator_group(&mut self, group: ManipulatorGroup<ManipulatorGroupId>) {
+		assert!(group.is_finite(), "Pushing non finite manipulator group");
 		self.manipulator_groups.push(group)
 	}
 

libraries/bezier-rs/src/subpath/structs.rs

@@ -106,6 +106,11 @@ impl<ManipulatorGroupId: crate::Identifier> ManipulatorGroup<ManipulatorGroupId>
 		self.in_handle = self.in_handle.map(|in_handle| affine_transform.transform_point2(in_handle));
 		self.out_handle = self.out_handle.map(|out_handle| affine_transform.transform_point2(out_handle));
 	}
+
+	/// Are all handles at finite positions
+	pub fn is_finite(&self) -> bool {
+		self.anchor.is_finite() && self.in_handle.map_or(true, |handle| handle.is_finite()) && self.out_handle.map_or(true, |handle| handle.is_finite())
+	}
 }
 
 #[derive(Copy, Clone)]