From 5093b59f72e7ab079baade6c4959b4dc2599aae6 Mon Sep 17 00:00:00 2001 From: Sameer Puri Date: Thu, 21 May 2026 21:44:44 -0700 Subject: [PATCH] Address stroke containment bug in polygon fill I was using the start point but it really should've been bbox --- star/src/lower/mod.rs | 1 - star/src/turtle/elements/fill.rs | 169 ++++++++++++++++++++++--------- star/src/turtle/elements/mod.rs | 11 ++ 3 files changed, 133 insertions(+), 48 deletions(-) diff --git a/star/src/lower/mod.rs b/star/src/lower/mod.rs index c8009bb..9b91093 100644 --- a/star/src/lower/mod.rs +++ b/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"}]}"#; diff --git a/star/src/turtle/elements/fill.rs b/star/src/turtle/elements/fill.rs index c696433..83b5d99 100644 --- a/star/src/turtle/elements/fill.rs +++ b/star/src/turtle/elements/fill.rs @@ -11,7 +11,6 @@ fn cross(a: Point, b: Point) -> 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,7 +50,7 @@ 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. @@ -59,15 +58,15 @@ fn signed_area(stroke: &Stroke) -> f64 { /// 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, to: Point, point: Point) -> 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) -> 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, fill_rule: FillRule) -> Vec { if subpaths.is_empty() { return vec![]; } - let areas: Vec = subpaths.iter().map(signed_area).collect(); - let starts: Vec> = 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> = (0..subpaths.len()) + // For each subpath, the indices of all other subpaths that enclose it. + let containers: 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> = match fill_rule { + FillRule::EvenOdd => containers + .iter() + .map(|containing| Some(containing.len().is_multiple_of(2))) + .collect(), + FillRule::NonZero => { + let areas: Vec = 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); + } } diff --git a/star/src/turtle/elements/mod.rs b/star/src/turtle/elements/mod.rs index fa2975c..ecab444 100644 --- a/star/src/turtle/elements/mod.rs +++ b/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 { + 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 + } }