diff --git a/crates/engine/src/cmd/shaper.rs b/crates/engine/src/cmd/shaper.rs index 85b852e5..f8bfe1ef 100644 --- a/crates/engine/src/cmd/shaper.rs +++ b/crates/engine/src/cmd/shaper.rs @@ -90,9 +90,13 @@ fn map_of(src: &[Arc]) -> Result { } let arrangement = po::shape_builder(&shapes, true); let bounded = |pieces: Vec| pieces.into_iter().filter_map(|p| p.path.bounds().map(|b| (p, b))).collect(); + // A Shaper Group's faces are what its shapes fill and what its open paths enclose: the gaps + // closed paths enclose are the Shape Builder's only. + let open = po::has_open_path(&shapes); let regions = arrangement .regions .into_iter() + .filter(|r| open || !r.sources.is_empty()) .filter_map(|r| { let bounds = r.path.bounds()?; let path = r.path.to_bezpath(); diff --git a/crates/engine/src/tests_build.rs b/crates/engine/src/tests_build.rs index 914ad2b7..76d13763 100644 --- a/crates/engine/src/tests_build.rs +++ b/crates/engine/src/tests_build.rs @@ -152,6 +152,39 @@ fn shape_builder_regions_query() { assert!(regs.iter().any(|x| x["sources"].as_array().unwrap().len() == 2)); } +/// #1691: four circles in a 2×2 grid, each touching its neighbours, enclose a diamond nothing +/// fills. It is a region, and a drag from a circle into it merges the two. +#[test] +fn shape_builder_merges_the_gap_touching_circles_enclose() { + let mut s = session(); + let mut circles = vec![]; + for (x, y) in [(0.0, 0.0), (100.0, 0.0), (0.0, 100.0), (100.0, 100.0)] { + let r = s.execute("shape.ellipse", &json!({"x": x, "y": y, "width": 100, "height": 100})).unwrap(); + circles.push(NodeId(r["id"].as_u64().unwrap())); + } + set_fill(&mut s, circles[0], Color::rgb(1.0, 0.0, 0.0)); + s.execute("select.set", &json!({"ids": circles.iter().map(|i| i.0).collect::>()})).unwrap(); + let r = s.execute("shapeBuilder.regions", &json!({})).unwrap(); + let regs = r["regions"].as_array().unwrap(); + assert_eq!(regs.len(), 5, "{regs:?}"); + assert_eq!(regs.iter().filter(|x| x["sources"].as_array().unwrap().is_empty()).count(), 1, "the diamond"); + let r = s.execute("shapeBuilder.merge", &json!({"points": [[50, 50], [100, 100]]})).unwrap(); + assert_eq!(r["regions"], 2); + let out = ids(&r); + assert_eq!(out.len(), 4, "the merged shape and the three other circles"); + let merged = NodeId(r["merged"].as_u64().unwrap()); + // A circle (its Bézier outline: 7,854.9) and the diamond between the four (2,145.1). + assert!((area(&s, merged) - 10000.0).abs() < 10.0, "{}", area(&s, merged)); + assert_eq!(node(&s, merged).appearance.fill_paint(), Paint::solid(Color::rgb(1.0, 0.0, 0.0)), "the look of the circle the drag began on"); + assert_eq!(layer_children(&s), 4); + // A click in the diamond alone makes it a shape of its own. + s.execute("edit.undo", &json!({})).unwrap(); + s.execute("select.set", &json!({"ids": circles.iter().map(|i| i.0).collect::>()})).unwrap(); + let r = s.execute("shapeBuilder.merge", &json!({"points": [[100, 100]]})).unwrap(); + assert_eq!(ids(&r).len(), 5); + assert!((area(&s, NodeId(r["merged"].as_u64().unwrap())) - 2146.0).abs() < 10.0); +} + #[test] fn shape_builder_tool_drives_the_command() { let mut s = session(); diff --git a/crates/pathops/src/lib.rs b/crates/pathops/src/lib.rs index 0e67a96d..d531b2cb 100644 --- a/crates/pathops/src/lib.rs +++ b/crates/pathops/src/lib.rs @@ -20,7 +20,7 @@ pub use edit::{ }; pub use offset::{Cap, Join, offset_path, outline_stroke, stroke_region}; pub use pathfinder::{FaceMerger, PathfinderOp, Region, Shape, merge_regions, pathfinder, region_at, regions}; -pub use planar::{BuilderArrangement, SHAPE_BUILDER_MAX_SEGMENTS, cut_out, encloses_area, interior_point, live_paint, shape_builder}; +pub use planar::{BuilderArrangement, SHAPE_BUILDER_MAX_SEGMENTS, cut_out, encloses_area, has_open_path, interior_point, live_paint, shape_builder}; /// Errors from fallible operations. #[derive(Clone, Copy, Debug, PartialEq, Eq, thiserror::Error)] diff --git a/crates/pathops/src/pathfinder.rs b/crates/pathops/src/pathfinder.rs index 4ffbde83..7325c8fa 100644 --- a/crates/pathops/src/pathfinder.rs +++ b/crates/pathops/src/pathfinder.rs @@ -102,7 +102,7 @@ pub fn pathfinder(op: PathfinderOp, shapes: &[Shape]) -> Vec { } // Open paths are cutting edges for Divide; the filled-only operations keep their // implicit closure. Reuse Shape Builder's bounded planar map and its fallback. - if op == PathfinderOp::Divide && shapes.iter().flat_map(|s| &s.path.subpaths).any(|sp| !sp.closed && sp.anchors.len() >= 2) { + if op == PathfinderOp::Divide && crate::has_open_path(shapes) { return crate::shape_builder(shapes, false) .regions .into_iter() @@ -298,21 +298,22 @@ pub fn merge_regions(regions: &[&Region]) -> PathData { /// outlines ([`merge_regions`]) could leave those edges in, as a hairline gap or a spur, where /// neighbouring faces' outlines had been refit apart. Shapes with open paths (faces lines cut, as /// [`shape_builder`](crate::shape_builder) gives them) merge in their planar map the same way -/// (#893). Either falls back to [`merge_regions`] when it can't place a face. +/// (#893), and so do faces with a gap closed paths enclose, which no shape covers (#1691). Either +/// falls back to [`merge_regions`] when it can't place a face. pub struct FaceMerger { /// The arrangement (`None`: open paths, or the sweep failed) and the number of shapes. arr: Option, shapes: usize, - /// With open paths, the shapes, and their planar map once a merge has needed it. + /// The shapes, and their planar map once a merge has needed it (open paths, or a gap). lines: Option<(Vec, std::cell::OnceCell>)>, } impl FaceMerger { pub fn new(shapes: &[Shape]) -> Self { - let has_open = shapes.iter().flat_map(|s| &s.path.subpaths).any(|sp| !sp.closed && sp.anchors.len() >= 2); + let has_open = crate::has_open_path(shapes); // The Shape Builder's faces come from the planar map only up to its cap. let segments: usize = shapes.iter().flat_map(|s| &s.path.subpaths).map(|sp| sp.segment_count()).sum(); - let lines = (has_open && segments <= crate::SHAPE_BUILDER_MAX_SEGMENTS).then(|| (shapes.to_vec(), std::cell::OnceCell::new())); + let lines = (segments <= crate::SHAPE_BUILDER_MAX_SEGMENTS).then(|| (shapes.to_vec(), std::cell::OnceCell::new())); Self { arr: if has_open { None } else { arrangement(shapes) }, shapes: shapes.len(), lines } } @@ -321,8 +322,8 @@ impl FaceMerger { self.exact(faces).or_else(|| self.planar(faces)).filter(|p| !p.is_empty()).unwrap_or_else(|| merge_regions(faces)) } - /// `faces` merged in the planar map of shapes with open paths: each face found again by a - /// point inside it, and the result checked to cover what they cover. + /// `faces` merged in the planar map of the shapes: each face found again by a point inside + /// it, and the result checked to cover what they cover. fn planar(&self, faces: &[&Region]) -> Option { let (shapes, map) = self.lines.as_ref()?; let map = map.get_or_init(|| crate::planar::map(shapes)).as_ref()?; @@ -335,6 +336,10 @@ impl FaceMerger { } fn exact(&self, faces: &[&Region]) -> Option { + // A gap no shape covers is no face of the arrangement: it merges in the planar map. + if faces.iter().any(|r| r.sources.is_empty()) { + return None; + } let arr = self.arr.as_ref()?; // Each face by its coverage and a point inside it. let mut picks: Vec<(Vec, Point)> = Vec::with_capacity(faces.len()); diff --git a/crates/pathops/src/planar.rs b/crates/pathops/src/planar.rs index a56fbfe3..39b780a9 100644 --- a/crates/pathops/src/planar.rs +++ b/crates/pathops/src/planar.rs @@ -307,8 +307,9 @@ pub fn live_paint(shapes: &[Shape]) -> (Vec, Vec) { build(shapes).unwrap_or_default() } -/// Most segments the Shape Builder sweeps with open paths as edges: the planar map costs far more -/// than the filled areas' arrangement, so past this only the filled areas count. +/// Most segments the Shape Builder sweeps into its planar map (open paths as edges, the gaps closed +/// paths enclose, outline edges): the map costs far more than the filled areas' arrangement, so past +/// this only the filled areas count. pub const SHAPE_BUILDER_MAX_SEGMENTS: usize = 4096; /// The Shape Builder's arrangement: see [`shape_builder`]. @@ -322,20 +323,26 @@ pub struct BuilderArrangement { pub edges: Vec, } +/// Does any of `shapes` have an open subpath (a cutting edge for the Shape Builder)? +pub fn has_open_path(shapes: &[Shape]) -> bool { + shapes.iter().flat_map(|s| &s.path.subpaths).any(|sp| !sp.closed && sp.anchors.len() >= 2) +} + /// Shape Builder regions of `shapes` (back → front), the pieces of their open paths and, with /// `edges`, the pieces of their outlines (what erasing an edge deletes). /// -/// Closed paths bound regions with their filled areas, as in [`regions`](crate::regions). Open -/// paths (the caller closes the ones whose fill should count) are cutting edges: a line across a -/// shape splits it, lines around an area enclose a region nothing fills (empty `sources`), and each -/// path with no closed subpath comes back cut wherever another path meets it, as pieces keyed by -/// its index in `shapes`. Outlines are cut the same way. With no open path, more than +/// Closed paths bound regions with their filled areas, as in [`regions`](crate::regions), and the +/// gaps they enclose that nothing fills are regions too (empty `sources`), such as the middle of +/// four circles that touch (#1691). Open paths (the caller closes the ones whose fill should count) +/// are cutting edges: a line across a shape splits it, lines around an area enclose a region +/// nothing fills, and each path with no closed subpath comes back cut wherever another path meets +/// it, as pieces keyed by its index in `shapes`. Outlines are cut the same way. With more than /// [`SHAPE_BUILDER_MAX_SEGMENTS`] segments or a map that can't be built, only the filled areas /// count and there are no pieces. pub fn shape_builder(shapes: &[Shape], edges: bool) -> BuilderArrangement { - let has_open = shapes.iter().flat_map(|s| &s.path.subpaths).any(|sp| !sp.closed && sp.anchors.len() >= 2); + let has_open = has_open_path(shapes); let segments: usize = shapes.iter().flat_map(|s| &s.path.subpaths).map(|sp| sp.segment_count()).sum(); - let planar = ((has_open || edges) && segments <= SHAPE_BUILDER_MAX_SEGMENTS) + let planar = (segments <= SHAPE_BUILDER_MAX_SEGMENTS) .then(|| { let keyed: Vec = shapes.iter().enumerate().map(|(i, s)| Shape::new(s.path.clone(), s.rule, i as u64)).collect(); live_paint(&keyed) @@ -346,11 +353,15 @@ pub fn shape_builder(shapes: &[Shape], edges: bool) -> BuilderArrangement { }; let is_line = |i: u64| shapes.get(i as usize).is_some_and(|s| !s.path.subpaths.iter().any(|sp| sp.closed)); let (lines, outline): (Vec, Vec) = pieces.into_iter().partition(|e| is_line(e.key)); - BuilderArrangement { - regions: if has_open { faces } else { crate::regions(shapes) }, - lines: if has_open { lines } else { Vec::new() }, - edges: if edges { outline } else { Vec::new() }, - } + let regions = if has_open { + faces + } else { + // The filled areas from their arrangement, whose faces merge exactly, then the gaps. + let mut regions = crate::regions(shapes); + regions.extend(faces.into_iter().filter(|f| f.sources.is_empty())); + regions + }; + BuilderArrangement { regions, lines: if has_open { lines } else { Vec::new() }, edges: if edges { outline } else { Vec::new() } } } /// Does `sp`, closed, enclose an area (rather than retrace itself)? The Shape Builder closes the @@ -1182,6 +1193,43 @@ mod tests { assert!((regions.iter().map(area).sum::() - 10000.0).abs() < 1e-6); } + fn circle(cx: f64, cy: f64, r: f64, key: u64) -> Shape { + Shape::new(shapes::ellipse(Rect::new(cx - r, cy - r, cx + r, cy + r)), FillRule::NonZero, key) + } + + #[test] + fn shape_builder_offers_the_gaps_closed_shapes_enclose() { + // Four circles in a 2×2 grid, each touching its neighbours (#1691): the four circles and + // the diamond between them, which nothing fills. + let s = [circle(50.0, 50.0, 50.0, 0), circle(150.0, 50.0, 50.0, 1), circle(50.0, 150.0, 50.0, 2), circle(150.0, 150.0, 50.0, 3)]; + let regions = shape_builder(&s, false).regions; + assert_eq!(regions.len(), 5, "{regions:?}"); + let gaps: Vec<&Region> = regions.iter().filter(|r| r.sources.is_empty()).collect(); + assert_eq!(gaps.len(), 1); + assert!(gaps[0].contains(Point::new(100.0, 100.0)) && !gaps[0].contains(Point::new(50.0, 50.0))); + let diamond = 10000.0 - std::f64::consts::PI * 2500.0; + assert!((area(gaps[0]).abs() - diamond).abs() < 0.01 * diamond, "{}", area(gaps[0])); + assert_eq!(shape_builder(&s, true).regions, regions, "the outline edges don't change them"); + // A ring of four bars that overlap at the corners: the eight pieces of the bars and the + // square they enclose. + let s = + [rect(0.0, 0.0, 300.0, 100.0, 0), rect(200.0, 0.0, 300.0, 300.0, 1), rect(0.0, 200.0, 300.0, 300.0, 2), rect(0.0, 0.0, 100.0, 300.0, 3)]; + let regions = shape_builder(&s, false).regions; + assert_eq!(regions.len(), 9, "{regions:?}"); + let gaps: Vec<&Region> = regions.iter().filter(|r| r.sources.is_empty()).collect(); + assert_eq!(gaps.len(), 1); + assert!((area(gaps[0]).abs() - 10000.0).abs() < 1e-6 && gaps[0].contains(Point::new(150.0, 150.0))); + // The gap merges with a bar in the planar map: the bar's piece and the square, no seam. + let merger = crate::FaceMerger::new(&s); + let bar = regions.iter().find(|r| r.sources == [0] && r.contains(Point::new(150.0, 50.0))).unwrap(); + let merged = merger.merge(&[bar, gaps[0]]); + assert_eq!(merged.subpaths.len(), 1, "{merged:?}"); + assert!((crate::area(&merged, FillRule::NonZero) - 20000.0).abs() < 1e-6); + // Shapes apart or overlapping without a hole enclose no gap. + let s = [circle(50.0, 50.0, 50.0, 0), circle(250.0, 50.0, 50.0, 1), rect(80.0, 0.0, 220.0, 60.0, 2)]; + assert!(shape_builder(&s, false).regions.iter().all(|r| !r.sources.is_empty())); + } + #[test] fn shape_builder_stays_quick_up_to_its_cap() { // A wavy ring of nearly the most segments the map takes, crossed by a few lines. @@ -1194,6 +1242,10 @@ mod tests { }) .collect(); let mut s = vec![Shape::new(PathData::single(SubPath::polyline(&ring, true)), FillRule::NonZero, 0)]; + // Closed paths alone are swept for the gaps they enclose. + let started = std::time::Instant::now(); + assert_eq!(shape_builder(&s, false).regions.len(), 1, "the ring encloses no gap"); + assert!(started.elapsed().as_secs_f64() < 5.0, "{:?}", started.elapsed()); for k in 0..4 { let y = -60.0 + 40.0 * f64::from(k); s.push(line((-150.0, y), (150.0, y + 7.0), 1)); diff --git a/docs/gaps.md b/docs/gaps.md index 51121ac6..b59be149 100644 --- a/docs/gaps.md +++ b/docs/gaps.md @@ -144,10 +144,10 @@ from G14 and the budget run of G15: **~170–270 h** one agent, ~50–80 h wall during a drag keeps the drag (#1856, #1719); a template layer dims its images by Dim Images instead of fading the whole layer (#1864, #1683); a failed save or export is reported in a dialog that names the file and stays until closed, the document left modified (#1865, #1685). -- **Missing (noted for reporters, 2026-10-14):** the Shape Builder offers no region for an empty area enclosed - only by closed shapes, such as the middle of four circles that touch (#1691); deleting an interior anchor of an - open path joins across the gap, where the documented behaviour removes the point and its connecting segments. - Inside the estimate below. +- **Done:** the Shape Builder offers the empty areas closed shapes enclose as regions, such as the middle of four + circles that touch, and merges them with the regions around them (#1691). Gap Detection is still missing. +- **Missing (noted for reporters, 2026-10-14):** deleting an interior anchor of an open path joins across the gap, + where the documented behaviour removes the point and its connecting segments. Inside the estimate below. - **Missing:** Shape Builder misses planar regions and targets whole shapes (#937); Transform effect dialog's invisible checkboxes and missing options (#885); PDF text boxes moving on open (#722); SVG units reverting to points (#864); bezier drag preview freezing (#834). - **Impact:** wrong output in everyday work.