Merge pull request #7628 from dotCooCoo/fix/clipping-members-drawn-budget

Effects: a clipping path's members count toward the member limit of a group's effects

crates/effects/src/clip.rs

@@ -12,7 +12,7 @@ use vectorcraft_geom::{BezPath, FillRule, PathData};
 /// fills and strokes: the region is then the union of the paths they give, as an export bakes them.
 /// `None` when there is nothing to clip by (the clipped art is then hidden).
 pub fn clip_outline(clip: &Node) -> Option<(BezPath, FillRule)> {
-    match clip_effects_applied(clip, true) {
+    match clip_effects_applied(clip, true, || true) {
         Some(m) => m.clip_outline(Some(&outline_text), &unite),
         None => clip.clip_outline(Some(&outline_text), &unite),
     }
@@ -31,14 +31,26 @@ pub fn clip_outline(clip: &Node) -> Option<(BezPath, FillRule)> {
 /// geometry effects of its own fills and strokes live instead. `None` when nothing in it has a
 /// geometry effect. Effects on the clip group or around it then reshape the result, after its own.
 pub(crate) fn with_clip_effects_applied(n: &Node) -> Option<Node> {
-    let paint = n.clip_paint();
-    clip_effects_applied(n, paint.fill.is_none() && paint.stroke.is_none())
+    with_clip_effects_applied_while(n, || true)
 }
 
-/// [`with_clip_effects_applied`]; with `items` unset (a clipping path that paints, and the paths of
-/// a compound path, which draw with the compound path's fills and strokes), the geometry effects of
-/// `n`'s own fills and strokes are left out.
-fn clip_effects_applied(n: &Node, items: bool) -> Option<Node> {
+/// [`with_clip_effects_applied`] that calls `take` before it visits each member of a group, layer
+/// or compound path in `n`, at every depth: once `take` returns `false`, that member and the
+/// members after it are not visited. The members of a group or layer that are not visited stay as
+/// they are, with their effects. A compound path with geometry effects of its own still becomes a
+/// path of its reshaped geometry. In a clipping path that does not paint, one whose fills or
+/// strokes have geometry effects still becomes a group of one path per fill and stroke. In both
+/// cases the paths in it that are not visited go
+/// into that geometry as they are, and their own effects are not applied.
+pub(crate) fn with_clip_effects_applied_while(n: &Node, take: fn() -> bool) -> Option<Node> {
+    let paint = n.clip_paint();
+    clip_effects_applied(n, paint.fill.is_none() && paint.stroke.is_none(), take)
+}
+
+/// [`with_clip_effects_applied_while`]; with `items` unset (a clipping path that paints, and the
+/// paths of a compound path, which draw with the compound path's fills and strokes), the geometry
+/// effects of `n`'s own fills and strokes are left out.
+fn clip_effects_applied(n: &Node, items: bool, take: fn() -> bool) -> Option<Node> {
     let mut m: Option<Node> = None;
     let members: &[std::sync::Arc<Node>] = match &n.kind {
         NodeKind::Group { children, clip: true } => children.get(..1).unwrap_or_default(),
@@ -53,7 +65,10 @@ fn clip_effects_applied(n: &Node, items: bool) -> Option<Node> {
     // The paths of a compound path draw with the compound path's fills and strokes, and stay paths.
     let compound = matches!(n.kind, NodeKind::Compound { .. });
     for (i, c) in members.iter().enumerate().skip(skip).filter(|(_, c)| all || c.visible) {
-        if let Some(r) = clip_effects_applied(c, items && !compound)
+        if !take() {
+            break;
+        }
+        if let Some(r) = clip_effects_applied(c, items && !compound, take)
             && let Some(slot) = m.get_or_insert_with(|| n.clone()).children_mut().and_then(|ch| ch.get_mut(i))
         {
             *slot = std::sync::Arc::new(r);

crates/effects/src/group.rs

@@ -904,7 +904,10 @@ pub fn evaluate_container(doc: Option<&Document>, n: &Node) -> Option<Node> {
 /// depth of its groups and layers, by the art it draws ([`evaluate_container`],
 /// [`crate::reshape::own_effects_art`]), so the geometry effects of `n` reshape what its members
 /// draw. A clip group's clipping path only takes the geometry effects in it into its geometry
-/// ([`crate::clip::with_clip_effects_applied`]). Images, symbol instances and the other objects
+/// ([`crate::clip::with_clip_effects_applied_while`]), and the members of a clipping group or
+/// compound path count toward the limit below. A group or layer in a clipping path that has
+/// geometry effects of its own is walked twice, once for its members' effects and once for its
+/// own, and its members count each time. Images, symbol instances and the other objects
 /// that `own_effects_art` leaves alone stay as they are, and so does a group or layer with a 3D
 /// effect with everything in it. Calls nested in one another, through the
 /// groups and layers it walks and the art it evaluates (symbol art and Pathfinder members
@@ -917,7 +920,7 @@ pub(crate) fn members_as_drawn(doc: Option<&Document>, n: &mut Node) {
     // The clipping path's own geometry effects shape it before the effects around it do.
     if clip == 1
         && let Some(first) = ch.first_mut()
-        && let Some(applied) = crate::clip::with_clip_effects_applied(first)
+        && let Some(applied) = crate::clip::with_clip_effects_applied_while(first, DrawnFrame::take_member)
     {
         *first = Arc::new(applied);
     }

crates/effects/src/group/tests.rs

@@ -849,6 +849,53 @@ fn nested_groups_with_effects_count_toward_the_depth_limit() {
     assert!(deepest.is_some_and(|d| crate::reshape::geometry_effects(&d).len() == 1), "the deepest member keeps its effect");
 }
 
+/// The members of a clipping path count toward the member limit: with room for two more members,
+/// the third member of a clipping group keeps its own effect as it is.
+#[test]
+fn clipping_path_members_past_the_member_limit_stay_as_they_are() {
+    let member = |id: u64| {
+        let mut m = with_effect(square_at_100(), offset_10());
+        m.id = NodeId(id);
+        Arc::new(m)
+    };
+    let clipping = Node::group(NodeId(20), vec![member(2), member(3), member(4)]);
+    let mut clipped = Node::new(NodeId(5), NodeKind::Group { children: vec![Arc::new(clipping), Arc::new(square_at_100())], clip: true });
+    // An enclosing call that has visited all the members it may but two.
+    let _outer = DrawnFrame::enter().unwrap();
+    DRAWN.with(|d| d.set((1, crate::SYMBOL_ART_MAX_NODES - 2)));
+    members_as_drawn(None, &mut clipped);
+    let clipping = clipped.children().unwrap()[0].clone();
+    let effects: Vec<usize> = clipping.children().unwrap().iter().map(|m| crate::reshape::geometry_effects(m).len()).collect();
+    assert_eq!(effects, [0, 0, 1], "the first two members take their Offset Path, the third keeps it");
+    let widths: Vec<f64> = clipping.children().unwrap().iter().map(|m| m.geometric_bounds().unwrap().width()).collect();
+    assert!((widths[0] - 120.0).abs() < 0.5 && (widths[1] - 120.0).abs() < 0.5 && (widths[2] - 100.0).abs() < 0.5, "{widths:?}");
+}
+
+/// A compound clipping path with an effect of its own takes that effect when the member limit is
+/// reached among its paths: with room for one more member, the first path (at 100 to 200) takes
+/// its Offset Path, the second (at 300 to 400) goes in as it is, and the compound path's Offset
+/// Path reshapes both, to 80 to 220 and 290 to 410.
+#[test]
+fn a_compound_clipping_path_takes_its_own_effect_at_the_member_limit() {
+    let member = |id: u64, x: f64| {
+        let path = Node::path(NodeId(id), shapes::rectangle(Rect::new(x, 100.0, x + 100.0, 200.0)), black());
+        Arc::new(with_effect(path, offset_10()))
+    };
+    let kind = NodeKind::Compound { children: vec![member(2, 100.0), member(3, 300.0)], rule: FillRule::NonZero };
+    let clipping = with_effect(Node::new(NodeId(20), kind), offset_10());
+    let mut clipped = Node::new(NodeId(5), NodeKind::Group { children: vec![Arc::new(clipping), Arc::new(square_at_100())], clip: true });
+    // An enclosing call that has visited all the members it may but one.
+    let _outer = DrawnFrame::enter().unwrap();
+    DRAWN.with(|d| d.set((1, crate::SYMBOL_ART_MAX_NODES - 1)));
+    members_as_drawn(None, &mut clipped);
+    let clipping = clipped.children().unwrap()[0].clone();
+    assert!(crate::reshape::geometry_effects(&clipping).is_empty(), "the compound path's Offset Path is in its geometry");
+    let subpaths = clipping.path_data().unwrap().subpaths.clone();
+    let bounds: Vec<(f64, f64)> = subpaths.into_iter().map(|sp| PathData::single(sp).bounds().unwrap()).map(|b| (b.x0, b.x1)).collect();
+    let near = |(a, b): (f64, f64), (x0, x1): (f64, f64)| (a - x0).abs() < 0.5 && (b - x1).abs() < 0.5;
+    assert!(bounds.len() == 2 && near(bounds[0], (80.0, 220.0)) && near(bounds[1], (290.0, 410.0)), "{bounds:?}");
+}
+
 /// An image or a symbol instance that a group's Transform effect moves takes the placed gradients
 /// of its own fills along, as a path does, and each copy takes them as many times as it takes the
 /// transform.