Canvas redraw: a trace object's source in each version; review follow-ups

Review of #6290.

- An Image Trace object's hidden source image counts as shown in each
  version by that version's own first member: an edit that puts it
  first again (undoing Send to Back on a traced shape), or a new source
  image in its place, redraws where it shows. Tested with an image
  source on screen with the objects' views.
- A shape of an Image Trace object in Outline view is left out by the
  rule every object is (node_checks::left_out), so the drawing and the
  measure of an edit can't disagree.
- The renderer keeps the brush library's key from its first measure of
  3D art until the library changes; the test says so, and that a change
  lets it go.
- The test of a 3D effect that builds nothing covers it on a fill and
  on a stroke, without effects of the object's own (exactly as without
  it) and under a raster effect.

On the commit before, "Canvas: a 3D effect on a fill that builds nothing
draws without it": it needs no effect of the object's own. A 3D effect
on a fill or stroke that builds nothing overflowed the stack on its own
too. With a raster effect, the object now draws from its art, where
without the 3D effect it draws as a path, whose raster effects are
clipped to the path's own geometry, cutting off the item's Roughen
spikes: the new drawing is the one that keeps them.

crates/render/src/changed.rs

@@ -86,12 +86,13 @@ pub fn changed(old: &Document, new: &Document) -> Changed {
 }
 
 impl crate::Renderer {
-    /// [`changed`], measured through this renderer (`Renderer::bounds_of`): what it drew
-    /// of `old` it measured to draw it, and what it measures of `new`, a 3D effect's art included,
-    /// it holds to draw that with `opts`; the objects `opts` leaves out (hidden, templates and
-    /// guides the view hides, the instance a symbol is edited from) draw nothing. The view that drew `old` with this renderer and `opts` asks this. With
-    /// Overprint Preview or Separations Preview the renderer draws copies of the objects
-    /// (`proof::prepare`), so they are measured afresh and nothing is held.
+    /// [`changed`], measured through this renderer (`Renderer::bounds_of`): what it drew of `old`
+    /// it measured to draw it, and what it measures of `new`, a 3D effect's art included, it holds
+    /// to draw that with `opts`; the objects `opts` leaves out (hidden, templates and guides the
+    /// view hides, the instance a symbol is edited from) draw nothing. The view that drew `old`
+    /// with this renderer and `opts` asks this. With Overprint Preview or Separations Preview the
+    /// renderer draws copies of the objects (`proof::prepare`), so they are measured afresh and
+    /// nothing is held.
     pub fn changed(&mut self, old: &Document, new: &Document, opts: &crate::RenderOptions) -> Changed {
         let renderer = (!crate::proof::overprints(opts)).then_some(self);
         changed_with(old, new, renderer, opts)
@@ -118,7 +119,7 @@ fn changed_with(old: &Document, new: &Document, renderer: Option<&mut crate::Ren
     if new.pattern_edit.is_some() || !old.same_outside_layers(new) {
         return Changed::Everything;
     }
-    let mut diff = Diff { old: old.symbol_boxes(), new: new.symbol_boxes(), area: None, renderer, opts, source: None };
+    let mut diff = Diff { old: old.symbol_boxes(), new: new.symbol_boxes(), area: None, renderer, opts, sources: (None, None) };
     diff.children(&old.layers, &new.layers);
     diff.area.map_or(Changed::Nothing, Changed::Area)
 }
@@ -137,14 +138,15 @@ pub fn view_pixels(area: Rect, view: Affine, w: u32, h: u32) -> Option<[u32; 4]>
 
 /// [`changed`] in progress: the symbols' boxes of both versions, the area so far, the renderer
 /// that measures, if any (else each object is measured afresh), the options of the frames it
-/// measures for, and the hidden source image of the Image Trace object whose members it compares.
+/// measures for, and the hidden source image, in each version, of the Image Trace object whose
+/// members it compares.
 struct Diff<'a, 'r> {
     old: SymbolBoxes<'a>,
     new: SymbolBoxes<'a>,
     area: Option<Rect>,
     renderer: Option<&'r mut crate::Renderer>,
     opts: &'r crate::RenderOptions,
-    source: Option<NodeId>,
+    sources: (Option<NodeId>, Option<NodeId>),
 }
 
 impl Diff<'_, '_> {
@@ -180,10 +182,11 @@ impl Diff<'_, '_> {
             && same_but_members(old, new)
         {
             // An Image Trace object draws its hidden source image, its first member, in some views.
-            let source = old.extras().trace.is_some().then(|| a.first().map(|s| s.id)).flatten();
-            let outer = std::mem::replace(&mut self.source, source);
+            // Each version's own: an edit can put another member first (undoing Send to Back).
+            let first = |m: &[Arc<Node>]| old.extras().trace.is_some().then(|| m.first().map(|s| s.id)).flatten();
+            let outer = std::mem::replace(&mut self.sources, (first(a), first(b)));
             self.children(a, b);
-            self.source = outer;
+            self.sources = outer;
         } else if let Some((area, moved, anchors)) = moved_anchors(old, new) {
             if moved <= FEW_MOVED && anchors >= MANY_ANCHORS {
                 self.add(area);
@@ -203,11 +206,11 @@ impl Diff<'_, '_> {
     /// (`node_checks::left_out`)? The source image of an Image Trace object draws in some views.
     /// The members of a group or layer left out are never compared ([`Self::node`]).
     fn unseen(&self, side: Side, n: &Node) -> bool {
-        let doc = match side {
-            Side::Old => self.old.document(),
-            Side::New => self.new.document(),
+        let (doc, source) = match side {
+            Side::Old => (self.old.document(), self.sources.0),
+            Side::New => (self.new.document(), self.sources.1),
         };
-        self.source != Some(n.id) && crate::node_checks::left_out(self.opts, doc, n)
+        source != Some(n.id) && crate::node_checks::left_out(self.opts, doc, n)
     }
 
     /// The cull bounds of `n` of the `side` version, through the renderer when there is one;

crates/render/src/fx/tests.rs

@@ -842,10 +842,10 @@ fn a_measuring_pass_sweeps_the_bounds_of_held_art_as_they_double() {
     assert!(r.three_d_sweeps <= 2, "{} sweeps", r.three_d_sweeps);
 }
 
-/// A renderer checks the brush library between frames only while it keeps bounds measured with
-/// brushed strokes in 3D art.
+/// A renderer checks the brush library between frames only once it has measured 3D art, and again
+/// only after measuring 3D art since the library last changed.
 #[test]
-fn the_brush_library_is_checked_only_with_3d_bounds_kept() {
+fn the_brush_library_is_checked_only_after_3d_art_is_measured() {
     let mut d = doc_with(Rect::new(30.0, 30.0, 60.0, 60.0), Color::BLACK, vec![]);
     let mut r = Renderer::new();
     r.render(&d, 100, 100, Affine::IDENTITY, &RenderOptions::default());
@@ -854,6 +854,12 @@ fn the_brush_library_is_checked_only_with_3d_bounds_kept() {
     d.node_mut(id).unwrap().appearance.effects = vec![Effect { id: "threeD.extrude".into(), params: json!({"depth": 20.0}), visible: true }];
     r.render(&d, 100, 100, Affine::IDENTITY, &RenderOptions::default());
     assert!(r.brush_library.is_some());
+    // A library changed: the bounds measured with the old one go, and so does its key.
+    let mut lib = vectorcraft_brush::library(&d);
+    lib.retain(|b| b.name != "3 pt. Round");
+    vectorcraft_brush::store(&mut d, &lib);
+    r.check_symbols(&d);
+    assert_eq!(r.brush_library, None);
 }
 
 /// An object whose 3D effect builds nothing (Revolve on a group) is measured by the renderer as the
@@ -1701,41 +1707,46 @@ fn an_image_inner_glow_follows_its_visible_pixels() {
     }
 }
 
-/// A 3D effect on a path's fill that builds nothing (Revolve after Roughen) draws the fill as
-/// without it, under the object's own effects too: the art made for the object is drawn without
-/// it, not as an object with effects again, and that art again, without end. With a raster effect
-/// the object draws from that art, where without the 3D effect it draws as a path: their edges
-/// differ a little. An Extrude may build.
+/// A 3D effect on a path's fill or stroke that builds nothing (Revolve after Roughen) draws the
+/// item as without it: the art made for the object is drawn without it, not as an object with
+/// effects again, and that art again, without end. Without effects of its own the object draws
+/// exactly as without the 3D effect. With a raster effect (Drop Shadow, Gaussian Blur) it draws from
+/// that art, where without the 3D effect it draws as a path, whose raster effects are clipped to
+/// the path's own geometry: there the item's Roughen spikes are cut off, so the two differ past
+/// the path, and match inside it.
 #[test]
-fn a_fills_3d_effect_that_builds_nothing_draws_the_fill_as_without_it() {
-    let rough = |detail: i64| Effect {
-        id: "distort.roughen".into(),
-        params: json!({"size": 10, "relative": false, "detail": detail, "points": "smooth"}),
-        visible: true,
-    };
-    let shadow = Effect { id: "stylize.dropShadow".into(), params: json!({}), visible: true };
+fn a_3d_effect_that_builds_nothing_draws_the_item_as_without_it() {
+    let rough =
+        Effect { id: "distort.roughen".into(), params: json!({"size": 10, "relative": false, "detail": 10, "points": "smooth"}), visible: true };
     let revolve = Effect { id: "threeD.revolve".into(), params: json!({}), visible: true };
-    let extrude = Effect { id: "threeD.extrude".into(), params: json!({"depth": 20.0}), visible: true };
-    for (object, fill, three) in [
-        (vec![shadow.clone()], rough(10), revolve.clone()),
-        (vec![], rough(10), revolve),
-        (vec![shadow], rough(60), extrude.clone()),
-        (vec![rough(60)], rough(60), extrude),
-    ] {
-        let draw = |three: Option<&Effect>| {
-            let mut d = doc_with(Rect::new(30.0, 30.0, 60.0, 60.0), Color::BLACK, vec![]);
-            let id = d.layers[0].children().unwrap()[0].id;
+    let shadow = Effect { id: "stylize.dropShadow".into(), params: json!({}), visible: true };
+    let blur = Effect { id: "blur.gaussian".into(), params: json!({"radius": 2}), visible: true };
+    for (object, on_fill) in [(vec![], true), (vec![], false), (vec![shadow], true), (vec![blur], false)] {
+        let draw = |three: bool| {
+            let mut d = Document::new(100.0, 100.0);
+            let ap = Appearance::basic(Paint::solid(Color::BLACK), Paint::solid(Color::rgb(0.0, 0.0, 1.0)), 2.0);
+            let n = Node::path(d.alloc_id(), shapes::rectangle(Rect::new(30.0, 30.0, 60.0, 60.0)), ap);
+            let l = d.layers[0].id;
+            let id = d.insert(Some(l), 0, n).unwrap();
             let n = d.node_mut(id).unwrap();
             n.appearance.effects = object.clone();
-            n.appearance.fill_mut().unwrap().effects = [Some(&fill), three].into_iter().flatten().cloned().collect();
+            let item = if on_fill {
+                n.appearance.fill_mut().unwrap().effects = vec![];
+                &mut n.appearance.items[0]
+            } else {
+                &mut n.appearance.items[1]
+            };
+            *item.effects_mut() = [Some(rough.clone()), three.then(|| revolve.clone())].into_iter().flatten().collect();
+            assert!(crate::effects::revolve_art(d.node(id).unwrap()).is_none(), "builds nothing");
             Renderer::new().render(&d, 100, 100, Affine::IDENTITY, &RenderOptions::default()).pixels
         };
-        let with = draw(Some(&three));
-        if three.id == "threeD.revolve" {
-            let without = draw(None);
-            let differ = with.iter().zip(&without).filter(|(a, b)| a != b).count();
-            let most = with.iter().zip(&without).map(|(a, b)| a.abs_diff(*b)).max().unwrap_or(0);
-            assert!(differ * 100 < with.len() && most <= 32, "{object:?}: {differ} channels differ, by up to {most}");
+        let (with, without) = (draw(true), draw(false));
+        if object.is_empty() {
+            assert!(with == without, "on the fill {on_fill}: as without it");
+        } else {
+            // Inside the path, well within its Roughen.
+            let at = |p: &[u8], x: usize, y: usize| p[(y * 100 + x) * 4..][..4].to_vec();
+            assert_eq!(at(&with, 45, 45), at(&without, 45, 45), "{object:?}");
         }
     }
 }

crates/render/src/lib.rs

@@ -84,9 +84,10 @@ pub struct Renderer {
     /// The plug-in revision ([`effects::plugin_revision`]) that `geom`, `fx_geom` and `fx_arts` were
     /// last checked against (see [`Self::check_plugins`]).
     plugin_revision: u64,
-    /// The key of the brush library ([`brush_fx::BrushCache::library_key`]) that `geom`'s bounds
-    /// of 3D art measured brushed strokes by (see [`Self::check_symbols`]), while `geom` may hold
-    /// such bounds (`Some`).
+    /// The key of the brush library ([`brush_fx::BrushCache::library_key`]) when `geom`'s bounds of
+    /// 3D art were measured with it (see [`Self::check_symbols`]): set by the first such measure
+    /// and kept, whether or not those bounds still are, until the library changes. `None` until
+    /// then, and nothing is checked.
     brush_library: Option<u64>,
     /// Is a live-effect plug-in that paints its pieces installed (found when `plugin_revision`
     /// changes)?

crates/render/src/node_checks.rs

@@ -115,8 +115,9 @@ pub(crate) fn left_out(opts: &crate::RenderOptions, doc: &vectorcraft_doc::Docum
     !a.visible || view_item || opts.hidden.contains(&a.id) || edited_instance(opts, doc, a.id)
 }
 
-/// Is `id` the symbol instance that `doc`'s symbol is edited from in isolation (drawn without it)?
-pub(crate) fn edited_instance(opts: &crate::RenderOptions, doc: &vectorcraft_doc::Document, id: vectorcraft_doc::NodeId) -> bool {
+/// Symbol editing mode on screen: is `id` the instance editing was entered from, which is not drawn
+/// (with its masks and effects), since the editing layer holds a copy of its art in its place?
+fn edited_instance(opts: &crate::RenderOptions, doc: &vectorcraft_doc::Document, id: vectorcraft_doc::NodeId) -> bool {
     opts.isolated.is_some() && doc.symbol_edit.as_ref().is_some_and(|e| e.instance == Some(id))
 }
 

crates/render/src/renderer/nodes.rs

@@ -216,13 +216,6 @@ impl Renderer {
         self.path_of(f.symbols, &owner)
     }
 
-    /// Symbol editing mode on screen: `id` is the instance editing was entered from, which is not
-    /// drawn (with its masks and effects), since the editing layer holds a copy of its art in its
-    /// place.
-    fn edited_instance(f: &Frame, id: NodeId) -> bool {
-        crate::node_checks::edited_instance(f.opts, f.doc, id)
-    }
-
     /// Whether `a` is left out of this frame: hidden, a skipped template or guide, the instance a
     /// symbol is edited from, or culled.
     fn skipped(&mut self, f: &Frame, a: &Arc<Node>) -> bool {
@@ -661,11 +654,8 @@ impl Renderer {
     /// `fx_owner`, so its path, its live-effect geometry and, through [`Self::draw_kept_node`], a
     /// live object's evaluation come from the caches.
     fn draw_outline_member(&mut self, ctx: &mut RenderContext, f: &Frame, c: &Arc<Node>) {
-        if !c.visible
-            || f.opts.hidden.contains(&c.id)
-            || f.opts.hide_guides && matches!(c.kind, NodeKind::Path { guide: true, .. })
-            || Self::edited_instance(f, c.id)
-        {
+        // Left out as any object is ([`Self::skipped`]).
+        if crate::node_checks::left_out(f.opts, f.doc, c) {
             return;
         }
         match self.bounds_of(f.symbols, c) {

crates/render/src/tests/changed.rs

@@ -63,24 +63,24 @@ fn redraw(old: &Document, new: &Document) -> Changed {
     let change = changed(old, new);
     let mut fresh = Renderer::new();
     let before = fresh.render(old, W, H, view(), &opts);
-    patched(&mut fresh, &before, new, change, &after);
+    patched(&mut fresh, &before, new, change, &after, &opts);
     let mut r = Renderer::new();
     let before = r.render(old, W, H, view(), &opts);
     let measured = r.changed(old, new, &opts);
-    patched(&mut r, &before, new, measured, &after);
+    patched(&mut r, &before, new, measured, &after, &opts);
     change
 }
 
-/// `before` with `new` drawn again by `r` only where `change` says, checked against `after`.
-fn patched(r: &mut Renderer, before: &Rendered, new: &Document, change: Changed, after: &Rendered) {
-    let opts = RenderOptions::default();
+/// `before` with `new` drawn again by `r` with `opts` only where `change` says, checked against
+/// `after`.
+fn patched(r: &mut Renderer, before: &Rendered, new: &Document, change: Changed, after: &Rendered, opts: &RenderOptions) {
     let mut patched = Rendered { width: W, height: H, pixels: before.pixels.clone() };
     match change {
         Changed::Everything => patched.pixels = after.pixels.clone(),
         Changed::Nothing => {}
         Changed::Area(area) => {
             if let Some([x0, y0, x1, y1]) = view_pixels(area, view(), W, H) {
-                let part = r.render(new, x1 - x0, y1 - y0, Affine::translate((-f64::from(x0), -f64::from(y0))) * view(), &opts);
+                let part = r.render(new, x1 - x0, y1 - y0, Affine::translate((-f64::from(x0), -f64::from(y0))) * view(), opts);
                 for y in 0..y1 - y0 {
                     let (from, to) = ((y * (x1 - x0) * 4) as usize, (((y0 + y) * W + x0) * 4) as usize);
                     let n = ((x1 - x0) * 4) as usize;
@@ -540,36 +540,84 @@ fn a_renderer_measures_an_edit_with_the_3d_art_it_draws() {
     assert_eq!(apart, 1, "changed keeps nothing for the renderer");
 }
 
-/// An Image Trace object shown with its source image (`view`): its hidden source, at the left, and
-/// two traced shapes, at the right. The ids of the source and of the second shape.
-fn traced(view: &str) -> (Document, NodeId, NodeId) {
+/// An Image Trace object shown with its source image (`view`): its hidden source, a red image at
+/// the left, and two traced shapes, at the right; the object's id, and the ids of the source and
+/// of the second shape.
+fn traced(view: &str) -> (Document, NodeId, NodeId, NodeId) {
     let mut d = Document::new(W as f64, H as f64);
-    let mut source = rect(&mut d, Rect::new(10.0, 10.0, 40.0, 40.0), Color::rgb(0.5, 0.5, 0.5));
-    source.visible = false;
+    let mut png = vec![];
+    let red = image::RgbaImage::from_pixel(1, 1, image::Rgba([255, 0, 0, 255]));
+    red.write_to(&mut std::io::Cursor::new(&mut png), image::ImageFormat::Png).unwrap();
+    d.images.insert("red".into(), vectorcraft_doc::ImageBlob::new("image/png", png));
+    let source = red_image(&mut d, 10.0);
     let a = rect(&mut d, Rect::new(60.0, 10.0, 70.0, 20.0), Color::rgb(0.0, 0.5, 0.0));
     let b = rect(&mut d, Rect::new(80.0, 10.0, 90.0, 20.0), Color::rgb(1.0, 0.5, 0.0));
     let ids = (source.id, b.id);
     let mut g = Node::group(d.alloc_id(), vec![Arc::new(source), Arc::new(a), Arc::new(b)]);
     g.extras_mut().trace = Some(Box::new(serde_json::json!({ "view": view })));
     let l = d.layers[0].id;
-    d.insert(Some(l), 0, g).unwrap();
-    (d, ids.0, ids.1)
+    let g = d.insert(Some(l), 0, g).unwrap();
+    (d, g, ids.0, ids.1)
+}
+
+/// A hidden 30 pt square of the red image at (`x`, 10), as an Image Trace object's source.
+fn red_image(d: &mut Document, x: f64) -> Node {
+    let xf = Affine::translate((x, 10.0)) * Affine::scale(30.0);
+    let im = vectorcraft_doc::ImageObject { key: "red".into(), width: 1, height: 1, xf, link: None, placement: Default::default(), pixels: None };
+    let mut source = Node::new(d.alloc_id(), NodeKind::Image(Box::new(im)));
+    source.visible = false;
+    source
+}
+
+/// Draws `new` again only where the renderer that drew `old` on screen, its Image Trace objects in
+/// their views, measures that it changed, and checks that this is `new` drawn whole.
+fn redraw_traced(old: &Document, new: &Document) -> Changed {
+    let opts = RenderOptions { trace_views: true, ..Default::default() };
+    let after = Renderer::new().render(new, W, H, view(), &opts);
+    let mut r = Renderer::new();
+    let before = r.render(old, W, H, view(), &opts);
+    let red = |r: &Rendered| r.pixels.chunks(4).any(|p| p[0] > 200 && p[1] < 60);
+    assert!(red(&before) || red(&after), "a source shows");
+    let measured = r.changed(old, new, &opts);
+    patched(&mut r, &before, new, measured, &after, &opts);
+    measured
 }
 
 #[test]
 fn moving_a_trace_objects_source_changes_where_it_shows() {
     for view in ["sourceImage", "outlinesWithSourceImage"] {
-        let (old, source, _) = traced(view);
+        let (old, _, source, _) = traced(view);
         let mut new = old.clone();
         new.node_mut(source).unwrap().transform(Affine::translate((5.0, 30.0)), false);
-        let a = area(redraw(&old, &new));
+        let a = area(redraw_traced(&old, &new));
         assert!(a.contains(Point::new(12.0, 12.0)) && a.contains(Point::new(44.0, 69.0)), "{view}: before and after: {a:?}");
     }
 }
 
+/// A trace object's source that is first again after an edit (undoing Send to Back on a traced
+/// shape), or a new source in its place, draws where it shows.
+#[test]
+fn a_trace_objects_new_first_member_is_its_source() {
+    let (back, g, source, shape) = traced("sourceImage");
+    // The shape sent to the back, before the source.
+    let mut sent = back.clone();
+    let children = sent.node_mut(g).unwrap().children_mut().unwrap();
+    let at = children.iter().position(|c| c.id == shape).unwrap();
+    let moved = children.remove(at);
+    children.insert(0, moved);
+    redraw_traced(&sent, &back);
+    // A new source image, with an id of its own.
+    let mut replaced = back.clone();
+    let new_source = red_image(&mut replaced, 55.0);
+    let children = replaced.node_mut(g).unwrap().children_mut().unwrap();
+    assert_eq!(children[0].id, source);
+    children[0] = Arc::new(new_source);
+    redraw_traced(&back, &replaced);
+}
+
 #[test]
 fn recolouring_a_traced_shape_changes_only_the_shape() {
-    let (old, _, shape) = traced("outlinesWithSourceImage");
+    let (old, _, _, shape) = traced("outlinesWithSourceImage");
     let mut new = old.clone();
     new.node_mut(shape).unwrap().appearance.fill_mut().unwrap().paint = Paint::solid(Color::rgb(0.0, 0.0, 1.0));
     let a = area(redraw(&old, &new));