M4: a Pencil stroke stays where it was drawn, without spikes off it (#1741)

kurbo's simplify_bezpath, which smooths Pencil samples and runs Simplify
path, fitted two common shapes with a curve that ran far off the path: a
stroke that goes out and back to the very point it left (kurbo keeps a
reversal as a smooth join) and nearly collinear samples (kurbo measures
its error across the path, not along it).

figcraft_geom::simplify_path cuts the path where it turns back by more
than a right angle, simplifies each run with kurbo, and keeps a run as it
was when its fit strays more than twice the accuracy from it. The Pencil
and vector.simplify use it.

crates/engine/src/brush_tests.rs

@@ -149,3 +149,49 @@ fn the_pencil_draws_white_on_a_dark_background() {
         assert_eq!(stroke(&mut s, 0.0, 600.0).0, want, "on {bg}");
     }
 }
+
+/// A Pencil stroke stays where it was drawn: the fitted curve shot far off the stroke, a spike
+/// (#1741, its second point), where the stroke went out and back to the very point it left and
+/// where samples ran on nearly straight. Pointer pixels become fractional page points, so going
+/// back to a pixel lands on exactly the same point. Both sample runs are ones kurbo's fit took
+/// hundreds of units off.
+#[test]
+fn the_pencil_keeps_its_stroke_where_it_was_drawn() {
+    let mut s = Session::new();
+    s.execute("tool.set", &json!({"tool": "pencil"})).unwrap();
+    let (p, q) = ((86.043_565_719_086_97, -173.744_402_349_100_94), (87.851_850_739_335_35, -179.202_846_445_954_52));
+    let back = [(80.0, -160.0), p, q, p, (95.0, -185.0)];
+    let straight = [
+        (204.0, -198.0),
+        (209.598_690_794_015_5, -204.909_328_119_739_88),
+        (211.277_914_263_870_97, -207.190_974_020_753_4),
+        (214.246_506_828_720_84, -211.228_827_590_623_24),
+        (218.0, -214.0),
+    ];
+    for (what, samples) in [("out and back", &back[..]), ("nearly straight", &straight[..])] {
+        let (first, last) = (samples[0], samples[samples.len() - 1]);
+        s.pointer(&PointerEvent::at(PointerKind::Down, first.0, first.1, 1.0)).unwrap();
+        for (x, y) in &samples[1..] {
+            s.pointer(&PointerEvent::at(PointerKind::Move, *x, *y, 1.0)).unwrap();
+        }
+        s.pointer(&PointerEvent::at(PointerKind::Up, last.0, last.1, 1.0)).unwrap();
+        let doc = s.doc().unwrap();
+        let n = doc.nodes().filter(|n| matches!(n.kind, NodeKind::Vector { .. })).max_by_key(|n| n.id).unwrap();
+        let NodeKind::Vector { network } = &n.kind else { panic!("{what}: not a vector") };
+        let drawn = doc.world_transform(n.id) * network.stroke_path();
+        let samples_box = figcraft_geom::points_bounds(samples.iter().map(|(x, y)| Point::new(*x, *y))).unwrap();
+        let (path_box, node_box) = (figcraft_geom::Shape::bounding_box(&drawn), doc.world_bounds(n.id).unwrap());
+        assert!(
+            samples_box.inflate(1.5, 1.5).contains_rect(path_box),
+            "{what}: the stroke {path_box:?} goes past the samples {samples_box:?}: {}",
+            drawn.to_svg()
+        );
+        assert!(samples_box.inflate(1.5, 1.5).contains_rect(node_box), "{what}: the layer {node_box:?}, the samples {samples_box:?}");
+    }
+    // The turn is a point of the stroke, where it was drawn.
+    let doc = s.doc().unwrap();
+    let turned = doc.nodes().find(|n| n.name == "Vector" && doc.world_bounds(n.id).is_some_and(|b| b.y1 < -150.0 && b.x1 < 100.0)).unwrap();
+    let NodeKind::Vector { network } = &turned.kind else { panic!("not a vector") };
+    let xf = doc.world_transform(turned.id);
+    assert!(network.vertices.iter().any(|v| (xf * Point::new(v.x, v.y) - Point::new(q.0, q.1)).hypot() < 1e-6), "no point at the turn");
+}

crates/engine/src/canvas/tools/pen.rs

@@ -313,7 +313,8 @@ impl Session {
         if points.len() < 2 {
             return Ok(());
         }
-        // Smooth: fit a curve through the samples.
+        // Smooth: fit a curve through the samples, kept from shooting past them where the stroke
+        // turns back or runs nearly straight (#1741).
         let mut poly = figcraft_geom::BezPath::new();
         for (k, p) in points.iter().enumerate() {
             if k == 0 {
@@ -322,7 +323,7 @@ impl Session {
                 poly.line_to(*p);
             }
         }
-        let fitted = figcraft_geom::kurbo::simplify::simplify_bezpath(poly.iter(), 0.75, &Default::default());
+        let fitted = figcraft_geom::simplify_path(&poly, 0.75);
         let d = fitted.to_svg();
         if matches!(self.tool.tool, ToolId::Marker | ToolId::Highlighter) {
             return self.ink_finish(&d);

crates/engine/src/cmd/structure.rs

@@ -799,7 +799,7 @@ fn simplify(s: &mut Session, p: &Value) -> Result<Value> {
                     WindingRule::NonZero
                 };
                 let path = drawn.fill_path();
-                let path = figcraft_geom::kurbo::simplify::simplify_bezpath(path.iter(), tol, &Default::default());
+                let path = figcraft_geom::simplify_path(&path, tol);
                 let (path, output_winding) = match figcraft_pathops::simplify_with_rule(&path, winding == WindingRule::EvenOdd) {
                     Ok(path) => (path, WindingRule::EvenOdd),
                     Err(_) => (path, winding),
@@ -809,7 +809,7 @@ fn simplify(s: &mut Session, p: &Value) -> Result<Value> {
             } else {
                 for (index, region) in source.regions.iter().enumerate() {
                     let Some(path) = drawn.region_path(index) else { continue };
-                    let path = figcraft_geom::kurbo::simplify::simplify_bezpath(path.iter(), tol, &Default::default());
+                    let path = figcraft_geom::simplify_path(&path, tol);
                     let path = figcraft_pathops::simplify_with_rule(&path, region.winding == WindingRule::EvenOdd).unwrap_or(path);
                     append_region_path(&mut net, &path, region, region.winding);
                 }
@@ -826,7 +826,7 @@ fn simplify(s: &mut Session, p: &Value) -> Result<Value> {
                 open.regions.clear();
                 if !open.segments.is_empty() {
                     let path = open.stroke_path();
-                    let simplified = figcraft_geom::kurbo::simplify::simplify_bezpath(path.iter(), tol, &Default::default());
+                    let simplified = figcraft_geom::simplify_path(&path, tol);
                     let mut loose = VectorNetwork::from_path(&simplified);
                     loose.regions.clear();
                     append_network(&mut net, loose);
@@ -834,7 +834,7 @@ fn simplify(s: &mut Session, p: &Value) -> Result<Value> {
             }
             if source.regions.is_empty() {
                 let path = source.stroke_path();
-                let simplified = figcraft_geom::kurbo::simplify::simplify_bezpath(path.iter(), tol, &Default::default());
+                let simplified = figcraft_geom::simplify_path(&path, tol);
                 net = VectorNetwork::from_path(&simplified);
                 net.regions.clear();
             }

crates/engine/src/vector_behaviour_tests.rs

@@ -326,6 +326,27 @@ fn ex_to_vector(doc: &mut figcraft_doc::Document, id: NodeId) {
     doc.get_mut(id).unwrap().kind = NodeKind::Vector { network };
 }
 
+/// Simplify path keeps nearly collinear points near where they were: kurbo's fit, which Simplify
+/// path and the Pencil share, ran a handle hundreds of units on along the line (#1741).
+#[test]
+fn simplify_keeps_nearly_collinear_points_near_them() {
+    let mut s = Session::new();
+    let id = id_of(&ex(&mut s, "vector.create", json!({"path": "M0 0 L10 10"}))["id"]).unwrap();
+    // The points exactly as sampled, at the page origin (`vector.create` would move them).
+    let d = "M209.5986907940155 -204.90932811973988 L211.27791426387097 -207.1909740207534 L214.24650682872084 -211.22882759062324";
+    let before = BezPath::from_svg(d).unwrap();
+    {
+        let n = s.doc_mut().unwrap().get_mut(id).unwrap();
+        n.transform = figcraft_geom::Affine::IDENTITY;
+        n.kind = NodeKind::Vector { network: VectorNetwork::from_path(&before) };
+    }
+    assert_eq!(net(&s, id).stroke_path().elements(), before.elements(), "the samples as they were");
+    ex(&mut s, "vector.simplify", json!({"ids": [id], "tolerance": 0.75}));
+    let after = s.doc().unwrap().world_transform(id) * net(&s, id).stroke_path();
+    let (b0, b1) = (before.bounding_box(), after.bounding_box());
+    assert!(b0.inflate(1.5, 1.5).contains_rect(b1), "{b1:?} past {b0:?}: {}", after.to_svg());
+}
+
 fn ex(s: &mut Session, cmd: &str, p: Value) -> Value {
     s.execute(cmd, &p).unwrap_or_else(|e| panic!("{cmd}: {e}"))
 }

crates/geom/src/lib.rs

@@ -12,7 +12,9 @@ use kurbo::ParamCurve;
 use std::f64::consts::{FRAC_PI_2, PI, TAU};
 
 pub mod measure;
+mod simplify;
 mod transform;
+pub use simplify::simplify_path;
 pub use transform::*;
 
 /// Corner radii in clockwise order from the top-left: `[tl, tr, br, bl]`.

crates/geom/src/simplify.rs

@@ -0,0 +1,140 @@
+//! Path simplification that keeps to its input where kurbo's fit would shoot past it.
+
+use kurbo::{BezPath, ParamCurve, ParamCurveNearest, PathEl, PathSeg, Point, Vec2};
+
+/// How far, as a multiple of the accuracy asked for, a fitted run may stray from the run it
+/// replaces before the run is kept as it was. kurbo's own fits stay within about the accuracy; its
+/// failures stray by tens to hundreds of units, so twice the accuracy tells them apart without
+/// undoing fits that are merely a little over.
+const STRAY: f64 = 2.0;
+
+/// `path` simplified to within `accuracy` by kurbo's [`simplify_bezpath`], kept from straying
+/// far from `path` where kurbo's fit does.
+///
+/// Two inputs made kurbo's fit shoot far past the path it simplified (#1741, a Pencil stroke with
+/// a spike off it), and both are common in pointer samples:
+/// - A join where the path turns straight back (samples going down and back up the same line):
+///   kurbo keeps two segments in one smooth fit when their tangents are parallel whichever way
+///   they point (`|cross| <= |dot| * angle_thresh`), so it fits a smooth curve through the cusp.
+///   The path is cut where it turns back by more than a right angle, each run between the cuts is
+///   simplified on its own, and the runs are joined again, so the turn stays a sharp corner. Any
+///   other join that isn't straight within kurbo's threshold is a corner to kurbo already, so the
+///   cut changes nothing there.
+/// - A run of nearly collinear segments: kurbo measures its error across the path, not along it, so
+///   a fitted cubic whose handle runs hundreds of units on along the line passes its check. Each
+///   fitted run is measured against the run it replaces, and one that strays more than
+///   twice `accuracy` from it is kept as it was instead.
+///
+/// Subpaths and closes are kept, and a closing segment stays implicit, as kurbo leaves it.
+///
+/// [`simplify_bezpath`]: kurbo::simplify::simplify_bezpath
+pub fn simplify_path(path: &BezPath, accuracy: f64) -> BezPath {
+    let mut out = BezPath::new();
+    let mut run = BezPath::new();
+    // Where the current subpath started, where the pen is, and the last segment's end tangent.
+    let mut start = Point::ZERO;
+    let mut at = Point::ZERO;
+    let mut last_tan: Option<Vec2> = None;
+    // Whether the next run that draws anything has to start the subpath with its move.
+    let mut fresh = true;
+    for el in path.elements() {
+        let seg = match *el {
+            PathEl::MoveTo(p) => {
+                flush(&mut out, &mut run, accuracy, &mut fresh);
+                (start, at, last_tan, fresh) = (p, p, None, true);
+                run.move_to(p);
+                continue;
+            }
+            PathEl::ClosePath => {
+                flush(&mut out, &mut run, accuracy, &mut fresh);
+                if !fresh {
+                    out.close_path();
+                }
+                // A segment after a close starts a new subpath at the closed one's start.
+                (at, last_tan, fresh) = (start, None, true);
+                run.move_to(start);
+                continue;
+            }
+            PathEl::LineTo(p) => PathSeg::Line(kurbo::Line::new(at, p)),
+            PathEl::QuadTo(p1, p2) => PathSeg::Quad(kurbo::QuadBez::new(at, p1, p2)),
+            PathEl::CurveTo(p1, p2, p3) => PathSeg::Cubic(kurbo::CubicBez::new(at, p1, p2, p3)),
+        };
+        if run.elements().is_empty() {
+            run.move_to(at);
+        }
+        let Some((t0, t1)) = tangents(seg) else { continue };
+        if last_tan.is_some_and(|last| last.dot(t0) < 0.0) {
+            flush(&mut out, &mut run, accuracy, &mut fresh);
+            run.move_to(at);
+        }
+        run.push(*el);
+        at = seg.end();
+        last_tan = Some(t1);
+    }
+    flush(&mut out, &mut run, accuracy, &mut fresh);
+    out
+}
+
+/// Simplify `run` (one move and the segments after it) with kurbo and append it to `out`; the
+/// run's move is kept only when it starts a subpath (`fresh`), else the run carries on from the
+/// last point drawn. `run` is left empty.
+fn flush(out: &mut BezPath, run: &mut BezPath, accuracy: f64, fresh: &mut bool) {
+    if run.elements().len() > 1 {
+        let fitted = kurbo::simplify::simplify_bezpath(run.iter(), accuracy, &Default::default());
+        let fitted = if keeps_to(&fitted, run, STRAY * accuracy) { fitted } else { std::mem::take(run) };
+        for el in fitted.elements() {
+            match el {
+                PathEl::MoveTo(_) if !*fresh => {}
+                _ => {
+                    out.push(*el);
+                    *fresh = false;
+                }
+            }
+        }
+    }
+    *run = BezPath::new();
+}
+
+/// Whether every point of `fitted` lies within `tol` of `source`: one subpath each, from the same
+/// start to the same end, `fitted` being kurbo's simplification of `source`.
+///
+/// kurbo ends each fitted segment on the source, so the two are walked together: for each fitted
+/// segment, the source segments from where it starts to the first one its end lies on are the only
+/// ones its sample points are measured against. The work is linear in the two lengths. A fitted end
+/// found on no source segment counts as straying.
+fn keeps_to(fitted: &BezPath, source: &BezPath, tol: f64) -> bool {
+    /// Parts each fitted segment is cut into: the points between them are the ones measured.
+    const SAMPLES: u32 = 16;
+    let src: Vec<PathSeg> = source.segments().collect();
+    // kurbo puts each end on the source by evaluating it: off by rounding at most.
+    let on2 = (tol * 1e-3).powi(2);
+    let (tol2, mut cur) = (tol * tol, 0);
+    for seg in fitted.segments() {
+        let Some(last) = (cur..src.len()).find(|&i| src.get(i).is_some_and(|s| s.nearest(seg.end(), 1e-9).distance_sq <= on2)) else {
+            return false;
+        };
+        let near = src.get(cur..=last).unwrap_or_default();
+        for i in 1..SAMPLES {
+            let p = seg.eval(f64::from(i) / f64::from(SAMPLES));
+            if !near.iter().any(|s| s.nearest(p, 1e-6).distance_sq <= tol2) {
+                return false;
+            }
+        }
+        cur = last;
+    }
+    true
+}
+
+/// The directions `seg` leaves its start and reaches its end, skipping control points that sit on
+/// an end; `None` for a segment of no length, which kurbo skips too.
+fn tangents(seg: PathSeg) -> Option<(Vec2, Vec2)> {
+    let pts: Vec<Point> = match seg {
+        PathSeg::Line(l) => vec![l.p0, l.p1],
+        PathSeg::Quad(q) => vec![q.p0, q.p1, q.p2],
+        PathSeg::Cubic(c) => vec![c.p0, c.p1, c.p2, c.p3],
+    };
+    let (first, last) = (*pts.first()?, *pts.last()?);
+    let t0 = pts.iter().map(|p| *p - first).find(|d| d.hypot2() > 0.0)?;
+    let t1 = pts.iter().rev().map(|p| last - *p).find(|d| d.hypot2() > 0.0)?;
+    Some((t0, t1))
+}

crates/geom/src/tests.rs

@@ -366,3 +366,120 @@ fn svg_path_refuses_numbers_past_the_limit_before_parsing() {
     assert!(number_past("M0 0L-2.5.5 1e-3-4e3z", 1e3));
     assert!(number_past("M.5.5e10", 1e3));
 }
+
+/// A path through `pts` with straight lines (pencil samples).
+fn polyline(pts: &[(f64, f64)]) -> BezPath {
+    let mut p = BezPath::new();
+    for (k, (x, y)) in pts.iter().enumerate() {
+        if k == 0 { p.move_to((*x, *y)) } else { p.line_to((*x, *y)) }
+    }
+    p
+}
+
+/// How far `out` strays from `input`: the largest distance from a point sampled along `out` to the
+/// nearest segment of `input` (closing segments included).
+fn deviation(out: &BezPath, input: &BezPath) -> f64 {
+    use kurbo::{ParamCurve, ParamCurveNearest};
+    let segs: Vec<kurbo::PathSeg> = input.segments().collect();
+    out.segments()
+        .flat_map(|seg| (0..=32).map(move |i| seg.eval(f64::from(i) / 32.0)))
+        .map(|p| segs.iter().map(|s| s.nearest(p, 1e-6).distance_sq).fold(f64::INFINITY, f64::min).sqrt())
+        .fold(0.0, f64::max)
+}
+
+/// Pencil strokes kurbo's fit shot far past keep to their samples (#1741): a stroke that goes out
+/// and back to the very point it left, nearly collinear samples, and strokes that turn back along
+/// their own line. The first two are from fuzzing pencil-like strokes, where kurbo's curve went
+/// about 390 and 160 units off; at whole-pixel coordinates the same shapes happened to fit well.
+#[test]
+fn simplifying_keeps_to_strokes_kurbo_shot_past() {
+    use kurbo::ParamCurve;
+    let cases: [(&str, &[(f64, f64)]); 5] = [
+        (
+            "out and back to the same point",
+            &[
+                (86.043_565_719_086_97, -173.744_402_349_100_94),
+                (87.851_850_739_335_35, -179.202_846_445_954_52),
+                (86.043_565_719_086_97, -173.744_402_349_100_94),
+            ],
+        ),
+        (
+            "nearly collinear",
+            &[
+                (209.598_690_794_015_5, -204.909_328_119_739_88),
+                (211.277_914_263_870_97, -207.190_974_020_753_4),
+                (214.246_506_828_720_84, -211.228_827_590_623_24),
+            ],
+        ),
+        (
+            "a dip straight down and back up",
+            &[(0.0, 0.0), (10.0, 0.0), (20.0, 2.0), (30.0, 6.0), (30.0, 12.0), (30.0, 6.0), (40.0, 2.0), (50.0, 0.0)],
+        ),
+        ("a slip back at the end", &[(0.0, 0.0), (10.0, 3.0), (20.0, 8.0), (30.0, 15.0), (30.0, 25.0), (30.0, 22.0)]),
+        (
+            "a near reversal in a curve",
+            &[(0.0, 0.0), (8.0, 2.0), (16.0, 6.0), (24.0, 12.0), (32.0, 20.0), (24.0, 12.000_01), (16.0, 6.0), (8.0, 2.0)],
+        ),
+    ];
+    for (what, pts) in cases {
+        let input = polyline(pts);
+        let out = simplify_path(&input, 0.75);
+        let d = deviation(&out, &input);
+        assert!(d <= 1.5, "{what}: strays {d} from the samples: {}", out.to_svg());
+        let (inb, outb) = (input.bounding_box(), out.bounding_box());
+        assert!(inb.inflate(1.5, 1.5).contains_rect(outb), "{what}: {outb:?} past {inb:?}");
+        // One subpath, starting and ending where the stroke does.
+        let moves = out.elements().iter().filter(|e| matches!(e, kurbo::PathEl::MoveTo(_))).count();
+        assert_eq!(moves, 1, "{what}: {}", out.to_svg());
+        let (first, last) = (pts[0], pts[pts.len() - 1]);
+        assert_eq!(out.segments().next().map(|s| s.start()), Some(Point::new(first.0, first.1)), "{what}");
+        assert_eq!(out.segments().last().map(|s| s.end()), Some(Point::new(last.0, last.1)), "{what}");
+    }
+}
+
+/// Where a path never turns back and kurbo's fit holds, the result is kurbo's own, element for
+/// element: smooth runs, corners sharper than a right angle, closed subpaths and several subpaths.
+#[test]
+fn simplifying_is_kurbos_where_its_fit_holds() {
+    let wave: Vec<(f64, f64)> = (0..=60).map(|i| (f64::from(i) * 4.0, (f64::from(i) / 6.0).sin() * 30.0)).collect();
+    let zigzag: Vec<(f64, f64)> = (0..=8).map(|i| (f64::from(i) * 10.0, if i % 2 == 0 { 0.0 } else { 40.0 })).collect();
+    let mut closed = polyline(&wave[..20]);
+    closed.close_path();
+    let mut several = polyline(&wave);
+    several.extend(polyline(&zigzag).elements().iter().map(|e| match *e {
+        kurbo::PathEl::MoveTo(p) => kurbo::PathEl::MoveTo(p + Vec2::new(0.0, 100.0)),
+        kurbo::PathEl::LineTo(p) => kurbo::PathEl::LineTo(p + Vec2::new(0.0, 100.0)),
+        e => e,
+    }));
+    let curves = BezPath::from_svg("M0 0 C10 -20 30 -20 40 0 C50 20 70 20 80 0 Q100 -30 120 0 L140 10 Z M200 0 L240 0").unwrap();
+    for (what, input) in
+        [("a wave", polyline(&wave)), ("a zigzag", polyline(&zigzag)), ("closed", closed), ("two subpaths", several), ("curves", curves)]
+    {
+        let ours = simplify_path(&input, 0.75);
+        let theirs = kurbo::simplify::simplify_bezpath(input.iter(), 0.75, &Default::default());
+        assert_eq!(ours.elements(), theirs.elements(), "{what}");
+    }
+}
+
+/// A closed path with a reversal in it stays one closed subpath, and each subpath of several keeps
+/// its own start.
+#[test]
+fn simplifying_keeps_closes_and_subpaths_across_a_reversal() {
+    let mut input = polyline(&[(0.0, 0.0), (40.0, 0.0), (40.0, 30.0), (40.0, 20.0), (0.0, 30.0)]);
+    input.close_path();
+    input.extend(polyline(&[(100.0, 0.0), (100.0, 20.0), (100.0, 10.0)]).elements().iter().copied());
+    let out = simplify_path(&input, 0.75);
+    let els = out.elements();
+    let moves: Vec<Point> = els.iter().filter_map(|e| if let kurbo::PathEl::MoveTo(p) = e { Some(*p) } else { None }).collect();
+    assert_eq!(moves, [Point::new(0.0, 0.0), Point::new(100.0, 0.0)], "{}", out.to_svg());
+    assert_eq!(els.iter().filter(|e| matches!(e, kurbo::PathEl::ClosePath)).count(), 1, "{}", out.to_svg());
+    let close = els.iter().position(|e| matches!(e, kurbo::PathEl::ClosePath)).unwrap();
+    assert!(matches!(els[close + 1], kurbo::PathEl::MoveTo(_)), "the first subpath closes before the second starts");
+    assert!(deviation(&out, &input) <= 1.5, "{}", out.to_svg());
+    // Nothing to draw: nothing out, not a stray move or close.
+    let mut empty = BezPath::new();
+    empty.move_to((5.0, 5.0));
+    empty.line_to((5.0, 5.0));
+    empty.close_path();
+    assert!(simplify_path(&empty, 0.75).elements().is_empty());
+}