Merge pull request #4501 from Silvertree2010/fix/4495-bug-eraser-tool-breaks-compound

Keep compound path holes when using the Eraser and Knife

crates/engine/src/cmd/draw2/common.rs

@@ -168,6 +168,33 @@ pub(super) fn target_paths(doc: &Document, sel: &Selection, area: Rect) -> Vec<N
     out
 }
 
+/// [`target_paths`] for the tools that cut fills, with each target's geometry and fill rule: the
+/// members of a compound path whose members are all closed paths give the compound path itself,
+/// once, so its fill (holes included) is cut as one shape under its own fill rule.
+pub(super) fn fill_targets(doc: &Document, sel: &Selection, area: Rect) -> Result<Vec<(NodeId, PathData, FillRule)>> {
+    let closed_compound = |n: &Node| {
+        matches!(&n.kind, NodeKind::Compound { children, .. }
+            if children.iter().all(|c| matches!(&c.kind, NodeKind::Path { path, .. } if path.is_closed())))
+    };
+    let editable = doc.editable();
+    let mut out: Vec<(NodeId, PathData, FillRule)> = vec![];
+    for id in target_paths(doc, sel, area) {
+        let compound = doc.parent_of(id).and_then(|p| doc.node(p)).filter(|p| closed_compound(p) && editable(p.id));
+        let target = compound.map_or(id, |c| c.id);
+        if out.iter().any(|t| t.0 == target) {
+            continue;
+        }
+        let (pd, rule) = match compound.map(|c| &c.kind) {
+            Some(NodeKind::Compound { children, rule }) => {
+                (PathData::new(children.iter().filter_map(|c| c.path_data()).flat_map(|p| p.subpaths.clone()).collect()), *rule)
+            }
+            _ => path_of(doc, id)?,
+        };
+        out.push((target, pd, rule));
+    }
+    Ok(out)
+}
+
 /// The members of a compound path with geometry `pd`: one path per subpath, with fresh ids.
 fn compound_members(d: &mut Document, pd: PathData) -> Vec<Arc<Node>> {
     pd.subpaths.into_iter().map(|sp| Arc::new(Node::path(d.alloc_id(), PathData::single(sp), Appearance::default()))).collect()

crates/engine/src/cmd/draw2/cut.rs

@@ -1,5 +1,6 @@
 use super::common::{
-    dist_to_polyline, ids_json, path_mut, path_of, points_param, poly_bounds, replace_with_pieces, sibling_with, target_paths, usize_opt,
+    dist_to_polyline, fill_targets, ids_json, path_mut, path_of, points_param, poly_bounds, replace_with_pieces, sibling_with, target_paths,
+    usize_opt,
 };
 use super::{
     Anchor, AnchorKind, Beside, BezPath, BoolOp, Cap, EngineError, FillRule, Join, NodeId, ParamCurve, PathData, Point, Result, Session, SubPath,
@@ -236,8 +237,7 @@ pub(super) fn knife(s: &mut Session, p: &Value) -> Result<Value> {
     let st = s.doc()?;
     let strip = po::outline_stroke(&PathData::single(SubPath::polyline(&pts, false)), KNIFE_GAP, Cap::Butt, Join::Round, 4.0);
     let mut plan: Vec<(NodeId, Vec<PathData>)> = vec![];
-    for id in target_paths(&st.doc, &st.selection, poly_bounds(&pts, 1.0)) {
-        let (pd, rule) = path_of(&st.doc, id)?;
+    for (id, pd, rule) in fill_targets(&st.doc, &st.selection, poly_bounds(&pts, 1.0))? {
         if !pd.is_closed() {
             continue;
         }
@@ -281,8 +281,7 @@ pub(super) fn erase_region(s: &mut Session, p: &Value) -> Result<Value> {
     let squash = eraser_tip(angle, roundness).inverse();
     let round: Vec<Point> = pts.iter().map(|q| squash * *q).collect();
     let mut plan: Vec<(NodeId, Vec<PathData>)> = vec![];
-    for id in target_paths(&st.doc, &st.selection, poly_bounds(&pts, r)) {
-        let (pd, rule) = path_of(&st.doc, id)?;
+    for (id, pd, rule) in fill_targets(&st.doc, &st.selection, poly_bounds(&pts, r))? {
         if pd.is_closed() {
             if po::area(&po::boolean(&pd, rule, &region, FillRule::NonZero, BoolOp::Intersect), FillRule::NonZero) <= 1e-6 {
                 continue;

crates/engine/src/tests/draw2.rs

@@ -809,6 +809,52 @@ fn eraser_removes_small_shape_entirely() {
     assert!(paths(&s).is_empty());
 }
 
+/// #4495: the Eraser cuts a compound path as one shape: its hole stays empty, the erased area is
+/// empty, and the rest stays filled, with either fill rule. A notch inside the ring keeps one
+/// compound path; a drag through the whole ring leaves compound paths whose holes stay holes.
+#[test]
+fn eraser_keeps_the_holes_of_a_compound_path() {
+    for rule in ["nonZero", "evenOdd"] {
+        for (drag, erased) in [([[200, 110], [200, 120]], (200.0, 115.0)), ([[200, 80], [200, 320]], (200.0, 120.0))] {
+            let mut s = session();
+            let outer = rect(&mut s, 100.0, 100.0, 200.0, 200.0);
+            let inner = rect(&mut s, 150.0, 150.0, 100.0, 100.0);
+            s.execute("select.set", &json!({"ids": [outer.0, inner.0]})).unwrap();
+            s.execute("object.compoundPath.make", &json!({})).unwrap();
+            s.execute("path.setFillRule", &json!({"rule": rule})).unwrap();
+            s.execute("select.none", &json!({})).unwrap();
+            s.execute("path.eraseRegion", &json!({"points": drag, "size": 10})).unwrap();
+            // Whether any top-level shape fills `p`, a compound path with all its members at once.
+            let filled = |x: f64, y: f64| {
+                let doc = &s.doc().unwrap().doc;
+                let mut hit = false;
+                for layer in &doc.layers {
+                    for n in layer.children().unwrap() {
+                        let (subs, rule) = match &n.kind {
+                            NodeKind::Path { path, rule, .. } => (path.subpaths.clone(), *rule),
+                            NodeKind::Compound { children, rule } => {
+                                (children.iter().flat_map(|c| c.path_data().unwrap().subpaths.clone()).collect(), *rule)
+                            }
+                            _ => continue,
+                        };
+                        hit |= vectorcraft_geom::hit::fill_contains(&PathData::new(subs).to_bezpath(), rule, Point::new(x, y));
+                    }
+                }
+                hit
+            };
+            assert!(!filled(170.0, 200.0), "{rule} {drag:?}: the hole stays empty");
+            assert!(!filled(erased.0, erased.1), "{rule} {drag:?}: the erased area is empty");
+            assert!(filled(120.0, 200.0) && filled(280.0, 200.0), "{rule} {drag:?}: the ring stays filled");
+            if drag[1][1] == 120 {
+                let doc = &s.doc().unwrap().doc;
+                let tops: Vec<_> = doc.layers.iter().flat_map(|l| l.children().unwrap().iter().cloned()).collect();
+                assert_eq!(tops.len(), 1, "{rule}: one object");
+                assert!(matches!(tops[0].kind, NodeKind::Compound { .. }), "{rule}: still a compound path");
+            }
+        }
+    }
+}
+
 /// The Eraser's tip is an ellipse `size` across its long axis, at `angle` counter-clockwise from
 /// horizontal, with a short axis `roundness` percent as long: a vertical drag across a square
 /// removes a band as wide as the tip is across, and a press on a horizontal line removes the