Merge pull request #3374 from dotCooCoo/fix/expand-keeps-random-transform-seeds

Effects: expanded, flattened and exported art keeps the random Transform the canvas drew

crates/effects/src/bake.rs

@@ -151,17 +151,19 @@ fn bake_node(d: &mut Document, n: &Node) -> Option<Node> {
     bake_leaf(d, n)
 }
 
-/// `m`, art evaluated from an object, with its children baked and given ids of their own (the
-/// pieces copied from the source keep the source's ids until then).
+/// `m`, art evaluated from an object, with its children baked and then given ids of their own (the
+/// pieces copied from the source keep the source's ids until then). The pieces are baked with the
+/// ids the renderer draws them with: Random in a Transform effect takes its seed from the id.
 fn bake_pieces(d: &mut Document, mut m: Node) -> Node {
     let mut seen = std::collections::HashSet::from([m.id]);
     let children = m.children().cloned().unwrap_or_default();
     let children = children
         .into_iter()
         .map(|c| {
-            let mut c = Arc::unwrap_or_clone(c);
+            let c = Arc::unwrap_or_clone(c);
+            let mut c = bake_node(d, &c).unwrap_or(c);
             fresh_ids(d, &mut c, &mut seen);
-            Arc::new(bake_node(d, &c).unwrap_or(c))
+            Arc::new(c)
         })
         .collect();
     if let Some(ch) = m.children_mut() {

crates/engine/src/cmd/effectcmd.rs

@@ -13,6 +13,7 @@ use vectorcraft_geom::{FillRule, PathData};
 use vectorcraft_render::effects;
 
 use super::appearance::{ItemTarget, appearance_targets, index_param, item_target, item_target_at};
+use super::expand::has_seeded;
 use super::rasterfx;
 use super::*;
 
@@ -442,18 +443,70 @@ fn raster_image(d: &mut Document, n: &Node) -> Option<(Node, bool)> {
     Some((rasterfx::raster_image(d, n, below, true)?, below))
 }
 
-/// Give the members of `m`, art evaluated from the object of the same id (its pieces share that
-/// id), ids of their own and expand the pieces: those made from its own fills and strokes, or with
-/// `all` every member.
-fn expand_pieces(d: &mut Document, m: &mut Node, all: bool, stroke_art: effects::StrokeArt) {
+/// Expand the pieces of `m`, art evaluated from the object of the same id (its pieces share that
+/// id): those made from its own fills and strokes, or with `all` every member. The other members
+/// that repeat an id ([`repeats_an_id`]) and hold a random Transform effect ([`has_seeded`]) are
+/// expanded as well ([`expand_in_place`]). Then the members get ids of their own. Random in a
+/// Transform effect takes its seed from the id, and the canvas draws the pieces with the ids they
+/// have before that. Returns what each of those other members' expansions lists, by the member's
+/// new id.
+fn expand_pieces(
+    d: &mut Document,
+    m: &mut Node,
+    all: bool,
+    brushes: &[vectorcraft_brush::Brush],
+    stroke_art: effects::StrokeArt,
+) -> std::collections::HashMap<NodeId, Vec<Listed>> {
     let id = m.id;
     let mut seen = std::collections::HashSet::from([id]);
+    let mut expanded = std::collections::HashMap::new();
     for c in m.children_mut().into_iter().flatten() {
-        let own = all || c.id == id;
         let c = Arc::make_mut(c);
-        effects::fresh_ids(d, c, &mut seen);
-        if own {
+        let mut listed = None;
+        if all || c.id == id {
             effects::expand_art(d, c, stroke_art);
+        } else if has_seeded(c) && repeats_an_id(c, &seen) {
+            listed = Some(expand_in_place(d, c, brushes));
+        }
+        fresh_ids_listed(d, c, &mut seen, listed.as_deref_mut().unwrap_or_default());
+        if let Some(listed) = listed {
+            expanded.insert(c.id, listed);
+        }
+    }
+    expanded
+}
+
+/// Does `n` hold an id of `seen`, or one id twice: does [`effects::fresh_ids`] give it new ids?
+fn repeats_an_id(n: &Node, seen: &std::collections::HashSet<NodeId>) -> bool {
+    let mut ids = std::collections::HashSet::new();
+    let mut repeats = false;
+    n.walk(&mut |c| repeats |= seen.contains(&c.id) || !ids.insert(c.id));
+    repeats
+}
+
+/// An object whose appearance expanded: its id, and its place in the walk ([`Node::walk`]) of the
+/// art it was listed in. Art evaluated from an object can hold one id more than once, and the place
+/// tells which of those nodes it is.
+type Listed = (NodeId, usize);
+
+/// [`expanded_node`] of `c` in place. Returns what the expansion lists, by places in the walk of `c`.
+fn expand_in_place(d: &mut Document, c: &mut Node, brushes: &[vectorcraft_brush::Brush]) -> Vec<Listed> {
+    let mut listed = vec![];
+    if let Some(e) = expanded_node(d, c.clone(), brushes, &mut listed) {
+        *c = e;
+    }
+    listed
+}
+
+/// [`effects::fresh_ids`] on `c`, with each of `listed` (places in the walk of `c`) given the new
+/// id of the node at its place.
+fn fresh_ids_listed(d: &mut Document, c: &mut Node, seen: &mut std::collections::HashSet<NodeId>, listed: &mut [Listed]) {
+    effects::fresh_ids(d, c, seen);
+    let mut ids = vec![];
+    c.walk(&mut |n| ids.push(n.id));
+    for (id, at) in listed {
+        if let Some(new) = ids.get(*at) {
+            *id = *new;
         }
     }
 }
@@ -462,46 +515,71 @@ fn expand_pieces(d: &mut Document, m: &mut Node, all: bool, stroke_art: effects:
 /// stroke becomes an object of its own (strokes outlined, brushes as their art), geometry effects
 /// are baked, raster effects become an embedded image, and the object keeps its transparency.
 pub(crate) fn expand_node(d: &mut Document, id: NodeId, brushes: &[vectorcraft_brush::Brush], out: &mut Vec<NodeId>) {
-    let Some(mut n) = d.node(id).cloned() else { return };
+    let Some(n) = d.node(id).cloned() else { return };
+    let mut listed = vec![];
+    let m = expanded_node(d, n, brushes, &mut listed);
+    out.extend(listed.into_iter().map(|(id, _)| id));
+    if let Some(m) = m
+        && let Some(slot) = d.node_mut(id)
+    {
+        *slot = m;
+    }
+}
+
+/// [`expand_node`] of `n`, which need not be in `d`: `n` with its appearance expanded, `None` when
+/// it has nothing to expand. `out` gets the objects whose appearance expands, by places in the walk
+/// of the expanded `n`, `n`'s first when its own does.
+fn expanded_node(d: &mut Document, mut n: Node, brushes: &[vectorcraft_brush::Brush], out: &mut Vec<Listed>) -> Option<Node> {
+    let id = n.id;
     if !expandable(&n) {
-        return;
+        return None;
     }
     // Colour adjustments: the colours inside become the adjusted ones (an embedded image a
     // recoloured copy), then the rest of the appearance expands.
+    let mut changed = false;
+    // Is `n` in `out` yet?
+    let mut listed_n = false;
     if effects::has_adjustment(&n)
         && let Some(m) = effects::adjust_in_document(d, &n)
-        && let Some(slot) = d.node_mut(id)
     {
-        *slot = m.clone();
-        out.push(id);
+        out.push((id, 0));
+        listed_n = true;
         n = m;
+        changed = true;
         if !expandable(&n) {
-            return;
+            return Some(n);
         }
     }
     // (A symbol instance has become its art, a group.)
     let container = is_container(&n);
-    // Crop Marks: a group of the object and its marks, then the object expands on its own.
+    // Crop Marks: the object and its marks become a group. The object expands on its own first,
+    // with `id`: the canvas draws it with that id, which Random in a Transform effect takes its
+    // seed from. Then the object and the marks get ids of their own.
     if let Some(mut m) = effects::crop_marks_art(&n) {
         let mut seen = std::collections::HashSet::from([id]);
-        for c in m.children_mut().into_iter().flatten() {
-            effects::fresh_ids(d, Arc::make_mut(c), &mut seen);
+        let mut expanded = vec![];
+        for (i, c) in m.children_mut().into_iter().flatten().enumerate() {
+            let c = Arc::make_mut(c);
+            if i == 0 {
+                expanded = expand_in_place(d, c, brushes);
+                fresh_ids_listed(d, c, &mut seen, &mut expanded);
+            } else {
+                effects::fresh_ids(d, c, &mut seen);
+            }
         }
-        let object = m.children().and_then(|c| c.first()).map(|c| c.id);
-        if let Some(slot) = d.node_mut(id) {
-            *slot = m;
-            out.push(id);
+        if !listed_n {
+            out.push((id, 0));
         }
-        if let Some(object) = object {
-            expand_node(d, object, brushes, out);
-        }
-        return;
+        out.extend(expanded.into_iter().map(|(id, at)| (id, at + 1)));
+        return Some(m);
     }
     let image = raster_image(d, &n);
     let mut v = n.clone();
     rasterfx::strip_raster(&mut v, false);
     let mut stroke_art =
         |d: &mut Document, path: &PathData, rule: FillRule, st: &StrokeLayer| super::pathops::outlined_stroke(d, brushes, path, rule, st);
+    // Members of a group or layer that `expand_pieces` expanded, by id, with what their expansion lists.
+    let mut members = std::collections::HashMap::new();
     let expanded = match image {
         // Blur, feather and inner glow change the object itself: it becomes the image (a layer
         // keeps it as its only member).
@@ -510,7 +588,7 @@ pub(crate) fn expand_node(d: &mut Document, id: NodeId, brushes: &[vectorcraft_b
             let m = if container {
                 // A group's or layer's own fills and strokes become art among its members.
                 effects::evaluate_container(&v).map(|mut m| {
-                    expand_pieces(d, &mut m, false, &mut stroke_art);
+                    members = expand_pieces(d, &mut m, false, brushes, &mut stroke_art);
                     m
                 })
             } else if matches!(n.kind, NodeKind::Path { .. } | NodeKind::Compound { .. }) && !effects::has_gradient_copies(&v) {
@@ -523,7 +601,7 @@ pub(crate) fn expand_node(d: &mut Document, id: NodeId, brushes: &[vectorcraft_b
                     _ => None,
                 };
                 effects::expand_outlined(&v, symbol.as_deref()).map(|mut m| {
-                    expand_pieces(d, &mut m, true, &mut stroke_art);
+                    expand_pieces(d, &mut m, true, brushes, &mut stroke_art);
                     m
                 })
             };
@@ -537,20 +615,34 @@ pub(crate) fn expand_node(d: &mut Document, id: NodeId, brushes: &[vectorcraft_b
             }
         }
     };
-    if let Some(m) = expanded
-        && let Some(slot) = d.node_mut(id)
-    {
-        *slot = m;
-        if out.last() != Some(&id) {
-            out.push(id);
+    if let Some(m) = expanded {
+        n = m;
+        changed = true;
+        if !listed_n {
+            out.push((id, 0));
         }
     }
     if container {
-        let children: Vec<NodeId> = d.node(id).and_then(|n| n.children()).map(|c| c.iter().map(|c| c.id).collect()).unwrap_or_default();
-        for c in children {
-            expand_node(d, c, brushes, out);
+        // The place of each member in the walk of `n`: after `n` and the members before it.
+        let mut at = 1;
+        for c in n.children_mut().into_iter().flatten() {
+            let from = out.len();
+            match members.remove(&c.id) {
+                Some(listed) => out.extend(listed),
+                None => {
+                    if let Some(e) = expanded_node(d, (**c).clone(), brushes, out) {
+                        *c = Arc::new(e);
+                        changed = true;
+                    }
+                }
+            }
+            for (_, place) in &mut out[from..] {
+                *place += at;
+            }
+            at += c.count();
         }
     }
+    changed.then_some(n)
 }
 
 fn expand_appearance(s: &mut Session, p: &Value) -> Result<Value> {

crates/engine/src/cmd/expand.rs

@@ -604,7 +604,7 @@ fn seeded_here(n: &Node) -> bool {
 
 /// Does `n`, something in it or one of their opacity masks have a random Transform effect
 /// ([`seeded_here`])?
-fn has_seeded(n: &Node) -> bool {
+pub(crate) fn has_seeded(n: &Node) -> bool {
     seeded_here(n)
         || n.extras().mask.as_ref().is_some_and(|m| has_seeded(&m.art))
         || (!matches!(n.kind, NodeKind::Compound { .. }) && n.children().is_some_and(|c| c.iter().any(|c| has_seeded(c))))

crates/engine/src/tests_expand.rs

@@ -84,6 +84,115 @@ fn two_fills_and_an_offset_stroke_become_three_paths_that_look_the_same() {
     assert_eq!(node(&s, id).appearance.items.len(), 3);
 }
 
+/// The objects [`random_transforms`] makes.
+pub(super) const RANDOM_CASES: [&str; 5] = ["copies", "group copies", "nested group copies", "crop marks", "group crop marks"];
+
+/// Selected art with Random in Transform effects, from 50 pt squares at y 60 with an 8 pt black
+/// stroke, as `case` says. "copies": a square at x 20 filled with a gradient, a random Transform
+/// effect on its stroke and a Transform effect that copies it 80 pt right. "group copies": the
+/// same square without the second effect, in a group that has it. "nested group copies": the same
+/// square in a group with a random Transform effect, in a group with the second effect. "crop
+/// marks": a red square at x 20 with a random Transform effect and crop marks. "group crop marks":
+/// red squares at x 20 and x 110 in a group with a random Transform effect and crop marks. Returns
+/// the object's id.
+pub(super) fn random_transforms(s: &mut Session, case: &str) -> NodeId {
+    let random = json!({"moveH": 15, "moveV": 15, "rotate": 40, "random": true});
+    let square = |s: &mut Session, x: f64, fill: Value| {
+        let id = run(s, "shape.rectangle", json!({"x": x, "y": 60, "width": 50, "height": 50}))["id"].as_u64().unwrap();
+        run(s, "paint.setFill", fill);
+        run(s, "paint.setStroke", json!({"color": "#000000"}));
+        run(s, "stroke.set", json!({"weight": 8}));
+        id
+    };
+    let red = json!({"color": "#ff0000"});
+    let id = match case {
+        "copies" | "group copies" | "nested group copies" => {
+            let a = square(s, 20.0, json!({"gradient": {"kind": "linear"}}));
+            run(s, "effect.apply", json!({"effect": "distort.transform", "params": random, "item": 1}));
+            if case == "nested group copies" {
+                run(s, "object.group", json!({}));
+                run(s, "effect.apply", json!({"effect": "distort.transform", "params": random, "item": null}));
+            }
+            if case == "copies" { a } else { run(s, "object.group", json!({}))["id"].as_u64().unwrap() }
+        }
+        "crop marks" => square(s, 20.0, red),
+        _ => {
+            let a = square(s, 20.0, red.clone());
+            let b = square(s, 110.0, red);
+            run(s, "select.set", json!({"ids": [a, b]}));
+            run(s, "object.group", json!({}))["id"].as_u64().unwrap()
+        }
+    };
+    if case.ends_with("copies") {
+        run(s, "effect.apply", json!({"effect": "distort.transform", "params": {"moveH": 80, "copies": 1}, "item": null}));
+    } else {
+        run(s, "effect.apply", json!({"effect": "distort.transform", "params": random, "item": null}));
+        run(s, "effect.apply", json!({"effect": "cropMarks", "item": null}));
+    }
+    NodeId(id)
+}
+
+/// Random in a Transform effect takes its seed from the object's id. The expanded art draws as the
+/// canvas drew each of [`random_transforms`]' objects.
+#[test]
+fn expanded_art_keeps_the_random_transforms_the_canvas_drew() {
+    let mut differ = vec![];
+    for case in RANDOM_CASES {
+        let mut s = session();
+        let id = random_transforms(&mut s, case);
+        let before = render(&s);
+        let r = run(&mut s, "effect.expandAppearance", json!({}));
+        differ.push((case, differing(&before, &render(&s), 64)));
+        let mut art = node(&s, id);
+        // Crop marks: the reply names the group of the object and its marks, then the object.
+        if case.ends_with("crop marks") {
+            art = (*art.children().unwrap()[0]).clone();
+            assert_eq!((&r["ids"][0], &r["ids"][1]), (&json!(id.0), &json!(art.id.0)), "{case}");
+        }
+        // No object keeps an effect, and in the copies cases no object keeps a stroke: every copy
+        // is expanded.
+        art.walk(&mut |n| {
+            let stroked = case.ends_with("copies") && n.appearance.stroke().is_some_and(|st| !st.paint.is_none());
+            assert!(n.appearance.effects.is_empty() && !stroked, "{case}: {n:?}");
+        });
+    }
+    assert_eq!(differ, RANDOM_CASES.map(|case| (case, 0)));
+}
+
+/// The reply lists each object whose appearance expanded once, by the id it has in the document.
+/// In "group copies" the two copies of the square share an id until Expand Appearance gives them
+/// ids of their own, and the reply names the group, then each copy by its new id.
+#[test]
+fn the_reply_names_each_expanded_copy_by_its_own_id() {
+    for case in RANDOM_CASES {
+        let mut s = session();
+        let id = random_transforms(&mut s, case);
+        let r = run(&mut s, "effect.expandAppearance", json!({}));
+        let ids: Vec<u64> = r["ids"].as_array().unwrap().iter().map(|v| v.as_u64().unwrap()).collect();
+        let unique: std::collections::HashSet<_> = ids.iter().collect();
+        assert_eq!(unique.len(), ids.len(), "{case}: {ids:?}");
+        assert!(ids.iter().all(|&i| s.doc().unwrap().doc.node(NodeId(i)).is_some()), "{case}: {ids:?}");
+        if case == "group copies" {
+            let art = node(&s, id);
+            let copies: Vec<u64> = art.children().unwrap()[0].children().unwrap().iter().map(|c| c.id.0).collect();
+            assert_eq!(ids, [vec![id.0], copies].concat(), "{case}");
+        }
+    }
+}
+
+/// A square with a color adjustment and crop marks: the reply names the group of the square and its
+/// marks once. The adjusted square has a basic appearance, so the reply does not name it.
+#[test]
+fn the_reply_names_an_adjusted_object_with_crop_marks_once() {
+    let mut s = session();
+    let id = square(&mut s);
+    run(&mut s, "effect.apply", json!({"effect": "adjust.hueSaturation", "params": {"hue": 120}}));
+    run(&mut s, "effect.apply", json!({"effect": "cropMarks", "item": null}));
+    let r = run(&mut s, "effect.expandAppearance", json!({}));
+    assert!(node(&s, id).children().is_some_and(|c| c.len() > 1));
+    assert_eq!(r["ids"], json!([id.0]));
+}
+
 #[test]
 fn a_shadow_becomes_an_image_under_the_art_and_transparency_stays_on_the_group() {
     let mut s = session();

crates/engine/src/tests_flatten.rs

@@ -7,6 +7,7 @@ use vectorcraft_doc::{AppearanceItem, Node, NodeKind};
 use vectorcraft_geom::{BezPath, Point, Rect, Shape};
 use vectorcraft_render::{Rendered, Renderer};
 
+use super::tests_expand::{RANDOM_CASES, random_transforms};
 use super::tests_menucmds::{place_undecodable_png, warns_undecoded};
 use super::*;
 
@@ -410,6 +411,27 @@ fn outlined_type_keeps_its_gradient_on_the_outlines_its_transform_effect_moves()
     assert!(diffs.iter().all(|&d| d < 0.0002), "shares of the pixels that changed: {diffs:?}");
 }
 
+/// Random in a Transform effect takes its seed from the object's id. Each of [`random_transforms`]'
+/// objects at 50% opacity draws as the canvas drew it with its geometry effects baked, as Flatten
+/// Transparency bakes them first, and after Flatten Transparency, within the seams between its
+/// regions (the same objects without Random change 1.4% to 2.3% of the pixels).
+#[test]
+fn flattened_art_keeps_the_random_transforms_the_canvas_drew() {
+    let _settings = settings_lock();
+    let mut diffs = vec![];
+    for case in RANDOM_CASES {
+        let mut s = session();
+        let id = random_transforms(&mut s, case);
+        run(&mut s, "transparency.set", json!({"ids": [id.0], "opacity": 50}));
+        let before = picture(&s);
+        let baked = vectorcraft_render::effects::bake_document(&s.doc().unwrap().doc).unwrap();
+        let baked = Renderer::new().render_region(&baked, Rect::new(0.0, 0.0, 200.0, 200.0), 1.0, true);
+        flatten(&mut s, json!({"ids": [id.0]}));
+        diffs.push((case, changed(&before, &baked), changed(&before, &picture(&s))));
+    }
+    assert!(diffs.iter().all(|&(_, baked, flat)| baked == 0.0 && flat < 0.03), "shares of the pixels that changed: {diffs:?}");
+}
+
 #[test]
 fn preserve_alpha_composites_over_nothing() {
     let _settings = settings_lock();