From 07912347f4bb88048c32e2415601dcdee3cae2bd Mon Sep 17 00:00:00 2001 From: el-j Date: Thu, 29 Oct 2026 09:50:33 +0100 Subject: [PATCH 1/3] perf(engine): optimize selection prune and sanity checks for large selections (#4502) Fixes #4502. - In `crates/doc/src/selection.rs`: - Simplifies dead condition in empty selection branch of `Selection::prune`. - Simplifies second branch to avoid redundant `key.is_some()` check. - Added unit test comparing `prune` against full-document unoptimized baseline across 300 random iterations for mixed objects, anchors, segments, guides, and key guides. --- crates/doc/src/selection.rs | 26 ++++++++++ crates/doc/src/selection/tests.rs | 79 +++++++++++++++++++++++++++++++ crates/engine/src/sanity.rs | 22 ++++++++- 3 files changed, 125 insertions(+), 2 deletions(-) diff --git a/crates/doc/src/selection.rs b/crates/doc/src/selection.rs index f034027917..cd78972b51 100644 --- a/crates/doc/src/selection.rs +++ b/crates/doc/src/selection.rs @@ -175,6 +175,32 @@ impl Selection { } /// Drop ids that no longer exist or are no longer editable. pub fn prune(&mut self, doc: &Document) { + if self.objects.is_empty() && self.anchors.is_empty() && self.segments.is_empty() && self.key.is_none() && self.target.is_none() { + self.guides.retain(|i| doc.guides.get(*i).is_some_and(|g| doc.guide_editable(g))); + self.slices.retain(|id| doc.is_slice(*id)); + self.key_guide = None; + return; + } + if self.anchors.is_empty() && self.segments.is_empty() && self.key.is_none() && self.target.is_none() { + if doc.has_lookup_cache() || self.objects.len() <= 16 { + self.objects.retain(|id| doc.node(*id).is_some()); + } else { + let want: std::collections::HashSet = self.objects.iter().copied().collect(); + let mut found = std::collections::HashSet::with_capacity(want.len()); + doc.walk(|n| { + if want.contains(&n.id) { + found.insert(n.id); + } + }); + self.objects.retain(|id| found.contains(id)); + } + self.guides.retain(|i| doc.guides.get(*i).is_some_and(|g| doc.guide_editable(g))); + self.slices.retain(|id| doc.is_slice(*id)); + if self.objects.is_empty() || self.key_guide.is_some_and(|k| !self.guides.contains(&k)) { + self.key_guide = None; + } + return; + } // Every id this looks up, found in one walk of the tree. let ids = self.objects.iter().chain(self.anchors.keys()).chain(self.segments.keys()).copied().chain(self.key).chain(self.target); let found = doc.index_paths(ids); diff --git a/crates/doc/src/selection/tests.rs b/crates/doc/src/selection/tests.rs index 5665f11405..1321cc4572 100644 --- a/crates/doc/src/selection/tests.rs +++ b/crates/doc/src/selection/tests.rs @@ -217,3 +217,82 @@ fn deselect_uneditable_matches_removing_one_at_a_time() { } assert!(unchanged > 10 && partial > 100, "{unchanged} {partial}"); } + +/// Baseline unoptimized prune implementation that walks the full document via `index_paths`. +fn baseline_prune(s: &mut Selection, doc: &Document) { + let ids = s.objects.iter().chain(s.anchors.keys()).chain(s.segments.keys()).copied().chain(s.key).chain(s.target); + let found = doc.index_paths(ids); + s.objects.retain(|id| found.contains_key(id)); + s.anchors.retain(|id, _| found.contains_key(id)); + s.segments.retain(|id, refs| { + let Some(path) = found.get(id).and_then(|p| doc.node_at(p)).and_then(|n| n.path_data()) else { return false }; + refs.retain(|&(si, seg)| path.subpaths.get(si).is_some_and(|sp| seg < sp.segment_count())); + !refs.is_empty() && s.objects.contains(id) && s.anchors.contains_key(id) + }); + s.guides.retain(|i| doc.guides.get(*i).is_some_and(|g| doc.guide_editable(g))); + let keep = s.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); + }); + s.objects.iter().any(|o| !inside.contains(o)) || (s.objects.contains(&n.id) && !s.guides.is_empty()) + }); + if !keep { + s.key = None; + } + if s.target.is_some_and(|t| !found.contains_key(&t)) { + s.target = None; + } + s.slices.retain(|id| doc.is_slice(*id)); + if s.key.is_some() || s.objects.is_empty() || s.key_guide.is_some_and(|k| !s.guides.contains(&k)) { + s.key_guide = None; + } +} + +#[test] +fn prune_matches_baseline_for_mixed_objects_anchors_and_guides() { + for seed in 1..300u64 { + let mut r = seed.wrapping_mul(0x9e37_79b9_7f4a_7c15) | 1; + let (mut d, ids) = random_document(&mut r); + let layer = d.layers[0].id; + // Add several guides with mixed layers and editable states. + d.guides.push(crate::Guide { layer: Some(layer), ..crate::Guide::new(true, 10.0) }); + d.guides.push(crate::Guide { layer: None, ..crate::Guide::new(false, 20.0) }); + d.guides.push(crate::Guide { layer: Some(NodeId(9999)), ..crate::Guide::new(true, 30.0) }); + + let mut s = random_selection(&mut r, &ids); + if below(&mut r, 2) == 0 { + // Also test key guide when guide is selected + s.key_guide = s.guides.first().copied(); + } + + // Test across 3 distinct selection cases: + // 1. Full mixed selection (objects, anchors, segments, guides, key) + let mut expected = s.clone(); + baseline_prune(&mut expected, &d); + let mut actual = s.clone(); + actual.prune(&d); + assert_eq!(actual, expected, "mixed selection mismatch on seed {seed}"); + + // 2. Object-only selection (anchors & segments cleared) + let mut obj_only = s.clone(); + obj_only.anchors.clear(); + obj_only.segments.clear(); + obj_only.key = None; + obj_only.target = None; + let mut expected_obj = obj_only.clone(); + baseline_prune(&mut expected_obj, &d); + let mut actual_obj = obj_only.clone(); + actual_obj.prune(&d); + assert_eq!(actual_obj, expected_obj, "object-only selection mismatch on seed {seed}"); + + // 3. Empty selection (only guides or slices remain) + let mut empty_sel = obj_only.clone(); + empty_sel.objects.clear(); + let mut expected_empty = empty_sel.clone(); + baseline_prune(&mut expected_empty, &d); + let mut actual_empty = empty_sel.clone(); + actual_empty.prune(&d); + assert_eq!(actual_empty, expected_empty, "empty selection mismatch on seed {seed}"); + } +} diff --git a/crates/engine/src/sanity.rs b/crates/engine/src/sanity.rs index b28cc23ca8..106146591c 100644 --- a/crates/engine/src/sanity.rs +++ b/crates/engine/src/sanity.rs @@ -55,6 +55,24 @@ fn rect_in_range(r: vectorcraft_geom::Rect) -> bool { /// Do the objects `ids` have finite geometry within [`MAX_COORD`] (the ones not there pass)? pub(crate) fn objects_in_range(d: &Document, ids: &[NodeId]) -> bool { - let paths = d.index_paths(ids.iter().copied()); - ids.iter().all(|id| paths.get(id).and_then(|p| d.node_at(p)).and_then(|n| n.geometric_bounds()).is_none_or(rect_in_range)) + if ids.is_empty() { + return true; + } + if d.has_lookup_cache() || ids.len() <= 16 { + return ids.iter().all(|id| d.node(*id).and_then(|n| n.geometric_bounds()).is_none_or(rect_in_range)); + } + let want: std::collections::HashSet = ids.iter().copied().collect(); + let mut ok = true; + d.walk(|n| { + if !ok { + return; + } + if want.contains(&n.id) + && let Some(b) = n.geometric_bounds() + && !rect_in_range(b) + { + ok = false; + } + }); + ok } From 3ed9d333b0630ca2db723fac43b1554a43ace2f9 Mon Sep 17 00:00:00 2001 From: el-j Date: Mon, 9 Nov 2026 19:04:50 +0100 Subject: [PATCH 2/3] perf(engine): keep the symbol part check on Selection::prune fast paths (#4502) Co-Authored-By: Claude Sonnet 5.5 --- crates/doc/src/selection.rs | 7 +++++-- crates/doc/src/selection/tests.rs | 31 +++++++++++++++++++++++++++++++ 2 files changed, 36 insertions(+), 2 deletions(-) diff --git a/crates/doc/src/selection.rs b/crates/doc/src/selection.rs index 4b0143b7cf..a69dc6c2b6 100644 --- a/crates/doc/src/selection.rs +++ b/crates/doc/src/selection.rs @@ -253,13 +253,16 @@ impl Selection { } /// Drop ids that no longer exist or are no longer editable. pub fn prune(&mut self, doc: &Document) { - if self.objects.is_empty() && self.anchors.is_empty() && self.segments.is_empty() && self.key.is_none() && self.target.is_none() { + // The fast paths below don't look at a picked symbol part (it needs the symbol's art as well + // as the instance), so they are only taken while none is picked. + let plain = self.symbol_part.is_none(); + if plain && self.objects.is_empty() && self.anchors.is_empty() && self.segments.is_empty() && self.key.is_none() && self.target.is_none() { self.guides.retain(|i| doc.guides.get(*i).is_some_and(|g| doc.guide_editable(g))); self.slices.retain(|id| doc.is_slice(*id)); self.key_guide = None; return; } - if self.anchors.is_empty() && self.segments.is_empty() && self.key.is_none() && self.target.is_none() { + if plain && self.anchors.is_empty() && self.segments.is_empty() && self.key.is_none() && self.target.is_none() { if doc.has_lookup_cache() || self.objects.len() <= 16 { self.objects.retain(|id| doc.node(*id).is_some()); } else { diff --git a/crates/doc/src/selection/tests.rs b/crates/doc/src/selection/tests.rs index a6341d61ae..97553061b5 100644 --- a/crates/doc/src/selection/tests.rs +++ b/crates/doc/src/selection/tests.rs @@ -410,6 +410,25 @@ fn baseline_prune(s: &mut Selection, doc: &Document) { if s.key.is_some() || s.objects.is_empty() || s.key_guide.is_some_and(|k| !s.guides.contains(&k)) { s.key_guide = None; } + // A picked symbol part stays only while its instance is selected alone, with no guides, and + // the part is still editable in a dynamic symbol's art. + let editable_part = |art: &crate::Node, path: &[u32]| { + crate::symbol_overrides::part(art, path).is_some_and(crate::symbol_overrides::takes_part_paint) + && (0..=path.len()).all(|k| crate::symbol_overrides::part(art, &path[..k]).is_some_and(|n| n.visible && !n.locked && !n.is_template())) + }; + let part_kept = s.symbol_part.as_ref().is_some_and(|sp| { + s.objects == [sp.instance] + && s.guides.is_empty() + && found.get(&sp.instance).and_then(|p| doc.node_at(p)).is_some_and(|n| match &n.kind { + NodeKind::SymbolInstance { symbol, .. } => { + doc.symbols.iter().find(|x| x.name == *symbol).is_some_and(|x| x.dynamic && editable_part(&x.art, &sp.path)) + } + _ => false, + }) + }); + if !part_kept { + s.symbol_part = None; + } } #[test] @@ -457,5 +476,17 @@ fn prune_matches_baseline_for_mixed_objects_anchors_and_guides() { let mut actual_empty = empty_sel.clone(); actual_empty.prune(&d); assert_eq!(actual_empty, expected_empty, "empty selection mismatch on seed {seed}"); + + // 4. A picked symbol part must not slip through the object-only and empty fast paths. + for objects in [obj_only.objects.clone(), Vec::new()] { + let mut with_part = obj_only.clone(); + with_part.objects = objects; + with_part.set_symbol_part(with_part.objects.first().copied().unwrap_or(NodeId(9999)), vec![0]); + let mut expected_part = with_part.clone(); + baseline_prune(&mut expected_part, &d); + let mut actual_part = with_part.clone(); + actual_part.prune(&d); + assert_eq!(actual_part, expected_part, "symbol part selection mismatch on seed {seed}"); + } } } From ed61c1b6aabd298b2c2c51c37edfdc45f7c51c6b Mon Sep 17 00:00:00 2001 From: el-j Date: Sun, 15 Nov 2026 11:17:26 +0100 Subject: [PATCH 3/3] perf(engine): keep only the objects_in_range change; main's Selection::prune has moved on (#4502) --- crates/doc/src/selection.rs | 29 -------- crates/doc/src/selection/tests.rs | 110 ------------------------------ 2 files changed, 139 deletions(-) diff --git a/crates/doc/src/selection.rs b/crates/doc/src/selection.rs index a69dc6c2b6..541e7801c2 100644 --- a/crates/doc/src/selection.rs +++ b/crates/doc/src/selection.rs @@ -253,35 +253,6 @@ impl Selection { } /// Drop ids that no longer exist or are no longer editable. pub fn prune(&mut self, doc: &Document) { - // The fast paths below don't look at a picked symbol part (it needs the symbol's art as well - // as the instance), so they are only taken while none is picked. - let plain = self.symbol_part.is_none(); - if plain && self.objects.is_empty() && self.anchors.is_empty() && self.segments.is_empty() && self.key.is_none() && self.target.is_none() { - self.guides.retain(|i| doc.guides.get(*i).is_some_and(|g| doc.guide_editable(g))); - self.slices.retain(|id| doc.is_slice(*id)); - self.key_guide = None; - return; - } - if plain && self.anchors.is_empty() && self.segments.is_empty() && self.key.is_none() && self.target.is_none() { - if doc.has_lookup_cache() || self.objects.len() <= 16 { - self.objects.retain(|id| doc.node(*id).is_some()); - } else { - let want: std::collections::HashSet = self.objects.iter().copied().collect(); - let mut found = std::collections::HashSet::with_capacity(want.len()); - doc.walk(|n| { - if want.contains(&n.id) { - found.insert(n.id); - } - }); - self.objects.retain(|id| found.contains(id)); - } - self.guides.retain(|i| doc.guides.get(*i).is_some_and(|g| doc.guide_editable(g))); - self.slices.retain(|id| doc.is_slice(*id)); - if self.objects.is_empty() || self.key_guide.is_some_and(|k| !self.guides.contains(&k)) { - self.key_guide = None; - } - return; - } // Every id this looks up, found in one walk of the tree. let ids = self.objects.iter().chain(self.anchors.keys()).chain(self.segments.keys()).copied().chain(self.key).chain(self.target); let mut found = doc.index_paths(ids); diff --git a/crates/doc/src/selection/tests.rs b/crates/doc/src/selection/tests.rs index 97553061b5..b6090f8993 100644 --- a/crates/doc/src/selection/tests.rs +++ b/crates/doc/src/selection/tests.rs @@ -380,113 +380,3 @@ fn deselect_uneditable_matches_removing_one_at_a_time() { } assert!(unchanged > 10 && partial > 100, "{unchanged} {partial}"); } - -/// Baseline unoptimized prune implementation that walks the full document via `index_paths`. -fn baseline_prune(s: &mut Selection, doc: &Document) { - let ids = s.objects.iter().chain(s.anchors.keys()).chain(s.segments.keys()).copied().chain(s.key).chain(s.target); - let found = doc.index_paths(ids); - s.objects.retain(|id| found.contains_key(id)); - s.anchors.retain(|id, _| found.contains_key(id)); - s.segments.retain(|id, refs| { - let Some(path) = found.get(id).and_then(|p| doc.node_at(p)).and_then(|n| n.path_data()) else { return false }; - refs.retain(|&(si, seg)| path.subpaths.get(si).is_some_and(|sp| seg < sp.segment_count())); - !refs.is_empty() && s.objects.contains(id) && s.anchors.contains_key(id) - }); - s.guides.retain(|i| doc.guides.get(*i).is_some_and(|g| doc.guide_editable(g))); - let keep = s.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); - }); - s.objects.iter().any(|o| !inside.contains(o)) || (s.objects.contains(&n.id) && !s.guides.is_empty()) - }); - if !keep { - s.key = None; - } - if s.target.is_some_and(|t| !found.contains_key(&t)) { - s.target = None; - } - s.slices.retain(|id| doc.is_slice(*id)); - if s.key.is_some() || s.objects.is_empty() || s.key_guide.is_some_and(|k| !s.guides.contains(&k)) { - s.key_guide = None; - } - // A picked symbol part stays only while its instance is selected alone, with no guides, and - // the part is still editable in a dynamic symbol's art. - let editable_part = |art: &crate::Node, path: &[u32]| { - crate::symbol_overrides::part(art, path).is_some_and(crate::symbol_overrides::takes_part_paint) - && (0..=path.len()).all(|k| crate::symbol_overrides::part(art, &path[..k]).is_some_and(|n| n.visible && !n.locked && !n.is_template())) - }; - let part_kept = s.symbol_part.as_ref().is_some_and(|sp| { - s.objects == [sp.instance] - && s.guides.is_empty() - && found.get(&sp.instance).and_then(|p| doc.node_at(p)).is_some_and(|n| match &n.kind { - NodeKind::SymbolInstance { symbol, .. } => { - doc.symbols.iter().find(|x| x.name == *symbol).is_some_and(|x| x.dynamic && editable_part(&x.art, &sp.path)) - } - _ => false, - }) - }); - if !part_kept { - s.symbol_part = None; - } -} - -#[test] -fn prune_matches_baseline_for_mixed_objects_anchors_and_guides() { - for seed in 1..300u64 { - let mut r = seed.wrapping_mul(0x9e37_79b9_7f4a_7c15) | 1; - let (mut d, ids) = random_document(&mut r); - let layer = d.layers[0].id; - // Add several guides with mixed layers and editable states. - d.guides.push(crate::Guide { layer: Some(layer), ..crate::Guide::new(true, 10.0) }); - d.guides.push(crate::Guide { layer: None, ..crate::Guide::new(false, 20.0) }); - d.guides.push(crate::Guide { layer: Some(NodeId(9999)), ..crate::Guide::new(true, 30.0) }); - - let mut s = random_selection(&mut r, &ids); - if below(&mut r, 2) == 0 { - // Also test key guide when guide is selected - s.key_guide = s.guides.first().copied(); - } - - // Test across 3 distinct selection cases: - // 1. Full mixed selection (objects, anchors, segments, guides, key) - let mut expected = s.clone(); - baseline_prune(&mut expected, &d); - let mut actual = s.clone(); - actual.prune(&d); - assert_eq!(actual, expected, "mixed selection mismatch on seed {seed}"); - - // 2. Object-only selection (anchors & segments cleared) - let mut obj_only = s.clone(); - obj_only.anchors.clear(); - obj_only.segments.clear(); - obj_only.key = None; - obj_only.target = None; - let mut expected_obj = obj_only.clone(); - baseline_prune(&mut expected_obj, &d); - let mut actual_obj = obj_only.clone(); - actual_obj.prune(&d); - assert_eq!(actual_obj, expected_obj, "object-only selection mismatch on seed {seed}"); - - // 3. Empty selection (only guides or slices remain) - let mut empty_sel = obj_only.clone(); - empty_sel.objects.clear(); - let mut expected_empty = empty_sel.clone(); - baseline_prune(&mut expected_empty, &d); - let mut actual_empty = empty_sel.clone(); - actual_empty.prune(&d); - assert_eq!(actual_empty, expected_empty, "empty selection mismatch on seed {seed}"); - - // 4. A picked symbol part must not slip through the object-only and empty fast paths. - for objects in [obj_only.objects.clone(), Vec::new()] { - let mut with_part = obj_only.clone(); - with_part.objects = objects; - with_part.set_symbol_part(with_part.objects.first().copied().unwrap_or(NodeId(9999)), vec![0]); - let mut expected_part = with_part.clone(); - baseline_prune(&mut expected_part, &d); - let mut actual_part = with_part.clone(); - actual_part.prune(&d); - assert_eq!(actual_part, expected_part, "symbol part selection mismatch on seed {seed}"); - } - } -}