Address stroke containment bug in polygon fill (#119)

I was using the start point but it really should've been bbox

Author: sameer

star/src/lower/mod.rs

@@ -339,7 +339,6 @@ mod test {
                     unit: LengthUnit::In,
                 }),
             ],
-            ..Default::default()
         };
         let json = r#"{"dimensions":[{"number":4.0,"unit":"Mm"},{"number":10.5,"unit":"In"}]}"#;
 

star/src/turtle/elements/fill.rs

@@ -11,7 +11,6 @@ fn cross(a: Point<f64>, b: Point<f64>) -> f64 {
 /// Signed area of a closed subpath using Green's theorem.
 /// A negative area means clockwise winding.
 ///
-///
 /// Lines and beziers follow kurbo's [`ParamCurveArea`] formulas;
 /// the elliptical arc contribution is a direct integration of the parametric ellipse.
 ///
@@ -51,23 +50,23 @@ fn signed_area(stroke: &Stroke) -> f64 {
     if !stroke.is_closed() {
         area += cross(stroke.end_point(), stroke.start_point());
     }
-    area * 0.5
+    area / 2.
 }
 
 /// Returns true if `from` => `to` crosses a ray cast from `point` rightwards.
 ///
 /// Used as the per-segment primitive for ray-casting. The `>= max_y` exclusion on the upper
 /// endpoint ensures a shared vertex between two segments is counted only once.
 fn edge_crosses_ray(from: Point<f64>, to: Point<f64>, point: Point<f64>) -> bool {
-    let (min_y, max_y) = if from.y <= to.y {
-        (from.y, to.y)
-    } else {
-        (to.y, from.y)
-    };
-    if point.y < min_y || point.y >= max_y {
+    let (min_y, max_y) = (from.y.min(to.y), from.y.max(to.y));
+
+    // >= max_y implicitly encoded here
+    if !(min_y..max_y).contains(&point.y) {
         return false;
     }
     let t = (point.y - from.y) / (to.y - from.y);
+
+    // Ray intersects because the edge x is greater
     from.x + t * (to.x - from.x) > point.x
 }
 
@@ -122,73 +121,94 @@ fn stroke_contains_point(stroke: &Stroke, point: Point<f64>) -> bool {
     !crossings.is_multiple_of(2)
 }
 
-/// Partitions raw SVG subpaths into [`FillPolygon`]s — one per outer contour — with holes
+fn stroke_contains_stroke(outer: &Stroke, inner: &Stroke) -> bool {
+    let outer_bbox = outer.bounding_box();
+    let inner_bbox = inner.bounding_box();
+    if !outer_bbox.contains_box(&inner_bbox) {
+        return false;
+    }
+    if !stroke_contains_point(outer, inner.start_point()) {
+        return false;
+    }
+    for cmd in inner.commands() {
+        if let Some(to) = cmd.end_point()
+            && !stroke_contains_point(outer, to)
+        {
+            return false;
+        }
+    }
+    true
+}
+
+/// Partitions raw SVG subpaths into [`FillPolygon`]s, one per outer contour, with holes
 /// assigned to their closest enclosing outer.
 ///
-/// The SVG `fill-rule` is consumed here: for `EvenOdd`, nesting depth determines outer vs. hole
-/// (even depth → outer); for `NonZero`, the cumulative signed winding of enclosing subpaths
-/// determines it (zero cumulative winding before entering → outer).
+/// `fill_rule` is flattened here:
+/// - `EvenOdd`: even = outer
+/// - `NonZero`: 0 cumulative winding = outer
 pub(crate) fn into_fill_polygons(subpaths: Vec<Stroke>, fill_rule: FillRule) -> Vec<FillPolygon> {
     if subpaths.is_empty() {
         return vec![];
     }
 
-    let areas: Vec<f64> = subpaths.iter().map(signed_area).collect();
-    let starts: Vec<Point<f64>> = subpaths.iter().map(|s| s.start_point()).collect();
-
-    // For each subpath, the indices of all other subpaths that contain its start point.
-    let containers: Vec<Vec<usize>> = (0..subpaths.len())
+    // For each subpath, the indices of all other subpaths that enclose it.
+    let containers: Vec<Vec<_>> = (0..subpaths.len())
         .map(|i| {
             (0..subpaths.len())
                 .filter(|&j| j != i)
-                .filter(|&j| stroke_contains_point(&subpaths[j], starts[i]))
+                .filter(|&j| stroke_contains_stroke(&subpaths[j], &subpaths[i]))
                 .collect()
         })
         .collect();
 
     // Classify each subpath as outer (contributes filled area) or hole (removes it).
-    let mut is_outer = vec![false; subpaths.len()];
-    let mut is_hole = vec![false; subpaths.len()];
+    let is_outer: Vec<Option<bool>> = match fill_rule {
+        FillRule::EvenOdd => containers
+            .iter()
+            .map(|containing| Some(containing.len().is_multiple_of(2)))
+            .collect(),
+        FillRule::NonZero => {
+            let areas: Vec<f64> = subpaths.iter().map(signed_area).collect();
 
-    for i in 0..subpaths.len() {
-        match fill_rule {
-            FillRule::EvenOdd => {
-                if containers[i].len().is_multiple_of(2) {
-                    is_outer[i] = true;
-                } else {
-                    is_hole[i] = true;
-                }
-            }
-            FillRule::NonZero => {
-                let cumulative_winding: i32 = containers[i]
-                    .iter()
-                    .map(|&j| if areas[j] > 0.0 { 1i32 } else { -1i32 })
-                    .sum();
+            containers
+                .iter()
+                .zip(areas.iter())
+                .map(|(container, &area)| {
+                    let cumulative_winding: i32 = container
+                        .iter()
+                        .map(|&j| if areas[j] > 0.0 { 1i32 } else { -1i32 })
+                        .sum();
 
-                if cumulative_winding == 0 {
-                    is_outer[i] = true;
-                } else {
-                    let winding_inside = cumulative_winding + if areas[i] > 0.0 { 1 } else { -1 };
-                    if winding_inside == 0 {
-                        is_hole[i] = true;
+                    if cumulative_winding == 0 {
+                        Some(true)
+                    } else {
+                        let winding_inside = cumulative_winding + if area > 0.0 { 1 } else { -1 };
+                        if winding_inside == 0 {
+                            Some(false)
+                        } else {
+                            // Ignore (why?)
+                            None
+                        }
                     }
-                }
-            }
+                })
+                .collect()
         }
-    }
+    };
 
-    // For each outer, collect its direct holes — holes for which this outer is the innermost
+    // For each outer, collect its immediate holes — holes for which this outer is the innermost
     // enclosing outer (no other outer sits between them).
     (0..subpaths.len())
-        .filter(|&i| is_outer[i])
+        .filter(|&i| is_outer[i] == Some(true))
         .map(|i| {
             let holes = (0..subpaths.len())
-                .filter(|&j| is_hole[j] && stroke_contains_point(&subpaths[i], starts[j]))
+                .filter(|&j| {
+                    is_outer[j] == Some(false) && stroke_contains_stroke(&subpaths[i], &subpaths[j])
+                })
                 .filter(|&j| {
                     // No other outer k is strictly between outer i and hole j.
                     !containers[j]
                         .iter()
-                        .any(|&k| k != i && is_outer[k] && containers[k].contains(&i))
+                        .any(|&k| k != i && is_outer[k] == Some(true) && containers[k].contains(&i))
                 })
                 .map(|j| subpaths[j].clone())
                 .collect();
@@ -288,4 +308,59 @@ mod tests {
         assert_eq!(polygons[0].outer.start_point(), s0.start_point());
         assert_eq!(polygons[0].holes.len(), 0);
     }
+
+    #[test]
+    fn test_nonzero_overlapping_not_nested() {
+        // Subpath 0: CCW square from (0,0) to (10,10)
+        let s0 = Stroke::new(
+            Point::new(0.0, 0.0),
+            vec![
+                DrawCommand::LineTo {
+                    from: Point::new(0.0, 0.0),
+                    to: Point::new(10.0, 0.0),
+                },
+                DrawCommand::LineTo {
+                    from: Point::new(10.0, 0.0),
+                    to: Point::new(10.0, 10.0),
+                },
+                DrawCommand::LineTo {
+                    from: Point::new(10.0, 10.0),
+                    to: Point::new(0.0, 10.0),
+                },
+                DrawCommand::LineTo {
+                    from: Point::new(0.0, 10.0),
+                    to: Point::new(0.0, 0.0),
+                },
+            ],
+        );
+
+        // Subpath 1: CCW square from (5,5) to (15,15) - overlapping but not nested
+        let s1 = Stroke::new(
+            Point::new(5.0, 5.0),
+            vec![
+                DrawCommand::LineTo {
+                    from: Point::new(5.0, 5.0),
+                    to: Point::new(15.0, 5.0),
+                },
+                DrawCommand::LineTo {
+                    from: Point::new(15.0, 5.0),
+                    to: Point::new(15.0, 15.0),
+                },
+                DrawCommand::LineTo {
+                    from: Point::new(15.0, 15.0),
+                    to: Point::new(5.0, 15.0),
+                },
+                DrawCommand::LineTo {
+                    from: Point::new(5.0, 15.0),
+                    to: Point::new(5.0, 5.0),
+                },
+            ],
+        );
+
+        let polygons = into_fill_polygons(vec![s0.clone(), s1.clone()], FillRule::NonZero);
+
+        // Since they overlap but are not nested (neither bbox is inside the other),
+        // they should be classified as two independent outer contours.
+        assert_eq!(polygons.len(), 2);
+    }
 }

star/src/turtle/elements/mod.rs

@@ -163,4 +163,15 @@ impl Stroke {
     pub fn is_closed(&self) -> bool {
         (self.start_point() - self.end_point()).square_length() < f64::EPSILON
     }
+
+    /// Calculate the bounding box of the stroke.
+    pub fn bounding_box(&self) -> Box2D<f64> {
+        let mut bbox = Box2D::new(self.start_point, self.start_point);
+        for command in &self.commands {
+            if let Some(b) = command.bounding_box() {
+                bbox = Box2D::from_points([bbox.min, bbox.max, b.min, b.max]);
+            }
+        }
+        bbox
+    }
 }