Merge pull request #1891 from storytold/fix/1691-shape-builder-gaps

Shape Builder: the gaps closed paths enclose are regions (#1691)

crates/engine/src/cmd/shaper.rs

@@ -90,9 +90,13 @@ fn map_of(src: &[Arc<Node>]) -> Result<BuilderMap> {
     }
     let arrangement = po::shape_builder(&shapes, true);
     let bounded = |pieces: Vec<po::Shape>| 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();

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::<Vec<_>>()})).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::<Vec<_>>()})).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();

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)]

crates/pathops/src/pathfinder.rs

@@ -102,7 +102,7 @@ pub fn pathfinder(op: PathfinderOp, shapes: &[Shape]) -> Vec<Shape> {
     }
     // 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<Arrangement>,
     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<Shape>, std::cell::OnceCell<Option<crate::planar::Map>>)>,
 }
 
 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<PathData> {
         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<PathData> {
+        // 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<bool>, Point)> = Vec::with_capacity(faces.len());

crates/pathops/src/planar.rs

@@ -307,8 +307,9 @@ pub fn live_paint(shapes: &[Shape]) -> (Vec<Region>, Vec<Shape>) {
     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<Shape>,
 }
 
+/// 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<Shape> = 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<Shape>, Vec<Shape>) = 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::<f64>() - 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));

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.