From 4b260c364d73fd895e9b32aac1cadc3615d23538 Mon Sep 17 00:00:00 2001 From: arhammusheer Date: Sun, 11 Oct 2026 16:08:28 +0530 Subject: [PATCH 1/8] #1172: edit a large selection in time linear in the document, not quadratic MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every lookup by id walked the whole tree, and the edit path did one per selected id, so editing a selection of n objects cost O(n²). Measured with the new `perf --deep` rows at 25,000 paths: - Select All: 458 → 3.5 ms - nudge the selection: 3,517 → 34 ms - drag it with the Selection tool, per pointer move: 3,413 → 35 ms - undo / redo: 407 → 3.1 ms - vectorcraft_doc: Document::index_paths (every id's index path, in one walk; the first in tree order for a duplicated id, as index_path), node_at, node_mut_at (copy-on-write, as node_mut) and ancestry_at. - apply_transform, roots_of, transform_patterns, doc_sane, Selection::prune and Document::paint_order look their ids up with one walk instead of one each. - Selection::set dedups with a hash set instead of Vec::contains per id. - vectorcraft-cli perf --deep: tracked rows for editing a whole selection (select, nudge, drag, undo, redo), live effects, every Pathfinder operation, typing in a long story, and memory. tests_lookups checks the batched lookups against the one-id ones on nested trees with missing and duplicated ids; storing the last duplicate instead of the first fails it. The doc and engine suites (2,000 tests) pass unchanged. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01VmMmc1J8XeNLxhRihWv9cc --- apps/vectorcraft-cli/src/perf.rs | 109 +++++++++++++++++++++++++++++++ crates/doc/src/assets.rs | 4 +- crates/doc/src/lib.rs | 68 +++++++++++++++++++ crates/doc/src/selection.rs | 17 +++-- crates/doc/src/tests_lookups.rs | 80 +++++++++++++++++++++++ crates/engine/src/cmd/edit.rs | 27 +++++--- crates/engine/src/cmd/object.rs | 9 ++- crates/engine/src/lib.rs | 7 +- 8 files changed, 297 insertions(+), 24 deletions(-) create mode 100644 crates/doc/src/tests_lookups.rs diff --git a/apps/vectorcraft-cli/src/perf.rs b/apps/vectorcraft-cli/src/perf.rs index 41da4abc..afafd6db 100644 --- a/apps/vectorcraft-cli/src/perf.rs +++ b/apps/vectorcraft-cli/src/perf.rs @@ -113,11 +113,117 @@ fn edit_drags(n: usize) -> Result<(f64, f64), String> { Ok((handle, segment)) } +/// Resident memory of this process in MB (`ps`), if available. +fn rss_mb() -> Option { + let pid = std::process::id().to_string(); + let out = std::process::Command::new("ps").args(["-o", "rss=", "-p", &pid]).output().ok().filter(|o| o.status.success())?; + String::from_utf8_lossy(&out.stdout).trim().parse::().ok().map(|kb| kb / 1024.0) +} + +/// `--deep`: tracked rows (no budget yet) for work the budgets above don't reach: editing a whole +/// selection, live effects, every Pathfinder operation, typing in a long story, and memory. +fn deep(n: usize) -> Result<(), String> { + let err = |e: vectorcraft_engine::EngineError| e.to_string(); + let mut rows: Vec<(String, f64)> = vec![]; + let mut mem: Vec<(String, f64)> = vec![]; + let mut note = |what: &str| { + if let Some(m) = rss_mb() { + mem.push((what.to_string(), m)); + } + }; + note("start"); + // Editing every object of an n-path document. + let mut s = vectorcraft_engine::Session::new(); + s.add_document(synthetic(n)?, None); + note("document built"); + rows.push((format!("select all, {n} paths"), median_ms(3, || drop(s.execute("select.all", &json!({})))))); + rows.push((format!("nudge the selection ({n} paths)"), median_ms(5, || drop(s.execute("object.move", &json!({"dx": 1, "dy": 0})))))); + rows.push((format!("undo a nudge ({n} paths)"), median_ms(5, || drop(s.execute("edit.undo", &json!({})))))); + rows.push((format!("redo a nudge ({n} paths)"), median_ms(5, || drop(s.execute("edit.redo", &json!({})))))); + // Dragging the whole selection with the Selection tool: one pointer move (the preview). + { + use vectorcraft_engine::tools::{Mods, PointerEvent, PointerKind}; + use vectorcraft_geom::Vec2; + let view = vectorcraft_engine::ViewInfo { zoom: 1.0, ..Default::default() }; + let first = s.doc().map_err(err)?.selection.objects.first().copied(); + let at = first.and_then(|id| s.doc().ok()?.doc.node(id)?.geometric_bounds()).map(|b| b.center()); + if let Some(down) = at { + let ev = |kind, dx: f64| PointerEvent { kind, pos: down + Vec2::new(dx, 0.0), mods: Mods::default(), pressure: 1.0 }; + s.select_tool("selection", view).map_err(err)?; + s.pointer(&ev(PointerKind::Down, 0.0), view).map_err(err)?; + let mut dx = 0.0; + rows.push(( + format!("drag the selection, per move ({n} paths)"), + median_ms(5, || { + dx += 2.0; + let _ = s.pointer(&ev(PointerKind::Drag, dx), view); + }), + )); + s.pointer(&ev(PointerKind::Up, dx), view).map_err(err)?; + } + } + for _ in 0..40 { + s.execute("object.move", &json!({"dx": 1, "dy": 0})).map_err(err)?; + } + note("after 45 whole-selection edits"); + drop(s); + // Live effects on 2,000 paths: render a page whose every object has the effect. + let opts = RenderOptions::default(); + for (fx, params) in [("path.offsetPath", json!({"offset": 3})), ("distort.roughen", json!({"size": 5, "relative": true}))] { + let mut s = vectorcraft_engine::Session::new(); + s.add_document(synthetic(2000)?, None); + s.execute("select.all", &json!({})).map_err(err)?; + s.execute("effect.apply", &json!({"effect": fx, "params": params})).map_err(err)?; + let doc = s.doc().map_err(err)?.doc.clone(); + let mut r = Renderer::new(); + r.render(&doc, 2880, 1800, Affine::scale(1.6), &opts); + rows.push((format!("render, 2,000 paths with {fx} (warm)"), median_ms(5, || drop(r.render(&doc, 2880, 1800, Affine::scale(1.6), &opts))))); + } + note("after live effects"); + // Every Pathfinder operation on 300 overlapping paths. + for op in ["unite", "intersect", "exclude", "minusFront", "minusBack", "divide", "trim", "merge", "crop", "outline"] { + let ms = median_ms(1, || { + let mut s = vectorcraft_engine::Session::new(); + if let Ok(d) = synthetic(300) { + s.add_document(d, None); + let _ = s.execute("select.all", &json!({})).and_then(|_| s.execute(&format!("object.pathfinder.{op}"), &json!({}))); + } + }); + rows.push((format!("Pathfinder {op}, 300 paths"), ms)); + } + note("after Pathfinder"); + // Typing one character in the middle of a 20,000-character area-type story. + let mut s = vectorcraft_engine::Session::new(); + s.add_document(Document::new(1600.0, 1200.0), None); + let text: String = (0..2000).map(|i| if i % 12 == 11 { "paragraph.\n" } else { "lorem ips " }).collect(); + let id = s.execute("text.create", &json!({"x": 20, "y": 20, "text": text, "size": 10, "area": {"width": 1500, "height": 1100}})).map_err(err)?; + let id = id.get("id").cloned().unwrap_or(json!(0)); + let mut at = 10_000; + rows.push(( + "type one character, 20,000-character area text".into(), + median_ms(9, || { + at += 1; + let _ = s.execute("text.editRange", &json!({"id": id, "start": at, "end": at, "insert": "x"})); + }), + )); + note("after typing"); + outln!(" tracked (no budget yet)"); + for (name, ms) in &rows { + outln!(" {name:<52} {ms:>10.2} ms"); + } + for (what, mb) in &mem { + outln!(" memory {what:<45} {mb:>8.0} MB"); + } + Ok(()) +} + pub fn run(args: &[String]) -> Result<(), String> { let mut n = 50_000usize; + let mut want_deep = false; let mut it = args.iter(); while let Some(a) = it.next() { match a.as_str() { + "--deep" => want_deep = true, "--paths" => n = it.next().and_then(|v| v.parse().ok()).ok_or("--paths needs a number")?, other => return Err(format!("unknown perf option `{other}`")), } @@ -200,6 +306,9 @@ pub fn run(args: &[String]) -> Result<(), String> { if let Some(e) = failed { return Err(e); } + if want_deep { + deep(n)?; + } let mut over = 0; outln!(" {:<40} {:>10} {:>10}", "", "measured", "budget"); diff --git a/crates/doc/src/assets.rs b/crates/doc/src/assets.rs index ec84333a..3c4451d5 100644 --- a/crates/doc/src/assets.rs +++ b/crates/doc/src/assets.rs @@ -50,7 +50,9 @@ impl Document { /// `ids` that are in the document, back to front. pub fn paint_order(&self, ids: impl IntoIterator) -> Vec { - let mut v: Vec<(Vec, NodeId)> = ids.into_iter().filter_map(|id| self.index_path(id).map(|p| (p, id))).collect(); + let ids: Vec = ids.into_iter().collect(); + let paths = self.index_paths(ids.iter().copied()); + let mut v: Vec<(Vec, NodeId)> = ids.into_iter().filter_map(|id| paths.get(&id).map(|p| (p.clone(), id))).collect(); v.sort(); v.into_iter().map(|(_, id)| id).collect() } diff --git a/crates/doc/src/lib.rs b/crates/doc/src/lib.rs index 259512b8..7825de13 100644 --- a/crates/doc/src/lib.rs +++ b/crates/doc/src/lib.rs @@ -795,6 +795,72 @@ impl Document { find(&self.layers, id, &mut p).then_some(p) } + /// [`Document::index_path`] of every id in `ids` that is in the tree, in one walk of it: an + /// edit of many objects looks them all up for the cost of one search instead of one each. Like + /// `index_path`, an id that appears twice maps to its first place in tree order. + pub fn index_paths(&self, ids: impl IntoIterator) -> std::collections::HashMap> { + use std::collections::{HashMap, HashSet}; + fn walk(nodes: &[Arc], want: &HashSet, path: &mut Vec, out: &mut HashMap>) -> bool { + for (i, n) in nodes.iter().enumerate() { + path.push(i); + if want.contains(&n.id) && !out.contains_key(&n.id) { + out.insert(n.id, path.clone()); + if out.len() == want.len() { + return true; + } + } + if let Some(ch) = n.children() + && walk(ch, want, path, out) + { + return true; + } + path.pop(); + } + false + } + let want: HashSet = ids.into_iter().collect(); + let mut out = HashMap::with_capacity(want.len()); + if !want.is_empty() { + walk(&self.layers, &want, &mut Vec::new(), &mut out); + } + out + } + + /// The node at index path `path` (as [`Document::index_path`] gives it). + pub fn node_at(&self, path: &[usize]) -> Option<&Node> { + let (first, rest) = path.split_first()?; + let mut cur: &Node = self.layers.get(*first)?; + for &i in rest { + cur = cur.children()?.get(i)?; + } + Some(cur) + } + + /// Mutable access to the node at index path `path`; clones the Arc path (copy-on-write), as + /// [`Document::node_mut`] does. + pub fn node_mut_at(&mut self, path: &[usize]) -> Option<&mut Node> { + let (first, rest) = path.split_first()?; + let mut cur: &mut Arc = self.layers.get_mut(*first)?; + for &i in rest { + cur = Arc::make_mut(cur).children_mut()?.get_mut(i)?; + } + Some(Arc::make_mut(cur)) + } + + /// [`Document::ancestry`] of the node at index path `path`. + pub fn ancestry_at(&self, path: &[usize]) -> Option> { + let mut out = Vec::with_capacity(path.len()); + let mut nodes = &self.layers; + for &i in path { + let n = nodes.get(i)?; + out.push(n.id); + if let Some(ch) = n.children() { + nodes = ch; + } + } + Some(out) + } + /// Ids from the top-level layer down to `id` inclusive. pub fn ancestry(&self, id: NodeId) -> Option> { let path = self.index_path(id)?; @@ -1391,6 +1457,8 @@ mod tests { } } +#[cfg(test)] +mod tests_lookups; #[cfg(test)] mod tests_slices; #[cfg(test)] diff --git a/crates/doc/src/selection.rs b/crates/doc/src/selection.rs index 2726d149..d8914465 100644 --- a/crates/doc/src/selection.rs +++ b/crates/doc/src/selection.rs @@ -62,9 +62,9 @@ impl Selection { } pub fn set(&mut self, ids: impl IntoIterator) { self.clear(); - for id in ids { - self.add(id); - } + // What `add` per id does after a clear, without its linear duplicate check per id. + let mut seen = std::collections::HashSet::new(); + self.objects.extend(ids.into_iter().filter(|id| seen.insert(*id))); } pub fn add(&mut self, id: NodeId) { self.target = None; @@ -118,10 +118,13 @@ impl Selection { } /// Drop ids that no longer exist or are no longer editable. pub fn prune(&mut self, doc: &Document) { - self.objects.retain(|id| doc.node(*id).is_some()); - self.anchors.retain(|id, _| doc.node(*id).is_some()); + // Every id this looks up, found in one walk of the tree. + let ids = self.objects.iter().chain(self.anchors.keys()).copied().chain(self.key).chain(self.target); + let found = doc.index_paths(ids); + self.objects.retain(|id| found.contains_key(id)); + self.anchors.retain(|id, _| found.contains_key(id)); // The key object stays while another selected object lies outside it. - let keep = self.key.and_then(|k| doc.node(k)).is_some_and(|n| { + let keep = self.key.and_then(|k| found.get(&k)).and_then(|p| doc.node_at(p)).is_some_and(|n| { let mut inside = BTreeSet::new(); n.walk(&mut |c| { inside.insert(c.id); @@ -131,7 +134,7 @@ impl Selection { if !keep { self.key = None; } - if self.target.is_some_and(|t| doc.node(t).is_none()) { + if self.target.is_some_and(|t| !found.contains_key(&t)) { self.target = None; } self.slices.retain(|id| doc.is_slice(*id)); diff --git a/crates/doc/src/tests_lookups.rs b/crates/doc/src/tests_lookups.rs new file mode 100644 index 00000000..3dff6215 --- /dev/null +++ b/crates/doc/src/tests_lookups.rs @@ -0,0 +1,80 @@ +//! The batched lookups ([`Document::index_paths`], `node_at`, `node_mut_at`, `ancestry_at`) agree +//! with the one-id ones on nested trees with missing and duplicated ids. +use super::*; +use vectorcraft_geom::shapes; + +/// Layers with paths and groups nested `depth` deep; every 7th id is reused (a damaged file), so +/// first-in-tree-order matters. +fn tree(seed: u64) -> (Document, Vec) { + let mut d = Document::new(612.0, 792.0); + let mut r = seed | 1; + let mut next = |n: u64| { + r ^= r << 13; + r ^= r >> 7; + r ^= r << 17; + r % n + }; + let mut ids = vec![]; + let leaf = |d: &mut Document, ids: &mut Vec| { + let id = if ids.len() % 7 == 6 { ids[ids.len() / 2] } else { d.alloc_id() }; + ids.push(id); + Arc::new(Node::path(id, shapes::rectangle(Rect::new(0.0, 0.0, 5.0, 5.0)), Appearance::default_art())) + }; + for l in 0..3 { + let lid = d.alloc_id(); + let mut layer = Node::layer(lid, &format!("L{l}"), LayerColor::Preset(0)); + ids.push(lid); + for _ in 0..(5 + next(10)) { + let child = if next(3) == 0 { + let mut inner: Vec> = (0..(1 + next(4))).map(|_| leaf(&mut d, &mut ids)).collect(); + for _ in 0..next(3) { + let gid = d.alloc_id(); + ids.push(gid); + inner = vec![Arc::new(Node::group(gid, inner))]; + } + let gid = d.alloc_id(); + ids.push(gid); + Arc::new(Node::group(gid, inner)) + } else { + leaf(&mut d, &mut ids) + }; + if let Some(ch) = layer.children_mut() { + ch.push(child); + } + } + d.layers.push(Arc::new(layer)); + } + ids.push(NodeId(999_999)); // not in the tree + (d, ids) +} + +#[test] +fn batched_lookups_match_one_at_a_time() { + for seed in 1..40 { + let (d, ids) = tree(seed); + let paths = d.index_paths(ids.iter().copied()); + for id in &ids { + assert_eq!(paths.get(id).cloned(), d.index_path(*id), "seed {seed} id {id:?}"); + let p = paths.get(id); + assert_eq!(p.and_then(|p| d.node_at(p)).map(|n| n as *const Node), d.node(*id).map(|n| n as *const Node), "seed {seed} node {id:?}"); + assert_eq!(p.and_then(|p| d.ancestry_at(p)), d.ancestry(*id), "seed {seed} ancestry {id:?}"); + } + // A subset, and nothing. + let some: Vec = ids.iter().copied().step_by(3).collect(); + let sub = d.index_paths(some.iter().copied()); + assert!(some.iter().all(|id| sub.get(id).cloned() == d.index_path(*id))); + assert!(d.index_paths(std::iter::empty()).is_empty()); + // Mutable access reaches the same node as node_mut, copy-on-write. + let mut a = d.clone(); + let mut b = d.clone(); + for id in ids.iter().step_by(5) { + if let Some(n) = a.node_mut(*id) { + n.opacity = 0.25; + } + if let Some(n) = d.index_path(*id).and_then(|p| b.node_mut_at(&p)) { + n.opacity = 0.25; + } + } + assert_eq!(serde_json::to_string(&a).ok(), serde_json::to_string(&b).ok(), "seed {seed}"); + } +} diff --git a/crates/engine/src/cmd/edit.rs b/crates/engine/src/cmd/edit.rs index e5e94c10..c6201b7b 100644 --- a/crates/engine/src/cmd/edit.rs +++ b/crates/engine/src/cmd/edit.rs @@ -251,22 +251,29 @@ pub(crate) fn selected_roots(s: &Session) -> Result> { /// The top-level objects among `ids` (in paint order): compound-path members stand for their /// compound, and ids whose ancestor is also listed, and layers, are dropped. pub(crate) fn roots_of(doc: &Document, ids: Vec) -> Vec { + // Every id's place in the tree, found in one walk (a lookup per id is a walk per id). + let paths = doc.index_paths(ids.iter().copied()); // A compound-path member acts as its compound (compounds are one object). - let mut sel: Vec = ids + let mut sel: Vec<(NodeId, Option>)> = ids .into_iter() - .map(|id| match doc.parent_of(id).and_then(|p| doc.node(p).map(|n| (p, n))) { - Some((p, n)) if matches!(n.kind, vectorcraft_doc::NodeKind::Compound { .. }) => p, - _ => id, + .map(|id| { + let path = paths.get(&id); + let parent = path.and_then(|p| p.split_last()).map(|(_, up)| up).filter(|up| !up.is_empty()); + match parent.and_then(|up| doc.node_at(up).map(|n| (up, n))) { + Some((up, n)) if matches!(n.kind, vectorcraft_doc::NodeKind::Compound { .. }) => (n.id, Some(up.to_vec())), + _ => (id, path.cloned()), + } }) .collect(); - sel.dedup(); + sel.dedup_by_key(|(id, _)| *id); + let listed: std::collections::HashSet = sel.iter().map(|(id, _)| *id).collect(); sel.iter() - .copied() - .filter(|id| { - let anc = doc.ancestry(*id).unwrap_or_default(); - !anc[..anc.len().saturating_sub(1)].iter().any(|a| sel.contains(a)) + .filter(|(_, path)| { + let anc = path.as_deref().and_then(|p| doc.ancestry_at(p)).unwrap_or_default(); + !anc[..anc.len().saturating_sub(1)].iter().any(|a| listed.contains(a)) }) - .filter(|id| doc.node(*id).is_some_and(|n| !n.is_layer())) + .filter(|(_, path)| path.as_deref().and_then(|p| doc.node_at(p)).is_some_and(|n| !n.is_layer())) + .map(|(id, _)| *id) .collect() } diff --git a/crates/engine/src/cmd/object.rs b/crates/engine/src/cmd/object.rs index ba3e580b..5c75c1a9 100644 --- a/crates/engine/src/cmd/object.rs +++ b/crates/engine/src/cmd/object.rs @@ -242,7 +242,8 @@ pub(crate) fn transform_patterns(s: &mut Session, p: &Value, ids: &[NodeId]) -> let on = bool_or(p, "patterns", s.prefs.transform_pattern_tiles); let d = &s.doc()?.doc; let mut uses = false; - for n in ids.iter().filter_map(|id| d.node(*id)) { + let paths = d.index_paths(ids.iter().copied()); + for n in ids.iter().filter_map(|id| paths.get(id)).filter_map(|p| d.node_at(p)) { n.walk(&mut |c| uses |= vectorcraft_doc::pattern::uses_pattern(c, None)); } if uses { @@ -263,14 +264,16 @@ pub(crate) fn apply_transform(s: &mut Session, label: &str, ids: Vec, xf // nothing: the document and its undo history stay as they are, and Transform Again repeats it. // With a copy, the reply lists the copies, and there are none. let d = &s.doc()?.doc; - let none = ids.iter().all(|id| d.node(*id).is_none()); + let none = d.index_paths(ids.iter().copied()).is_empty(); let ids = if none || (!copy && xf == Affine::IDENTITY) { if copy { vec![] } else { ids } } else { s.edit(label, |d, sel| { let targets = if copy { duplicate_in(d, sel, &ids, Affine::IDENTITY)? } else { ids.clone() }; + // Transforms don't move nodes in the tree, so the paths found up front stay right. + let paths = d.index_paths(targets.iter().copied()); for id in &targets { - if let Some(n) = d.node_mut(*id) + if let Some(n) = paths.get(id).and_then(|p| d.node_mut_at(p)) && !(areas && resize_type_area(n, xf)) { n.transform(xf, sc); diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index f3041d6c..fc819f78 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -421,9 +421,10 @@ fn strip_replay_only(id: &str, p: &mut Value) { /// have finite, in-range geometry, so saved files always reload and renderers never see NaN/∞. fn doc_sane(d: &Document, sel: &Selection) -> bool { let ok = |r: vectorcraft_geom::Rect| [r.x0, r.y0, r.x1, r.y1].iter().all(|v| v.is_finite() && v.abs() <= MAX_COORD); - d.artboards.iter().all(|a| ok(a.rect)) - && d.guides.iter().all(|g| g.pos.is_finite() && g.pos.abs() <= MAX_COORD) - && sel.objects.iter().all(|id| d.node(*id).and_then(|n| n.geometric_bounds()).is_none_or(ok)) + d.artboards.iter().all(|a| ok(a.rect)) && d.guides.iter().all(|g| g.pos.is_finite() && g.pos.abs() <= MAX_COORD) && { + let paths = d.index_paths(sel.objects.iter().copied()); + sel.objects.iter().all(|id| paths.get(id).and_then(|p| d.node_at(p)).and_then(|n| n.geometric_bounds()).is_none_or(ok)) + } } /// Where new art goes (Illustrator's drawing modes, Shift+D cycles). From add27cb8fcb70f590b6960ed0d79ad7a4ab7fafa Mon Sep 17 00:00:00 2001 From: arhammusheer Date: Sun, 11 Oct 2026 16:20:52 +0530 Subject: [PATCH 2/8] #1172: keep live-effect geometry across frames MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A node with live geometry effects (Offset Path, Outline Stroke, Warp, Transform, Roughen…) had them computed again on every frame: Offset Path's curve offsetting and boolean sweep for every object, every time the canvas redrew. Rendering 2,000 paths that each have Offset Path, warm: 175 → 11.8 ms. The renderer keeps each node's effected geometry, and each appearance item's own geometry effects on it, keyed by the node's Arc identity like its other per-node caches. The entry keeps the Arc alive, so a hit is exact and an edited object (a new node) gets new geometry. Only the node draw_arc is drawing uses the cache: its path comes from the node alone, so its effects give the same result every frame. Entries not drawn for three frames are swept like the rest. cached_effect_geometry_draws_the_same_pixels renders a page of Offset Path, Roughen + Drop Shadow, Warp + Outline Stroke and Transform objects (with item-level Offset Path) cold, warm, after a pan and with a fresh renderer, and gets the same pixels each time; after an edit, the warm renderer draws what a fresh one does. Keying the cache by node id instead of identity fails it. The render suite (216 tests, golden images included) passes unchanged. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01VmMmc1J8XeNLxhRihWv9cc --- crates/render/src/fx.rs | 129 ++++++++++++++++++++++++++++++++++++--- crates/render/src/lib.rs | 12 ++++ 2 files changed, 131 insertions(+), 10 deletions(-) diff --git a/crates/render/src/fx.rs b/crates/render/src/fx.rs index 266bfe0b..cc79f6a3 100644 --- a/crates/render/src/fx.rs +++ b/crates/render/src/fx.rs @@ -261,6 +261,16 @@ pub(crate) struct ShadowEntry { pub(crate) stamp: u64, } +/// Live-effect geometry of a node, kept across frames: the object-level result and each +/// appearance item's own result on it (filled in as items are drawn). The node is kept alive, so +/// its address names it alone and a hit is exact. +pub(crate) struct FxGeom { + node: Arc, + g: Arc, + items: Vec>>, + pub(crate) stamp: u64, +} + impl Renderer { /// Drop shadow / outer glow `fx` (shadow-cache slot `i` of `n`) from a cached raster: /// the silhouette is rendered once into a small crop, blurred and tinted on the CPU, and reused @@ -340,12 +350,43 @@ impl Renderer { /// `draw_shape` for nodes with live effects. pub(crate) fn draw_shape_fx(&mut self, ctx: &mut RenderContext, f: &Frame, n: &Node, bp: &BezPath, rule: FillRule) { - let g = effected_path(n, bp); + // The node `draw_arc` is drawing gets its geometry from the cache: `bp` is its own path, + // made from the node alone, so the effects give the same result every frame. + let owner = self.fx_owner.as_ref().filter(|o| std::ptr::eq(&***o, n)).cloned(); + let mut cached = owner.map(|o| { + let key = Arc::as_ptr(&o) as usize; + let stamp = self.stamp; + let e = match self.fx_geom.remove(&key) { + Some(e) if Arc::ptr_eq(&e.node, &o) => e, + _ => FxGeom { g: Arc::new(effected_path(n, bp)), items: vec![None; n.appearance.items.len()], node: o, stamp }, + }; + (key, FxGeom { stamp, ..e }) + }); + let g = match &cached { + Some((_, e)) => e.g.clone(), + None => Arc::new(effected_path(n, bp)), + }; + self.draw_shape_fx_geom(ctx, f, n, &g, rule, cached.as_mut().map(|(_, e)| &mut e.items)); + if let Some((key, e)) = cached { + self.fx_geom.insert(key, e); + } + } + + /// [`Self::draw_shape_fx`] on the effected geometry `g`, with the items' cached geometry. + fn draw_shape_fx_geom( + &mut self, + ctx: &mut RenderContext, + f: &Frame, + n: &Node, + g: &BezPath, + rule: FillRule, + items: Option<&mut Vec>>>, + ) { if g.elements().is_empty() { return; } if f.opts.outline { - self.hairline(ctx, f, &g, [0, 0, 0, 255]); + self.hairline(ctx, f, g, [0, 0, 0, 255]); return; } let gctx = GeomContext::of(n); @@ -353,8 +394,9 @@ impl Renderer { // Document-space reach of the painted geometry (strokes, arrowheads…). let outset = n.appearance.outset(); let reach = g.bounding_box().inflate(outset, outset); - let content = Content { node: n, cache: Some(0), reach, outline: Some((&g, rule)) }; - self.raster_fx(ctx, f, &content, &rfx, &mut |r, c, fr| r.paint_items(c, fr, n, &g, rule, &gctx)); + let content = Content { node: n, cache: Some(0), reach, outline: Some((g, rule)) }; + let mut items = items; + self.raster_fx(ctx, f, &content, &rfx, &mut |r, c, fr| r.paint_items(c, fr, n, g, rule, &gctx, items.as_deref_mut())); ctx.set_transform(Affine::IDENTITY); } @@ -698,32 +740,49 @@ impl Renderer { /// Paint the fills and strokes of `n` on geometry `g`, applying per-item geometry effects and /// painting per-item raster effects around that item alone. - fn paint_items(&mut self, ctx: &mut RenderContext, f: &Frame, n: &Node, g: &BezPath, rule: FillRule, gctx: &GeomContext) { + #[allow(clippy::too_many_arguments)] + fn paint_items( + &mut self, + ctx: &mut RenderContext, + f: &Frame, + n: &Node, + g: &BezPath, + rule: FillRule, + gctx: &GeomContext, + mut cache: Option<&mut Vec>>>, + ) { let bounds = g.bounding_box(); for (j, item) in n.appearance.items.iter().enumerate() { if !item.visible() || item.paint().is_none() { continue; } - let ig = apply(item_effects(item), g, &gctx.item(item)); + // The item's own geometry effects on `g`, from the cache when there is one. + let slot = cache.as_deref_mut().and_then(|c| c.get_mut(j)); + let ig: Arc = match slot { + Some(Some(ig)) => ig.clone(), + Some(s) => s.insert(Arc::new(apply(item_effects(item), g, &gctx.item(item)))).clone(), + None => Arc::new(apply(item_effects(item), g, &gctx.item(item))), + }; + let ig = &*ig; let ib = if effects::has_geometry(item_effects(item)) && !ig.elements().is_empty() { ig.bounding_box() } else { bounds }; let rfx = effects::raster_effects(item_effects(item)); if rfx.is_empty() { - self.paint_item(ctx, f, n, item, &ig, rule, ib); + self.paint_item(ctx, f, n, item, ig, rule, ib); continue; } // The item's silhouette: its fill area, or the outline of its stroke. let outline = match item { AppearanceItem::Fill(_) => ig.clone(), AppearanceItem::Stroke(st) => { - let w = effects::stroke::aligned_width(st, effects::stroke::is_closed(&ig)); - effects::stroke::line_outline(&effects::stroke::stroke_pieces(&ig, st).line, st, w, f.px * 0.25) + let w = effects::stroke::aligned_width(st, effects::stroke::is_closed(ig)); + effects::stroke::line_outline(&effects::stroke::stroke_pieces(ig, st).line, st, w, f.px * 0.25) } }; let outset = n.appearance.outset(); let orule = if item.is_fill() { rule } else { FillRule::NonZero }; // Each item's shadows get their own cache slots. let content = Content { node: n, cache: Some((j + 1) << 8), reach: ib.inflate(outset, outset), outline: Some((&outline, orule)) }; - self.raster_fx(ctx, f, &content, &rfx, &mut |r, c, fr| r.paint_item(c, fr, n, item, &ig, rule, ib)); + self.raster_fx(ctx, f, &content, &rfx, &mut |r, c, fr| r.paint_item(c, fr, n, item, ig, rule, ib)); ctx.set_transform(Affine::IDENTITY); } } @@ -790,6 +849,56 @@ mod tests { const WHITE: u32 = 765; + /// A page of objects with object-level and item-level geometry effects (Offset Path, Roughen, + /// Warp, Outline Stroke, Transform) under drop shadows and in groups. + fn effects_page() -> Document { + let mut d = Document::new(200.0, 200.0); + let l = d.layers[0].id; + let fx = |id: &str, p: serde_json::Value| Effect { id: id.into(), params: p, visible: true }; + for i in 0..24 { + let (x, y) = (10.0 + (i % 6) as f64 * 30.0, 10.0 + (i / 6) as f64 * 45.0); + let id = d.alloc_id(); + let r = Rect::new(x, y, x + 20.0, y + 30.0); + let shape = if i % 2 == 0 { shapes::ellipse(r) } else { shapes::star(r.center(), 12.0, 5.0, 5, 0.0) }; + let mut n = Node::path(id, shape, Appearance::basic(Paint::solid(Color::rgb(0.2, 0.4, 0.9)), Paint::solid(Color::BLACK), 1.5)); + n.appearance.effects = match i % 4 { + 0 => vec![fx("path.offsetPath", json!({"offset": 3}))], + 1 => vec![fx("distort.roughen", json!({"size": 4, "relative": true})), fx("stylize.dropShadow", json!({}))], + 2 => vec![fx("warp.arc", json!({"bend": 30})), fx("path.outlineStroke", json!({}))], + _ => vec![fx("path.offsetPath", json!({"offset": -2})), fx("distort.transform", json!({"moveH": 4, "rotate": 10}))], + }; + if let Some(f) = n.appearance.fill_mut() { + f.effects.push(fx("path.offsetPath", json!({"offset": 2}))); + } + d.insert(Some(l), usize::MAX, n).unwrap(); + } + d + } + + /// Cached live-effect geometry draws exactly what computing it afresh draws: a second (warm) + /// frame, a frame after panning, and a fresh renderer all give the same pixels, and an edited + /// object (a new node) draws its new geometry, not the cached one. + #[test] + fn cached_effect_geometry_draws_the_same_pixels() { + let d = effects_page(); + let opts = RenderOptions { background: Some([255, 255, 255, 255]), ..Default::default() }; + let view = Affine::scale(2.0); + let mut r = Renderer::new(); + let cold = r.render(&d, 400, 400, view, &opts); + let warm = r.render(&d, 400, 400, view, &opts); + assert!(cold.pixels == warm.pixels, "a warm frame differs from the cold one"); + let _ = r.render(&d, 400, 400, Affine::translate((15.0, 0.0)) * view, &opts); + assert!(r.render(&d, 400, 400, view, &opts).pixels == cold.pixels, "after a pan"); + assert!(Renderer::new().render(&d, 400, 400, view, &opts).pixels == cold.pixels, "a fresh renderer"); + // Edit one object's effect: the warm renderer must draw the new result. + let mut e = d.clone(); + let id = e.layers[0].children().unwrap()[0].id; + e.node_mut(id).unwrap().appearance.effects = vec![Effect { id: "path.offsetPath".into(), params: json!({"offset": 9}), visible: true }]; + let edited = r.render(&e, 400, 400, view, &opts); + assert!(edited.pixels != cold.pixels, "the edit shows"); + assert!(edited.pixels == Renderer::new().render(&e, 400, 400, view, &opts).pixels, "and matches a fresh render"); + } + #[test] fn thumbnails_of_glowing_objects_render_without_panicking() { // Regression: thumbnails used vello's default (multithreaded) context, which panics on filters. diff --git a/crates/render/src/lib.rs b/crates/render/src/lib.rs index e2f93205..50d11558 100644 --- a/crates/render/src/lib.rs +++ b/crates/render/src/lib.rs @@ -358,6 +358,10 @@ pub struct Renderer { strokes: PtrMap<(usize, u64, i32), StrokeEntry>, /// The node being drawn on the plain-path fast path (owner of cached stroke outlines). cur: Option>, + /// The node `draw_shape_fx` may cache the live-effect geometry of (the one `draw_arc` is drawing). + fx_owner: Option>, + /// Live-effect geometry per node ([`fx::FxGeom`]), keyed by `Arc` identity like `geom`. + fx_geom: PtrMap, stamp: u64, /// Opacity folded into paint alpha for the leaf being drawn (avoids a compositing layer). alpha: f32, @@ -480,6 +484,8 @@ impl Renderer { geom: PtrMap::default(), strokes: PtrMap::default(), cur: None, + fx_owner: None, + fx_geom: PtrMap::default(), stamp: 0, alpha: 1.0, ctx: None, @@ -572,6 +578,9 @@ impl Renderer { if self.shadows.len() > 256 { self.shadows.retain(|_, e| g - e.stamp <= 3); } + if self.fx_geom.len() > 256 { + self.fx_geom.retain(|_, e| g - e.stamp <= 3); + } // Filtered rasters are large: only those of the last frame stay. self.pixel_fx.retain(|_, e| e.stamp == g); if !inks { @@ -911,7 +920,10 @@ impl Renderer { if vectorcraft_doc::live::is_live(a) || effects::has_pathfinder(a) { return self.draw_live(ctx, f, a); } + // Its live-effect geometry depends on the node alone, so it can be kept across frames. + let outer = self.fx_owner.replace(a.clone()); self.draw_node(ctx, f, a, true); + self.fx_owner = outer; } fn draw_node(&mut self, ctx: &mut RenderContext, f: &Frame, n: &Node, force: bool) { From f8064ba3ce94d5b177032e5c309ac9225d7cb283 Mon Sep 17 00:00:00 2001 From: arhammusheer Date: Sun, 11 Oct 2026 16:36:15 +0530 Subject: [PATCH 3/8] #1172: rasterize with up to 8 threads instead of 4 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The automatic rasterizer thread count was one per core up to 4. On a 10-core Apple M2 Pro, 8 workers are faster (9 are slower again), and vello_cpu draws the same pixels with any number: - fit page, 50,000 paths: 66.7 → 56.6 ms; after a pan 66.4 → 51.2 ms - 400 % zoom, 50,000 paths: 17.8 → 15.7 ms (back under its 16 ms budget) - fit page, 100,000 paths: 137 → 105 ms; 400 % zoom 33.0 → 25.8 ms Machines with 5 cores or fewer keep their count, and Preferences → Performance and VECTORCRAFT_RENDER_THREADS still override it. pixels_do_not_depend_on_the_thread_count renders a page of live effects and 400 overlapping translucent objects with 0, 1, 4 and 8 workers and gets the same pixels. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01VmMmc1J8XeNLxhRihWv9cc --- crates/render/src/fx.rs | 29 +++++++++++++++++++++++++++++ crates/render/src/lib.rs | 6 ++++-- 2 files changed, 33 insertions(+), 2 deletions(-) diff --git a/crates/render/src/fx.rs b/crates/render/src/fx.rs index cc79f6a3..b8098790 100644 --- a/crates/render/src/fx.rs +++ b/crates/render/src/fx.rs @@ -899,6 +899,35 @@ mod tests { assert!(edited.pixels == Renderer::new().render(&e, 400, 400, view, &opts).pixels, "and matches a fresh render"); } + /// The multithreaded rasterizer draws the same pixels with any number of workers, so the + /// automatic thread count can follow the machine. + #[test] + fn pixels_do_not_depend_on_the_thread_count() { + let mut d = effects_page(); + // Overlapping translucent and blended objects on top of the effects. + let l = d.layers[0].id; + for i in 0..400 { + let id = d.alloc_id(); + let (x, y) = ((i * 37 % 190) as f64, (i * 53 % 190) as f64); + let mut n = Node::path( + id, + shapes::ellipse(Rect::new(x, y, x + 14.0, y + 9.0)), + Appearance::basic(Paint::solid(Color::rgb(0.9, 0.3, 0.1)), Paint::solid(Color::BLACK), 0.7), + ); + n.opacity = 0.6; + d.insert(Some(l), usize::MAX, n).unwrap(); + } + let opts = RenderOptions { background: Some([255, 255, 255, 255]), ..Default::default() }; + let mut frames = vec![]; + for threads in [Some(0), Some(1), Some(4), Some(8)] { + crate::set_default_threads(threads); + let mut r = Renderer::new(); + frames.push(r.render(&d, 1200, 900, Affine::scale(6.0), &opts).pixels); + } + crate::set_default_threads(None); + assert!(frames.windows(2).all(|w| w[0] == w[1]), "the thread count changed pixels"); + } + #[test] fn thumbnails_of_glowing_objects_render_without_panicking() { // Regression: thumbnails used vello's default (multithreaded) context, which panics on filters. diff --git a/crates/render/src/lib.rs b/crates/render/src/lib.rs index 50d11558..36bc4b06 100644 --- a/crates/render/src/lib.rs +++ b/crates/render/src/lib.rs @@ -1767,7 +1767,9 @@ pub fn set_default_threads(n: Option) { THREADS_OVERRIDE.store(n.map_or(-1, i32::from), std::sync::atomic::Ordering::Relaxed); } -/// Rasterizer worker threads: 0 on wasm; otherwise up to 4 (vello's sweet spot), leaving a core free. +/// Rasterizer worker threads: 0 on wasm; otherwise one per core up to 8, leaving a core free. On a +/// 10-core machine 8 workers render a 50,000-path page 1.4× faster than 4 (9 are slower again); +/// the pixels don't depend on the count (`pixels_do_not_depend_on_the_thread_count`). pub fn default_threads() -> u16 { #[cfg(target_arch = "wasm32")] { @@ -1782,7 +1784,7 @@ pub fn default_threads() -> u16 { if std::env::var_os("VECTORCRAFT_RENDER_THREADS").is_some() { return std::env::var("VECTORCRAFT_RENDER_THREADS").ok().and_then(|v| v.parse().ok()).unwrap_or(0); } - std::thread::available_parallelism().map(|n| (n.get().saturating_sub(1)).min(4) as u16).unwrap_or(0) + std::thread::available_parallelism().map(|n| (n.get().saturating_sub(1)).min(8) as u16).unwrap_or(0) } } From 7d79a327482284a647d1a68c0e3c99be46056f20 Mon Sep 17 00:00:00 2001 From: arhammusheer Date: Sun, 11 Oct 2026 17:02:40 +0530 Subject: [PATCH 4/8] #1172: look nodes up by id through a checked cache of their tree paths MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A lookup by id walked the tree, so every edit, click and pointer move of a single object cost time in proportion to the whole document. At 100,000 paths: - hit test, per click: 1.85 → 0.35 ms - handle drag, 5,000-anchor path, per move: 6.5 → 2.6 ms - segment drag, per move: 5.8 → 2.7 ms Document keeps every node's index path in a cache (id_index, never saved). The tree is edited through public fields in many places, so the cache isn't kept in step with it; it is checked instead. A cached path is used only while the node at that path still has the id, a check as deep as the tree. A path that doesn't check out rebuilds the cache, and an id it doesn't have walks, as before. A rebuild costs a few walks, so it happens only after a few such misses, which keeps any edit pattern within a small factor of the walk. Ids that appear more than once are never cached, so they keep their first place in tree order. index_paths uses the cache for small batches and one walk for large ones. The one case that answers differently from a walk: a second node with an existing id put in the tree after the cache was built, earlier in tree order. The lookup then returns the original node, also one with that id. The app doesn't make such nodes; damaged files with duplicates are cached afresh when opened. cached_lookups_follow_any_edit_of_the_tree checks index_path, node and index_paths against a walk after inserting at the front, removing, swapping, moving between layers, deleting a layer, and in a clone sharing the cache. Using cached paths without the check fails it. The doc and engine suites (2,002 tests) pass. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01VmMmc1J8XeNLxhRihWv9cc --- crates/doc/src/id_index.rs | 85 +++++++++++++++++++++++++++++++++ crates/doc/src/lib.rs | 65 ++++++++++++++++++------- crates/doc/src/tests_lookups.rs | 76 +++++++++++++++++++++++++++++ 3 files changed, 209 insertions(+), 17 deletions(-) create mode 100644 crates/doc/src/id_index.rs diff --git a/crates/doc/src/id_index.rs b/crates/doc/src/id_index.rs new file mode 100644 index 00000000..da36e952 --- /dev/null +++ b/crates/doc/src/id_index.rs @@ -0,0 +1,85 @@ +//! A cache of every node's index path, so a lookup by id checks one path instead of walking the +//! tree. +//! +//! The tree is edited in many places through public fields, so the cache can't be kept in step +//! with it. It is checked instead: a cached path is used only while the node at that path still +//! has the id (a check as deep as the tree). A path that no longer checks out rebuilds the cache, +//! and an id the cache doesn't have (deleted, or added since) is looked up by walking, as before. +//! An id that appears more than once (a damaged file) is never cached, so lookups keep returning +//! its first place in tree order. +//! +//! The one case a cached lookup can answer differently from a walk: a second node with an +//! existing id put in the tree *after* the cache was built, earlier in tree order than the first. +//! The lookup then returns the original node (also one with that id) instead of the new one. The +//! app never makes such a node (paste and duplicate give new ids, undo restores whole trees); +//! duplicated ids come from damaged files, which are cached afresh when opened. + +use std::collections::HashMap; +use std::sync::{Arc, RwLock}; + +use crate::{Node, NodeId}; + +/// Each id's index path; `None` for an id that appears more than once. +pub(crate) type Paths = HashMap>>; + +/// Ids found by walking because the cache didn't have them, before the cache is rebuilt: each +/// rebuild costs a few walks, so rebuilding only after a few keeps a lookup's cost within a small +/// factor of a walk whatever the edit pattern (say, add an object, look it up, again and again). +const MISSES_PER_REBUILD: u32 = 4; + +/// The cache, shared by clones of a document (each checks every path it uses against its own tree). +#[derive(Default)] +pub struct IdIndex(RwLock>>, std::sync::atomic::AtomicU32); + +impl IdIndex { + /// The current cache, if one was built. + pub(crate) fn get(&self) -> Option> { + self.0.read().ok().and_then(|g| g.clone()) + } + + /// Rebuild the cache from `layers` (one walk) and return it. + pub(crate) fn rebuild(&self, layers: &[Arc]) -> Arc { + fn walk(nodes: &[Arc], path: &mut Vec, out: &mut Paths) { + for (i, n) in nodes.iter().enumerate() { + path.push(i); + out.entry(n.id).and_modify(|e| *e = None).or_insert_with(|| Some(path.clone())); + if let Some(ch) = n.children() { + walk(ch, path, out); + } + path.pop(); + } + } + let mut out = Paths::new(); + walk(layers, &mut Vec::new(), &mut out); + let out = Arc::new(out); + if let Ok(mut g) = self.0.write() { + *g = Some(out.clone()); + } + self.1.store(0, std::sync::atomic::Ordering::Relaxed); + out + } + + /// Note an id found by walking that the cache didn't have; true when it's time to rebuild. + pub(crate) fn missed(&self) -> bool { + self.1.fetch_add(1, std::sync::atomic::Ordering::Relaxed) + 1 >= MISSES_PER_REBUILD + } +} + +impl Clone for IdIndex { + fn clone(&self) -> Self { + IdIndex(RwLock::new(self.get()), Default::default()) + } +} + +/// A cache, not document content: two documents are equal whatever their caches hold. +impl PartialEq for IdIndex { + fn eq(&self, _: &Self) -> bool { + true + } +} + +impl std::fmt::Debug for IdIndex { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.write_str("IdIndex") + } +} diff --git a/crates/doc/src/lib.rs b/crates/doc/src/lib.rs index 7825de13..6e529106 100644 --- a/crates/doc/src/lib.rs +++ b/crates/doc/src/lib.rs @@ -13,6 +13,7 @@ pub mod cmyk; pub mod corners; pub mod graph; pub mod hit; +mod id_index; pub mod inks; mod inline; pub use inline::{SYMBOL_HALF, SYMBOL_SIZES}; @@ -658,6 +659,9 @@ pub struct Document { /// state, never saved. #[serde(skip)] pub puppet: Option>, + /// Where each node is in the tree, for lookups by id ([`id_index`]): a cache, never saved. + #[serde(skip)] + index: id_index::IdIndex, } fn ppi72() -> f64 { @@ -729,6 +733,7 @@ impl Document { print_setup: None, assets: vec![], puppet: None, + index: Default::default(), }; let id = d.alloc_id(); d.layers.push(Arc::new(Node::layer(id, "Layer 1", LayerColor::Preset(0)))); @@ -758,24 +763,37 @@ impl Document { /// Find a node anywhere in the tree. pub fn node(&self, id: NodeId) -> Option<&Node> { - fn find(nodes: &[Arc], id: NodeId) -> Option<&Node> { - for n in nodes { - if n.id == id { - return Some(n); - } - if let Some(ch) = n.children() - && let Some(f) = find(ch, id) - { - return Some(f); - } - } - None - } - find(&self.layers, id) + self.index_path(id).and_then(|p| self.node_at(&p)) } - /// Index path from the top-level layer list to `id` (e.g. `[0, 3, 1]`). + /// Index path from the top-level layer list to `id` (e.g. `[0, 3, 1]`): the first place in + /// tree order where `id` appears. pub fn index_path(&self, id: NodeId) -> Option> { + // The cached path, while the node there still has the id ([`id_index`]). + let cached = self.index.get(); + match cached.as_ref().and_then(|m| m.get(&id)) { + Some(Some(p)) if self.node_at(p).is_some_and(|n| n.id == id) => return Some(p.clone()), + Some(None) => return self.walk_index_path(id), + Some(Some(_)) => { + // Stale: the tree changed. Rebuild, then answer from it (what a walk gives now). + return match self.index.rebuild(&self.layers).get(&id) { + Some(Some(p)) => Some(p.clone()), + Some(None) => self.walk_index_path(id), + None => None, + }; + } + None => {} + } + // Not cached: deleted, added since, or no cache yet. + let found = self.walk_index_path(id); + if cached.is_none() || (found.is_some() && self.index.missed()) { + self.index.rebuild(&self.layers); + } + found + } + + /// [`Document::index_path`] by walking the tree. + fn walk_index_path(&self, id: NodeId) -> Option> { fn find(nodes: &[Arc], id: NodeId, path: &mut Vec) -> bool { for (i, n) in nodes.iter().enumerate() { path.push(i); @@ -818,10 +836,23 @@ impl Document { } false } - let want: HashSet = ids.into_iter().collect(); + let mut want: HashSet = ids.into_iter().collect(); let mut out = HashMap::with_capacity(want.len()); + // Cached paths that still check out ([`id_index`]); the rest take one walk. For a good + // share of the document, one walk is quicker than checking each cached path. + if let Some(cache) = self.index.get().filter(|c| want.len().saturating_mul(8) < c.len()) { + want.retain(|id| match cache.get(id) { + Some(Some(p)) if self.node_at(p).is_some_and(|n| n.id == *id) => { + out.insert(*id, p.clone()); + false + } + _ => true, + }); + } if !want.is_empty() { - walk(&self.layers, &want, &mut Vec::new(), &mut out); + let mut rest = HashMap::with_capacity(want.len()); + walk(&self.layers, &want, &mut Vec::new(), &mut rest); + out.extend(rest); } out } diff --git a/crates/doc/src/tests_lookups.rs b/crates/doc/src/tests_lookups.rs index 3dff6215..e42d2468 100644 --- a/crates/doc/src/tests_lookups.rs +++ b/crates/doc/src/tests_lookups.rs @@ -78,3 +78,79 @@ fn batched_lookups_match_one_at_a_time() { assert_eq!(serde_json::to_string(&a).ok(), serde_json::to_string(&b).ok(), "seed {seed}"); } } + +/// What a walk of the tree gives for every id, checked against the cached lookups. +fn agree(d: &Document, ids: &[NodeId], what: &str) { + agree_except(d, ids, None, what); +} + +/// [`agree`], except for `dup`, an id given a second node after the cache was built: lookups of it +/// may return either of its nodes (see `id_index`), but always one with that id. +fn agree_except(d: &Document, ids: &[NodeId], dup: Option, what: &str) { + if let Some(dup) = dup { + assert_eq!(d.node(dup).map(|n| n.id), Some(dup), "{what}: node {dup:?} has its id"); + } + let ids: Vec = ids.iter().copied().filter(|id| Some(*id) != dup).collect(); + let ids = &ids[..]; + for id in ids { + let walked = d.walk_index_path(*id); + assert_eq!(d.index_path(*id), walked, "{what}: index_path {id:?}"); + let by_walk = walked.as_deref().and_then(|p| d.node_at(p)).map(|n| n as *const Node); + assert_eq!(d.node(*id).map(|n| n as *const Node), by_walk, "{what}: node {id:?}"); + } + let batched = d.index_paths(ids.iter().copied()); + for id in ids { + assert_eq!(batched.get(id).cloned(), d.walk_index_path(*id), "{what}: index_paths {id:?}"); + } +} + +/// The id cache never answers differently from a walk, however the tree is changed behind its +/// back (through the public fields), including in a clone that shares the cache. +#[test] +fn cached_lookups_follow_any_edit_of_the_tree() { + for seed in 1..30 { + let (mut d, ids) = tree(seed); + agree(&d, &ids, "fresh"); + let snapshot = d.clone(); // shares the cache + // Insert at the front of the first layer: every path in it shifts. + let extra = d.alloc_id(); + let leaf = Arc::new(Node::path(extra, shapes::rectangle(Rect::new(0.0, 0.0, 1.0, 1.0)), Appearance::default_art())); + if let Some(ch) = Arc::make_mut(&mut d.layers[0]).children_mut() { + ch.insert(0, leaf.clone()); + } + let mut all = ids.clone(); + all.push(extra); + agree(&d, &all, "inserted at the front"); + // Remove a child, swap two, and move one into another layer. + if let Some(ch) = Arc::make_mut(&mut d.layers[1]).children_mut() + && ch.len() > 3 + { + ch.remove(1); + ch.swap(0, 2); + } + let moved = Arc::make_mut(&mut d.layers[2]).children_mut().and_then(|ch| (!ch.is_empty()).then(|| ch.remove(0))); + if let Some(m) = moved + && let Some(ch) = Arc::make_mut(&mut d.layers[0]).children_mut() + { + ch.push(m); + } + agree(&d, &all, "removed, swapped and moved"); + // A new duplicate of an existing id, earlier in tree order than the original. + let dup = ids[ids.len() / 3]; + let copy = Arc::new(Node::path(dup, shapes::rectangle(Rect::new(0.0, 0.0, 2.0, 2.0)), Appearance::default_art())); + if let Some(ch) = Arc::make_mut(&mut d.layers[0]).children_mut() { + ch.insert(0, copy); + } + agree_except(&d, &all, Some(dup), "a duplicate inserted earlier"); + // Delete a whole layer, then look everything up again, missing ids included. + d.layers.remove(1); + agree_except(&d, &all, Some(dup), "a layer deleted"); + // A rebuilt cache (as after loading a file) knows the duplicate and walks for it. + let mut fresh = d.clone(); + fresh.layers = d.layers.clone(); + fresh.index = Default::default(); + agree(&fresh, &all, "a damaged tree, cached afresh"); + // The clone made before all this still answers for its own tree. + agree(&snapshot, &ids, "the earlier clone"); + } +} From 2bb7646a39515bfb6522a315422fd95941eab1be Mon Sep 17 00:00:00 2001 From: arhammusheer Date: Sun, 11 Oct 2026 17:56:48 +0530 Subject: [PATCH 5/8] #1172: make the id cache cheap to rebuild, and walk on a stale path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous commit rebuilt the cache as soon as a cached path went stale, and built it as one Vec path per node, hashed with SipHash. An edit that removes many objects one by one (Pathfinder Unite of 1,000 paths) went stale on every removal, and Unite took 105 ms instead of 42 ms. - The cache is now one flat table of (parent entry, index among siblings) with a one-multiply id hasher, so a rebuild is about one walk and allocates nothing per node. - A stale path walks, as an id the cache doesn't have does, and rebuilds only after a few. Either way a lookup stays within a small factor of a walk. At 1,000 paths, Unite takes 45 ms against main's 42. At 100,000 paths the gains of the cache hold: hit test 1.7 → 0.36 ms, handle drag 6.5 → 2.7 ms per move. The cached-lookup tests pass unchanged. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01VmMmc1J8XeNLxhRihWv9cc --- crates/doc/src/id_index.rs | 83 ++++++++++++++++++++++++++++++++------ crates/doc/src/lib.rs | 22 ++++------ 2 files changed, 78 insertions(+), 27 deletions(-) diff --git a/crates/doc/src/id_index.rs b/crates/doc/src/id_index.rs index da36e952..cdb32c32 100644 --- a/crates/doc/src/id_index.rs +++ b/crates/doc/src/id_index.rs @@ -15,16 +15,72 @@ //! duplicated ids come from damaged files, which are cached afresh when opened. use std::collections::HashMap; +use std::hash::{BuildHasherDefault, Hasher}; use std::sync::{Arc, RwLock}; use crate::{Node, NodeId}; -/// Each id's index path; `None` for an id that appears more than once. -pub(crate) type Paths = HashMap>>; +/// Hasher for node ids: one multiply. Ids are small counters, not attacker-chosen keys, and the +/// default SipHash dominated the cost of building the cache. +#[derive(Default)] +pub struct IdHasher(u64); -/// Ids found by walking because the cache didn't have them, before the cache is rebuilt: each -/// rebuild costs a few walks, so rebuilding only after a few keeps a lookup's cost within a small -/// factor of a walk whatever the edit pattern (say, add an object, look it up, again and again). +impl Hasher for IdHasher { + fn write(&mut self, bytes: &[u8]) { + for &b in bytes { + self.0 = (self.0 ^ u64::from(b)).wrapping_mul(0x100_0000_01b3); + } + } + fn write_u64(&mut self, v: u64) { + self.0 = (self.0 ^ v).wrapping_mul(0x9e37_79b9_7f4a_7c15); + } + fn finish(&self) -> u64 { + self.0 ^ (self.0 >> 29) + } +} + +/// A map keyed by node id with [`IdHasher`]. +pub type IdMap = HashMap>; +/// Where each node is: its parent's entry (`u32::MAX` for a top-level layer) and its index among +/// its siblings. One flat table, so building it allocates nothing per node. +#[derive(Default)] +pub(crate) struct Paths { + /// Each id's entry in `at`; `None` for an id that appears more than once. + ids: IdMap>, + at: Vec<(u32, u32)>, +} + +impl Paths { + /// The index path of `id`: `Some(None)` when the id appears more than once, `None` when it + /// isn't in the table. + pub(crate) fn get(&self, id: &NodeId) -> Option>> { + let e = (*self.ids.get(id)?)?; + let mut path = Vec::new(); + let mut cur = e; + // Each step goes to a parent entry, made before its children: at most `at.len()` steps. + for _ in 0..=self.at.len() { + let Some(&(parent, i)) = self.at.get(cur as usize) else { return Some(None) }; + path.push(i as usize); + if parent == u32::MAX { + path.reverse(); + return Some(Some(path)); + } + cur = parent; + } + Some(None) + } + pub(crate) fn contains_key(&self, id: &NodeId) -> bool { + self.ids.contains_key(id) + } + pub(crate) fn len(&self) -> usize { + self.ids.len() + } +} + +/// Lookups that had to walk (the cache didn't have the id, or its path no longer checks out) +/// before the cache is rebuilt: a rebuild costs a few walks, so rebuilding only after a few keeps +/// a lookup's cost within a small factor of a walk whatever the edit pattern (say, delete an +/// object and look the next one up, again and again). const MISSES_PER_REBUILD: u32 = 4; /// The cache, shared by clones of a document (each checks every path it uses against its own tree). @@ -39,18 +95,18 @@ impl IdIndex { /// Rebuild the cache from `layers` (one walk) and return it. pub(crate) fn rebuild(&self, layers: &[Arc]) -> Arc { - fn walk(nodes: &[Arc], path: &mut Vec, out: &mut Paths) { + fn walk(nodes: &[Arc], parent: u32, out: &mut Paths) { for (i, n) in nodes.iter().enumerate() { - path.push(i); - out.entry(n.id).and_modify(|e| *e = None).or_insert_with(|| Some(path.clone())); + let e = out.at.len() as u32; + out.at.push((parent, i as u32)); + out.ids.entry(n.id).and_modify(|x| *x = None).or_insert(Some(e)); if let Some(ch) = n.children() { - walk(ch, path, out); + walk(ch, e, out); } - path.pop(); } } - let mut out = Paths::new(); - walk(layers, &mut Vec::new(), &mut out); + let mut out = Paths::default(); + walk(layers, u32::MAX, &mut out); let out = Arc::new(out); if let Ok(mut g) = self.0.write() { *g = Some(out.clone()); @@ -59,7 +115,8 @@ impl IdIndex { out } - /// Note an id found by walking that the cache didn't have; true when it's time to rebuild. + /// Note a lookup that had to walk (an id the cache didn't have, or a path that no longer + /// checks out); true when it's time to rebuild. pub(crate) fn missed(&self) -> bool { self.1.fetch_add(1, std::sync::atomic::Ordering::Relaxed) + 1 >= MISSES_PER_REBUILD } diff --git a/crates/doc/src/lib.rs b/crates/doc/src/lib.rs index 6e529106..831d67ea 100644 --- a/crates/doc/src/lib.rs +++ b/crates/doc/src/lib.rs @@ -772,21 +772,15 @@ impl Document { // The cached path, while the node there still has the id ([`id_index`]). let cached = self.index.get(); match cached.as_ref().and_then(|m| m.get(&id)) { - Some(Some(p)) if self.node_at(p).is_some_and(|n| n.id == id) => return Some(p.clone()), + Some(Some(p)) if self.node_at(&p).is_some_and(|n| n.id == id) => return Some(p), Some(None) => return self.walk_index_path(id), - Some(Some(_)) => { - // Stale: the tree changed. Rebuild, then answer from it (what a walk gives now). - return match self.index.rebuild(&self.layers).get(&id) { - Some(Some(p)) => Some(p.clone()), - Some(None) => self.walk_index_path(id), - None => None, - }; - } - None => {} + _ => {} } - // Not cached: deleted, added since, or no cache yet. + // A stale path (the tree changed), an id the cache doesn't have (deleted or added since), + // or no cache yet: walk, and rebuild now and then. let found = self.walk_index_path(id); - if cached.is_none() || (found.is_some() && self.index.missed()) { + let stale = cached.as_ref().is_some_and(|m| m.contains_key(&id)); + if cached.is_none() || ((stale || found.is_some()) && self.index.missed()) { self.index.rebuild(&self.layers); } found @@ -842,8 +836,8 @@ impl Document { // share of the document, one walk is quicker than checking each cached path. if let Some(cache) = self.index.get().filter(|c| want.len().saturating_mul(8) < c.len()) { want.retain(|id| match cache.get(id) { - Some(Some(p)) if self.node_at(p).is_some_and(|n| n.id == *id) => { - out.insert(*id, p.clone()); + Some(Some(p)) if self.node_at(&p).is_some_and(|n| n.id == *id) => { + out.insert(*id, p); false } _ => true, From 4c5809302ddb2510ad5a79fbd3c1504a85078381 Mon Sep 17 00:00:00 2001 From: arhammusheer Date: Sun, 11 Oct 2026 17:56:48 +0530 Subject: [PATCH 6/8] #1172: Pathfinder Trim, Merge and Crop 9-12x faster, same output MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Trim, Merge and Crop walk the arrangement once per input shape, and every walk tested each edge side with a coverage mask built from a winding vector with one slot per input. On 300 paths, against main: - Trim: 362 → 31 ms - Merge: 366 → 31 ms - Crop: 303 → 28 ms - Unite, Intersect, Exclude, Minus Front and Minus Back: unchanged or a little faster - Multi, the arrangement's winding, keeps only the non-zero windings as sorted (input, winding) pairs, added in place. A face is covered by a few inputs, so a test reads a few entries instead of one per input. The winding arithmetic is unchanged. - Arrangement::covered lists the inputs that cover a face (the true entries of its coverage mask). Trim, Merge, Crop, Minus Front, Minus Back, Intersect and Exclude test that list; each test's old mask form is in a comment beside it. mask() still builds the same mask for the rest. pathfinder_pinned hashes the output of all ten operations on 2, 5, 12 and 30 overlapping ellipses, rectangles and stars (even-odd and non-zero, shared keys for Merge). The 40 hashes were computed with main's dense windings, and the new code gives every one of them. A wrong Crop test fails it. The pathops suite (103 tests) passes. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01VmMmc1J8XeNLxhRihWv9cc --- crates/pathops/src/boolean.rs | 70 +++++++++++------ crates/pathops/src/pathfinder.rs | 34 +++++--- crates/pathops/tests/pathfinder_pinned.rs | 94 +++++++++++++++++++++++ 3 files changed, 165 insertions(+), 33 deletions(-) create mode 100644 crates/pathops/tests/pathfinder_pinned.rs diff --git a/crates/pathops/src/boolean.rs b/crates/pathops/src/boolean.rs index d3143c8c..79f2e532 100644 --- a/crates/pathops/src/boolean.rs +++ b/crates/pathops/src/boolean.rs @@ -50,24 +50,22 @@ pub(crate) fn inside(rule: FillRule, w: i32) -> bool { } } -/// Winding numbers for an arbitrary number of input sets (tag = input index). -#[derive(Clone, Debug, Default)] -pub(crate) struct Multi(pub(crate) Vec); +/// Winding numbers for an arbitrary number of input sets (tag = input index): the non-zero ones, +/// as (tag, winding) sorted by tag. A face is covered by a few of the inputs, so this stays short +/// where a vector of every input's winding would make each Pathfinder test cost one step per input. +#[derive(Clone, Debug, Default, PartialEq, Eq)] +pub(crate) struct Multi(Vec<(usize, i32)>); impl Multi { fn get(&self, i: usize) -> i32 { - self.0.get(i).copied().unwrap_or(0) + self.0.binary_search_by_key(&i, |e| e.0).ok().and_then(|k| self.0.get(k)).map_or(0, |e| e.1) + } + /// The non-zero windings, by input. + pub(crate) fn iter(&self) -> impl DoubleEndedIterator + '_ { + self.0.iter().copied() } } -impl PartialEq for Multi { - fn eq(&self, other: &Self) -> bool { - let n = self.0.len().max(other.0.len()); - (0..n).all(|i| self.get(i) == other.get(i)) - } -} -impl Eq for Multi {} - impl std::ops::Add for Multi { type Output = Multi; fn add(mut self, rhs: Self) -> Multi { @@ -78,11 +76,21 @@ impl std::ops::Add for Multi { impl std::ops::AddAssign for Multi { fn add_assign(&mut self, rhs: Self) { - if self.0.len() < rhs.0.len() { - self.0.resize(rhs.0.len(), 0); - } - for (a, b) in self.0.iter_mut().zip(rhs.0) { - *a += b; + // In place, input by input (the sweep mostly adds one input's ±1 to a short list): add to + // the input's winding, or insert it in tag order, and drop it if it reaches zero. + for (t, k) in rhs.0 { + match self.0.binary_search_by_key(&t, |e| e.0) { + Ok(i) => { + if let Some(e) = self.0.get_mut(i) { + e.1 += k; + if e.1 == 0 { + self.0.remove(i); + } + } + } + Err(i) if k != 0 => self.0.insert(i, (t, k)), + Err(_) => {} + } } } } @@ -90,14 +98,11 @@ impl std::ops::AddAssign for Multi { impl WindingNumber for Multi { type Tag = usize; fn single(tag: usize, positive: bool) -> Self { - let mut v = vec![0; tag + 1]; - v[tag] = if positive { 1 } else { -1 }; - Multi(v) + Multi(vec![(tag, if positive { 1 } else { -1 })]) } fn of_tag(&self, tag: usize) -> Self { - let mut v = vec![0; tag + 1]; - v[tag] = self.get(tag); - Multi(v) + let w = self.get(tag); + Multi(if w == 0 { vec![] } else { vec![(tag, w)] }) } } @@ -288,7 +293,24 @@ impl Arrangement { } pub(crate) fn mask(&self, w: &Multi) -> Vec { - self.rules.iter().enumerate().map(|(i, &r)| inside(r, w.get(i))).collect() + // A zero winding is outside under both rules, so only the non-zero ones can set a flag. + let mut m = vec![false; self.rules.len()]; + for (i, k) in w.iter() { + if let (Some(&r), Some(b)) = (self.rules.get(i), m.get_mut(i)) { + *b = inside(r, k); + } + } + m + } + + /// The inputs that cover a face of winding `w` (`mask(w)`'s true indices), back to front. + pub(crate) fn covered<'a>(&'a self, w: &'a Multi) -> impl DoubleEndedIterator + 'a { + w.iter().filter(|&(i, k)| self.rules.get(i).is_some_and(|&r| inside(r, k))).map(|(i, _)| i) + } + + /// [`Arrangement::contours`] with the test on the windings themselves (see [`Self::covered`]). + pub(crate) fn contours_where(&self, pred: impl Fn(&Multi) -> bool) -> Contours { + self.top.contours(pred) } pub(crate) fn contours(&self, pred: impl Fn(&[bool]) -> bool) -> Contours { diff --git a/crates/pathops/src/pathfinder.rs b/crates/pathops/src/pathfinder.rs index 123fb2c5..1bd65c8f 100644 --- a/crates/pathops/src/pathfinder.rs +++ b/crates/pathops/src/pathfinder.rs @@ -75,8 +75,10 @@ fn arrangement(shapes: &[Shape]) -> Option { Arrangement::new(bps).ok() } -fn topmost(m: &[bool]) -> Option { - m.iter().rposition(|&b| b) +/// The one item of `it`, if it has exactly one. +fn only(mut it: impl Iterator) -> Option { + let first = it.next()?; + it.next().is_none().then_some(first) } fn one(path: PathData, key: u64) -> Vec { @@ -102,10 +104,16 @@ pub fn pathfinder(op: PathfinderOp, shapes: &[Shape]) -> Vec { let p = &arr.tidy; match op { PathfinderOp::Unite => unite(), - PathfinderOp::MinusFront => one(all_contours_to_path(&arr.contours(|m| m[0] && !m[1..].iter().any(|&b| b)), p), shapes[0].key), - PathfinderOp::MinusBack => one(all_contours_to_path(&arr.contours(|m| m[n - 1] && !m[..n - 1].iter().any(|&b| b)), p), front_key), - PathfinderOp::Intersect => one(all_contours_to_path(&arr.contours(|m| m.iter().all(|&b| b)), p), front_key), - PathfinderOp::Exclude => one(all_contours_to_path(&arr.contours(|m| m.iter().filter(|&&b| b).count() % 2 == 1), p), front_key), + // The tests read the covering inputs straight off the windings ([`Arrangement::covered`]); + // each is the coverage-mask test in its comment. + // m[0] && !m[1..].any() + PathfinderOp::MinusFront => one(all_contours_to_path(&arr.contours_where(|w| only(arr.covered(w)) == Some(0)), p), shapes[0].key), + // m[n - 1] && !m[..n - 1].any() + PathfinderOp::MinusBack => one(all_contours_to_path(&arr.contours_where(|w| only(arr.covered(w)) == Some(n - 1)), p), front_key), + // m.all() + PathfinderOp::Intersect => one(all_contours_to_path(&arr.contours_where(|w| arr.covered(w).count() == n), p), front_key), + // m.count() is odd + PathfinderOp::Exclude => one(all_contours_to_path(&arr.contours_where(|w| arr.covered(w).count() % 2 == 1), p), front_key), PathfinderOp::Divide => regions_of(&arr) .into_iter() .filter_map(|r| { @@ -113,7 +121,10 @@ pub fn pathfinder(op: PathfinderOp, shapes: &[Shape]) -> Vec { Some(Shape::new(r.path, FillRule::NonZero, key)) }) .collect(), - PathfinderOp::Trim => (0..n).flat_map(|i| one(all_contours_to_path(&arr.contours(|m| topmost(m) == Some(i)), p), shapes[i].key)).collect(), + // topmost(m) == Some(i) + PathfinderOp::Trim => { + (0..n).flat_map(|i| one(all_contours_to_path(&arr.contours_where(|w| arr.covered(w).next_back() == Some(i)), p), shapes[i].key)).collect() + } PathfinderOp::Merge => { let mut keys: Vec = Vec::new(); for s in shapes { @@ -123,7 +134,8 @@ pub fn pathfinder(op: PathfinderOp, shapes: &[Shape]) -> Vec { } let mut out = Vec::new(); for k in keys { - let c = arr.contours(|m| topmost(m).is_some_and(|t| shapes[t].key == k)); + // topmost(m) is a shape with key k + let c = arr.contours_where(|w| arr.covered(w).next_back().is_some_and(|t| shapes[t].key == k)); for g in c.grouped() { out.extend(one(contours_to_path(&c, g, p), k)); } @@ -136,7 +148,11 @@ pub fn pathfinder(op: PathfinderOp, shapes: &[Shape]) -> Vec { } (0..n - 1) .flat_map(|i| { - let c = arr.contours(|m| m[n - 1] && topmost(&m[..n - 1]) == Some(i)); + // m[n - 1] && topmost(&m[..n - 1]) == Some(i): the front shape, then i below it + let c = arr.contours_where(|w| { + let mut c = arr.covered(w).rev(); + c.next() == Some(n - 1) && c.next() == Some(i) + }); one(all_contours_to_path(&c, p), shapes[i].key) }) .collect() diff --git a/crates/pathops/tests/pathfinder_pinned.rs b/crates/pathops/tests/pathfinder_pinned.rs new file mode 100644 index 00000000..b23c1499 --- /dev/null +++ b/crates/pathops/tests/pathfinder_pinned.rs @@ -0,0 +1,94 @@ +//! The exact output of every Pathfinder operation, pinned so that work on the windings and the +//! coverage tests can't change it: overlapping ellipses, rectangles and stars (even-odd and +//! non-zero, repeated keys for Merge) at a few counts. +#![allow(clippy::unwrap_used, clippy::expect_used, clippy::panic)] + +use vectorcraft_geom::{FillRule, Point, Rect, shapes}; +use vectorcraft_pathops::{PathfinderOp, Shape, pathfinder}; + +fn shapes_of(n: usize, seed: u64) -> Vec { + let mut r = seed | 1; + let mut next = || { + r ^= r << 13; + r ^= r >> 7; + r ^= r << 17; + (r % 1_000_000) as f64 / 1_000_000.0 + }; + (0..n) + .map(|i| { + let (x, y, s) = (next() * 200.0, next() * 150.0, 15.0 + next() * 40.0); + let path = match i % 3 { + 0 => shapes::ellipse(Rect::from_center_size(Point::new(x, y), (2.0 * s, 1.4 * s))), + 1 => shapes::rectangle(Rect::new(x, y, x + 1.5 * s, y + s)), + _ => shapes::star(Point::new(x, y), s, s / 2.0, 5, 0.3), + }; + let rule = if i % 4 == 3 { FillRule::EvenOdd } else { FillRule::NonZero }; + Shape::new(path, rule, (i % 5) as u64) + }) + .collect() +} + +fn fnv(s: &str) -> u64 { + s.bytes().fold(0xcbf2_9ce4_8422_2325, |h, b| (h ^ u64::from(b)).wrapping_mul(0x100_0000_01b3)) +} + +#[test] +fn every_pathfinder_operation_is_pinned() { + use PathfinderOp::*; + let ops = [Unite, MinusFront, MinusBack, Intersect, Exclude, Divide, Trim, Merge, Crop, Outline]; + let mut got = vec![]; + for (n, seed) in [(2, 3), (5, 7), (12, 11), (30, 13)] { + let input = shapes_of(n, seed); + for op in ops { + let out = pathfinder(op, &input); + let text: String = out.iter().map(|s| format!("{}|{:?}|{:?};", s.key, s.rule, s.path)).collect(); + got.push(fnv(&text)); + } + } + assert_eq!(got.len(), PINNED.len()); + assert_eq!(got, PINNED); +} + +/// Computed with the dense windings (main at b08482d6). +const PINNED: [u64; 40] = [ + 0x5e62_baae_d8b4_dba5, + 0x2bbc_e138_a97c_5a28, + 0xce78_992e_750b_1092, + 0x30cf_cc0b_861c_48b4, + 0xe6e4_2a29_1e16_5e86, + 0x390b_0633_5066_6292, + 0x06b7_3884_f961_c343, + 0x06b7_3884_f961_c343, + 0x0384_d599_5a03_dafd, + 0x3dcd_029a_dd3f_fa38, + 0x8249_71f8_fda4_2bb6, + 0xe7f2_8633_cef9_a76c, + 0x1a11_084e_721e_ac3b, + 0xcbf2_9ce4_8422_2325, + 0xa786_d973_a0d7_ca2a, + 0x389b_c89a_0ccf_578f, + 0xff9d_cf53_d872_681e, + 0xff9d_cf53_d872_681e, + 0xcbf2_9ce4_8422_2325, + 0x6be3_02a6_d9c9_e80e, + 0x3525_cabe_b235_aaef, + 0xf969_aa07_a44d_1770, + 0xed13_60c7_0aaa_7854, + 0xcbf2_9ce4_8422_2325, + 0x6550_a381_db6b_a35f, + 0x1ba9_6dfb_89ea_ac78, + 0x1344_b467_dd20_5f02, + 0x948f_e2a1_d657_0d4c, + 0x49a4_5c3c_2d96_14af, + 0x16a7_ce9e_4a3c_0974, + 0x0b12_c643_a446_da04, + 0xcbf2_9ce4_8422_2325, + 0xeb6a_6614_cea5_a508, + 0xcbf2_9ce4_8422_2325, + 0xb9c4_2803_7db9_bd4f, + 0x34b6_90d5_e443_b799, + 0xdfaf_ecbd_2d58_3227, + 0x6d5e_6e8d_b05b_8688, + 0x59dc_9268_a73f_e92e, + 0x5a83_5cbd_1f8c_5143, +]; From 7652439b4007af0cabc9dffbe58d5e5037666e20 Mon Sep 17 00:00:00 2001 From: arhammusheer Date: Sun, 11 Oct 2026 18:34:50 +0530 Subject: [PATCH 7/8] #1172: Pathfinder Outline 19x, SVG export 1.5x and open 1.2x faster, same output MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit At 50,000 paths unless noted, against main: - Pathfinder Outline (300 paths): 145 → 7.5 ms. Each segment was tested against every junction of the arrangement, and each chain against every chain kept so far. Now a segment tests only the junctions inside its control box (junctions are sorted by x; a point within reach of a curve is within reach of that box), with the same nearest-point test. Coincident chains are found through a grid on their midpoints, taking the first match in the same order a scan does. - export SVG: 267 → 176 ms. Path data is written straight into one string (`push_num`, the same digits and trimming as `fmt_num`) instead of a string per number, point and segment. - open .vectorcraft: 239 → 195 ms. The JSON was parsed once only to find where it ends: - A file with no NUL has no data after it. - Otherwise the marker (which holds the first NUL, as JSON can't) is taken when it directly follows the JSON's last byte, as the writer puts it. - Anything else takes the old parse. `perf --deep` prints output hashes of an SVG export and a save → open → save round trip; main and this branch print the same ones. pathfinder_pinned (Outline included) passes, and so do the pathops, format and svg suites (345 tests). Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01VmMmc1J8XeNLxhRihWv9cc --- apps/vectorcraft-cli/src/perf.rs | 7 +++++ crates/format/src/lib.rs | 13 +++++++++ crates/pathops/src/pathfinder.rs | 47 +++++++++++++++++++++++++------- crates/svg/src/export.rs | 20 ++++++++++---- crates/svg/src/lib.rs | 22 +++++++++++++-- 5 files changed, 91 insertions(+), 18 deletions(-) diff --git a/apps/vectorcraft-cli/src/perf.rs b/apps/vectorcraft-cli/src/perf.rs index afafd6db..9652d4f7 100644 --- a/apps/vectorcraft-cli/src/perf.rs +++ b/apps/vectorcraft-cli/src/perf.rs @@ -207,6 +207,13 @@ fn deep(n: usize) -> Result<(), String> { }), )); note("after typing"); + // Output hashes, to compare builds: the SVG export and a save → open → save round trip. + let fnv = |b: &[u8]| b.iter().fold(0xcbf2_9ce4_8422_2325u64, |h, &x| (h ^ u64::from(x)).wrapping_mul(0x100_0000_01b3)); + let doc = synthetic(n)?; + let svg = vectorcraft_svg::export(&doc, &vectorcraft_svg::ExportOptions::default()); + let saved = vectorcraft_format::save_file(&doc); + let reopened = vectorcraft_format::load(&saved).map_err(|e| e.to_string())?; + outln!(" output hash: SVG export {:016x}, save→open→save {:016x}", fnv(svg.as_bytes()), fnv(&vectorcraft_format::save_file(&reopened))); outln!(" tracked (no budget yet)"); for (name, ms) in &rows { outln!(" {name:<52} {ms:>10.2} ms"); diff --git a/crates/format/src/lib.rs b/crates/format/src/lib.rs index 42ea6f3d..48e82f7c 100644 --- a/crates/format/src/lib.rs +++ b/crates/format/src/lib.rs @@ -145,6 +145,19 @@ fn preview_mime(bytes: &[u8]) -> &'static str { /// The JSON of a file and the data after it (v4), split at [`BLOB_MAGIC`] after the first JSON /// value. fn split_tail(text: &[u8]) -> (&[u8], &[u8]) { + // JSON holds no raw NUL, so the marker (which has one) starts at the first NUL, if the file + // has one. The writer puts it right after the JSON's last byte; found there, it is where the + // first JSON value ends, without parsing the JSON twice. Anything else takes the parse. + let Some(nul) = text.iter().position(|&b| b == 0) else { + // No NUL, so no marker: the parse below would find none either. + return (text, &[]); + }; + if let Some(at) = nul.checked_sub(1) + && text.get(at..).is_some_and(|r| r.starts_with(BLOB_MAGIC)) + && at.checked_sub(1).and_then(|k| text.get(k)).is_some_and(|b| matches!(b, b'}' | b']' | b'"')) + { + return (text.get(..at).unwrap_or(text), text.get(at + BLOB_MAGIC.len()..).unwrap_or_default()); + } let mut values = serde_json::Deserializer::from_slice(text).into_iter::(); if values.next().is_some_and(|v| v.is_ok()) { let end = values.byte_offset(); diff --git a/crates/pathops/src/pathfinder.rs b/crates/pathops/src/pathfinder.rs index 1bd65c8f..c9861266 100644 --- a/crates/pathops/src/pathfinder.rs +++ b/crates/pathops/src/pathfinder.rs @@ -358,9 +358,11 @@ impl FaceMerger { /// Pathfinder Outline: split every shape's boundary at junctions of the arrangement. fn outline_edges(arr: &Arrangement, shapes: &[Shape]) -> Vec { + // Sorted by x (then y), so a segment needs only the junctions inside its control box. let junctions = arr.junctions(); let scale = shapes.iter().filter_map(|s| s.path.control_bounds()).map(|r| r.width().max(r.height())).fold(1.0, f64::max); let tol = scale * 1e-7; + let reach = tol.max(1e-6); // (chain, key, source index) let mut chains: Vec<(Vec, u64)> = Vec::new(); for s in shapes { @@ -375,11 +377,19 @@ fn outline_edges(arr: &Arrangement, shapes: &[Shape]) -> Vec { for i in 0..sp.segment_count() { let c = sp.segment(i); let line = sp.segment_is_line(i); + // A point within `reach` of the curve is within `reach` of its control box (the + // curve stays inside its control points' hull), so the other junctions can't match. + let b = kurbo::Shape::bounding_box(&c).union(kurbo::Rect::from_points(c.p1, c.p2)).inflate(reach, reach); + let lo = junctions.partition_point(|j| j.x < b.x0); let mut ts: Vec = junctions + .get(lo..) + .unwrap_or_default() .iter() + .take_while(|j| j.x <= b.x1) + .filter(|j| j.y >= b.y0 && j.y <= b.y1) .filter_map(|&j| { let nr = c.nearest(j, 1e-9); - (nr.distance_sq.sqrt() <= tol.max(1e-6)).then_some(nr.t) + (nr.distance_sq.sqrt() <= reach).then_some(nr.t) }) .collect(); ts.sort_by(f64::total_cmp); @@ -417,24 +427,41 @@ fn outline_edges(arr: &Arrangement, shapes: &[Shape]) -> Vec { } } } - // De-duplicate coincident chains, keeping the front-most key (later shapes overwrite). + // De-duplicate coincident chains, keeping the front-most key (later shapes overwrite). A + // match has its midpoint within `near` of this one's, so only the chains in the grid cells + // around it are candidates; the first of them in `out` order is the one a scan would find. let mut out: Vec<(Vec, u64)> = Vec::new(); + let mut sigs: Vec<(Point, Point, Point)> = Vec::new(); let sig = |c: &[Seg]| -> (Point, Point, Point) { let a = c[0].c.p0; let b = c[c.len() - 1].c.p3; let mid = mid_point(c); (a, b, mid) }; + let d = reach * 10.0; + let near = |p: Point, q: Point| p.distance(q) <= d; + let cell = |p: Point| ((p.x / d).floor() as i64, (p.y / d).floor() as i64); + let mut grid: HashMap<(i64, i64), Vec> = HashMap::new(); for (chain, key) in chains { let (a, b, m) = sig(&chain); - let near = |p: Point, q: Point| p.distance(q) <= tol.max(1e-6) * 10.0; - if let Some(e) = out.iter_mut().find(|(c, _)| { - let (a2, b2, m2) = sig(c); - near(m, m2) && ((near(a, a2) && near(b, b2)) || (near(a, b2) && near(b, a2))) - }) { - e.1 = key; - } else { - out.push((chain, key)); + let (cx, cy) = cell(m); + let found = (cx - 1..=cx + 1) + .flat_map(|x| (cy - 1..=cy + 1).map(move |y| (x, y))) + .filter_map(|k| grid.get(&k)) + .flatten() + .copied() + .filter(|&i| { + let (a2, b2, m2) = sigs[i]; + near(m, m2) && ((near(a, a2) && near(b, b2)) || (near(a, b2) && near(b, a2))) + }) + .min(); + match found { + Some(i) => out[i].1 = key, + None => { + grid.entry((cx, cy)).or_default().push(out.len()); + sigs.push((a, b, m)); + out.push((chain, key)); + } } } out.into_iter() diff --git a/crates/svg/src/export.rs b/crates/svg/src/export.rs index 2605109e..50e7291a 100644 --- a/crates/svg/src/export.rs +++ b/crates/svg/src/export.rs @@ -656,9 +656,13 @@ impl Writer<'_> { fn path_d(&self, p: &PathData, m: Affine) -> String { let mut s = String::new(); - let pt = |p: Point| { + // Each point as "x y", appended in place (no string per number or per point). + let dec = self.opts.decimals; + let pt = |s: &mut String, p: Point| { let p = m * p; - format!("{} {}", self.num(p.x), self.num(p.y)) + crate::push_num(s, p.x, dec); + s.push(' '); + crate::push_num(s, p.y, dec); }; for sp in &p.subpaths { let n = sp.anchors.len(); @@ -669,16 +673,22 @@ impl Writer<'_> { s.push(' '); } s.push('M'); - s.push_str(&pt(sp.anchors[0].p)); + pt(&mut s, sp.anchors[0].p); for i in 0..sp.segment_count() { let (a, b) = (&sp.anchors[i], &sp.anchors[(i + 1) % n]); if sp.segment_is_line(i) { if sp.closed && i == n - 1 { break; } - s.push_str(&format!(" L{}", pt(b.p))); + s.push_str(" L"); + pt(&mut s, b.p); } else { - s.push_str(&format!(" C{} {} {}", pt(a.h_out), pt(b.h_in), pt(b.p))); + s.push_str(" C"); + pt(&mut s, a.h_out); + s.push(' '); + pt(&mut s, b.h_in); + s.push(' '); + pt(&mut s, b.p); } } if sp.closed { diff --git a/crates/svg/src/lib.rs b/crates/svg/src/lib.rs index 3789d8f4..7cbc8531 100644 --- a/crates/svg/src/lib.rs +++ b/crates/svg/src/lib.rs @@ -567,10 +567,26 @@ pub fn xml_escape(s: &str) -> String { /// Format a number with at most `decimals` places, trimming trailing zeros. pub fn fmt_num(v: f64, decimals: u8) -> String { + let mut s = String::new(); + push_num(&mut s, v, decimals); + s +} + +/// [`fmt_num`] appended to `out`, without allocating. +pub fn push_num(out: &mut String, v: f64, decimals: u8) { + use std::fmt::Write as _; let v = if v.is_finite() { v } else { 0.0 }; - let s = format!("{:.*}", decimals as usize, v); - let s = if s.contains('.') { s.trim_end_matches('0').trim_end_matches('.').to_string() } else { s }; - if s == "-0" || s.is_empty() { "0".into() } else { s } + let start = out.len(); + let _ = write!(out, "{:.*}", decimals as usize, v); // writing to a String can't fail + let s = out.get(start..).unwrap_or_default(); + if s.contains('.') { + let keep = s.trim_end_matches('0').trim_end_matches('.').len(); + out.truncate(start + keep); + } + if matches!(out.get(start..), Some("-0" | "")) { + out.truncate(start); + out.push('0'); + } } #[cfg(test)] From 0724aafbb68d77effe636e9938c451bb316a98d6 Mon Sep 17 00:00:00 2001 From: arhammusheer Date: Sun, 11 Oct 2026 19:14:58 +0530 Subject: [PATCH 8/8] #1172: share an object's appearance with its undo history until it changes Every edit of an object copied its whole appearance (its fills, strokes and effects, about 540 bytes for a basic object) into the new node, though a move leaves it as it is. Over 45 whole-selection moves of 10,000 paths, undo history grew by 589 MB; now 349 MB (41 % less), and a nudge or a drag of the selection takes 12 ms instead of 15. - Node::appearance is a vectorcraft_doc::Shared: copy-on-write. A clone shares it, and the first change through DerefMut copies it (Arc::make_mut). Reads, writes, equality (also against a plain Appearance) and the saved form are those of the Appearance itself, so call sites read and change it as before. Sites that build or assign a whole appearance add .into(), and the few that need a plain copy take (*n.appearance).clone(). - Node::transform changes the appearance only when it has gradients to transform, and pattern paints are transformed only when there are some: a move no longer copies an appearance it doesn't change. The workspace suite (5,301 tests) and clippy pass with the change; the doc and engine suites pass again after the transform guard. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01VmMmc1J8XeNLxhRihWv9cc --- crates/brush/src/defaults.rs | 4 +- crates/brush/src/tests.rs | 2 +- crates/cad/src/import/build.rs | 4 +- crates/doc/src/appearance.rs | 2 +- crates/doc/src/blend.rs | 2 +- crates/doc/src/hit.rs | 2 +- crates/doc/src/lib.rs | 2 + crates/doc/src/live.rs | 4 +- crates/doc/src/node.rs | 12 ++-- crates/doc/src/pattern.rs | 8 +++ crates/doc/src/shared.rs | 72 +++++++++++++++++++++ crates/effects/src/bake.rs | 2 +- crates/effects/src/reshape.rs | 2 +- crates/effects/src/revolve.rs | 2 +- crates/engine/src/cmd/appearance.rs | 8 +-- crates/engine/src/cmd/buildcmds.rs | 9 +-- crates/engine/src/cmd/create.rs | 2 +- crates/engine/src/cmd/fileio/affinity.rs | 6 +- crates/engine/src/cmd/flatten.rs | 5 +- crates/engine/src/cmd/menucmds.rs | 2 +- crates/engine/src/cmd/newart.rs | 2 +- crates/engine/src/cmd/object.rs | 4 +- crates/engine/src/cmd/paint.rs | 2 +- crates/engine/src/cmd/pathops.rs | 10 +-- crates/engine/src/cmd/rasterfx.rs | 4 +- crates/engine/src/cmd/shaper.rs | 10 +-- crates/engine/src/cmd/style.rs | 4 +- crates/engine/src/cmd/textedit.rs | 2 +- crates/engine/src/cmd/typecmd.rs | 4 +- crates/engine/src/tests_containers.rs | 2 +- crates/engine/src/tests_envelope_distort.rs | 2 +- crates/engine/src/tests_eyedropper.rs | 2 +- crates/engine/src/tests_pathops.rs | 4 +- crates/eps/src/import/ai/read.rs | 2 +- crates/eps/src/import/graphics.rs | 2 +- crates/pdf/src/tests.rs | 2 +- crates/render/src/fx.rs | 2 +- crates/svg/src/import.rs | 6 +- crates/svg/tests/css_rules.rs | 6 +- crates/svg/tests/roundtrip.rs | 2 +- crates/svg/tests/symbols_layers.rs | 2 +- crates/tools/src/distort/width.rs | 2 +- 42 files changed, 159 insertions(+), 71 deletions(-) create mode 100644 crates/doc/src/shared.rs diff --git a/crates/brush/src/defaults.rs b/crates/brush/src/defaults.rs index 0a9f4367..8cf74673 100644 --- a/crates/brush/src/defaults.rs +++ b/crates/brush/src/defaults.rs @@ -26,7 +26,7 @@ fn group(children: Vec) -> Node { fn compound(paths: Vec, c: Color) -> Node { let children = paths.into_iter().map(|p| Arc::new(Node::path(NodeId(0), p, Appearance::default()))).collect(); let mut n = Node::new(NodeId(0), NodeKind::Compound { children, rule: FillRule::EvenOdd }); - n.appearance = fill(c); + n.appearance = fill(c).into(); n } @@ -121,7 +121,7 @@ pub(crate) fn stitch_tile() -> Node { let dash = path(shapes::rounded_rectangle(Rect::new(1.5, -0.75, 8.5, 0.75), 0.75), Color::BLACK); // Invisible spacer that fixes the tile width. let mut spacer = path(shapes::rectangle(Rect::new(0.0, -0.75, 10.0, 0.75)), Color::BLACK); - spacer.appearance = Appearance::default(); + spacer.appearance = Appearance::default().into(); group(vec![spacer, dash]) } diff --git a/crates/brush/src/tests.rs b/crates/brush/src/tests.rs index 631be2d5..b012fb97 100644 --- a/crates/brush/src/tests.rs +++ b/crates/brush/src/tests.rs @@ -118,7 +118,7 @@ fn art_brush_arc_length_mapping_is_linear() { Node::path(NodeId(0), shapes::rectangle(Rect::new(5.0, -1.0, 6.0, 1.0)), Appearance::basic(Paint::solid(Color::BLACK), Paint::None, 0.0)); let frame = rect_art(10.0, 2.0); let mut spacer = frame.clone(); - spacer.appearance = Appearance::default(); + spacer.appearance = Appearance::default().into(); let art = Node::group(NodeId(0), vec![std::sync::Arc::new(spacer), std::sync::Arc::new(marker)]); let out = stroke_pieces(&art_brush(art, ArtScale::Stretch), &line(0.0, 0.0, 300.0, 0.0), &stroke(1.0)); let g = &out[0]; diff --git a/crates/cad/src/import/build.rs b/crates/cad/src/import/build.rs index 404b6a16..b729d800 100644 --- a/crates/cad/src/import/build.rs +++ b/crates/cad/src/import/build.rs @@ -479,7 +479,7 @@ impl<'d, 'a> Build<'d, 'a> { .into_iter() .map(|sp| { let mut c = Node::new(self.doc.alloc_id(), kind(PathData::single(sp))); - c.appearance = appearance.clone(); + c.appearance = appearance.clone().into(); Arc::new(c) }) .collect(); @@ -487,7 +487,7 @@ impl<'d, 'a> Build<'d, 'a> { } else { Node::new(self.doc.alloc_id(), kind(path)) }; - n.appearance = appearance; + n.appearance = appearance.into(); Some(n) } diff --git a/crates/doc/src/appearance.rs b/crates/doc/src/appearance.rs index e7ad78a8..5b1dd736 100644 --- a/crates/doc/src/appearance.rs +++ b/crates/doc/src/appearance.rs @@ -942,7 +942,7 @@ impl Appearance { }) } /// Gradient paints of the fills and strokes. - fn gradients(&self) -> impl Iterator { + pub(crate) fn gradients(&self) -> impl Iterator { self.items.iter().filter_map(|i| match i { AppearanceItem::Fill(FillLayer { paint: Paint::Gradient(g), .. }) | AppearanceItem::Stroke(StrokeLayer { paint: Paint::Gradient(g), .. }) => Some(&**g), diff --git a/crates/doc/src/blend.rs b/crates/doc/src/blend.rs index 5695834a..e52bd366 100644 --- a/crates/doc/src/blend.rs +++ b/crates/doc/src/blend.rs @@ -619,7 +619,7 @@ impl Lerp { } }; n.id = NodeId(0); - n.appearance = lerp_appearance(&a.appearance, &b.appearance, t); + n.appearance = lerp_appearance(&a.appearance, &b.appearance, t).into(); n.opacity = lerp_f32(a.opacity, b.opacity, t as f32); n.blend = base.blend; n.visible = true; diff --git a/crates/doc/src/hit.rs b/crates/doc/src/hit.rs index 7cc00f83..12c58bad 100644 --- a/crates/doc/src/hit.rs +++ b/crates/doc/src/hit.rs @@ -463,7 +463,7 @@ mod tests { d.insert(Some(l), 0, Node::path(a, shapes::rectangle(Rect::new(0.0, 0.0, 50.0, 50.0)), Appearance::default_art())).unwrap(); let inner = Node::path(d.alloc_id(), shapes::rectangle(Rect::new(100.0, 0.0, 150.0, 50.0)), Appearance::default()); let mut compound = Node::new(c, NodeKind::Compound { children: vec![Arc::new(inner)], rule: vectorcraft_geom::FillRule::NonZero }); - compound.appearance = Appearance::default_art(); + compound.appearance = Appearance::default_art().into(); d.insert(Some(l), 9, compound).unwrap(); let path_only = HitOptions { path_only: true, ..Default::default() }; for (id, inside, edge) in [(a, Point::new(25.0, 25.0), Point::new(0.5, 25.0)), (c, Point::new(125.0, 25.0), Point::new(100.5, 25.0))] { diff --git a/crates/doc/src/lib.rs b/crates/doc/src/lib.rs index 831d67ea..ca7d744d 100644 --- a/crates/doc/src/lib.rs +++ b/crates/doc/src/lib.rs @@ -16,7 +16,9 @@ pub mod hit; mod id_index; pub mod inks; mod inline; +pub mod shared; pub use inline::{SYMBOL_HALF, SYMBOL_SIZES}; +pub use shared::Shared; pub mod links; pub mod live; pub mod marks; diff --git a/crates/doc/src/live.rs b/crates/doc/src/live.rs index 51641b14..0759e293 100644 --- a/crates/doc/src/live.rs +++ b/crates/doc/src/live.rs @@ -1524,7 +1524,7 @@ fn pattern_art(n: &Node, tiles: PatternTiles) -> Option { }; let plain = |items: Vec| { Arc::new(Node { - appearance: Appearance { items, ..Default::default() }, + appearance: Appearance { items, ..Default::default() }.into(), opacity: 1.0, blend: Default::default(), isolate: false, @@ -1560,7 +1560,7 @@ fn pattern_art(n: &Node, tiles: PatternTiles) -> Option { } Some(Node { kind: NodeKind::Group { children: pieces, clip: false }, - appearance: Appearance { effects: n.appearance.effects.clone(), ..Default::default() }, + appearance: Appearance { effects: n.appearance.effects.clone(), ..Default::default() }.into(), ..n.clone() }) } diff --git a/crates/doc/src/node.rs b/crates/doc/src/node.rs index 48d7fc7b..9eb4b0cd 100644 --- a/crates/doc/src/node.rs +++ b/crates/doc/src/node.rs @@ -514,7 +514,7 @@ pub struct Node { #[serde(default, skip_serializing_if = "crate::skip::is_default")] pub knockout_shape: bool, #[serde(default, skip_serializing_if = "crate::skip::is_default")] - pub appearance: Appearance, + pub appearance: crate::Shared, /// Opacity mask (Transparency panel). Its art lives here, outside the layer tree. #[serde(default, skip_serializing_if = "Option::is_none")] pub mask: Option>, @@ -594,7 +594,7 @@ impl Node { isolate: false, knockout: Knockout::Neutral, knockout_shape: false, - appearance: Appearance::default(), + appearance: Appearance::default().into(), mask: None, trace: None, wrap: None, @@ -611,7 +611,7 @@ impl Node { } pub fn path(id: NodeId, path: PathData, appearance: Appearance) -> Self { let mut n = Self::new(id, NodeKind::Path { path, rule: FillRule::NonZero, live: None, clipping: false, guide: false }); - n.appearance = appearance; + n.appearance = appearance.into(); n } pub fn group(id: NodeId, children: Vec>) -> Self { @@ -854,7 +854,11 @@ impl Node { self.appearance.scale_effects(k, f); } } - self.appearance.transform_gradients(a); + // Only through `DerefMut` when there is something to change: the appearance is shared + // copy-on-write (`Shared`), and a move must not copy what it leaves as it is. + if self.appearance.gradients().next().is_some() { + self.appearance.transform_gradients(a); + } if sc.patterns { crate::pattern::transform_pattern_paints(self, a); } diff --git a/crates/doc/src/pattern.rs b/crates/doc/src/pattern.rs index aa185815..39f4e11a 100644 --- a/crates/doc/src/pattern.rs +++ b/crates/doc/src/pattern.rs @@ -343,6 +343,14 @@ pub fn uses_pattern(n: &Node, name: Option<&str>) -> bool { /// Apply `m` to the pattern placement of every pattern paint of `n` (Transform Patterns). pub fn transform_pattern_paints(n: &mut Node, m: Affine) { + // Without pattern paints there is nothing to change: don't copy the shared appearance. + let patterned = |it: &AppearanceItem| { + matches!(it, AppearanceItem::Fill(f) if matches!(f.paint, Paint::Pattern { .. })) + || matches!(it, AppearanceItem::Stroke(s) if matches!(s.paint, Paint::Pattern { .. })) + }; + if !n.appearance.items.iter().any(patterned) { + return; + } for it in n.appearance.items.iter_mut() { let p = match it { AppearanceItem::Fill(f) => &mut f.paint, diff --git a/crates/doc/src/shared.rs b/crates/doc/src/shared.rs new file mode 100644 index 00000000..208ccd4c --- /dev/null +++ b/crates/doc/src/shared.rs @@ -0,0 +1,72 @@ +//! [`Shared`]: a value shared between copies of a node until one of them changes it. + +use std::sync::Arc; + +use serde::{Deserialize, Deserializer, Serialize, Serializer}; + +/// A copy-on-write value: cloning shares it, and the first change through `DerefMut` makes the +/// changed copy its own (`Arc::make_mut`). A node moved or recoloured keeps sharing what it didn't +/// change with the version the undo history holds. Reads, writes, equality and the saved form are +/// exactly those of the value itself. +#[derive(Clone, Default)] +pub struct Shared(Arc); + +impl Shared { + pub fn new(v: T) -> Self { + Shared(Arc::new(v)) + } + /// Whether `a` and `b` share one value (an unchanged copy). + pub fn ptr_eq(a: &Self, b: &Self) -> bool { + Arc::ptr_eq(&a.0, &b.0) + } +} + +impl std::ops::Deref for Shared { + type Target = T; + fn deref(&self) -> &T { + &self.0 + } +} + +impl std::ops::DerefMut for Shared { + fn deref_mut(&mut self) -> &mut T { + Arc::make_mut(&mut self.0) + } +} + +impl From for Shared { + fn from(v: T) -> Self { + Shared::new(v) + } +} + +impl PartialEq for Shared { + fn eq(&self, other: &Self) -> bool { + Arc::ptr_eq(&self.0, &other.0) || *self.0 == *other.0 + } +} + +/// A shared value equals the plain value it holds (`node.appearance == style.appearance`). +impl PartialEq for Shared { + fn eq(&self, other: &T) -> bool { + *self.0 == *other + } +} + +impl std::fmt::Debug for Shared { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + self.0.fmt(f) + } +} + +impl Serialize for Shared { + fn serialize(&self, s: S) -> Result { + self.0.serialize(s) + } +} + +impl<'de, T: Deserialize<'de>> Deserialize<'de> for Shared { + fn deserialize>(d: D) -> Result { + T::deserialize(d).map(Shared::new) + } +} diff --git a/crates/effects/src/bake.rs b/crates/effects/src/bake.rs index 148f2c01..6538b2de 100644 --- a/crates/effects/src/bake.rs +++ b/crates/effects/src/bake.rs @@ -166,7 +166,7 @@ fn piece(d: &mut Document, n: &Node, path: PathData, rule: FillRule, item: Appea Node { id, name: None, - appearance: Appearance { items: vec![item], ..Default::default() }, + appearance: Appearance { items: vec![item], ..Default::default() }.into(), opacity: 1.0, blend: Default::default(), isolate: false, diff --git a/crates/effects/src/reshape.rs b/crates/effects/src/reshape.rs index bf405990..e37a0d20 100644 --- a/crates/effects/src/reshape.rs +++ b/crates/effects/src/reshape.rs @@ -141,7 +141,7 @@ pub(crate) fn as_art(n: &Node, symbol_art: Option<&Node>) -> Option { if let NodeKind::Path { clipping, .. } = &mut clip.kind { *clipping = true; } - let image = Node { appearance: Appearance::default(), ..Node::new(n.id, n.kind.clone()) }; + let image = Node { appearance: Appearance::default().into(), ..Node::new(n.id, n.kind.clone()) }; Node::new(n.id, NodeKind::Group { children: vec![Arc::new(clip), Arc::new(image)], clip: true }) } _ => outline_art(n, symbol_art)?, diff --git a/crates/effects/src/revolve.rs b/crates/effects/src/revolve.rs index 2d5fa3eb..5465c448 100644 --- a/crates/effects/src/revolve.rs +++ b/crates/effects/src/revolve.rs @@ -200,7 +200,7 @@ fn art(n: &Node, expand: bool) -> Option { } let mut art = n.clone(); art.kind = NodeKind::Group { children, clip: false }; - art.appearance = Appearance { effects: remaining, ..Default::default() }; + art.appearance = Appearance { effects: remaining, ..Default::default() }.into(); Some(art) } diff --git a/crates/engine/src/cmd/appearance.rs b/crates/engine/src/cmd/appearance.rs index ff48b838..b4c8ad42 100644 --- a/crates/engine/src/cmd/appearance.rs +++ b/crates/engine/src/cmd/appearance.rs @@ -443,7 +443,7 @@ fn clear_appearance(s: &mut Session, p: &Value) -> Result { /// Clear Appearance on `n`: no fill, no stroke, and the object's Opacity row back to Default. A /// group keeps no fill or stroke rows of its own. fn clear(n: &mut Node) { - n.appearance = if is_group(n) { Appearance::default() } else { Appearance::basic(Paint::None, Paint::None, 1.0) }; + n.appearance = if is_group(n) { Appearance::default().into() } else { Appearance::basic(Paint::None, Paint::None, 1.0).into() }; n.opacity = 1.0; n.blend = BlendMode::Normal; } @@ -496,7 +496,7 @@ fn reduce_basic(s: &mut Session, p: &Value) -> Result { // keeps the rows it had. let fill = (fill.is_some() || !is_group(n)).then(|| AppearanceItem::Fill(FillLayer::new(fill.map_or(Paint::None, |f| f.paint().clone())))); - n.appearance = Appearance { items: fill.into_iter().collect(), ..Default::default() }; + n.appearance = Appearance { items: fill.into_iter().collect(), ..Default::default() }.into(); if let Some(AppearanceItem::Stroke(mut st)) = stroke { st.effects.clear(); st.opacity = 1.0; @@ -758,7 +758,7 @@ impl Picked { let st = &t.runs[0].style; (st.basic_appearance(), Some(t.local_bounds()), Some((st.clone(), t.para.clone()))) } - _ => (n.appearance.clone(), n.geometric_bounds(), None), + _ => ((*n.appearance).clone(), n.geometric_bounds(), None), }; Self { appearance, bounds, opacity: n.opacity, blend: n.blend, text } } @@ -831,7 +831,7 @@ impl Picked { } let placed = self.placed(n.geometric_bounds()); if ap.fill == FillAttrs::ALL && ap.stroke == StrokeAttrs::ALL { - n.appearance = placed; + n.appearance = placed.into(); } else { focal_attrs(&mut n.appearance, &placed, ap); } diff --git a/crates/engine/src/cmd/buildcmds.rs b/crates/engine/src/cmd/buildcmds.rs index ef3a10fe..107d8996 100644 --- a/crates/engine/src/cmd/buildcmds.rs +++ b/crates/engine/src/cmd/buildcmds.rs @@ -478,7 +478,7 @@ fn lp_merge(s: &mut Session, p: &Value) -> Result { .filter_map(|e| { let pd = e.path_data()?; let sp = pd.subpaths.first().filter(|sp| sp.segment_count() > 0)?; - Some((kurbo::ParamCurve::eval(&sp.segment(0), 0.5), e.appearance.clone())) + Some((kurbo::ParamCurve::eval(&sp.segment(0), 0.5), (*e.appearance).clone())) }) .collect(); let mut sources = source_nodes(&old); @@ -505,7 +505,7 @@ fn lp_merge(s: &mut Session, p: &Value) -> Result { && let Some(ch) = g.children_mut() && let Some(e) = ch.iter_mut().find(|c| c.id == eid) { - Arc::make_mut(e).appearance = app; + Arc::make_mut(e).appearance = app.into(); } } for r in roots.iter().filter(|r| **r != target) { @@ -777,8 +777,9 @@ fn trace_make(s: &mut Session, p: &Value, expand: bool) -> Result { ..StrokeLayer::new(paint, width * unit) })], ..Default::default() - }, - None => fill_only(paint), + } + .into(), + None => fill_only(paint).into(), }; children.push(Arc::new(n)); } diff --git a/crates/engine/src/cmd/create.rs b/crates/engine/src/cmd/create.rs index 5b4dd8c1..84370f57 100644 --- a/crates/engine/src/cmd/create.rs +++ b/crates/engine/src/cmd/create.rs @@ -97,7 +97,7 @@ pub(crate) fn add_look(s: &mut Session, label: &str, kind: NodeKind, look: NewAr let (par, idx, _) = d.position(target).ok_or(crate::EngineError::NoNode(target))?; let mut mask = d.node(target).cloned().ok_or(crate::EngineError::NoNode(target))?; mask.id = d.alloc_id(); - mask.appearance = Appearance::basic(vectorcraft_color::Paint::None, vectorcraft_color::Paint::None, 0.0); + mask.appearance = Appearance::basic(vectorcraft_color::Paint::None, vectorcraft_color::Paint::None, 0.0).into(); if let NodeKind::Path { clipping, .. } = &mut mask.kind { *clipping = true; } diff --git a/crates/engine/src/cmd/fileio/affinity.rs b/crates/engine/src/cmd/fileio/affinity.rs index 92c8b3a4..70dff78f 100644 --- a/crates/engine/src/cmd/fileio/affinity.rs +++ b/crates/engine/src/cmd/fileio/affinity.rs @@ -236,13 +236,13 @@ impl Builder { let appearance = self.appearance(fills, strokes, to_doc); if children.is_empty() { let mut p = self.base(n, NodeKind::Path { path: data, rule, live: None, clipping: false, guide: false }); - p.appearance = appearance; + p.appearance = appearance.into(); p } else { // A shape clips its children: VectorCraft's clipping path keeps its paint, its // fill behind the clipped art and its stroke over it, as Affinity draws it. let mut clip = Node::new(self.doc.alloc_id(), NodeKind::Path { path: data, rule, live: None, clipping: true, guide: false }); - clip.appearance = appearance; + clip.appearance = appearance.into(); let mut kids = vec![Arc::new(clip)]; kids.extend(children); self.base(n, NodeKind::Group { children: kids, clip: true }) @@ -259,7 +259,7 @@ impl Builder { guide: false, }, ); - clip.appearance = self.appearance(background, strokes, to_doc); + clip.appearance = self.appearance(background, strokes, to_doc).into(); let mut kids = vec![Arc::new(clip)]; kids.extend(children); let mut g = self.base(n, NodeKind::Group { children: kids, clip: true }); diff --git a/crates/engine/src/cmd/flatten.rs b/crates/engine/src/cmd/flatten.rs index ad010c13..8d11365b 100644 --- a/crates/engine/src/cmd/flatten.rs +++ b/crates/engine/src/cmd/flatten.rs @@ -655,7 +655,7 @@ impl FlattenReport { art.iter_mut().for_each(opaque); for path in areas { let mut n = shape_node(&mut tmp, path.clone(), None); - n.appearance = Appearance::basic(Paint::solid(Color::rgb(0.0, 0.0, 0.0)), Paint::None, 0.0); + n.appearance = Appearance::basic(Paint::solid(Color::rgb(0.0, 0.0, 0.0)), Paint::None, 0.0).into(); art.push(n); } Some(isolated_doc(&tmp, art)) @@ -1054,7 +1054,8 @@ fn apply(d: &mut Document, sel: &mut vectorcraft_doc::Selection, plan: Plan, o: n.appearance = Appearance { items: vec![AppearanceItem::Fill(FillLayer { overprint: fill.overprint, ..FillLayer::new(fill.paint) })], ..Appearance::default() - }; + } + .into(); n.opacity = fill.alpha.min(1.0); children.push(Arc::new(n)); vector += 1; diff --git a/crates/engine/src/cmd/menucmds.rs b/crates/engine/src/cmd/menucmds.rs index 84082d02..7e013da8 100644 --- a/crates/engine/src/cmd/menucmds.rs +++ b/crates/engine/src/cmd/menucmds.rs @@ -575,7 +575,7 @@ fn crop_image(s: &mut Session, p: &Value) -> Result { let mut bare = node.clone(); bare.opacity = 1.0; bare.blend = Default::default(); - bare.appearance = Appearance::default(); + bare.appearance = Appearance::default().into(); bare.visible = true; let tmp = isolated_doc(&st.doc, vec![bare]); let region = Rect::new(r.x0, r.y0, r.x0 + (r.width() * scale).round().max(1.0) / scale, r.y0 + (r.height() * scale).round().max(1.0) / scale); diff --git a/crates/engine/src/cmd/newart.rs b/crates/engine/src/cmd/newart.rs index 1d7607a2..429ac840 100644 --- a/crates/engine/src/cmd/newart.rs +++ b/crates/engine/src/cmd/newart.rs @@ -94,7 +94,7 @@ impl NewArt { /// Give new object `n` (in `d`) this look: its placed gradients land relative to its bounds, and /// it links to the style (type keeps its own transparency). pub(crate) fn apply(&self, d: &mut vectorcraft_doc::Document, n: &mut Node) { - n.appearance = self.appearance.clone(); + n.appearance = self.appearance.clone().into(); if self.unit_box && n.appearance.has_placed_gradient() && let Some(b) = n.geometric_bounds() diff --git a/crates/engine/src/cmd/object.rs b/crates/engine/src/cmd/object.rs index 5c75c1a9..385d394e 100644 --- a/crates/engine/src/cmd/object.rs +++ b/crates/engine/src/cmd/object.rs @@ -661,7 +661,7 @@ fn compound_make(s: &mut Session, _: &Value) -> Result { let back = paths.first().map_or(0.0, |p| direction(p)); for (i, mut p) in paths.into_iter().enumerate() { let pn = Arc::make_mut(&mut p); - pn.appearance = Appearance::default(); + pn.appearance = Appearance::default().into(); let flip = i > 0 && back != 0.0 && direction(pn) == back; if let NodeKind::Path { path, live, .. } = &mut pn.kind { *live = None; @@ -742,7 +742,7 @@ pub(super) fn as_clipping_path(c: &mut Node) -> Result<()> { if !matches!(c.kind, NodeKind::Path { guide: false, .. } | NodeKind::Compound { .. } | NodeKind::Text(_)) { return Err(EngineError::Other("the top object must be a path, compound path or text object to use as a clipping mask".into())); } - c.appearance = Appearance::basic(Paint::None, Paint::None, 0.0); + c.appearance = Appearance::basic(Paint::None, Paint::None, 0.0).into(); match &mut c.kind { NodeKind::Path { clipping, .. } => *clipping = true, NodeKind::Text(t) => { diff --git a/crates/engine/src/cmd/paint.rs b/crates/engine/src/cmd/paint.rs index 2bc62004..fc9424e1 100644 --- a/crates/engine/src/cmd/paint.rs +++ b/crates/engine/src/cmd/paint.rs @@ -524,7 +524,7 @@ fn default_paint(s: &mut Session, p: &Value) -> Result { r.style.stroke = Paint::None; } } - _ => n.appearance = Appearance::default_art(), + _ => n.appearance = Appearance::default_art().into(), } } Ok(()) diff --git a/crates/engine/src/cmd/pathops.rs b/crates/engine/src/cmd/pathops.rs index adf6afe7..89ab4b82 100644 --- a/crates/engine/src/cmd/pathops.rs +++ b/crates/engine/src/cmd/pathops.rs @@ -203,10 +203,10 @@ struct Style { impl Style { fn of(n: &Node) -> Self { - Style { appearance: n.appearance.clone(), opacity: n.opacity, blend: n.blend } + Style { appearance: (*n.appearance).clone(), opacity: n.opacity, blend: n.blend } } fn apply(&self, n: &mut Node) { - n.appearance = self.appearance.clone(); + n.appearance = self.appearance.clone().into(); n.opacity = self.opacity; n.blend = self.blend; } @@ -377,7 +377,7 @@ fn run_pf(s: &mut Session, op: PathfinderOp) -> Result { // Outline: open edges stroked with the face's former fill. let fill = st.appearance.fill_paint(); let paint = if fill.is_none() { st.appearance.stroke_paint() } else { fill }; - n.appearance = Appearance { items: vec![AppearanceItem::Stroke(StrokeLayer::new(paint, 1.0))], ..Default::default() }; + n.appearance = Appearance { items: vec![AppearanceItem::Stroke(StrokeLayer::new(paint, 1.0))], ..Default::default() }.into(); } children.push(Arc::new(n)); } @@ -519,7 +519,7 @@ pub(crate) fn outlined_stroke( overprint: st.overprint, ..vectorcraft_doc::appearance::FillLayer::new(st.paint.clone()) }; - n.appearance = Appearance { items: vec![AppearanceItem::Fill(fill)], ..Default::default() }; + n.appearance = Appearance { items: vec![AppearanceItem::Fill(fill)], ..Default::default() }.into(); Some(n) } @@ -554,7 +554,7 @@ pub(crate) fn outline_strokes(d: &mut Document, brushes: &[vectorcraft_brush::Br /// A plain copy of path `l` (new ids, no transparency, name or mask) painted with `items`. pub(crate) fn styled_copy(d: &mut Document, l: &Node, items: Vec) -> Node { let mut f = d.reid(l); - f.appearance = Appearance { items, ..Default::default() }; + f.appearance = Appearance { items, ..Default::default() }.into(); f.opacity = 1.0; f.blend = BlendMode::Normal; f.name = None; diff --git a/crates/engine/src/cmd/rasterfx.rs b/crates/engine/src/cmd/rasterfx.rs index 3339502b..013ed127 100644 --- a/crates/engine/src/cmd/rasterfx.rs +++ b/crates/engine/src/cmd/rasterfx.rs @@ -136,7 +136,7 @@ fn settings(s: &mut Session, p: &Value) -> Result { /// `art` in a clip group clipped by `path` filled by `rule` (a clipping path that paints nothing). pub(crate) fn clip_group(d: &mut Document, path: PathData, rule: FillRule, art: Node) -> Node { let mut clip = super::pathops::shape_node(d, path, None); - clip.appearance = Appearance::basic(Paint::None, Paint::None, 0.0); + clip.appearance = Appearance::basic(Paint::None, Paint::None, 0.0).into(); match &mut clip.kind { NodeKind::Path { clipping, rule: r, .. } => (*clipping, *r) = (true, rule), NodeKind::Compound { rule: r, .. } => *r = rule, @@ -363,7 +363,7 @@ fn split_items(out: &mut Document, n: &Node) -> Option { } let mut stroke_piece = |d: &mut Document, path: &PathData, rule: FillRule, st: &StrokeLayer| { let mut p = super::pathops::shape_node(d, path.clone(), None); - p.appearance = Appearance { items: vec![AppearanceItem::Stroke(st.clone())], ..Default::default() }; + p.appearance = Appearance { items: vec![AppearanceItem::Stroke(st.clone())], ..Default::default() }.into(); if let NodeKind::Path { rule: r, .. } | NodeKind::Compound { rule: r, .. } = &mut p.kind { *r = rule; } diff --git a/crates/engine/src/cmd/shaper.rs b/crates/engine/src/cmd/shaper.rs index b0966ccf..b7863217 100644 --- a/crates/engine/src/cmd/shaper.rs +++ b/crates/engine/src/cmd/shaper.rs @@ -334,7 +334,7 @@ fn generated(d: &mut Document, src: &[Arc], spec: &ShaperSpec, map: &Build let edit = edits.get(&i); let Some(source) = edit.and_then(|e| e.paint_source).or_else(|| fill_source(src, region)).and_then(|i| src.get(i)) else { continue }; let path = po::unite_all(&indices.iter().filter_map(|i| map.regions.get(*i).map(|r| (&r.0.path, FillRule::NonZero))).collect::>()); - let style = edit.and_then(|e| e.paint.clone()).unwrap_or_else(|| source.appearance.clone()); + let style = edit.and_then(|e| e.paint.clone()).unwrap_or_else(|| (*source.appearance).clone()); let mut n = Node::path(d.alloc_id(), path, fill_style(style)); n.opacity = source.opacity; n.blend = source.blend; @@ -358,7 +358,7 @@ fn generated(d: &mut Document, src: &[Arc], spec: &ShaperSpec, map: &Build continue; } let Some(source) = src.get(piece.key as usize) else { continue }; - let style = edge_edits.get(&i).and_then(|e| e.paint.clone()).unwrap_or_else(|| source.appearance.clone()); + let style = edge_edits.get(&i).and_then(|e| e.paint.clone()).unwrap_or_else(|| (*source.appearance).clone()); if style.stroke_paint().is_none() { continue; } @@ -448,7 +448,7 @@ pub(crate) fn refresh(before: &Document, d: &mut Document) -> Result<()> { for i in related { let Some(selector) = face_selector(&map, &src, i) else { continue }; if let Some(e) = spec.faces.iter_mut().rev().find(|e| e.selector == selector) { - e.paint = Some(n.appearance.clone()); + e.paint = Some((*n.appearance).clone()); } else { spec.faces.push(FaceEdit { selector, @@ -456,7 +456,7 @@ pub(crate) fn refresh(before: &Document, d: &mut Document) -> Result<()> { erase_edges: false, paint_source: None, merge, - paint: Some(n.appearance.clone()), + paint: Some((*n.appearance).clone()), }); } } @@ -472,7 +472,7 @@ pub(crate) fn refresh(before: &Document, d: &mut Document) -> Result<()> { && piece_point(&piece.path).is_some_and(|point| n.path_data().and_then(|p| p.nearest(point)).is_some_and(|p| p.4 < 1e-5)); if contained && let Some(mut e) = edge_selector(&map, &src, piece) { e.erase = false; - e.paint = Some(n.appearance.clone()); + e.paint = Some((*n.appearance).clone()); spec.edges.push(e); } } diff --git a/crates/engine/src/cmd/style.rs b/crates/engine/src/cmd/style.rs index 275a5364..c6f720fc 100644 --- a/crates/engine/src/cmd/style.rs +++ b/crates/engine/src/cmd/style.rs @@ -217,7 +217,7 @@ fn unitized(mut ap: Appearance, from: Option) -> Appearance { /// and stroke. Placed gradients come relative to the unit box (from the bounds of the object /// they're on). The pen pressure of strokes stays with their own path. pub(crate) fn captured(n: &Node) -> Appearance { - let mut ap = n.appearance.clone().without_pressure(); + let mut ap = (*n.appearance).clone().without_pressure(); if !ap.items.is_empty() { return unitized(ap, n.geometric_bounds()); } @@ -261,7 +261,7 @@ fn override_chars(n: &mut Node, ap: &Appearance) { /// Give `n` style `g`: its appearance (a group or layer too: its own, above its contents; type its /// characters' colour too with `chars`, Override Character Color) and transparency. fn style_node(n: &mut Node, g: &GraphicStyle, chars: bool) { - n.appearance = g.appearance_on(n).into_owned(); + n.appearance = g.appearance_on(n).into_owned().into(); if chars { override_chars(n, &g.appearance); } diff --git a/crates/engine/src/cmd/textedit.rs b/crates/engine/src/cmd/textedit.rs index 364693d4..d830d824 100644 --- a/crates/engine/src/cmd/textedit.rs +++ b/crates/engine/src/cmd/textedit.rs @@ -561,7 +561,7 @@ fn create_in_path(s: &mut Session, p: &Value) -> Result { d.remove(pid)?; let id = d.alloc_id(); let mut n = Node::new(id, NodeKind::Text(Box::new(t))); - n.appearance = Appearance::default(); + n.appearance = Appearance::default().into(); n.opacity = node.opacity; n.name = node.name.clone(); d.insert(par, idx, n)?; diff --git a/crates/engine/src/cmd/typecmd.rs b/crates/engine/src/cmd/typecmd.rs index 5230c5c5..99c960c9 100644 --- a/crates/engine/src/cmd/typecmd.rs +++ b/crates/engine/src/cmd/typecmd.rs @@ -248,14 +248,14 @@ fn create_outlines(s: &mut Session, _: &Value) -> Result { } let st = t.runs.get(g.run).map(|r| r.style.clone()).unwrap_or_else(|| t.first_style()); let mut node = shape_node(d, path, None); - node.appearance = st.appearance(); + node.appearance = st.appearance().into(); children.push(Arc::new(node)); } // Underline and strikethrough bars become paths of their run's paint too (#847). for (run, bar) in vectorcraft_text::decorations(&lay, vectorcraft_text::FontDb::global(), t) { let st = t.runs.get(run).map(|r| r.style.clone()).unwrap_or_else(|| t.first_style()); let mut node = shape_node(d, PathData::from_bezpath(&bar).transformed(t.xf), None); - node.appearance = st.appearance(); + node.appearance = st.appearance().into(); children.push(Arc::new(node)); } let (par, idx, _) = d.position(tid).ok_or(EngineError::NoNode(tid))?; diff --git a/crates/engine/src/tests_containers.rs b/crates/engine/src/tests_containers.rs index dc11f0c0..211bca2e 100644 --- a/crates/engine/src/tests_containers.rs +++ b/crates/engine/src/tests_containers.rs @@ -103,7 +103,7 @@ fn effects_and_fills_apply_to_layers_by_id() { // Two undo steps take both off again. run(&mut s, "edit.undo", json!({})); run(&mut s, "edit.undo", json!({})); - assert_eq!(node(&s, layer).appearance, Default::default()); + assert_eq!(node(&s, layer).appearance, vectorcraft_doc::Appearance::default()); // Nothing selected and no ids: an error, not a silent no-op. assert!(s.execute("appearance.addFill", &json!({})).is_err()); } diff --git a/crates/engine/src/tests_envelope_distort.rs b/crates/engine/src/tests_envelope_distort.rs index 246b13ef..300a6ec5 100644 --- a/crates/engine/src/tests_envelope_distort.rs +++ b/crates/engine/src/tests_envelope_distort.rs @@ -34,7 +34,7 @@ fn sel(s: &mut Session, ids: &[NodeId]) { fn rect(s: &mut Session) -> NodeId { let a = id_of(&s.execute("shape.rectangle", &json!({"x": 100, "y": 100, "width": 200, "height": 100})).unwrap()); s.edit("red", |d, _| { - d.node_mut(a).unwrap().appearance = vectorcraft_doc::Appearance::basic(Paint::solid(Color::rgb(1.0, 0.0, 0.0)), Paint::None, 0.0); + d.node_mut(a).unwrap().appearance = vectorcraft_doc::Appearance::basic(Paint::solid(Color::rgb(1.0, 0.0, 0.0)), Paint::None, 0.0).into(); Ok(()) }) .unwrap(); diff --git a/crates/engine/src/tests_eyedropper.rs b/crates/engine/src/tests_eyedropper.rs index 9dd0a9bd..d3f83a01 100644 --- a/crates/engine/src/tests_eyedropper.rs +++ b/crates/engine/src/tests_eyedropper.rs @@ -126,7 +126,7 @@ fn shift_alt_click_appends_the_appearance() { let mut s = session(); let (_, dst) = source_and_target(&mut s); click(&mut s, 325.0, 35.0, Mods { alt: true, shift: true, ..Default::default() }); - let items = node(&s, dst).appearance.items; + let items = node(&s, dst).appearance.items.clone(); assert_eq!(items.len(), 4, "the source's fill and stroke on top of the target's"); assert_eq!((items[0].paint().color().unwrap().to_hex(), items[2].paint().color().unwrap().to_hex()), ("#ffffff".into(), "#ff0000".into())); } diff --git a/crates/engine/src/tests_pathops.rs b/crates/engine/src/tests_pathops.rs index 8a2d9396..91b991fb 100644 --- a/crates/engine/src/tests_pathops.rs +++ b/crates/engine/src/tests_pathops.rs @@ -229,7 +229,7 @@ fn outline_stroke_of_line() { let r = s.execute("shape.line", &json!({"x1": 0, "y1": 50, "x2": 100, "y2": 50})).unwrap(); let l = NodeId(r["id"].as_u64().unwrap()); s.edit("t", |d, _| { - d.node_mut(l).unwrap().appearance = Appearance::basic(Paint::None, Paint::solid(Color::rgb(1.0, 0.0, 0.0)), 10.0); + d.node_mut(l).unwrap().appearance = Appearance::basic(Paint::None, Paint::solid(Color::rgb(1.0, 0.0, 0.0)), 10.0).into(); Ok(()) }) .unwrap(); @@ -304,7 +304,7 @@ fn clean_up_removes_junk() { let keep = rect(&mut s, 0.0, 0.0, 10.0, 10.0); let unpainted = rect(&mut s, 20.0, 0.0, 10.0, 10.0); s.edit("t", |d, _| { - d.node_mut(unpainted).unwrap().appearance = Appearance::basic(Paint::None, Paint::None, 1.0); + d.node_mut(unpainted).unwrap().appearance = Appearance::basic(Paint::None, Paint::None, 1.0).into(); Ok(()) }) .unwrap(); diff --git a/crates/eps/src/import/ai/read.rs b/crates/eps/src/import/ai/read.rs index 4ef288a8..2a3204aa 100644 --- a/crates/eps/src/import/ai/read.rs +++ b/crates/eps/src/import/ai/read.rs @@ -1330,7 +1330,7 @@ impl<'a> Reader<'a> { } let kind = NodeKind::Path { path, rule: gs.rule, live: None, clipping: clip, guide }; let mut node = self.new_node(kind, &gs); - node.appearance = Appearance { items, ..Appearance::default() }; + node.appearance = Appearance { items, ..Appearance::default() }.into(); node.opacity = gs.opacity; node.blend = gs.blend; node.knockout_shape = gs.knockout_shape; diff --git a/crates/eps/src/import/graphics.rs b/crates/eps/src/import/graphics.rs index 1bbf8337..10c462f6 100644 --- a/crates/eps/src/import/graphics.rs +++ b/crates/eps/src/import/graphics.rs @@ -1688,7 +1688,7 @@ pub(crate) fn collapse(nodes: &mut [Arc], shadings: &[NodeId]) { && shadings.contains(&fill.id) && let (NodeKind::Path { path, rule, clipping: true, .. }, NodeKind::Path { .. }) = (&clip.kind, &fill.kind) { - let mut p = Node::path(fill.id, path.clone(), fill.appearance.clone()); + let mut p = Node::path(fill.id, path.clone(), (*fill.appearance).clone()); if let NodeKind::Path { rule: r, .. } = &mut p.kind { *r = *rule; } diff --git a/crates/pdf/src/tests.rs b/crates/pdf/src/tests.rs index 447018e2..99c69e7d 100644 --- a/crates/pdf/src/tests.rs +++ b/crates/pdf/src/tests.rs @@ -469,7 +469,7 @@ fn import_invalid_dash_array_strokes_solid() { // And the exporter never writes an invalid array back. let mut d2 = doc(100.0, 100.0); let mut n = rect_node(Rect::new(10.0, 10.0, 90.0, 90.0), Color::rgb(1.0, 0.0, 0.0)); - n.appearance = Appearance::basic(Paint::None, Paint::solid(Color::BLACK), 2.0); + n.appearance = Appearance::basic(Paint::None, Paint::solid(Color::BLACK), 2.0).into(); if let Some(AppearanceItem::Stroke(st)) = n.appearance.items.last_mut() { st.dash = Some(Dash { pattern: vec![-5.0, 3.0], offset: 0.0, align_corners: false }); } diff --git a/crates/render/src/fx.rs b/crates/render/src/fx.rs index b8098790..80ca5c52 100644 --- a/crates/render/src/fx.rs +++ b/crates/render/src/fx.rs @@ -947,7 +947,7 @@ mod tests { let l = d.layers[0].id; let id = d.layers[0].children().unwrap()[0].id; let mut n = (*d.node(id).unwrap()).clone(); - n.appearance = Appearance::basic(Paint::None, Paint::solid(Color::BLACK), 4.0); + n.appearance = Appearance::basic(Paint::None, Paint::solid(Color::BLACK), 4.0).into(); if let Some(vectorcraft_doc::AppearanceItem::Stroke(s)) = n.appearance.items.iter_mut().find(|i| matches!(i, vectorcraft_doc::AppearanceItem::Stroke(_))) { diff --git a/crates/svg/src/import.rs b/crates/svg/src/import.rs index 4deefef7..67983cfb 100644 --- a/crates/svg/src/import.rs +++ b/crates/svg/src/import.rs @@ -860,7 +860,7 @@ impl Importer { text::fold_scale(&mut obj, grow); let mut n = self.named(&name, NodeKind::Text(Box::new(obj))); if let Some(ap) = under { - n.appearance = ap; + n.appearance = ap.into(); } Some(vec![Arc::new(n)]) } @@ -1190,7 +1190,7 @@ impl Importer { .map(|sp| { let id = self.doc.alloc_id(); let mut c = Node::new(id, NodeKind::Path { path: PathData::single(sp), rule: r, live: None, clipping: false, guide: false }); - c.appearance = appearance.clone(); + c.appearance = appearance.clone().into(); Arc::new(c) }) .collect(); @@ -1198,7 +1198,7 @@ impl Importer { } else { self.named(p.id(), NodeKind::Path { path, rule: r, live: None, clipping: false, guide: false }) }; - n.appearance = appearance; + n.appearance = appearance.into(); Some(n) } diff --git a/crates/svg/tests/css_rules.rs b/crates/svg/tests/css_rules.rs index d3065416..4004c04c 100644 --- a/crates/svg/tests/css_rules.rs +++ b/crates/svg/tests/css_rules.rs @@ -188,7 +188,7 @@ fn shared_properties_read_as_svg_export_writes_them() { fn gradients_give_css_gradients() { let mut d = doc(); let mut n = rect(&mut d, (0.0, 0.0, 200.0, 100.0), 0.0); - n.appearance = Appearance::basic(Paint::Gradient(Box::new(GradientPaint::new(Gradient::default()))), Paint::None, 0.0); + n.appearance = Appearance::basic(Paint::Gradient(Box::new(GradientPaint::new(Gradient::default()))), Paint::None, 0.0).into(); let id = add(&mut d, n); let r = one(&d, id, &CssOptions::default()); assert_eq!(prop(&r, "background-image"), Some("linear-gradient(90deg, #ffffff 0%, #000000 100%)")); @@ -196,13 +196,13 @@ fn gradients_give_css_gradients() { let mut g = GradientPaint::new(Gradient::default()); g.geom = Some(GradientGeom { start: Point::new(100.0, 0.0), end: Point::new(100.0, 50.0), aspect: 1.0, focal: None }); let mut n = rect(&mut d, (0.0, 0.0, 200.0, 100.0), 0.0); - n.appearance = Appearance::basic(Paint::Gradient(Box::new(g)), Paint::None, 0.0); + n.appearance = Appearance::basic(Paint::Gradient(Box::new(g)), Paint::None, 0.0).into(); let id = add(&mut d, n); assert_eq!(prop(&one(&d, id, &CssOptions::default()), "background-image"), Some("linear-gradient(180deg, #ffffff 0%, #000000 50%)")); // Radial: a circle at the box's centre. let radial = GradientPaint::new(Gradient { kind: GradientKind::Radial, ..Gradient::default() }); let mut n = rect(&mut d, (0.0, 0.0, 100.0, 100.0), 0.0); - n.appearance = Appearance::basic(Paint::Gradient(Box::new(radial)), Paint::None, 0.0); + n.appearance = Appearance::basic(Paint::Gradient(Box::new(radial)), Paint::None, 0.0).into(); let id = add(&mut d, n); let bg = one(&d, id, &CssOptions::default()).props.into_iter().find(|(k, _)| *k == "background-image").unwrap().1; assert!(bg.starts_with("radial-gradient(circle ") && bg.contains(" at 50px 50px, #ffffff 0%, #000000 100%)"), "{bg}"); diff --git a/crates/svg/tests/roundtrip.rs b/crates/svg/tests/roundtrip.rs index c7433a7a..53c732ee 100644 --- a/crates/svg/tests/roundtrip.rs +++ b/crates/svg/tests/roundtrip.rs @@ -144,7 +144,7 @@ fn compound_evenodd_roundtrip() { let i = Node::path(d.alloc_id(), shapes::rectangle(Rect::new(25.0, 25.0, 75.0, 75.0)), Appearance::default()); let cid = d.alloc_id(); let mut c = Node::new(cid, NodeKind::Compound { children: vec![Arc::new(o), Arc::new(i)], rule: FillRule::EvenOdd }); - c.appearance = Appearance::basic(solid("#123456"), Paint::None, 1.0); + c.appearance = Appearance::basic(solid("#123456"), Paint::None, 1.0).into(); let d = doc_with(vec![c]); let s = export(&d, &ExportOptions::default()); assert_eq!(s.matches("