From 386c970b605fa8ccba20a6f42278baa7ae0a7011 Mon Sep 17 00:00:00 2001 From: Paul Kupper <11900073+pkupper@users.noreply.github.com> Date: Sun, 6 Feb 2022 23:06:23 +0100 Subject: [PATCH] Fix shape not closed after using Fill Tool (#510) * Always set shape property 'closed' on fill * Remove closed property on Shape * Make color mandatory in Fill * Fix intersection for filled but open shapes * Code style tweak * Add TODO note to rework ClosePath check --- .../src/document/document_message_handler.rs | 3 ++- editor/src/viewport_tools/tools/eyedropper.rs | 10 ++++----- editor/src/viewport_tools/tools/freehand.rs | 2 +- editor/src/viewport_tools/tools/path.rs | 4 ++-- editor/src/viewport_tools/tools/pen.rs | 2 +- editor/src/viewport_tools/tools/select.rs | 2 +- editor/src/viewport_tools/tools/text.rs | 2 +- graphene/src/intersection.rs | 12 ++++++++--- graphene/src/layers/simple_shape.rs | 21 +++---------------- graphene/src/layers/style/mod.rs | 21 +++++++------------ 10 files changed, 32 insertions(+), 47 deletions(-) diff --git a/editor/src/document/document_message_handler.rs b/editor/src/document/document_message_handler.rs index d5776034..f8ef8f60 100644 --- a/editor/src/document/document_message_handler.rs +++ b/editor/src/document/document_message_handler.rs @@ -151,7 +151,8 @@ impl DocumentMessageHandler { }; let (path, closed) = match &layer.ok()?.data { - LayerDataType::Shape(shape) => Some((shape.path.clone(), shape.closed)), + // TODO: This ClosePath check does not handle all cases, fix this soon + LayerDataType::Shape(shape) => Some((shape.path.clone(), shape.path.elements().last() == Some(&kurbo::PathEl::ClosePath))), LayerDataType::Text(text) => Some((text.to_bez_path_nonmut(), true)), _ => None, }?; diff --git a/editor/src/viewport_tools/tools/eyedropper.rs b/editor/src/viewport_tools/tools/eyedropper.rs index 468a3bab..e6a73544 100644 --- a/editor/src/viewport_tools/tools/eyedropper.rs +++ b/editor/src/viewport_tools/tools/eyedropper.rs @@ -102,12 +102,10 @@ impl Fsm for EyedropperToolFsmState { if let Ok(layer) = document.graphene_document.layer(path) { if let LayerDataType::Shape(shape) = &layer.data { if let Some(fill) = shape.style.fill() { - if let Some(color) = fill.color() { - match lmb_or_rmb { - EyedropperMessage::LeftMouseDown => responses.push_back(ToolMessage::SelectPrimaryColor { color }.into()), - EyedropperMessage::RightMouseDown => responses.push_back(ToolMessage::SelectSecondaryColor { color }.into()), - _ => {} - } + match lmb_or_rmb { + EyedropperMessage::LeftMouseDown => responses.push_back(ToolMessage::SelectPrimaryColor { color: fill.color() }.into()), + EyedropperMessage::RightMouseDown => responses.push_back(ToolMessage::SelectSecondaryColor { color: fill.color() }.into()), + _ => {} } } } diff --git a/editor/src/viewport_tools/tools/freehand.rs b/editor/src/viewport_tools/tools/freehand.rs index 3bf96b9b..7e2fe6cb 100644 --- a/editor/src/viewport_tools/tools/freehand.rs +++ b/editor/src/viewport_tools/tools/freehand.rs @@ -225,7 +225,7 @@ fn make_operation(data: &FreehandToolData, tool_data: &DocumentToolData) -> Mess insert_index: -1, transform: DAffine2::IDENTITY.to_cols_array(), points, - style: style::PathStyle::new(Some(style::Stroke::new(tool_data.primary_color, data.weight as f32)), Some(style::Fill::none())), + style: style::PathStyle::new(Some(style::Stroke::new(tool_data.primary_color, data.weight as f32)), None), } .into() } diff --git a/editor/src/viewport_tools/tools/path.rs b/editor/src/viewport_tools/tools/path.rs index 9520989d..13398507 100644 --- a/editor/src/viewport_tools/tools/path.rs +++ b/editor/src/viewport_tools/tools/path.rs @@ -612,7 +612,7 @@ fn add_anchor_handle_line(responses: &mut VecDeque) -> Vec { let operation = Operation::AddOverlayLine { path: layer_path.clone(), transform: DAffine2::IDENTITY.to_cols_array(), - style: style::PathStyle::new(Some(Stroke::new(COLOR_ACCENT, 1.0)), Some(Fill::none())), + style: style::PathStyle::new(Some(Stroke::new(COLOR_ACCENT, 1.0)), None), }; responses.push_back(DocumentMessage::Overlays(operation.into()).into()); @@ -625,7 +625,7 @@ fn add_shape_outline(responses: &mut VecDeque) -> Vec { let operation = Operation::AddOverlayShape { path: layer_path.clone(), bez_path: BezPath::default(), - style: style::PathStyle::new(Some(Stroke::new(COLOR_ACCENT, 1.0)), Some(Fill::none())), + style: style::PathStyle::new(Some(Stroke::new(COLOR_ACCENT, 1.0)), None), closed: false, }; responses.push_back(DocumentMessage::Overlays(operation.into()).into()); diff --git a/editor/src/viewport_tools/tools/pen.rs b/editor/src/viewport_tools/tools/pen.rs index e81e4adb..f6c99d57 100644 --- a/editor/src/viewport_tools/tools/pen.rs +++ b/editor/src/viewport_tools/tools/pen.rs @@ -264,7 +264,7 @@ fn make_operation(data: &PenToolData, tool_data: &DocumentToolData, show_preview insert_index: -1, transform: DAffine2::IDENTITY.to_cols_array(), points, - style: style::PathStyle::new(Some(style::Stroke::new(tool_data.primary_color, data.weight as f32)), Some(style::Fill::none())), + style: style::PathStyle::new(Some(style::Stroke::new(tool_data.primary_color, data.weight as f32)), None), } .into() } diff --git a/editor/src/viewport_tools/tools/select.rs b/editor/src/viewport_tools/tools/select.rs index 0b8e8dd4..d58b1ae7 100644 --- a/editor/src/viewport_tools/tools/select.rs +++ b/editor/src/viewport_tools/tools/select.rs @@ -367,7 +367,7 @@ fn add_bounding_box(responses: &mut Vec) -> Vec { let operation = Operation::AddOverlayRect { path: path.clone(), transform: DAffine2::ZERO.to_cols_array(), - style: style::PathStyle::new(Some(Stroke::new(COLOR_ACCENT, 1.0)), Some(Fill::none())), + style: style::PathStyle::new(Some(Stroke::new(COLOR_ACCENT, 1.0)), None), }; responses.push(DocumentMessage::Overlays(operation.into()).into()); diff --git a/editor/src/viewport_tools/tools/text.rs b/editor/src/viewport_tools/tools/text.rs index 5b62c7fd..8d54690c 100644 --- a/editor/src/viewport_tools/tools/text.rs +++ b/editor/src/viewport_tools/tools/text.rs @@ -150,7 +150,7 @@ fn resize_overlays(overlays: &mut Vec>, responses: &mut VecDeque Point { Point::new(vec.x, vec.y) } -pub fn intersect_quad_bez_path(quad: Quad, shape: &BezPath, closed: bool) -> bool { +pub fn intersect_quad_bez_path(quad: Quad, shape: &BezPath, filled: bool) -> bool { + let mut shape = shape.clone(); + // for filled shapes act like shape was closed even if it isn't + if filled && shape.elements().last() != Some(&kurbo::PathEl::ClosePath) { + shape.close_path(); + } + // check if outlines intersect if shape.segments().any(|path_segment| quad.lines().iter().any(|line| !path_segment.intersect_line(*line).is_empty())) { return true; } // check if selection is entirely within the shape - if closed && shape.contains(to_point(quad.0[0])) { + if filled && shape.contains(to_point(quad.0[0])) { return true; } // check if shape is entirely within selection - get_arbitrary_point_on_path(shape).map(|shape_point| quad.path().contains(shape_point)).unwrap_or_default() + get_arbitrary_point_on_path(&shape).map(|shape_point| quad.path().contains(shape_point)).unwrap_or_default() } pub fn get_arbitrary_point_on_path(path: &BezPath) -> Option { diff --git a/graphene/src/layers/simple_shape.rs b/graphene/src/layers/simple_shape.rs index 9f53a42f..7666593e 100644 --- a/graphene/src/layers/simple_shape.rs +++ b/graphene/src/layers/simple_shape.rs @@ -17,7 +17,6 @@ pub struct Shape { pub path: BezPath, pub style: style::PathStyle, pub render_index: i32, - pub closed: bool, } impl LayerData for Shape { @@ -54,7 +53,7 @@ impl LayerData for Shape { } fn intersects_quad(&self, quad: Quad, path: &mut Vec, intersections: &mut Vec>) { - if intersect_quad_bez_path(quad, &self.path, self.closed) { + if intersect_quad_bez_path(quad, &self.path, self.style.fill().is_some()) { intersections.push(path.clone()); } } @@ -75,7 +74,6 @@ impl Shape { path: bez_path, style, render_index: 1, - closed, } } @@ -104,12 +102,7 @@ impl Shape { path.close_path(); - Self { - path, - style, - render_index: 1, - closed: true, - } + Self { path, style, render_index: 1 } } pub fn rectangle(style: PathStyle) -> Self { @@ -117,7 +110,6 @@ impl Shape { path: kurbo::Rect::new(0., 0., 1., 1.).to_path(0.01), style, render_index: 1, - closed: true, } } @@ -126,7 +118,6 @@ impl Shape { path: kurbo::Ellipse::from_rect(kurbo::Rect::new(0., 0., 1., 1.)).to_path(0.01), style, render_index: 1, - closed: true, } } @@ -135,7 +126,6 @@ impl Shape { path: kurbo::Line::new((0., 0.), (1., 0.)).to_path(0.01), style, render_index: 1, - closed: false, } } @@ -148,11 +138,6 @@ impl Shape { .enumerate() .for_each(|(i, p)| if i == 0 { path.move_to(p) } else { path.line_to(p) }); - Self { - path, - style, - render_index: 0, - closed: false, - } + Self { path, style, render_index: 0 } } } diff --git a/graphene/src/layers/style/mod.rs b/graphene/src/layers/style/mod.rs index 6ecc1feb..7a9d4a9c 100644 --- a/graphene/src/layers/style/mod.rs +++ b/graphene/src/layers/style/mod.rs @@ -29,25 +29,21 @@ impl Default for ViewMode { #[repr(C)] #[derive(Debug, Clone, Copy, PartialEq, Default, Serialize, Deserialize)] pub struct Fill { - color: Option, + color: Color, } impl Fill { pub fn new(color: Color) -> Self { - Self { color: Some(color) } + Self { color } } - pub fn color(&self) -> Option { + pub fn color(&self) -> Color { self.color } - pub const fn none() -> Self { - Self { color: None } - } - - pub fn render(&self) -> String { - match self.color { - Some(c) => format!(r##" fill="#{}"{}"##, c.rgb_hex(), format_opacity("fill", c.a())), + pub fn render(fill: Option) -> String { + match fill { + Some(c) => format!(r##" fill="#{}"{}"##, c.color.rgb_hex(), format_opacity("fill", c.color.a())), None => r#" fill="none""#.to_string(), } } @@ -116,9 +112,8 @@ impl PathStyle { pub fn render(&self, view_mode: ViewMode) -> String { let fill_attribute = match (view_mode, self.fill) { - (ViewMode::Outline, _) => Fill::none().render(), - (_, Some(fill)) => fill.render(), - (_, None) => String::new(), + (ViewMode::Outline, _) => Fill::render(None), + (_, fill) => Fill::render(fill), }; let stroke_attribute = match (view_mode, self.stroke) { (ViewMode::Outline, _) => Stroke::new(LAYER_OUTLINE_STROKE_COLOR, LAYER_OUTLINE_STROKE_WIDTH).render(),