From 8c2cfa2c8f803672fde10adb007370c62c92812f Mon Sep 17 00:00:00 2001 From: Karim Jerbi Date: Tue, 6 Oct 2026 19:35:09 +0100 Subject: [PATCH] fix(viewcube): cache UCS picker names behind an epoch The two per-frame ViewCube blocks rebuilt the named-UCS name list (N String clones plus a Vec) every frame; now Scene::cached_ucs_names memoises it behind ucs_epoch and the picker takes the Arc, so frames cost a refcount bump. Bumped on UCS SAVE, UCS DELETE, RENAME UCS, record-API ucss patch, file-open, and undo Full-swap. Tests: cached_ucs_names_returns_same_arc_until_bump pins the hit path; cached_ucs_names_invalidated_by_bump pins rebuild with new content. Full suite: 1976 passed / 0 failed / 25 ignored. --- src/app/automation.rs | 1 + src/app/commands/layerprops.rs | 2 ++ src/app/commands/styleprops.rs | 1 + src/app/history.rs | 1 + src/app/record_api.rs | 7 +++- src/app/update/file.rs | 1 + src/app/view/mod.rs | 22 ++++--------- src/app/view/viewcube.rs | 11 +++++-- src/scene/mod.rs | 59 ++++++++++++++++++++++++++++++++++ 9 files changed, 85 insertions(+), 20 deletions(-) diff --git a/src/app/automation.rs b/src/app/automation.rs index 24d86ab88..d9830b568 100644 --- a/src/app/automation.rs +++ b/src/app/automation.rs @@ -648,6 +648,7 @@ impl OpenCADStudio { self.tabs[i].scene.document = doc; self.tabs[i].scene.bump_layout_epoch(); self.tabs[i].scene.bump_scale_epoch(); + self.tabs[i].scene.bump_ucs_epoch(); self.tabs[i].scene.load_named_parameters_from_document(); self.tabs[i] .scene diff --git a/src/app/commands/layerprops.rs b/src/app/commands/layerprops.rs index 99858ea5c..6d49ab04d 100644 --- a/src/app/commands/layerprops.rs +++ b/src/app/commands/layerprops.rs @@ -247,6 +247,7 @@ impl OpenCADStudio { .document .ucss .add_or_replace(ucs.clone()); + self.tabs[i].scene.bump_ucs_epoch(); self.tabs[i].active_ucs = Some(ucs); active_changed = true; self.tabs[i].dirty = true; @@ -261,6 +262,7 @@ impl OpenCADStudio { } else if let Some(removed) = self.tabs[i].scene.document.ucss.remove(&name) { + self.tabs[i].scene.bump_ucs_epoch(); let removed_handle = removed.handle; for entity in self.tabs[i].scene.document.entities_mut() { if let codec::EntityType::Viewport(viewport) = entity { diff --git a/src/app/commands/styleprops.rs b/src/app/commands/styleprops.rs index 392e8c91b..e38b4adfa 100644 --- a/src/app/commands/styleprops.rs +++ b/src/app/commands/styleprops.rs @@ -2997,6 +2997,7 @@ impl OpenCADStudio { self.tabs[i].scene.bump_geometry_no_blocks(); } if type_str == "UCS" { + self.tabs[i].scene.bump_ucs_epoch(); if let Some(active) = self.tabs[i].active_ucs.as_mut() { if active.name.eq_ignore_ascii_case(&old_name) { active.name = new_name.clone(); diff --git a/src/app/history.rs b/src/app/history.rs index 4ea0a0e99..ef97360a3 100644 --- a/src/app/history.rs +++ b/src/app/history.rs @@ -855,6 +855,7 @@ impl OpenCADStudio { self.tabs[i].scene.invalidate_dependency_index(); self.tabs[i].scene.bump_layout_epoch(); self.tabs[i].scene.bump_scale_epoch(); + self.tabs[i].scene.bump_ucs_epoch(); } StructureSnapshot::Layers(entries) => { let names: Vec = diff --git a/src/app/record_api.rs b/src/app/record_api.rs index 309515560..0c5c0ae8a 100644 --- a/src/app/record_api.rs +++ b/src/app/record_api.rs @@ -1611,7 +1611,12 @@ impl OpenCADStudio { "app_ids" => patch_table!(app_ids), "views" => patch_table!(views), "vports" => patch_table!(vports), - "ucss" => patch_table!(ucss), + "ucss" => { + patch_table!(ucss); + if changed { + self.tabs[i].scene.bump_ucs_epoch(); + } + } "vx_table" => patch_table!(vx_table), _ => unreachable!(), } diff --git a/src/app/update/file.rs b/src/app/update/file.rs index 491f349d9..a31cb0886 100644 --- a/src/app/update/file.rs +++ b/src/app/update/file.rs @@ -1860,6 +1860,7 @@ impl OpenCADStudio { self.tabs[i].scene.document = doc; self.tabs[i].scene.bump_layout_epoch(); self.tabs[i].scene.bump_scale_epoch(); + self.tabs[i].scene.bump_ucs_epoch(); // Load parameters first so imported dimensional constraints // can resolve their named driving values. self.tabs[i].scene.load_named_parameters_from_document(); diff --git a/src/app/view/mod.rs b/src/app/view/mod.rs index 5c275089d..d3caa7a3c 100644 --- a/src/app/view/mod.rs +++ b/src/app/view/mod.rs @@ -1416,14 +1416,9 @@ bg={bg_ms:.1}ms n={view_count}" .as_ref() .map(|u| u.name.clone()) .unwrap_or_default(); - let ucs_names: Vec = tab - .scene - .document - .ucss - .iter() - .map(|u| u.name.clone()) - .filter(|n| !n.is_empty()) - .collect(); + // Epoch-memoised (#34): Arc clone per frame, rebuilt only + // after a UCS-table bump. + let ucs_names = tab.scene.cached_ucs_names(); let picker = iced::widget::pin(iced::widget::opaque(viewcube_ucs_picker( ucs_current, ucs_names, @@ -1456,14 +1451,9 @@ bg={bg_ms:.1}ms n={view_count}" .as_ref() .map(|u| u.name.clone()) .unwrap_or_default(); - let ucs_names: Vec = tab - .scene - .document - .ucss - .iter() - .map(|u| u.name.clone()) - .filter(|n| !n.is_empty()) - .collect(); + // Epoch-memoised (#34): Arc clone per frame, rebuilt only + // after a UCS-table bump. + let ucs_names = tab.scene.cached_ucs_names(); let picker = iced::widget::pin(iced::widget::opaque(viewcube_ucs_picker( ucs_current, ucs_names, diff --git a/src/app/view/viewcube.rs b/src/app/view/viewcube.rs index 79c1db16a..e203274f0 100644 --- a/src/app/view/viewcube.rs +++ b/src/app/view/viewcube.rs @@ -150,10 +150,15 @@ pub(super) fn viewcube_nav_controls<'a>( /// Fixed width of the UCS picker, so it can be centred under the cube. pub(super) const UCS_PICKER_W: f32 = 84.0; -/// The WCS / named-UCS selector shown under the cube. -pub(super) fn viewcube_ucs_picker<'a>(current: String, names: Vec) -> Element<'a, Message> { +/// The WCS / named-UCS selector shown under the cube. `names` is the +/// epoch-memoised `Scene::cached_ucs_names` Arc (#34) — a refcount bump per +/// frame instead of N String clones. +pub(super) fn viewcube_ucs_picker<'a>( + current: String, + names: std::sync::Arc<[String]>, +) -> Element<'a, Message> { let mut options = vec!["WCS".to_string()]; - options.extend(names); + options.extend(names.iter().cloned()); let selected = if current.is_empty() { "WCS".to_string() } else { diff --git a/src/scene/mod.rs b/src/scene/mod.rs index 464f65aa3..8669fd033 100644 --- a/src/scene/mod.rs +++ b/src/scene/mod.rs @@ -2365,6 +2365,10 @@ pub struct Scene { scale_epoch: u64, scale_picker_cache: RefCell, String, std::sync::Arc>)>>, + /// Bump on any named-UCS table mutation (UCS SAVE / DELETE / RENAME, + /// record-API patch, file-open, undo Full-swap). + ucs_epoch: u64, + ucs_names_cache: RefCell)>>, /// 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 @@ -2756,6 +2760,8 @@ impl Scene { layout_names_cache: RefCell::new(None), scale_epoch: 0, scale_picker_cache: RefCell::new(None), + ucs_epoch: 0, + ucs_names_cache: RefCell::new(None), dependency_index_cache: RefCell::new(None), associative_hatch_source_cache: RefCell::new(None), parametric_constraints: Vec::new(), @@ -6578,6 +6584,32 @@ impl Scene { list } + pub fn bump_ucs_epoch(&mut self) { + self.ucs_epoch += 1; + } + + /// Named-UCS names for the ViewCube picker, memoised behind `ucs_epoch` + /// (#34, mirrors `cached_layout_names`). Same empty-name filter the view + /// blocks used when building the list inline every frame. + pub fn cached_ucs_names(&self) -> std::sync::Arc<[String]> { + if let Some((epoch, names)) = self.ucs_names_cache.borrow().as_ref() { + if *epoch == self.ucs_epoch { + return std::sync::Arc::clone(names); + } + } + let names: std::sync::Arc<[String]> = self + .document + .ucss + .iter() + .map(|u| u.name.clone()) + .filter(|n| !n.is_empty()) + .collect::>() + .into(); + *self.ucs_names_cache.borrow_mut() = + Some((self.ucs_epoch, std::sync::Arc::clone(&names))); + names + } + /// Wire set for the Model layout, shared by every tile. /// /// The model wire geometry is **camera-independent**, so it is tessellated @@ -14253,6 +14285,33 @@ mod layout_cache_tests { ); assert!(after.iter().any(|(n, _, _)| n == "1:50")); } + + #[test] + fn cached_ucs_names_returns_same_arc_until_bump() { + // RED: `Scene::cached_ucs_names` does not exist yet (#34). + let scene = Scene::new(); + let first = scene.cached_ucs_names(); + let second = scene.cached_ucs_names(); + assert!(std::sync::Arc::ptr_eq(&first, &second)); + } + + #[test] + fn cached_ucs_names_invalidated_by_bump() { + // RED: `Scene::bump_ucs_epoch` does not exist yet (#34). + let mut scene = Scene::new(); + let before = scene.cached_ucs_names(); + scene + .document + .ucss + .add_or_replace(codec::tables::Ucs::new("TEST_CACHED_UCS")); + scene.bump_ucs_epoch(); + let after = scene.cached_ucs_names(); + assert!( + !std::sync::Arc::ptr_eq(&before, &after), + "ucs mutation + bump must replace the cached Arc" + ); + assert!(after.iter().any(|n| n == "TEST_CACHED_UCS")); + } } #[cfg(test)]