From abd8e7fab6612179a0ae41012135ddba58f6a389 Mon Sep 17 00:00:00 2001 From: 0HyperCube <78500760+0HyperCube@users.noreply.github.com> Date: Sun, 4 Jun 2023 04:51:07 +0100 Subject: [PATCH] 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 --- .../transform_utils.rs | 1 + .../common_functionality/overlay_renderer.rs | 50 ++++++++++------ .../messages/tool/tool_messages/pen_tool.rs | 60 ++++++++++++++----- .../bezier-rs/src/subpath/manipulators.rs | 2 + libraries/bezier-rs/src/subpath/structs.rs | 5 ++ 5 files changed, 84 insertions(+), 34 deletions(-) diff --git a/editor/src/messages/portfolio/document/node_graph/graph_operation_message_handler/transform_utils.rs b/editor/src/messages/portfolio/document/node_graph/graph_operation_message_handler/transform_utils.rs index 57938d83..5dd99558 100644 --- a/editor/src/messages/portfolio/document/node_graph/graph_operation_message_handler/transform_utils.rs +++ b/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 { diff --git a/editor/src/messages/tool/common_functionality/overlay_renderer.rs b/editor/src/messages/tool/common_functionality/overlay_renderer.rs index 1d8eb0d0..8486babe 100644 --- a/editor/src/messages/tool/common_functionality/overlay_renderer.rs +++ b/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>, - manipulator_group_overlay_cache: HashMap<(LayerId, ManipulatorGroupId), ManipulatorGroupOverlays>, + manipulator_group_overlay_cache: HashMap>, } 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) { - if let Some(vector_data) = layer.as_vector_data() { - for manipulator_group in vector_data.manipulator_groups() { - let id = manipulator_group.id; - 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)); - } - } - } + let Some(layer_cache) = self.manipulator_group_overlay_cache.remove(layer_id) else { return }; + + for manipulator_group_overlays in layer_cache.values() { + Self::remove_manipulator_group_overlays(manipulator_group_overlays, responses); } } @@ -142,15 +149,20 @@ impl OverlayRenderer { } // Hide the manipulator group overlays - if let Ok(layer) = document.layer(&layer_path) { - if let Some(vector_data) = layer.as_vector_data() { - for manipulator_group in vector_data.manipulator_groups() { - let id = manipulator_group.id; - if let Some(manipulator_group_overlays) = self.manipulator_group_overlay_cache.get(&(*layer_id, id)) { - Self::set_manipulator_group_overlay_visibility(manipulator_group_overlays, visibility, responses); - } + let Some(manipulator_groups) = self.manipulator_group_overlay_cache.get(layer_id) else { return }; + if visibility { + let Ok(layer) = document.layer(&layer_path) else { return }; + let Some(vector_data) = layer.as_vector_data() else { return }; + for manipulator_group in vector_data.manipulator_groups() { + 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); + } } } diff --git a/editor/src/messages/tool/tool_messages/pen_tool.rs b/editor/src/messages/tool/tool_messages/pen_tool.rs index 98013faa..ccd3d0ae 100644 --- a/editor/src/messages/tool/tool_messages/pen_tool.rs +++ b/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 manipulator_groups = vector_data.subpaths[subpath_index].manipulator_groups(); + let Some(subpaths) = get_subpaths(layer, document) else { return }; + 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) -> 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 = &vector_data.subpaths[self.subpath_index]; + let subpath = &get_subpaths(layer_path, document)?[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) -> 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 = &vector_data.subpaths[self.subpath_index]; + let subpath = &get_subpaths(layer_path, document)?[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) -> 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 = &vector_data.subpaths[self.subpath_index]; + let subpath = &get_subpaths(layer_path, document)?[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) -> 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 = &vector_data.subpaths[self.subpath_index]; + let subpath = &get_subpaths(layer_path, document)?[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) -> 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 = &vector_data.subpaths[self.subpath_index]; + let subpath = &get_subpaths(layer_path, document)?[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, ) -> 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 }; - for (subpath_index, subpath) in vector_data.subpaths.iter().enumerate() { + let subpaths = get_subpaths(layer_path, document)?; + 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>> { + 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 +} diff --git a/libraries/bezier-rs/src/subpath/manipulators.rs b/libraries/bezier-rs/src/subpath/manipulators.rs index 9d35da87..336b81c8 100644 --- a/libraries/bezier-rs/src/subpath/manipulators.rs +++ b/libraries/bezier-rs/src/subpath/manipulators.rs @@ -31,11 +31,13 @@ impl Subpath { /// Insert a manipulator group at an index. pub fn insert_manipulator_group(&mut self, index: usize, group: ManipulatorGroup) { + 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) { + assert!(group.is_finite(), "Pushing non finite manipulator group"); self.manipulator_groups.push(group) } diff --git a/libraries/bezier-rs/src/subpath/structs.rs b/libraries/bezier-rs/src/subpath/structs.rs index 89728010..8b0ce22f 100644 --- a/libraries/bezier-rs/src/subpath/structs.rs +++ b/libraries/bezier-rs/src/subpath/structs.rs @@ -106,6 +106,11 @@ impl ManipulatorGroup 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)]