diff --git a/src/scene/mod.rs b/src/scene/mod.rs index e19534afa..68d04fb91 100644 --- a/src/scene/mod.rs +++ b/src/scene/mod.rs @@ -1860,7 +1860,20 @@ pub struct Scene { /// (geometry_epoch, layout block). The status bar asks for this on every /// frame to populate the selection-filter menu, but the answer only /// changes when entities are added or removed. - layout_type_names_cache: RefCell>)>>, + /// `(epoch, block, present type names, the list handed to the UI)`. + /// + /// The set is kept alongside the list so an edit can be folded in without + /// rebuilding either: drawing a line into a drawing that already has lines + /// does not change the answer, and the walk that produces it visits every + /// entity the layout owns. + layout_type_names_cache: RefCell< + Option<( + u64, + Handle, + std::collections::BTreeSet, + std::sync::Arc>, + )>, + >, /// Reverse dependencies from layer/style/block definitions to the top-level /// entities whose resident wire runs actually change. Kept independent from /// `geometry_epoch`: a layer colour toggle can reuse the index, invalidate diff --git a/src/scene/selection.rs b/src/scene/selection.rs index 590635f04..de9248c31 100644 --- a/src/scene/selection.rs +++ b/src/scene/selection.rs @@ -361,18 +361,71 @@ impl Scene { } /// Sorted entity types in the current layout, cached until geometry or layout changes. + /// The entity type names present in the current layout, for the selection + /// filter menu. + /// + /// The status bar asks for this on **every** frame, and the cache used to + /// be keyed on `geometry_epoch` alone — so every drawn point invalidated it + /// and the walk below visited all 444 937 handles a model space owns. That + /// was ~100 ms per edit, the largest single cost of drawing a line, and + /// almost always to arrive at the same answer: adding a line to a drawing + /// that already has lines changes nothing here. + /// + /// An edit is now folded in from the delta journal instead. Additions only + /// ever grow the set, so they are cheap to apply; a removal can retire a + /// type and cannot be resolved from a handle that is already gone, so those + /// fall back to the full walk. pub fn entity_type_names_in_layout(&self) -> std::sync::Arc> { use crate::entities::traits::entity_type_name; let block = self.current_layout_block_handle(); + let mut cached_epoch = None; { let cache = self.layout_type_names_cache.borrow(); - if let Some((epoch, cached_block, names)) = cache.as_ref() { - if *epoch == self.geometry_epoch && *cached_block == block { - return std::sync::Arc::clone(names); + if let Some((epoch, cached_block, _, names)) = cache.as_ref() { + if *cached_block == block { + if *epoch == self.geometry_epoch { + return std::sync::Arc::clone(names); + } + cached_epoch = Some(*epoch); } } } - let mut names: std::collections::BTreeSet<&str> = + + // Incremental: fold the changes since the cached epoch into the set. + if let Some(since) = cached_epoch { + if let Some(deltas) = self.replay_since(since) { + if !deltas.iter().any(|(_, kind)| *kind == ChangeKind::Removed) { + let mut cache = self.layout_type_names_cache.borrow_mut(); + if let Some((epoch, _, present, names)) = cache.as_mut() { + let mut added = false; + for (handle, _) in &deltas { + if let Some(entity) = self.document.get_entity(*handle) { + if entity.common().owner_handle == block { + // `contains` first: the common case is a + // type already present, and that path must + // not allocate. + let name = entity_type_name(entity); + if !present.contains(name) { + present.insert(name.to_string()); + added = true; + } + } + } + } + // Only rebuild the list when the set actually moved; + // otherwise the existing `Arc` is still the answer. + if added { + *names = + std::sync::Arc::new(present.iter().cloned().collect()); + } + *epoch = self.geometry_epoch; + return std::sync::Arc::clone(names); + } + } + } + } + + let mut present: std::collections::BTreeSet = std::collections::BTreeSet::new(); if let Some(record) = self .document @@ -382,15 +435,21 @@ impl Scene { { for &handle in &record.entity_handles { if let Some(entity) = self.document.get_entity(handle) { - names.insert(entity_type_name(entity)); + let name = entity_type_name(entity); + if !present.contains(name) { + present.insert(name.to_string()); + } } } } - let names: std::sync::Arc> = std::sync::Arc::new( - names.into_iter().map(str::to_string).collect(), - ); - *self.layout_type_names_cache.borrow_mut() = - Some((self.geometry_epoch, block, std::sync::Arc::clone(&names))); + let names: std::sync::Arc> = + std::sync::Arc::new(present.iter().cloned().collect()); + *self.layout_type_names_cache.borrow_mut() = Some(( + self.geometry_epoch, + block, + present, + std::sync::Arc::clone(&names), + )); names } @@ -954,6 +1013,46 @@ impl Scene { mod tests { use super::*; + /// The reason the cache carries a set as well as a list: drawing into a + /// drawing that already has that type must not rebuild either, because the + /// rebuild walks every entity the layout owns — 444 937 of them on the + /// reproducer, ~100 ms, on every single drawn point. + #[test] + fn adding_a_type_already_present_reuses_the_list() { + use acadrust::entities::{Circle, EntityType, Line}; + use acadrust::types::Vector3; + use std::sync::Arc; + + let line = || { + EntityType::Line(Line::from_points( + Vector3::new(0.0, 0.0, 0.0), + Vector3::new(1.0, 0.0, 0.0), + )) + }; + let mut scene = Scene::new(); + scene.add_entity(line()); + let first = scene.entity_type_names_in_layout(); + + scene.add_entity(line()); + let second = scene.entity_type_names_in_layout(); + assert!( + Arc::ptr_eq(&first, &second), + "a second line changes no type name, so the list must be reused", + ); + + scene.add_entity(EntityType::Circle(Circle::new())); + let third = scene.entity_type_names_in_layout(); + assert_eq!(third.as_slice(), ["Circle", "Line"], "a new type must appear"); + + // The incremental answer has to be the answer a full walk gives. + scene.layout_type_names_cache.borrow_mut().take(); + assert_eq!( + scene.entity_type_names_in_layout().as_slice(), + third.as_slice(), + "folding edits in must match rebuilding from scratch", + ); + } + #[test] fn layout_type_cache_reuses_and_invalidates_on_edits_undo_and_layout() { use acadrust::entities::{Circle, EntityType, Line};