diff --git a/crates/engine/src/brush_tests.rs b/crates/engine/src/brush_tests.rs index 35dba6a7f..50317496d 100644 --- a/crates/engine/src/brush_tests.rs +++ b/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"); +} diff --git a/crates/engine/src/canvas/tools/pen.rs b/crates/engine/src/canvas/tools/pen.rs index 4b98ccee1..3f8b47e59 100644 --- a/crates/engine/src/canvas/tools/pen.rs +++ b/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); diff --git a/crates/engine/src/cmd/structure.rs b/crates/engine/src/cmd/structure.rs index 3339faa38..34cc91fbb 100644 --- a/crates/engine/src/cmd/structure.rs +++ b/crates/engine/src/cmd/structure.rs @@ -799,7 +799,7 @@ fn simplify(s: &mut Session, p: &Value) -> Result { 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 { } 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 { 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 { } 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(); } diff --git a/crates/engine/src/vector_behaviour_tests.rs b/crates/engine/src/vector_behaviour_tests.rs index c990ad348..4666c3028 100644 --- a/crates/engine/src/vector_behaviour_tests.rs +++ b/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}")) } diff --git a/crates/geom/src/lib.rs b/crates/geom/src/lib.rs index a775a4170..191b30419 100644 --- a/crates/geom/src/lib.rs +++ b/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]`. diff --git a/crates/geom/src/simplify.rs b/crates/geom/src/simplify.rs new file mode 100644 index 000000000..e768f303a --- /dev/null +++ b/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 = 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 = 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 = 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)) +} diff --git a/crates/geom/src/tests.rs b/crates/geom/src/tests.rs index 459b2b101..10e09318c 100644 --- a/crates/geom/src/tests.rs +++ b/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 = 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 = 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()); +}