Close subpaths when computing area for expansion (#580)

Previously the area() method on the Shape trait was used to compute the
area for the purposes of determining the orientation for expansion.
However, this method is not valid unless the input has closed subpaths,
which can lead to inconsistent results.

This patch adds an iterator to close subpaths, and computes area based
on that. The iterator is made publicly available, as it may be handy.

Author: raphlinus

kurbo/src/bezpath.rs

@@ -793,6 +793,73 @@ impl Mul<&BezPath> for TranslateScale {
     }
 }
 
+/// Close all open subpaths in a path, expressed as iterator transform.
+pub(crate) fn close_subpaths<I>(elements: I) -> CloseSubpaths<I::IntoIter>
+where
+    I: IntoIterator<Item = PathEl>,
+{
+    CloseSubpaths {
+        elements: elements.into_iter(),
+        state: CloseSubpathState::Start,
+    }
+}
+
+/// An iterator that closes all open subpaths.
+///
+/// This struct is created by the [`close_subpaths`] function.
+#[derive(Clone)]
+pub(crate) struct CloseSubpaths<I: Iterator<Item = PathEl>> {
+    elements: I,
+    state: CloseSubpathState,
+}
+
+#[derive(Clone)]
+enum CloseSubpathState {
+    Start,
+    InSubpath,
+    ClosedLast,
+    PendingMoveTo(Point),
+}
+
+impl<I: Iterator<Item = PathEl>> Iterator for CloseSubpaths<I> {
+    type Item = PathEl;
+
+    fn next(&mut self) -> Option<PathEl> {
+        match self.state {
+            CloseSubpathState::Start => {
+                let el = self.elements.next();
+                if !matches!(el, Some(PathEl::ClosePath) | None) {
+                    self.state = CloseSubpathState::InSubpath;
+                }
+                el
+            }
+            CloseSubpathState::InSubpath => {
+                let el = self.elements.next();
+                match el {
+                    None => {
+                        self.state = CloseSubpathState::ClosedLast;
+                        Some(PathEl::ClosePath)
+                    }
+                    Some(PathEl::MoveTo(point)) => {
+                        self.state = CloseSubpathState::PendingMoveTo(point);
+                        Some(PathEl::ClosePath)
+                    }
+                    Some(PathEl::ClosePath) => {
+                        self.state = CloseSubpathState::Start;
+                        el
+                    }
+                    _ => el,
+                }
+            }
+            CloseSubpathState::ClosedLast => None,
+            CloseSubpathState::PendingMoveTo(point) => {
+                self.state = CloseSubpathState::Start;
+                Some(PathEl::MoveTo(point))
+            }
+        }
+    }
+}
+
 /// Transform an iterator over path elements into one over path
 /// segments.
 ///
@@ -2183,4 +2250,62 @@ mod tests {
         assert!(bez.contains((100.0, 299.9).into()));
         assert!(bez.contains((100.0, 300.0).into()));
     }
+
+    #[test]
+    fn check_close_subpaths() {
+        let mut bez = BezPath::new();
+        bez.move_to((10.0, 10.0));
+        bez.line_to((100.0, 20.0));
+        bez.line_to((60.0, 100.0));
+        let elements = close_subpaths(&bez).collect::<Vec<_>>();
+        bez.close_path();
+        assert_eq!(&elements, bez.elements());
+
+        // Test implicit closepath in middle of path
+        let mut bez2 = BezPath::new();
+        bez2.move_to((10.0, 10.0));
+        bez2.line_to((100.0, 20.0));
+        bez2.line_to((60.0, 100.0));
+        bez2.move_to((110.0, 10.0));
+        bez2.line_to((200.0, 20.0));
+        bez2.line_to((160.0, 100.0));
+        let elements2 = close_subpaths(&bez2).collect::<Vec<_>>();
+        let mut bez3 = BezPath::new();
+        bez3.move_to((10.0, 10.0));
+        bez3.line_to((100.0, 20.0));
+        bez3.line_to((60.0, 100.0));
+        bez3.close_path();
+        bez3.move_to((110.0, 10.0));
+        bez3.line_to((200.0, 20.0));
+        bez3.line_to((160.0, 100.0));
+        bez3.close_path();
+        assert_eq!(&elements2, bez3.elements());
+    }
+
+    #[test]
+    fn close_subpaths_does_not_duplicate_existing_closepath() {
+        let path = BezPath::from_vec(vec![
+            PathEl::MoveTo((0.0, 0.0).into()),
+            PathEl::LineTo((10.0, 0.0).into()),
+            PathEl::LineTo((10.0, 10.0).into()),
+            PathEl::ClosePath,
+            PathEl::MoveTo((20.0, 0.0).into()),
+            PathEl::LineTo((30.0, 0.0).into()),
+        ]);
+
+        let closed = close_subpaths(path.iter()).collect::<Vec<_>>();
+
+        assert_eq!(
+            closed,
+            vec![
+                PathEl::MoveTo((0.0, 0.0).into()),
+                PathEl::LineTo((10.0, 0.0).into()),
+                PathEl::LineTo((10.0, 10.0).into()),
+                PathEl::ClosePath,
+                PathEl::MoveTo((20.0, 0.0).into()),
+                PathEl::LineTo((30.0, 0.0).into()),
+                PathEl::ClosePath,
+            ]
+        );
+    }
 }

kurbo/src/expand.rs

@@ -3,7 +3,7 @@
 
 use crate::{
     Affine, Arc, BezPath, CubicBez, Join, ParamCurve, ParamCurveDeriv, PathEl, PathSeg, Point,
-    QuadBez, Shape, Vec2,
+    QuadBez, Shape, Vec2, bezpath::close_subpaths, segments,
 };
 
 #[cfg(not(feature = "std"))]
@@ -131,7 +131,7 @@ pub fn expand_path(
     miter_limit: f64,
     tolerance: f64,
 ) -> BezPath {
-    if path.area() >= 0.0 {
+    if segments(close_subpaths(path.path_elements(tolerance))).area() >= 0.0 {
         expand = -expand;
     }
     expand_path_signed(path, expand, join, miter_limit, tolerance)
@@ -584,4 +584,21 @@ mod tests {
 
         assert_eq!(actual, expected);
     }
+
+    #[test]
+    fn expand_open_subpath_uses_implicit_close_for_area_sign() {
+        let mut open = BezPath::new();
+        open.move_to((100.0, 100.0));
+        open.line_to((110.0, 100.0));
+        open.line_to((100.0, 110.0));
+
+        let mut closed = open.clone();
+        closed.close_path();
+
+        let expand = Diagonal2::new(1.0, 1.0);
+        let open_expanded = expand_path(open, expand, Join::Miter, 4.0, 1e-3);
+        let closed_expanded = expand_path(closed, expand, Join::Miter, 4.0, 1e-3);
+
+        assert_eq!(open_expanded, closed_expanded);
+    }
 }