Fix #1770: Shape Builder merges leave no spur or split outline when shared edges differ by rounding

The Shape Builder merges faces by uniting their contours, and the edges
two faces share cancel out. The sweep can give two faces copies of a
shared edge whose ends lie a few times its tolerance apart, with the
difference left as pieces shorter than the tolerance. The union could
then keep the space between the copies: a hairline that joined a hole
as a spur, or a seam that cut a petal tip off the ring's outer outline
as an outline of its own. On Apple Silicon, turning one petal of the
rosette test by -180 degrees instead of 180 leaves a spur.

A Shape Builder action unites the picked faces first, and then what is
left of each shape. When two copies of a shared edge differ in the
first of these unions that unites the faces' contours, each point of
the faces' contours that lies within 16 times the sweep's tolerance of
a point kept before it now moves onto that point before that union and
each later one, with the handles next to it. A piece left within that
distance of one point is dropped. The planar map joins the ends of
copies of an edge within the same distance. Copies that share their
ends leave at most a hairline contour, which the merge drops as a
sliver. When the copies in that union all match exactly, the unions
take the contours unchanged. Only the Shape Builder's merges and
deletions unite faces this way, so Pathfinder output does not change.

crates/engine/src/tests_build.rs

@@ -1127,8 +1127,9 @@ fn image_trace_views_draw_on_screen_only_and_expand_to_the_result() {
 /// #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() {
+/// Petal `k` turns about the center by 45° × `k`, or by each angle `turns` lists for it in turn.
+#[track_caller]
+fn rosette_rings_merge_cleanly(turns: &[(u32, &[f64])]) {
     let mut s = session();
     s.execute("paint.setFill", &json!({"none": true})).unwrap();
     let c = [400.0, 320.0];
@@ -1137,7 +1138,10 @@ fn shape_builder_merges_rosette_rings_cleanly() {
         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();
+        let angles = turns.iter().find(|t| t.0 == k).map_or_else(|| vec![45.0 * f64::from(k)], |t| t.1.to_vec());
+        for angle in angles {
+            s.execute("object.rotate", &json!({"angle": angle, "origin": c})).unwrap();
+        }
         petals.push(id);
     }
     s.execute("select.set", &json!({"ids": petals})).unwrap();
@@ -1187,3 +1191,18 @@ fn shape_builder_merges_rosette_rings_cleanly() {
     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());
 }
+
+#[test]
+fn shape_builder_merges_rosette_rings_cleanly() {
+    rosette_rings_merge_cleanly(&[]);
+}
+
+/// #1770: the rosette merges as cleanly with a petal turned by an equivalent angle (-180° for 180°,
+/// -855° for 225°, -45° for 315°) or in two steps, which moves its outline by a rounding error.
+#[test]
+fn shape_builder_merges_rosette_rings_cleanly_however_a_petal_turns() {
+    rosette_rings_merge_cleanly(&[(4, &[-180.0])]);
+    rosette_rings_merge_cleanly(&[(5, &[-855.0])]);
+    rosette_rings_merge_cleanly(&[(7, &[-45.0])]);
+    rosette_rings_merge_cleanly(&[(4, &[100.0, 80.0])]);
+}

crates/pathops/src/pathfinder.rs

@@ -3,11 +3,13 @@
 
 use std::collections::HashMap;
 
-use kurbo::{BezPath, ParamCurve, ParamCurveNearest, Point, Shape as _};
+use kurbo::{BezPath, Line, ParamCurve, ParamCurveNearest, PathSeg, 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, normalize_bez, segs_to_subpath, unite_all};
+use crate::boolean::{
+    Arrangement, Multi, Seg, Tidy, all_contours_to_path, contours_to_path, eps_for, fill_bezpath, normalize_bez, segs_to_subpath, to_seg, 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.
@@ -294,7 +296,9 @@ pub fn merge_regions(regions: &[&Region]) -> PathData {
 
 /// 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
+/// share their edges, so the edges between merged faces vanish (when two faces' copies of an edge
+/// differ in the first merge to unite contours, every point within 16 eps of a point kept before
+/// it moves onto that point first, in that merge and every later one). 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, as
 /// [`shape_builder`](crate::shape_builder) gives them) merge in their planar map the same way
@@ -306,6 +310,9 @@ pub struct FaceMerger {
     shapes: usize,
     /// 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>>)>,
+    /// Whether every merge moves the points of the faces' contours together first: the first
+    /// merge that unites contours decides it (`unite_faces`).
+    snap: std::cell::OnceCell<bool>,
 }
 
 impl FaceMerger {
@@ -314,7 +321,7 @@ impl FaceMerger {
         // 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 = (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 }
+        Self { arr: if has_open { None } else { arrangement(shapes) }, shapes: shapes.len(), lines, snap: std::cell::OnceCell::new() }
     }
 
     /// The union of `faces`.
@@ -368,11 +375,175 @@ impl FaceMerger {
                 }
             }
         }
-        let merged = normalize_bez(&raw, FillRule::NonZero).ok()?;
-        Some(all_contours_to_path(&merged, &arr.tidy))
+        unite_faces(&raw, &arr.tidy, &self.snap)
     }
 }
 
+/// The union of the faces whose contours are `raw`, tidied with `tidy`. The points of the
+/// contours move together first ([`snap_points`]) when `snap` holds `true`. When `snap` is empty,
+/// it is set to whether two copies of an edge in `raw` differ.
+fn unite_faces(raw: &BezPath, tidy: &Tidy, snap: &std::cell::OnceCell<bool>) -> Option<PathData> {
+    // The planar map joins the ends of copies of an edge within 16 eps too.
+    let eps = eps_for([raw]).ok()?;
+    let snapped = if snap.get() == Some(&false) {
+        None
+    } else {
+        let (snapped, differ) = snap_points(raw, 16.0 * eps);
+        snap.get_or_init(|| differ).then_some(snapped)
+    };
+    let merged = normalize_bez(snapped.as_ref().unwrap_or(raw), FillRule::NonZero).ok()?;
+    Some(all_contours_to_path(&merged, tidy))
+}
+
+/// `raw` with the points of its contours gathered onto points more than `tol` apart: each point
+/// moves onto a point kept before it within `tol`, or is kept, and the handles next to it move
+/// along. A piece left within `tol` of one point is dropped. It also returns whether two copies of
+/// an edge differ, before the move or after it ([`copies_differ`]).
+///
+/// Two faces' contours can carry the edge they share as pieces whose ends lie a few eps apart, the
+/// difference left as pieces shorter than eps: linesweeper 0.5 rebuilds a contour's run of pieces
+/// of one input curve from that curve and does not move the rebuilt ends onto the sweep's points.
+/// Uniting the faces as they are can leave the space between the two copies in, as a hairline that
+/// reaches a hole through those short pieces (a spur) or as a seam that splits the outline (#1770).
+/// Copies that share their ends leave at most a hairline contour of their own, which
+/// [`is_sliver`](crate::boolean::is_sliver) drops.
+fn snap_points(raw: &BezPath, tol: f64) -> (BezPath, bool) {
+    let cell = |p: Point| ((p.x / tol).floor() as i64, (p.y / tol).floor() as i64);
+    let mut grid: HashMap<(i64, i64), Vec<(usize, Point)>> = HashMap::new();
+    let mut snapped: HashMap<(u64, u64), (usize, Point)> = HashMap::new();
+    let mut kept: usize = 0;
+    let mut snap = |p: Point| {
+        *snapped.entry(vertex_key(p)).or_insert_with(|| {
+            let (cx, cy) = cell(p);
+            let near = (cx - 1..=cx + 1)
+                .flat_map(|x| (cy - 1..=cy + 1).map(move |y| (x, y)))
+                .filter_map(|k| grid.get(&k))
+                .flatten()
+                .find(|(_, q)| q.distance(p) <= tol)
+                .copied();
+            near.unwrap_or_else(|| {
+                let v = (kept, p);
+                kept += 1;
+                grid.entry((cx, cy)).or_default().push(v);
+                v
+            })
+        })
+    };
+    let mut out = BezPath::new();
+    // Each contour's pieces, each with the segment it was and the indices of the two points it runs
+    // between.
+    let mut contours: Vec<Vec<(PathSeg, PathSeg, [usize; 2])>> = Vec::new();
+    for (range, closed) in vectorcraft_geom::bez::subpaths(raw) {
+        let Some(els) = raw.elements().get(range) else { continue };
+        let mut pieces = Vec::new();
+        for s in vectorcraft_geom::bez::segments(els) {
+            let ((i, a), (j, b)) = (snap(s.start()), snap(s.end()));
+            let mut seg = to_seg(s);
+            crate::planar::pin_ends(std::slice::from_mut(&mut seg), a, b);
+            if a == b && seg.c.p1.distance(a) <= tol && seg.c.p2.distance(a) <= tol {
+                continue;
+            }
+            if pieces.is_empty() {
+                out.move_to(a);
+            }
+            let piece = if (a, b) == (s.start(), s.end()) {
+                s
+            } else if seg.line {
+                PathSeg::Line(Line::new(a, b))
+            } else {
+                PathSeg::Cubic(seg.c)
+            };
+            out.push(piece.as_path_el());
+            pieces.push((piece, s, [i, j]));
+        }
+        if !pieces.is_empty() && closed {
+            out.close_path();
+        }
+        contours.push(pieces);
+    }
+    let differ = copies_differ(&contours, kept, tol);
+    (out, differ)
+}
+
+/// Whether two copies of an edge in `contours` differ. Each piece comes with the segment it was
+/// before the move and the indices of the two points it runs between, out of `points`. A copy is a
+/// chain of pieces between junctions, the points where other than two piece ends meet. Two chains
+/// that run opposite ways between the same two junctions, with the middle and the end of each
+/// piece within `tol` of the other chain, are copies of one edge, and they differ when one is not
+/// the exact reverse of the other, after the move or before it.
+fn copies_differ(contours: &[Vec<(PathSeg, PathSeg, [usize; 2])>], points: usize, tol: f64) -> bool {
+    let mut degree = vec![0usize; points];
+    for &v in contours.iter().flatten().flat_map(|(_, _, ends)| ends) {
+        if let Some(d) = degree.get_mut(v) {
+            *d += 1;
+        }
+    }
+    let junction = |v: usize| degree.get(v) != Some(&2);
+    // (pieces, the segments they were, first point, last point). A contour through no junction
+    // shares no point with another contour.
+    let mut chains: Vec<(Vec<PathSeg>, Vec<PathSeg>, usize, usize)> = Vec::new();
+    for pieces in contours {
+        let Some(first) = pieces.iter().position(|(_, _, [a, _])| junction(*a)) else { continue };
+        let (mut cur, mut was): (Vec<PathSeg>, Vec<PathSeg>) = (Vec::new(), Vec::new());
+        let (mut from, mut to) = (0, 0);
+        for &(s, w, [a, b]) in pieces.iter().cycle().skip(first).take(pieces.len()) {
+            if junction(a) && !cur.is_empty() {
+                chains.push((std::mem::take(&mut cur), std::mem::take(&mut was), from, to));
+            }
+            if cur.is_empty() {
+                from = a;
+            }
+            cur.push(s);
+            was.push(w);
+            to = b;
+        }
+        chains.push((cur, was, from, to));
+    }
+    let key = |a: usize, b: usize| (a.min(b), a.max(b));
+    let mut by_ends: HashMap<(usize, usize), Vec<usize>> = HashMap::new();
+    for (k, (_, _, a, b)) in chains.iter().enumerate() {
+        by_ends.entry(key(*a, *b)).or_default().push(k);
+    }
+    let reversed = |a: &[PathSeg], b: &[PathSeg]| a.iter().rev().map(|s| s.reverse()).eq(b.iter().copied());
+    let mut paired = vec![false; chains.len()];
+    for (k, (segs, was, a, b)) in chains.iter().enumerate() {
+        if paired.get(k) != Some(&false) {
+            continue;
+        }
+        let Some(candidates) = by_ends.get(&key(*a, *b)) else { continue };
+        let found = candidates.iter().filter(|&&j| j > k && paired.get(j) == Some(&false)).find_map(|&j| {
+            let (other, other_was, c, d) = chains.get(j)?;
+            (c == b && d == a && along(segs, other, tol)).then_some((j, other, other_was))
+        });
+        let Some((j, other, other_was)) = found else { continue };
+        for i in [k, j] {
+            if let Some(p) = paired.get_mut(i) {
+                *p = true;
+            }
+        }
+        if !reversed(segs, other) || !reversed(was, other_was) {
+            return true;
+        }
+    }
+    false
+}
+
+/// Whether the middle and the end of every piece of `a` lie within `tol` of `b`, and the middle
+/// and the end of every piece of `b` lie within `tol` of `a`. `a` and `b` run opposite ways.
+fn along(a: &[PathSeg], b: &[PathSeg], tol: f64) -> bool {
+    let covers = |from: &[PathSeg], to: &[PathSeg]| {
+        // Each point's search goes back along `to` from the piece the point before it was near.
+        let mut at = to.len();
+        from.iter().flat_map(|s| [s.eval(0.5), s.end()]).all(|p| {
+            let mut order = to.iter().enumerate().take(at).rev().chain(to.iter().enumerate().skip(at).rev());
+            let Some((i, _)) = order.find(|(_, t)| t.nearest(p, 1e-12).distance_sq.sqrt() <= tol) else { return false };
+            at = i + 1;
+            true
+        })
+    };
+    covers(a, b) && covers(b, a)
+}
+
 /// Pathfinder Outline: split every shape's boundary at junctions of the arrangement.
 fn outline_edges(arr: &Arrangement, shapes: &[Shape]) -> Vec<Shape> {
     // Sorted by x (then y), so a segment needs only the junctions inside its control box.
@@ -582,6 +753,94 @@ mod tests {
         }
     }
 
+    /// The edge that the two faces of the #1770 tests share.
+    fn shared_edge() -> kurbo::CubicBez {
+        kurbo::CubicBez::new((100.0, 0.0), (130.0, 30.0), (130.0, 70.0), (100.0, 100.0))
+    }
+
+    /// Two faces on either side of [`shared_edge`]. The right face's copy of the edge has its ends
+    /// moved along the bottom and top edges by `bottom` and `top`, its handles moved with them, and
+    /// its first handle moved by `nudge` more.
+    fn faces_apart(bottom: f64, top: f64, nudge: f64) -> BezPath {
+        use kurbo::{CubicBez, Vec2};
+        let edge = shared_edge();
+        let (p0, p3) = (edge.p0 + Vec2::new(bottom, 0.0), edge.p3 + Vec2::new(top, 0.0));
+        let copy = CubicBez::new(p0, edge.p1 + (p0 - edge.p0) + Vec2::new(0.0, nudge), edge.p2 + (p3 - edge.p3), p3);
+        let mut raw = BezPath::new();
+        raw.move_to((0.0, 0.0));
+        raw.line_to(edge.p0);
+        raw.curve_to(edge.p1, edge.p2, edge.p3);
+        raw.line_to((0.0, 100.0));
+        raw.close_path();
+        raw.move_to(copy.p0);
+        raw.line_to((200.0, 0.0));
+        raw.line_to((200.0, 100.0));
+        raw.line_to(copy.p3);
+        raw.curve_to(copy.p2, copy.p1, copy.p0);
+        raw.close_path();
+        raw
+    }
+
+    /// Two faces whose copies of [`shared_edge`] are exact reverses of each other. The left face
+    /// has a point 5e-6 pt from another point.
+    fn faces_matching() -> BezPath {
+        let edge = shared_edge();
+        let mut raw = BezPath::new();
+        raw.move_to((0.0, 0.0));
+        raw.line_to(edge.p0);
+        raw.curve_to(edge.p1, edge.p2, edge.p3);
+        raw.line_to((0.0, 100.0));
+        raw.curve_to((-20.0, 90.0), (-30.0, 70.0), (-30.0, 50.0));
+        raw.line_to((-30.0, 50.0 - 5e-6));
+        raw.curve_to((-20.0, 40.0), (-20.0, 10.0), (0.0, 0.0));
+        raw.close_path();
+        raw.move_to(edge.p0);
+        raw.line_to((200.0, 0.0));
+        raw.line_to((200.0, 100.0));
+        raw.line_to(edge.p3);
+        raw.curve_to(edge.p2, edge.p1, edge.p0);
+        raw.close_path();
+        raw
+    }
+
+    /// #1770: two faces' contours carry the edge they share as copies whose ends lie a few eps
+    /// apart, at one end or at both, with or without a handle a rounding error apart. The faces
+    /// unite into one outline, with no hairline left between the copies.
+    #[test]
+    fn faces_unite_into_one_outline_when_their_copies_of_an_edge_end_a_few_eps_apart() {
+        let tidy = Tidy::free(crate::DEFAULT_PRECISION);
+        for (bottom, top, nudge) in [(0.0, 3e-6, 4e-7), (3e-6, 3e-6, 4e-7), (0.0, 3e-6, 0.0), (3e-6, 3e-6, 0.0)] {
+            let merged = unite_faces(&faces_apart(bottom, top, nudge), &tidy, &std::cell::OnceCell::new()).unwrap();
+            assert_eq!(merged.subpaths.len(), 1, "ends apart by {bottom} and {top}, nudge {nudge}: {merged:?}");
+            assert!((merged.length() - 600.0).abs() < 1e-3, "ends apart by {bottom} and {top}, nudge {nudge}: {} pt of outline", merged.length());
+        }
+    }
+
+    /// #1770: when the faces' copies of every edge they share are exact reverses of each other,
+    /// the faces unite as they are, and a point 5e-6 pt from another point stays where it is.
+    #[test]
+    fn faces_whose_copies_of_each_shared_edge_match_unite_as_they_are() {
+        let tidy = Tidy::free(crate::DEFAULT_PRECISION);
+        let raw = faces_matching();
+        let as_they_are = all_contours_to_path(&normalize_bez(&raw, FillRule::NonZero).unwrap(), &tidy);
+        assert_eq!(as_they_are.subpaths.len(), 1, "{as_they_are:?}");
+        assert_eq!(unite_faces(&raw, &tidy, &std::cell::OnceCell::new()), Some(as_they_are));
+    }
+
+    /// #1770: the first union decides for every later union of the same set whether the points
+    /// move together. After faces whose copies of an edge end apart, the faces whose copies match
+    /// lose their point 5e-6 pt from another point.
+    #[test]
+    fn later_unions_move_points_as_the_first_union_decided() {
+        let tidy = Tidy::free(crate::DEFAULT_PRECISION);
+        let snap = std::cell::OnceCell::new();
+        let first = unite_faces(&faces_apart(0.0, 3e-6, 0.0), &tidy, &snap).unwrap();
+        assert_eq!(first.subpaths.len(), 1, "{first:?}");
+        let later = unite_faces(&faces_matching(), &tidy, &snap).unwrap();
+        let moved = Point::new(-30.0, 50.0 - 5e-6);
+        assert!(later.subpaths.iter().flat_map(|sp| &sp.anchors).all(|a| a.p != moved), "{later:?}");
+    }
+
     #[test]
     fn divide_open_paths_cut_faces_without_taking_the_cutters_paint() {
         let line = |a: (f64, f64), b: (f64, f64)| shapes::line(Point::from(a), Point::from(b));

crates/pathops/src/planar.rs

@@ -150,7 +150,7 @@ struct Edge {
 }
 
 /// `segs` (a chain) moved to start at `a` and end at `b`, the handles next to each moving with it.
-fn pin_ends(segs: &mut [Seg], a: Point, b: Point) {
+pub(crate) fn pin_ends(segs: &mut [Seg], a: Point, b: Point) {
     if let Some(s) = segs.first_mut() {
         let d = a - s.c.p0;
         s.c.p0 = a;