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

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,
 			}?;

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 {
-										match lmb_or_rmb {
+										EyedropperMessage::LeftMouseDown => responses.push_back(ToolMessage::SelectPrimaryColor { color: fill.color() }.into()),
-											EyedropperMessage::LeftMouseDown => responses.push_back(ToolMessage::SelectPrimaryColor { color }.into()),
+										EyedropperMessage::RightMouseDown => responses.push_back(ToolMessage::SelectSecondaryColor { color: fill.color() }.into()),
-											EyedropperMessage::RightMouseDown => responses.push_back(ToolMessage::SelectSecondaryColor { color }.into()),
+										_ => {}
-											_ => {}
-										}
 									}
 								}
 							}

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()
 }

editor/src/viewport_tools/tools/path.rs

@@ -612,7 +612,7 @@ fn add_anchor_handle_line(responses: &mut VecDeque<Message>) -> Vec<LayerId> {
 	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<Message>) -> Vec<LayerId> {
 	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());

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()
 }

editor/src/viewport_tools/tools/select.rs

@@ -367,7 +367,7 @@ fn add_bounding_box(responses: &mut Vec<Message>) -> Vec<LayerId> {
 	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());
 

editor/src/viewport_tools/tools/text.rs

@@ -150,7 +150,7 @@ fn resize_overlays(overlays: &mut Vec<Vec<LayerId>>, responses: &mut VecDeque<Me
 		let operation = Operation::AddOverlayRect {
 			path,
 			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_back(DocumentMessage::Overlays(operation.into()).into());
 	}

graphene/src/intersection.rs

@@ -47,18 +47,24 @@ fn to_point(vec: DVec2) -> 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<Point> {

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<LayerId>, intersections: &mut Vec<Vec<LayerId>>) {
-		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 {
+		Self { path, style, render_index: 1 }
-			path,
-			style,
-			render_index: 1,
-			closed: true,
-		}
 	}
 
 	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 {
+		Self { path, style, render_index: 0 }
-			path,
-			style,
-			render_index: 0,
-			closed: false,
-		}
 	}
 }

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: Color,
 }
 
 impl Fill {
 	pub fn new(color: Color) -> Self {
-		Self { color: Some(color) }
+		Self { color }
 	}
 
-	pub fn color(&self) -> Option<Color> {
+	pub fn color(&self) -> Color {
 		self.color
 	}
 
-	pub const fn none() -> Self {
+	pub fn render(fill: Option<Fill>) -> String {
-		Self { color: None }
+		match fill {
-	}
+			Some(c) => format!(r##" fill="#{}"{}"##, c.color.rgb_hex(), format_opacity("fill", c.color.a())),
-
-	pub fn render(&self) -> String {
-		match self.color {
-			Some(c) => format!(r##" fill="#{}"{}"##, c.rgb_hex(), format_opacity("fill", c.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(),
+			(ViewMode::Outline, _) => Fill::render(None),
-			(_, Some(fill)) => fill.render(),
+			(_, fill) => Fill::render(fill),
-			(_, None) => String::new(),
 		};
 		let stroke_attribute = match (view_mode, self.stroke) {
 			(ViewMode::Outline, _) => Stroke::new(LAYER_OUTLINE_STROKE_COLOR, LAYER_OUTLINE_STROKE_WIDTH).render(),