Merge pull request #1074 from mechpanos/pr/selection-filter-perframe

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<Option<(u64, Handle, std::sync::Arc<Vec<String>>)>>,
+    /// `(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<String>,
+            std::sync::Arc<Vec<String>>,
+        )>,
+    >,
     /// 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

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<Vec<String>> {
         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<String> =
             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<Vec<String>> = 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<Vec<String>> =
+            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};