Merge pull request #4035 from dotCooCoo/fix/shape-builder-drag-narrow-regions

Shape Builder: a drag picks every region it crosses, however narrow

crates/engine/src/cmd/buildcmds.rs

@@ -19,8 +19,7 @@ use vectorcraft_doc::{Appearance, Document, GapOptions, GapSize, LiveShape, Node
 use vectorcraft_geom::{FillRule, PathData, Point, Rect, SubPath};
 use vectorcraft_pathops as po;
 use vectorcraft_tools::builder::{
-    self as b, BuilderMap, EDGE_NAME, FACE_NAME, LIVE_PAINT_NAME, SOURCES_NAME, Touched, edge_near, face_at, is_live_paint, sample_polyline,
-    sorted_roots,
+    self as b, BuilderMap, EDGE_NAME, FACE_NAME, LIVE_PAINT_NAME, SOURCES_NAME, Touched, edge_near, face_at, is_live_paint, sorted_roots,
 };
 use vectorcraft_trace as tr;
 
@@ -296,7 +295,6 @@ fn sb_merge(s: &mut Session, p: &Value) -> Result<Value> {
         if map.shapes.is_empty() {
             return Err(EngineError::Other("Shape Builder: select paths or compound paths".into()));
         }
-        let step = (d.bounds_of(&roots, false).map(|r| r.width().max(r.height())).unwrap_or(100.0) / 2000.0).max(0.25);
         if let Some(tol) = split_tol
             && let Some(&q) = pts.first()
             && let Some((i, run)) = map.stroke_at(&outlines, q, tol)
@@ -329,9 +327,7 @@ fn sb_merge(s: &mut Session, p: &Value) -> Result<Value> {
             return Ok((ids, None, touched, Some(split)));
         }
         let mut touched = Touched { regions: marquee.map(|(a, b)| map.regions_in_marquee(a, b)).unwrap_or_default(), ..Default::default() };
-        for q in sample_polyline(&pts, step) {
-            touched.add(map.hit(q, piece_tol));
-        }
+        touched.extend(&map.hits_along(&pts, piece_tol));
         touched.edges = map.erased_edges(&touched);
         if touched.is_empty() {
             return Err(EngineError::Other("Shape Builder: no region under the pointer".into()));

crates/engine/src/cmd/shaper.rs

@@ -9,7 +9,7 @@ use vectorcraft_doc::shaper::{EdgeEdit, FILL_PREFIX, FaceEdit, FaceSelector, SOU
 use vectorcraft_doc::{Appearance, AppearanceItem, Document, Node, NodeId, NodeKind};
 use vectorcraft_geom::{FillRule, PathData, Point, Rect, Shape as _};
 use vectorcraft_pathops as po;
-use vectorcraft_tools::builder::{self as b, BuilderMap, Hit};
+use vectorcraft_tools::builder::{self as b, BuilderMap};
 
 const MAX_SOURCES: usize = 64;
 const MAX_POINTS: usize = 4096;
@@ -617,8 +617,9 @@ fn scribble(s: &mut Session, p: &Value) -> Result<Value> {
     let edits = face_edits(&map, &src, &spec);
     let filled = |i: usize| edits.get(&i).is_none_or(|e| !e.erase) && map.regions.get(i).and_then(|r| fill_source(&src, &r.0)).is_some();
     let components = visible_components(&map, &src, &edits);
+    // Every face the scribble crosses, however narrow.
     let faces: std::collections::BTreeSet<usize> =
-        points.iter().filter_map(|p| map.region_at(*p)).filter(|i| filled(*i)).filter_map(|i| components.get(i).copied()).collect();
+        map.hits_along(&pts, None).regions.into_iter().filter(|i| filled(*i)).filter_map(|i| components.get(i).copied()).collect();
     let touched: Vec<usize> = components.iter().enumerate().filter(|(_, c)| faces.contains(c)).map(|(i, _)| i).collect();
     let first = points.first().copied().unwrap_or(Point::ZERO);
     let last = points.last().copied().unwrap_or(first);
@@ -642,19 +643,12 @@ fn scribble(s: &mut Session, p: &Value) -> Result<Value> {
     }
     let mut strokes = 0;
     if touched.is_empty() {
-        let mut piece_ids = vec![];
+        // Every stroke the scribble crosses or passes within `tolerance` of, however small that is.
+        let mut hits = map.hits_along(&pts, Some(tolerance));
         for point in &points {
-            let index = match map.hit(*point, Some(tolerance)) {
-                Some(Hit::Line(i)) => Some(map.edges.len() + i),
-                Some(Hit::Edge(i)) => Some(i),
-                _ => None,
-            };
-            if let Some(i) = index
-                && !piece_ids.contains(&i)
-            {
-                piece_ids.push(i);
-            }
+            hits.add(map.hit(*point, Some(tolerance)));
         }
+        let piece_ids = hits.edges.iter().copied().chain(hits.lines.iter().map(|i| map.edges.len() + i));
         for i in piece_ids {
             if let Some(e) = pieces(&map).get(i).and_then(|p| edge_selector(&map, &src, p)) {
                 spec.edges.push(e);

crates/engine/src/tests_build.rs

@@ -49,8 +49,13 @@ fn area(s: &Session, id: NodeId) -> f64 {
 
 /// Rect A (0..100) and rect B (50..150), both 100 tall, selected. A red, B blue.
 fn two(s: &mut Session) -> (NodeId, NodeId) {
+    two_at(s, 50.0)
+}
+
+/// [`two`] with B at `x`..`x`+100.
+fn two_at(s: &mut Session, x: f64) -> (NodeId, NodeId) {
     let a = rect(s, 0.0, 0.0, 100.0, 100.0);
-    let b = rect(s, 50.0, 0.0, 100.0, 100.0);
+    let b = rect(s, x, 0.0, 100.0, 100.0);
     set_fill(s, a, Color::rgb(1.0, 0.0, 0.0));
     set_fill(s, b, Color::rgb(0.0, 0.0, 1.0));
     s.execute("select.set", &json!({"ids": [a.0, b.0]})).unwrap();
@@ -81,6 +86,35 @@ fn shape_builder_drag_merges_touched_regions() {
     assert_eq!(layer_children(&s), 2);
 }
 
+#[test]
+fn shape_builder_drag_merges_a_region_thinner_than_the_sampling_step() {
+    let mut s = session();
+    two_at(&mut s, 99.9);
+    assert_eq!(s.execute("shapeBuilder.regions", &json!({})).unwrap()["regions"].as_array().unwrap().len(), 3);
+    let r = s.execute("shapeBuilder.merge", &json!({"points": [[50, 50], [150, 50]]})).unwrap();
+    assert_eq!(r["regions"], 3, "the 0.1 pt overlap too");
+    let out = ids(&r);
+    assert_eq!(out.len(), 1, "{out:?}");
+    assert_eq!(outline(&node(&s, out[0])).0.subpaths.len(), 1);
+    assert!((area(&s, out[0]) - 19990.0).abs() < 1.0, "{}", area(&s, out[0]));
+}
+
+#[test]
+fn shape_builder_drag_merges_a_narrow_overlap_when_far_art_is_selected() {
+    let mut s = session();
+    let (a, b) = two_at(&mut s, 99.0);
+    // A far square makes the selection 4,010 pt wide.
+    let far = rect(&mut s, 4000.0, 4000.0, 10.0, 10.0);
+    s.execute("select.set", &json!({"ids": [a.0, b.0, far.0]})).unwrap();
+    let r = s.execute("shapeBuilder.merge", &json!({"points": [[50.3, 50], [150.3, 50]]})).unwrap();
+    assert_eq!(r["regions"], 3);
+    let out = ids(&r);
+    assert_eq!(out.len(), 2, "the merged shape and the far square: {out:?}");
+    let merged = NodeId(r["merged"].as_u64().unwrap());
+    assert_eq!(outline(&node(&s, merged)).0.subpaths.len(), 1);
+    assert!((area(&s, merged) - 19900.0).abs() < 1.0, "{}", area(&s, merged));
+}
+
 #[test]
 fn shape_builder_click_splits_out_one_region() {
     let mut s = session();
@@ -331,6 +365,54 @@ fn shape_builder_tool_drives_the_command() {
     assert_eq!(layer_children(&s), 1);
 }
 
+#[test]
+fn shape_builder_tool_merges_the_narrow_shape_it_highlights_in_a_wide_selection() {
+    let mut s = session();
+    let strip = rect(&mut s, 199.5, 100.0, 1.0, 100.0);
+    // A far square makes the selection 9,800.5 pt wide.
+    let far = rect(&mut s, 9900.0, 100.0, 100.0, 100.0);
+    s.execute("select.set", &json!({"ids": [strip.0, far.0]})).unwrap();
+    let v = ViewInfo::default();
+    s.select_tool("shapeBuilder", v).unwrap();
+    s.pointer(&PointerEvent::new(PointerKind::Down, 190.0, 150.0), v).unwrap();
+    s.pointer(&PointerEvent::new(PointerKind::Drag, 210.0, 150.0), v).unwrap();
+    assert_eq!(s.overlays(v).len(), 3, "the strip's hatch and outline, and the drag path");
+    s.pointer(&PointerEvent::new(PointerKind::Up, 210.0, 150.0), v).unwrap();
+    assert_eq!(s.journal.last().unwrap().0, "shapeBuilder.merge");
+    assert_eq!(layer_children(&s), 2);
+}
+
+#[test]
+fn shape_builder_tool_highlights_and_merges_a_narrow_overlap() {
+    let mut s = session();
+    two_at(&mut s, 99.95);
+    let v = ViewInfo::default();
+    s.select_tool("shapeBuilder", v).unwrap();
+    s.pointer(&PointerEvent::new(PointerKind::Down, 50.3, 50.0), v).unwrap();
+    s.pointer(&PointerEvent::new(PointerKind::Drag, 150.3, 50.0), v).unwrap();
+    assert_eq!(s.overlays(v).len(), 7, "the three regions' hatches and outlines, and the drag path");
+    s.pointer(&PointerEvent::new(PointerKind::Up, 150.3, 50.0), v).unwrap();
+    assert_eq!(layer_children(&s), 1, "one shape, no leftover strips");
+    let d = &s.doc().unwrap().doc;
+    let merged = d.children(d.default_layer()).unwrap()[0].id;
+    assert_eq!(outline(&node(&s, merged)).0.subpaths.len(), 1);
+}
+
+#[test]
+fn shape_builder_tool_zoomed_out_picks_a_narrow_shape() {
+    let mut s = session();
+    let strip = rect(&mut s, 199.5, 100.0, 1.0, 100.0);
+    let near = rect(&mut s, 300.0, 100.0, 3.0, 100.0);
+    s.execute("select.set", &json!({"ids": [strip.0, near.0]})).unwrap();
+    let v = ViewInfo { zoom: 0.05, ..Default::default() };
+    s.select_tool("shapeBuilder", v).unwrap();
+    s.pointer(&PointerEvent::new(PointerKind::Down, 190.0, 150.0), v).unwrap();
+    s.pointer(&PointerEvent::new(PointerKind::Drag, 260.0, 150.0), v).unwrap();
+    assert_eq!(s.overlays(v).len(), 3, "the strip's hatch and outline, and the drag path");
+    s.pointer(&PointerEvent::new(PointerKind::Up, 260.0, 150.0), v).unwrap();
+    assert_eq!(s.journal.last().unwrap().0, "shapeBuilder.merge");
+}
+
 /// The first and last stop colours of a gradient paint.
 fn gradient_ends(p: &Paint) -> Option<([f32; 3], [f32; 3])> {
     let Paint::Gradient(g) = p else { return None };

crates/engine/src/tests_shaper.rs

@@ -108,6 +108,18 @@ fn shaper_interior_punch_removes_the_whole_visible_front_face() {
     }
 }
 
+#[test]
+fn shaper_scribble_merges_a_face_narrower_than_its_sampling_step() {
+    let mut s = session();
+    rectangle(&mut s, 0.0, "#0000ff");
+    s.execute("paint.setFill", &json!({"color": "#ff0000", "ids": []})).unwrap();
+    s.execute("shape.rectangle", &json!({"x": 49.8, "y": 10, "width": 0.4, "height": 80})).unwrap();
+    // Every crossing of the 0.4 pt strip falls between two of the 2 pt samples.
+    let g = scribble(&mut s, &[[25.0, 20.0], [75.0, 35.0], [30.0, 50.0], [75.0, 65.0]]);
+    assert_eq!(fill(&s, g, 50.0, 50.0).as_deref(), Some("#0000ff"), "the strip merges into the face around it");
+    assert_eq!(fill(&s, g, 25.0, 50.0).as_deref(), Some("#0000ff"));
+}
+
 #[test]
 fn shaper_exiting_scribble_erases_only_touched_fill_and_boundary_strokes() {
     let mut s = session();
@@ -141,6 +153,22 @@ fn shaper_erases_only_protruding_stroke_pieces_keeps_fill_and_original_line() {
     assert_eq!(node(&s, line), &original_line);
 }
 
+#[test]
+fn shaper_scribble_with_a_small_tolerance_erases_a_stroke_it_crosses_between_samples() {
+    for (tolerance, from, to) in [(0.1, 140.25, 160.25), (0.01, 50.0123, 250.0123)] {
+        let mut s = session();
+        s.execute("paint.setFill", &json!({"color":"#ffff00"})).unwrap();
+        s.execute("shape.ellipse", &json!({"x":100,"y":100,"width":100,"height":100})).unwrap();
+        s.execute("shape.line", &json!({"x1":50,"y1":150,"x2":250,"y2":150})).unwrap();
+        // No sample along the stroke lands within `tolerance` of the line at y = 150.
+        let v = s.execute("shaper.scribble", &json!({"points": [[60.0, from], [60.0, to]], "tolerance": tolerance})).unwrap();
+        assert_eq!(v["strokes"], json!(1), "tolerance {tolerance}: {v}");
+        let g = NodeId(v["id"].as_u64().unwrap());
+        assert!(!stroke(&s, g, 75.0, 150.0));
+        assert!(stroke(&s, g, 150.0, 150.0));
+    }
+}
+
 /// Export Selection crops a Shaper group to the art it shows, not to its transparent source art.
 #[test]
 fn shaper_export_selection_crops_to_the_shown_art() {

crates/tools/src/builder.rs

@@ -392,6 +392,74 @@ impl BuilderMap {
         Some((i, run))
     }
 
+    /// What a gesture along the polyline `pts` touches, in order: [`Self::hit`] at each vertex and
+    /// at the points [`Self::probes`] picks between them, so it touches every region it crosses,
+    /// however narrow.
+    pub fn hits_along(&self, pts: &[Point], piece_tol: Option<f64>) -> Touched {
+        let mut touched = Touched::default();
+        let mut last: Option<Point> = None;
+        for &p in pts {
+            if let Some(a) = last {
+                for q in self.probes(a, p, piece_tol) {
+                    touched.add(self.hit(q, piece_tol));
+                }
+            }
+            touched.add(self.hit(p, piece_tol));
+            last = Some(p);
+        }
+        touched
+    }
+
+    /// The points strictly between `a` and `b`, in order, that [`Self::hits_along`] tests: the
+    /// middle of each stretch between two places where the segment crosses a region's outline, and
+    /// with `piece_tol` (erasing) those crossings, the places where it crosses a line piece or an
+    /// edge, and points `piece_tol / 2` apart (at most 4,000 per segment), so the line pieces and
+    /// edges the segment crosses or passes close to are touched too.
+    fn probes(&self, a: Point, b: Point, piece_tol: Option<f64>) -> Vec<Point> {
+        let len = a.distance(b);
+        if !(len.is_finite() && len > 0.0) {
+            return vec![];
+        }
+        let line = kurbo::Line::new(a, b);
+        let lb = Rect::from_points(a, b);
+        let mut cross: Vec<f64> = vec![];
+        for (_, bp, rb) in &self.regions {
+            if rb.x0 > lb.x1 || rb.x1 < lb.x0 || rb.y0 > lb.y1 || rb.y1 < lb.y0 {
+                continue;
+            }
+            for seg in bp.segments() {
+                cross.extend(seg.intersect_line(line).iter().map(|x| x.line_t));
+            }
+        }
+        cross.retain(|t| t.is_finite() && *t > 0.0 && *t < 1.0);
+        cross.sort_by(f64::total_cmp);
+        // Crossings closer than this along the segment are one place.
+        let gap = 1e-9 / len;
+        let mut ts: Vec<f64> = vec![];
+        let mut from = 0.0;
+        for &t in cross.iter().chain([1.0].iter()) {
+            if t - from > gap {
+                ts.push((from + t) / 2.0);
+                from = t;
+            }
+        }
+        if let Some(tol) = piece_tol {
+            ts.extend(cross.iter().copied());
+            for (piece, pb) in self.lines.iter().chain(&self.edges) {
+                if pb.x0 > lb.x1 || pb.x1 < lb.x0 || pb.y0 > lb.y1 || pb.y1 < lb.y0 {
+                    continue;
+                }
+                for seg in piece.path.to_bezpath().segments() {
+                    ts.extend(seg.intersect_line(line).iter().map(|x| x.line_t).filter(|t| t.is_finite() && *t > 0.0 && *t < 1.0));
+                }
+            }
+            let n = ((len / (tol / 2.0).max(1e-3)).ceil() as usize).min(4000);
+            ts.extend((1..n).map(|k| k as f64 / n as f64));
+            ts.sort_by(f64::total_cmp);
+        }
+        ts.into_iter().map(|t| a.lerp(b, t)).collect()
+    }
+
     /// The regions just either side of the middle of `piece` (None: outside every region).
     pub fn sides(&self, piece: &po::Shape) -> [Option<usize>; 2] {
         let Some(sp) = piece.path.subpaths.first() else { return [None; 2] };
@@ -446,6 +514,14 @@ impl Touched {
         }
     }
 
+    /// Adds what `other` touched, in its order.
+    pub fn extend(&mut self, other: &Touched) {
+        let regions = other.regions.iter().map(|&i| Hit::Region(i));
+        for hit in regions.chain(other.lines.iter().map(|&i| Hit::Line(i))).chain(other.edges.iter().map(|&i| Hit::Edge(i))) {
+            self.add(Some(hit));
+        }
+    }
+
     pub fn is_empty(&self) -> bool {
         self.regions.is_empty() && self.lines.is_empty() && self.edges.is_empty()
     }
@@ -741,16 +817,12 @@ impl ShapeBuilderTool {
         alt.then(|| cx.tol(LINE_TOL_PX))
     }
 
+    /// Adds what the drag from `from` to `to` touches, picked as `shapeBuilder.merge` picks it.
     fn touch(&mut self, cx: &ToolContext, from: Option<Point>, to: Point) {
-        let pts = match from {
-            Some(a) => sample_polyline(&[a, to], cx.tol(2.0)),
-            None => vec![to],
-        };
+        let pts: Vec<Point> = from.into_iter().chain([to]).collect();
         let tol = Self::line_tol(cx, self.alt);
-        let map = self.cache.get(cx, self.alt, self.opts.open_filled_as_closed);
-        for p in pts {
-            self.touched.add(map.hit(p, tol));
-        }
+        let touched = self.cache.get(cx, self.alt, self.opts.open_filled_as_closed).hits_along(&pts, tol);
+        self.touched.extend(&touched);
     }
 
     /// Selection Straight Line: the drag touches what the line from its first point to `p` crosses.