From 3617c678b74bc0800e18a599d4bf527cf5fa93ef Mon Sep 17 00:00:00 2001 From: Drew Dibble Date: Mon, 9 Nov 2026 00:29:06 -0700 Subject: [PATCH] Canvas redraw: what the renderer leaves out measured as nothing, alike Review of #6290. - An edit is measured as the renderer draws: an object it leaves out (hidden, a template layer or guide the view hides, drawn hidden, the instance a symbol is edited from) is measured as nothing, by one predicate the renderer and the measure share (node_checks::left_out, before culling). - An Image Trace object's members are compared one by one again, its hidden source image, which some views draw, counted as shown: moving the source redraws where it was and is, and recoloring a traced shape redraws only the shape. - The renderer checks the brush library between frames only while it keeps bounds measured with brushed strokes in 3D art. - Tests: the sweep of 3D art's bounds counted while the art is held, and Fit to Artwork Bounds' menu answer kept for a revision. --- crates/render/src/changed.rs | 41 ++++++++----- crates/render/src/fx/renderer.rs | 4 ++ crates/render/src/fx/tests.rs | 37 +++++++++++ crates/render/src/lib.rs | 8 ++- crates/render/src/node_checks.rs | 26 ++++++++ crates/render/src/renderer.rs | 4 +- crates/render/src/renderer/nodes.rs | 34 ++++------- crates/render/src/tests/changed.rs | 64 ++++++++++++++++++++ crates/ui-egui/src/panels/artboards/tests.rs | 23 +++++++ 9 files changed, 200 insertions(+), 41 deletions(-) diff --git a/crates/render/src/changed.rs b/crates/render/src/changed.rs index 9d57b7431..5db3e3ab0 100644 --- a/crates/render/src/changed.rs +++ b/crates/render/src/changed.rs @@ -82,19 +82,19 @@ fn add_placed(symbols: &SymbolBoxes, n: &Node, files: &HashSet, area: &m /// art also draws in the copies round it). Objects that an edit doesn't touch /// keep their identity, so this costs what changed, not the size of the document. pub fn changed(old: &Document, new: &Document) -> Changed { - changed_with(old, new, None, &[]) + changed_with(old, new, None, &crate::RenderOptions::default()) } 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` draws hidden count as hidden objects - /// do, drawing nothing. The view that drew `old` with this renderer and `opts` asks this. With + /// 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.hidden) + changed_with(old, new, renderer, opts) } /// Let go of the art made to measure objects ([`crate::Renderer::changed`]) and held for this @@ -114,11 +114,11 @@ impl crate::Renderer { } } -fn changed_with(old: &Document, new: &Document, renderer: Option<&mut crate::Renderer>, hidden: &[NodeId]) -> Changed { +fn changed_with(old: &Document, new: &Document, renderer: Option<&mut crate::Renderer>, opts: &crate::RenderOptions) -> Changed { 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, hidden }; + let mut diff = Diff { old: old.symbol_boxes(), new: new.symbol_boxes(), area: None, renderer, opts, source: None }; diff.children(&old.layers, &new.layers); diff.area.map_or(Changed::Nothing, Changed::Area) } @@ -136,13 +136,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), and the objects it draws hidden. +/// 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. struct Diff<'a, 'r> { old: SymbolBoxes<'a>, new: SymbolBoxes<'a>, area: Option, renderer: Option<&'r mut crate::Renderer>, - hidden: &'r [NodeId], + opts: &'r crate::RenderOptions, + source: Option, } impl Diff<'_, '_> { @@ -171,15 +173,17 @@ impl Diff<'_, '_> { /// only had anchors moved changed around them ([`moved_anchors`]). Any other object that /// changed counts whole, before and after. fn node(&mut self, old: &Arc, new: &Arc) { - if Arc::ptr_eq(old, new) || self.unseen(old) && self.unseen(new) { + if Arc::ptr_eq(old, new) || self.unseen(Side::Old, old) && self.unseen(Side::New, new) { return; } - // An Image Trace object draws its hidden source image in some views: it changed whole. if let (Some(a), Some(b)) = (members(old), members(new)) && same_but_members(old, new) - && old.extras().trace.is_none() { + // 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); self.children(a, b); + self.source = outer; } else if let Some((area, moved, anchors)) = moved_anchors(old, new) { if moved <= FEW_MOVED && anchors >= MANY_ANCHORS { self.add(area); @@ -195,16 +199,21 @@ impl Diff<'_, '_> { } } - /// Does `n` draw nothing: hidden, or drawn hidden? The members of a hidden group or layer are - /// never compared ([`Self::node`]). - fn unseen(&self, n: &Node) -> bool { - !n.visible || self.hidden.contains(&n.id) + /// Does `n` of the `side` version draw nothing, as the renderer leaves it out + /// (`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(), + }; + self.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; /// `None` when it draws nothing ([`Self::unseen`]), as nothing is measured to draw it. fn bounds(&mut self, side: Side, n: &Arc) -> Option { - if self.unseen(n) { + if self.unseen(side, n) { return None; } let Diff { old, new, renderer, .. } = self; diff --git a/crates/render/src/fx/renderer.rs b/crates/render/src/fx/renderer.rs index 8477662b7..99ea4e97f 100644 --- a/crates/render/src/fx/renderer.rs +++ b/crates/render/src/fx/renderer.rs @@ -621,6 +621,10 @@ impl Renderer { if self.three_d_bounds.len() >= (2 * self.three_d_kept).max(1024) { self.three_d_bounds.retain(|_, e| e.0.strong_count() > 0); self.three_d_kept = self.three_d_bounds.len(); + #[cfg(test)] + { + self.three_d_sweeps += 1; + } } self.three_d_bounds.insert(Arc::as_ptr(&art) as usize, (Arc::downgrade(&art), None)); } diff --git a/crates/render/src/fx/tests.rs b/crates/render/src/fx/tests.rs index 7e4802f3a..b18462820 100644 --- a/crates/render/src/fx/tests.rs +++ b/crates/render/src/fx/tests.rs @@ -819,6 +819,43 @@ fn a_measuring_pass_keeps_the_bounds_of_held_art_only() { assert!(r.three_d_bounds.len() <= 1024, "{} kept", r.three_d_bounds.len()); } +/// While its 3D art is held, a pass that only measures sweeps the bounds kept for 3D art each time +/// they double, not on every new art past a size. +#[test] +fn a_measuring_pass_sweeps_the_bounds_of_held_art_as_they_double() { + let mut d = Document::new(100.0, 100.0); + let l = d.layers[0].id; + for i in 0..3000 { + let x = f64::from(i) * 30.0; + let mut n = Node::path( + d.alloc_id(), + shapes::rectangle(Rect::new(x, 30.0, x + 20.0, 50.0)), + Appearance::basic(Paint::solid(Color::BLACK), Paint::None, 0.0), + ); + n.appearance.effects = vec![Effect { id: "threeD.extrude".into(), params: json!({"depth": 2.0}), visible: true }]; + d.insert(Some(l), 0, n).unwrap(); + } + let mut r = Renderer::new(); + // Held here, as the art kept to draw is. + let held: Vec<_> = d.layers[0].children().unwrap().iter().map(|n| r.measured_fx_art(&d, n)).collect(); + assert_eq!((held.len(), r.three_d_bounds.len()), (3000, 3000)); + 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. +#[test] +fn the_brush_library_is_checked_only_with_3d_bounds_kept() { + 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()); + assert_eq!(r.brush_library, None); + let id = d.layers[0].children().unwrap()[0].id; + 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()); +} + /// An object whose 3D effect builds nothing (Revolve on a group) is measured by the renderer as the /// free measure does it: as the renderer draws it, by its members. #[test] diff --git a/crates/render/src/lib.rs b/crates/render/src/lib.rs index 6c660710a..89a673855 100644 --- a/crates/render/src/lib.rs +++ b/crates/render/src/lib.rs @@ -85,8 +85,9 @@ pub struct Renderer { /// 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`]). - brush_library: u64, + /// of 3D art measured brushed strokes by (see [`Self::check_symbols`]), while `geom` may hold + /// such bounds (`Some`). + brush_library: Option, /// Is a live-effect plug-in that paints its pieces installed (found when `plugin_revision` /// changes)? painting_plugins: bool, @@ -178,6 +179,9 @@ pub struct Renderer { /// How many entries `three_d_bounds` kept when it last let go of the art no longer held: it /// does so again when it holds twice as many. three_d_kept: usize, + /// How many times `three_d_bounds` was swept for art no longer held between frames. + #[cfg(test)] + three_d_sweeps: usize, /// Symbol art as instances and inline graphics draw it (see [`Self::placed_art`]). placed_arts: FrameCache, /// The bitmap clock of [`placed_document`] when this frame first drew a placed document from diff --git a/crates/render/src/node_checks.rs b/crates/render/src/node_checks.rs index 07a1b9dc2..8a150fd3c 100644 --- a/crates/render/src/node_checks.rs +++ b/crates/render/src/node_checks.rs @@ -107,6 +107,32 @@ pub(crate) fn holds_instance(n: &Node) -> bool { holds } +/// Is `a` left out of a frame `doc` draws with `opts`, before any culling: hidden, a template or +/// guide the view hides, drawn hidden, or the instance its symbol is being edited from? +pub(crate) fn left_out(opts: &crate::RenderOptions, doc: &vectorcraft_doc::Document, a: &Node) -> bool { + let view_item = (opts.skip_templates || opts.hide_templates) && matches!(a.kind, NodeKind::Layer { template: true, .. }) + || (opts.skip_templates || opts.hide_guides) && matches!(a.kind, NodeKind::Path { guide: true, .. }); + !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 { + opts.isolated.is_some() && doc.symbol_edit.as_ref().is_some_and(|e| e.instance == Some(id)) +} + +/// Does [`crate::Renderer::bounds_of`] measure `n` by the 3D art the renderer draws and keeps for +/// it: a Revolve, Extrude & Bevel, Inflate or Rotate without Crop Marks or Vector Halftone, which +/// draw other art? +pub(crate) fn measures_three_d(n: &Node) -> bool { + (effects::has_revolve(n) || effects::has_solid(n)) && !effects::has_crop_marks(n) && !effects::addons::halftone::has_halftone(n) +} + +/// Does `n` hold an object that [`crate::Renderer::bounds_of`] measures by its 3D effect's art +/// ([`measures_three_d`])? +pub(crate) fn holds_measured_three_d(n: &Node) -> bool { + measures_three_d(n) || n.children().is_some_and(|c| c.iter().any(|c| holds_measured_three_d(c))) +} + /// Does `n` hold an object with a visible plug-in effect ([`effects::has_plugin_effect`]), in it or /// in the opacity masks of the objects in it? pub(crate) fn holds_plugin_effect(n: &Node) -> bool { diff --git a/crates/render/src/renderer.rs b/crates/render/src/renderer.rs index bfb49add4..2a188fc0a 100644 --- a/crates/render/src/renderer.rs +++ b/crates/render/src/renderer.rs @@ -45,7 +45,7 @@ impl Renderer { kept: PtrMap::default(), symbols: vec![], plugin_revision: 0, - brush_library: 0, + brush_library: None, painting_plugins: false, paint_names: 0, strokes: PtrMap::default(), @@ -77,6 +77,8 @@ impl Renderer { pass_bounds: PtrMap::default(), three_d_bounds: PtrMap::default(), three_d_kept: 0, + #[cfg(test)] + three_d_sweeps: 0, placed_arts: FrameCache::new(lru::PLACED_ART_BUDGET, 256), placed_frame: None, nested: 0, diff --git a/crates/render/src/renderer/nodes.rs b/crates/render/src/renderer/nodes.rs index 12f7675e9..4f67b16ec 100644 --- a/crates/render/src/renderer/nodes.rs +++ b/crates/render/src/renderer/nodes.rs @@ -5,20 +5,7 @@ use super::{ shape_masked_image, single_plain_fill, text_geom_snapped, white_shape_mask, }; use crate::kept_paths::{self, KeptPath}; - -/// Does [`Renderer::bounds_of`] measure `n` by the 3D art this renderer draws and keeps for it: a -/// Revolve, Extrude & Bevel, Inflate or Rotate without Crop Marks or Vector Halftone, which draw -/// other art? -fn measures_three_d(n: &Node) -> bool { - (effects::has_revolve(n) || effects::has_solid(n)) && !effects::has_crop_marks(n) && !effects::addons::halftone::has_halftone(n) -} - -/// Does `n` hold an object that [`Renderer::bounds_of`] measures by its 3D effect's art? -fn holds_measured_three_d(n: &Node) -> bool { - let mut holds = false; - n.walk(&mut |c| holds |= measures_three_d(c)); - holds -} +use crate::node_checks::{holds_measured_three_d, measures_three_d}; impl Renderer { /// Cached cull bounds of a node (containers union their cached children), symbol instances @@ -55,6 +42,9 @@ impl Renderer { { // Culling leaves room for brush art as for any object ([`brush_fx::cull_bounds`]); // the measure is made again when the brush library changes ([`Self::check_symbols`]). + if self.brush_library.is_none() { + self.brush_library = Some(self.brushes.library_key_now(symbols.document(), self.stamp)); + } let pad = brush_fx::pad(symbols, a); b.map(|b| fx::with_own_raster_reach(symbols, a, b).inflate(pad, pad)) } @@ -119,11 +109,13 @@ impl Renderer { self.geom.retain(|_, e| !holds_instance(&e.node)); self.symbols = doc.symbols.iter().map(|s| (s.name.clone(), Arc::downgrade(&s.art))).collect(); } - // That art is measured with the art its brushed strokes draw ([`Self::bounds_of`]). - let brushes = self.brushes.library_key_now(doc, self.stamp); - if brushes != self.brush_library { - self.brush_library = brushes; + // That art is measured with the art its brushed strokes draw ([`Self::bounds_of`]). Without + // such bounds, nothing is checked. + if let Some(was) = self.brush_library + && self.brushes.library_key_now(doc, self.stamp) != was + { self.geom.retain(|_, e| !holds_measured_three_d(&e.node)); + self.brush_library = None; } } @@ -228,15 +220,13 @@ impl Renderer { /// 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 { - f.opts.isolated.is_some() && f.doc.symbol_edit.as_ref().is_some_and(|e| e.instance == Some(id)) + 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) -> bool { - let skipped_view_item = (f.opts.skip_templates || f.opts.hide_templates) && matches!(a.kind, NodeKind::Layer { template: true, .. }) - || (f.opts.skip_templates || f.opts.hide_guides) && matches!(a.kind, NodeKind::Path { guide: true, .. }); - if !a.visible || skipped_view_item || f.opts.hidden.contains(&a.id) || Self::edited_instance(f, a.id) { + if crate::node_checks::left_out(f.opts, f.doc, a) { return true; } match self.bounds_of(f.symbols, a) { diff --git a/crates/render/src/tests/changed.rs b/crates/render/src/tests/changed.rs index a65205bb0..cb32de870 100644 --- a/crates/render/src/tests/changed.rs +++ b/crates/render/src/tests/changed.rs @@ -539,3 +539,67 @@ fn a_renderer_measures_an_edit_with_the_3d_art_it_draws() { assert_eq!(built, 0, "drawn with the art made to measure it"); 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) { + 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 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) +} + +#[test] +fn moving_a_trace_objects_source_changes_where_it_shows() { + for view in ["sourceImage", "outlinesWithSourceImage"] { + 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)); + assert!(a.contains(Point::new(12.0, 12.0)) && a.contains(Point::new(44.0, 69.0)), "{view}: before and after: {a:?}"); + } +} + +#[test] +fn recolouring_a_traced_shape_changes_only_the_shape() { + 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)); + assert!(a.x0 > 72.0, "not the source or the other shape: {a:?}"); +} + +/// Templates and guides that the view hides draw nothing: an edit to them changes nothing. +#[test] +fn an_edit_to_what_the_view_hides_changes_nothing() { + let mut old = Document::new(W as f64, H as f64); + let template = Node::layer(old.alloc_id(), "Template", vectorcraft_doc::LayerColor::Preset(1)); + let template = old.insert(None, usize::MAX, template).unwrap(); + if let Some(NodeKind::Layer { template: t, .. }) = old.node_mut(template).map(|n| &mut n.kind) { + *t = true; + } + let traced = rect(&mut old, Rect::new(10.0, 10.0, 40.0, 40.0), Color::rgb(0.5, 0.5, 0.5)); + let traced = old.insert(Some(template), 0, traced).unwrap(); + let mut guide = rect(&mut old, Rect::new(60.0, 10.0, 90.0, 40.0), Color::BLACK); + if let NodeKind::Path { guide: g, .. } = &mut guide.kind { + *g = true; + } + let l = old.layers[0].id; + let guide = old.insert(Some(l), 0, guide).unwrap(); + let mut new = old.clone(); + for id in [traced, guide] { + new.node_mut(id).unwrap().transform(Affine::translate((5.0, 5.0)), false); + } + let opts = RenderOptions { hide_templates: true, hide_guides: true, ..Default::default() }; + let mut r = Renderer::new(); + r.render(&old, W, H, view(), &opts); + assert_eq!(r.changed(&old, &new, &opts), Changed::Nothing); + assert!(matches!(changed(&old, &new), Changed::Area(_)), "shown, they change"); +} diff --git a/crates/ui-egui/src/panels/artboards/tests.rs b/crates/ui-egui/src/panels/artboards/tests.rs index 256e7ceaf..e10fd8ab3 100644 --- a/crates/ui-egui/src/panels/artboards/tests.rs +++ b/crates/ui-egui/src/panels/artboards/tests.rs @@ -1656,6 +1656,29 @@ fn the_open_menu_reads_what_the_right_click_measured() { assert!(labels.contains(&"Delete Empty Artboards") && !labels.contains(&"Fit to Artwork Bounds"), "{labels:?}"); } +/// Whether Fit to Artwork Bounds has art to fit is measured once for a revision of the document: +/// the canvas menu asks on every frame it is open. +#[test] +fn fit_to_art_is_measured_once_a_revision() { + let mut app = menu_app(2); + assert!(!super::fit_has_art(&app, &[0, 1])); + // Art on the first artboard, the revision left as it was: the answer kept stands. + let st = app.session.active_mut().unwrap(); + let mut doc = (*st.doc).clone(); + let l = doc.layers[0].id; + let id = doc.alloc_id(); + let art = vectorcraft_doc::Node::path( + id, + vectorcraft_geom::shapes::rectangle(vectorcraft_geom::Rect::new(10.0, 10.0, 20.0, 20.0)), + vectorcraft_doc::Appearance::basic(vectorcraft_color::Paint::solid(vectorcraft_color::Color::BLACK), vectorcraft_color::Paint::None, 0.0), + ); + doc.insert(Some(l), 0, art).unwrap(); + st.doc = std::sync::Arc::new(doc); + assert!(!super::fit_has_art(&app, &[0, 1]), "kept for the revision"); + app.session.active_mut().unwrap().revision += 1; + assert!(super::fit_has_art(&app, &[0, 1]), "measured again"); +} + /// `artboard.selectAll` chooses the Artboard tool first when another tool is in use, and fails /// without a document. #[test]