Merge pull request #572 from storytold/fix/issue-569

Fix #569: Shape Builder merges leave no stray edges in their outlines

crates/engine/src/cmd/buildcmds.rs

@@ -250,7 +250,8 @@ fn sb_merge(s: &mut Session, p: &Value) -> Result<Value> {
         // Edges come back from the planar map within a few hundredths of the outlines they follow.
         let cut_tol = step * 0.2;
         let picked: Vec<&po::Region> = touched.regions.iter().filter_map(|&i| map.regions.get(i).map(|r| &r.0)).collect();
-        let union = if picked.is_empty() { PathData::default() } else { po::merge_regions(&picked) };
+        let faces = po::FaceMerger::new(&map.shapes);
+        let union = if picked.is_empty() { PathData::default() } else { faces.merge(&picked) };
         // The merged shape takes the look of the front-most object over the first region, or (for an
         // area only lines enclose) of the front-most object and the current fill.
         let covered = picked.iter().find_map(|r| r.top());
@@ -278,7 +279,17 @@ fn sb_merge(s: &mut Session, p: &Value) -> Result<Value> {
                 // its outline, shared edges from every shape along them).
                 let in_union = picked.iter().any(|r| r.sources.contains(&i));
                 let mut rest = if in_union {
-                    po::boolean(&sh.path, sh.rule, &union, FillRule::NonZero, po::BoolOp::Difference)
+                    // What is left of it: its faces not merged or deleted, united. They share their
+                    // edges with the merged faces exactly, where subtracting the merged shape left
+                    // slivers of the outline along them (#569).
+                    let kept: Vec<&po::Region> = map
+                        .regions
+                        .iter()
+                        .enumerate()
+                        .filter(|(k, (r, ..))| r.sources.contains(&i) && !touched.regions.contains(k))
+                        .map(|(_, (r, ..))| r)
+                        .collect();
+                    if kept.is_empty() { PathData::default() } else { faces.merge(&kept) }
                 } else {
                     b::node_outline(leaf).map_or_else(|| sh.path.clone(), |o| o.0)
                 };

crates/engine/src/tests_build.rs

@@ -704,3 +704,67 @@ fn image_trace_object_remembers_its_settings() {
     let id = st.selection.in_paint_order(&st.doc)[0];
     assert!(st.doc.node(id).unwrap().trace.is_none());
 }
+
+/// #569: merging the regions of a rosette of unfilled ellipses (each off the centre, petals
+/// overlapping) merges every region the drags touch into one shape whose outline is only the
+/// merged area's border: no edge between merged regions stays in, as a stray line or a spur.
+#[test]
+fn shape_builder_merges_rosette_rings_cleanly() {
+    let mut s = session();
+    s.execute("paint.setFill", &json!({"none": true})).unwrap();
+    let c = [400.0, 320.0];
+    let mut petals = vec![];
+    for k in 0..8 {
+        let id =
+            s.execute("shape.ellipse", &json!({"x": c[0] - 60.0, "y": c[1] - 172.0, "width": 120, "height": 324})).unwrap()["id"].as_u64().unwrap();
+        s.execute("select.set", &json!({"ids": [id]})).unwrap();
+        s.execute("object.rotate", &json!({"angle": 45.0 * f64::from(k), "origin": c})).unwrap();
+        petals.push(id);
+    }
+    s.execute("select.set", &json!({"ids": petals})).unwrap();
+    // Drags around the ring of petals, from its inside out; the outer ones miss the petals.
+    let mut merged = None;
+    for radius in [110.0, 150.0, 190.0, 230.0] {
+        let pts: Vec<[f64; 2]> = (0..=120)
+            .map(|i| {
+                let a = std::f64::consts::TAU * f64::from(i) / 120.0;
+                [c[0] + radius * a.cos(), c[1] + radius * a.sin()]
+            })
+            .collect();
+        if let Ok(r) = s.execute("shapeBuilder.merge", &json!({"points": pts})) {
+            merged = r["merged"].as_u64().map(NodeId);
+        }
+    }
+    let (ring, rule) = outline(&node(&s, merged.expect("the drags merged")));
+    let inside = |p: kurbo::Point| kurbo::Shape::winding(&ring.to_bezpath(), p) != 0;
+    for k in 0..8 {
+        let a = std::f64::consts::FRAC_PI_4 * f64::from(k) - std::f64::consts::FRAC_PI_2;
+        let tip = kurbo::Point::new(c[0] + 165.0 * a.cos(), c[1] + 165.0 * a.sin());
+        assert!(inside(tip), "petal {k}'s tip at {tip:?} is in the merged shape");
+    }
+    // What is left of the petals stays out of the merged area (a piece left over it is a stray
+    // outline inside it).
+    let merged = merged.unwrap();
+    for id in s.doc().unwrap().selection.objects.clone().into_iter().filter(|id| *id != merged) {
+        let (p, r) = outline(&node(&s, id));
+        let over = vectorcraft_pathops::boolean(&p, r, &ring, rule, vectorcraft_pathops::BoolOp::Intersect);
+        let a = vectorcraft_pathops::area(&over, FillRule::NonZero);
+        assert!(a < 1.0, "{id:?} overlaps the merged shape by {a} pt²");
+    }
+    // No outline is a sliver or carries a spur (2 × area / length: its mean width).
+    for id in s.doc().unwrap().selection.objects.clone() {
+        let (p, r) = outline(&node(&s, id));
+        for sp in &p.subpaths {
+            let one = PathData::new(vec![sp.clone()]);
+            let width = 2.0 * vectorcraft_pathops::area(&one, r) / one.length();
+            assert!(width > 1.0, "{id:?} has an outline {width:.3} pt wide on average, {:.1} pt long", one.length());
+        }
+    }
+    // The ring is one outline around the centre (the regions the drags missed aside): the
+    // petals' tips are no outlines of their own beside it, apart by a hairline.
+    let big = ring.subpaths.iter().filter(|sp| vectorcraft_pathops::area(&PathData::new(vec![(*sp).clone()]), rule) > 100.0).count();
+    assert_eq!(big, 2, "{:?}", ring.subpaths.iter().map(|sp| vectorcraft_pathops::area(&PathData::new(vec![sp.clone()]), rule)).collect::<Vec<_>>());
+    // An inner edge would be a second outline over the merged area: normalising would drop it.
+    let clean = vectorcraft_pathops::normalize(&ring, rule);
+    assert!((ring.length() - clean.length()).abs() < 1.0, "{} pt of outline, {} without inner edges", ring.length(), clean.length());
+}

crates/pathops/src/lib.rs

@@ -19,7 +19,7 @@ pub use edit::{
     split_into_grid,
 };
 pub use offset::{Cap, Join, offset_path, outline_stroke, stroke_region};
-pub use pathfinder::{PathfinderOp, Region, Shape, merge_regions, pathfinder, region_at, regions};
+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};
 
 /// Errors from fallible operations.

crates/pathops/src/pathfinder.rs

@@ -7,7 +7,7 @@ use kurbo::{BezPath, ParamCurve, ParamCurveNearest, Point, Shape as _};
 use linesweeper::topology::ContourIdx;
 use vectorcraft_geom::{FillRule, PathData};
 
-use crate::boolean::{Arrangement, Multi, Seg, all_contours_to_path, contours_to_path, fill_bezpath, segs_to_subpath, unite_all};
+use crate::boolean::{Arrangement, Multi, Seg, all_contours_to_path, contours_to_path, fill_bezpath, normalize_bez, segs_to_subpath, unite_all};
 
 /// A filled shape in a Pathfinder stack. `key` identifies its paint (e.g. a hashed fill colour);
 /// results carry the key of the object whose paint they keep.
@@ -264,6 +264,63 @@ pub fn merge_regions(regions: &[&Region]) -> PathData {
     unite_all(&v)
 }
 
+/// Merges faces of the planar arrangement of a set of shapes (as [`regions`] gives them) into one
+/// path, from the arrangement itself, made once for every merge of the set: the faces' contours
+/// share their edges exactly, so the edges between merged faces vanish. Uniting the faces' tidied
+/// 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)
+/// fall back to [`merge_regions`].
+pub struct FaceMerger {
+    /// The arrangement (`None`: open paths, or the sweep failed) and the number of shapes.
+    arr: Option<Arrangement>,
+    shapes: usize,
+}
+
+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);
+        Self { arr: if has_open { None } else { arrangement(shapes) }, shapes: shapes.len() }
+    }
+
+    /// The union of `faces`.
+    pub fn merge(&self, faces: &[&Region]) -> PathData {
+        self.exact(faces).filter(|p| !p.is_empty()).unwrap_or_else(|| merge_regions(faces))
+    }
+
+    fn exact(&self, faces: &[&Region]) -> Option<PathData> {
+        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());
+        for r in faces {
+            let mut mask = vec![false; self.shapes];
+            for &s in &r.sources {
+                *mask.get_mut(s)? = true;
+            }
+            picks.push((mask, crate::planar::interior_point(&r.path)?));
+        }
+        let mut raw = BezPath::new();
+        let mut done: Vec<&[bool]> = vec![];
+        for (mask, _) in &picks {
+            if done.contains(&mask.as_slice()) {
+                continue;
+            }
+            done.push(mask);
+            let c = arr.contours(|k| k == mask.as_slice());
+            for group in c.grouped() {
+                let mut face = BezPath::new();
+                for i in group {
+                    face.extend(c[i].path.iter());
+                }
+                if picks.iter().any(|(m, p)| m == mask && face.winding(*p) != 0) {
+                    raw.extend(face.iter());
+                }
+            }
+        }
+        let merged = normalize_bez(&raw, FillRule::NonZero).ok()?;
+        Some(all_contours_to_path(&merged, &arr.tidy))
+    }
+}
+
 /// Pathfinder Outline: split every shape's boundary at junctions of the arrangement.
 fn outline_edges(arr: &Arrangement, shapes: &[Shape]) -> Vec<Shape> {
     let junctions = arr.junctions();