From e9198a47e5aabd9e1b0a52f8dda2b2fd07d803d3 Mon Sep 17 00:00:00 2001 From: Kevin Griffin Date: Wed, 22 Jul 2026 13:03:01 -0700 Subject: [PATCH] perf(render): don't duplicate the wire set when no Face3D wire exists MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The per-epoch Face3D/other split cloned every wire into two owned lists even when the face3d list came out empty — for pure 2-D drawings and mesh imports (#358) the 'other' list was a wire-for-wire copy of the entire base set, held resident in split_cache alongside the original. Cache None as an 'everything is other' marker and hand the render the base set itself for that frame. The base Arc is deliberately not stored in the cache: try_resident_patch can only apply an incremental patch while the resident Arc is uniquely held, so pinning it there would silently degrade every edit to a full rebuild. Co-Authored-By: Claude Fable 5 --- src/scene/mod.rs | 8 +++++++- src/scene/view/render.rs | 39 ++++++++++++++++++++++++++------------- 2 files changed, 33 insertions(+), 14 deletions(-) diff --git a/src/scene/mod.rs b/src/scene/mod.rs index c498108a2..729944b00 100644 --- a/src/scene/mod.rs +++ b/src/scene/mod.rs @@ -1061,9 +1061,15 @@ pub struct Scene { /// [`WIRE_CONTENT_GEN`] id. `split_face3d_wires` is an O(N) per-wire /// handle lookup + clone that otherwise re-runs every frame. A map, not a /// slot: a paper frame walks several sources (sheet + viewports). + /// + /// `None` for the second element means "no Face3D wire in this set — use + /// the base set itself". The base `Arc` is deliberately NOT stored here: + /// `try_resident_patch` can only move a resident set out for an + /// incremental patch while its `Arc` is uniquely held, so pinning it in + /// this cache would silently force a full rebuild on every edit (#358). #[allow(clippy::type_complexity)] split_cache: - RefCell>, Arc>)>>, + RefCell>, Option>>)>>, /// Cached `selected ∪ hover` handle set for the GPU xray overlay, keyed by /// `selection_generation`. Rebuilt only when the selection changes so /// `build_primitive` doesn't clone the set every frame. diff --git a/src/scene/view/render.rs b/src/scene/view/render.rs index 1a710f690..491d342fa 100644 --- a/src/scene/view/render.rs +++ b/src/scene/view/render.rs @@ -1228,9 +1228,18 @@ impl Scene { // each frame — for every source, since all ids are stable now. let (face3d_wires, other_arc) = { let cached = { self.split_cache.borrow().get(&wire_content_id).cloned() }; - cached.unwrap_or_else(|| { - let (f, o) = split_face3d_wires(&base_arc, &self.document); - let (fa, oa) = (Arc::new(f), Arc::new(o)); + let (fa, oa) = cached.unwrap_or_else(|| { + // No Face3D wire at all (pure 2-D drawings, mesh imports): + // "others" would be a wire-for-wire copy of the base set — + // mark it `None` and use the base set directly instead of + // duplicating it (#358). The base Arc itself must not be + // stored in the cache (see the `split_cache` field docs). + let (fa, oa) = if base_arc.iter().any(|w| is_face3d_wire(w, &self.document)) { + let (f, o) = split_face3d_wires(&base_arc, &self.document); + (Arc::new(f), Some(Arc::new(o))) + } else { + (Arc::new(Vec::new()), None) + }; let mut c = self.split_cache.borrow_mut(); // Ids change on rebuild, so old keys die naturally; the cap // just bounds pathological churn. @@ -1239,7 +1248,8 @@ impl Scene { } c.insert(wire_content_id, (fa.clone(), oa.clone())); (fa, oa) - }) + }); + (fa, oa.unwrap_or_else(|| Arc::clone(&base_arc))) }; // Base wire set — the cached `other` Arc directly, never cloned to // append overlays. Preview / interim wires ride in their own small @@ -1541,9 +1551,19 @@ pub(crate) fn resolve_pattern( (pat_len, pat) } +/// Whether a wire belongs to a Face3D entity, by document handle lookup — +/// so no changes to WireModel are needed. +fn is_face3d_wire(w: &WireModel, document: &acadrust::CadDocument) -> bool { + w.name + .parse::() + .ok() + .and_then(|v| document.get_entity(Handle::new(v))) + .map(|e| matches!(e, EntityType::Face3D(_))) + .unwrap_or(false) +} + /// Partition a wire list into (face3d_wires, other_wires). /// -/// Uses a document handle lookup so no changes to WireModel are needed. /// O(N) per geometry epoch — acceptable since it runs once per epoch. fn split_face3d_wires( wires: &[WireModel], @@ -1552,14 +1572,7 @@ fn split_face3d_wires( let mut face3d = Vec::new(); let mut others = Vec::new(); for w in wires { - let is_face3d = w - .name - .parse::() - .ok() - .and_then(|v| document.get_entity(Handle::new(v))) - .map(|e| matches!(e, EntityType::Face3D(_))) - .unwrap_or(false); - if is_face3d { + if is_face3d_wire(w, document) { face3d.push(w.clone()); } else { others.push(w.clone());