diff --git a/crates/apps/nigig-build/src/construction_frame/pages/workspace/cad/ARCHITECTURE.md b/crates/apps/nigig-build/src/construction_frame/pages/workspace/cad/ARCHITECTURE.md index d78bf5a..cf8966b 100644 --- a/crates/apps/nigig-build/src/construction_frame/pages/workspace/cad/ARCHITECTURE.md +++ b/crates/apps/nigig-build/src/construction_frame/pages/workspace/cad/ARCHITECTURE.md @@ -171,19 +171,26 @@ compile error. wipe redundant. It previously did both on every sync, for all three viewports, on every frame of a drag. - Each viewport also owns a `next_part_id` allocator. A sync must - reconcile it with `scene_holder::reconcile_next_part_id`, never assign - it: only one viewport wins a frame, so the loser's allocations are - absent from the winner's snapshot and a direct assignment moves the - allocator *backwards*, reissuing live ids. + **Part ids are shared, not copied.** All three viewports hold clones + of one `PartIdAllocator` (`Rc>`), wired up by + `CadWorkspace::share_part_id_allocator`. Allocate with + `part_ids.allocate()`; never keep a private counter. - This is still a copy, not shared ownership, and the id allocator is - the clearest evidence that copying is the wrong model — one counter - duplicated three ways cannot be made correct, only bounded. The - remaining Phase 5.2 work is to hand all three viewports one - `Rc>` and delete the snapshot pair; until then - the generation check bounds the cost and the reconcile bounds the - damage. + This was three independent `u64`s synced by copy. Only one viewport + wins a sync frame, so the loser's allocations were absent from the + winner's snapshot and the assignment moved its counter *backwards* — + two parts drawn in different views between syncs got the same + `NodeId`, which keys `MeshCache`, `part_geoms` and every command. + Reconciling on each sync bounded that; sharing removes it. + + Parts adopted from outside the allocator — script rebuild, file load, + undo restore — carry ids it never issued, so `reserve_past_nodes` runs + on every `replace_parts_snapshot`. + + The parts list itself is still copied. That is the rest of Phase 5.2: + one `Rc>` and no snapshot pair. The id allocator + is the proof of the approach — one counter duplicated three ways could + only ever be bounded, never made correct. 5. **`CadTransform::rotation_euler_xyz` is in DEGREES.** This is the crate-wide contract: `math::rot_*_mat`, `makepad_csg::Solid::rotate_*`, diff --git a/crates/apps/nigig-build/src/construction_frame/pages/workspace/cad/mod.rs b/crates/apps/nigig-build/src/construction_frame/pages/workspace/cad/mod.rs index 7299538..9916e6d 100644 --- a/crates/apps/nigig-build/src/construction_frame/pages/workspace/cad/mod.rs +++ b/crates/apps/nigig-build/src/construction_frame/pages/workspace/cad/mod.rs @@ -89,7 +89,7 @@ pub use cad_scene::{ NodeMetadata, PartKind, SceneBuilder, SceneMeta, SceneUnits, SceneVisitor, SheetId, walk_scene, }; -pub use scene_holder::{PartsStore, SceneCache}; +pub use scene_holder::{PartIdAllocator, PartsStore, SceneCache}; // Re-exports so call sites need not name the sub-module. pub(crate) use constants::{DEFAULT_CAD_SCRIPT, LIVE_UPDATE_INTERVAL, local_openai_url, local_openai_model, GENERATED_DIR, GENERATED_SCRIPT_FILE, GENERATED_OBJ_FILE, DEMO_MAX_CURVE_SEGMENTS, DEMO_MAX_SPHERE_RINGS, DEMO_MAX_TORUS_MINOR_SEGMENTS, PART_SELECT_COLOR, PART_PICK_RADIUS, MAX_UNDO_LEVELS, HOVER_PICK_MIN_MOVE_PX, CAD_SCRIPT_TIME_BUDGET, CAD_SCRIPT_BUDGET_SAMPLE_INSTRUCTIONS}; pub(crate) use persistence::{PENDING_ATTACHED_IMAGE, CAD_SCRIPT_OUTPUT, cad_manifest_path, cad_generated_dir_path, cad_generated_script_path, cad_generated_obj_path, load_saved_cad_script, save_cad_state, set_cad_script_output, clear_cad_script_output, take_cad_script_output}; @@ -1659,8 +1659,11 @@ pub struct CadViewport { /// `mark_dirty()` + `invalidate_node(id)`. #[rust] scene_cache: SceneCache, - #[rust(1u64)] - next_part_id: u64, + /// Shared with the other two viewports (see + /// `CadWorkspace::share_part_id_allocator`). Not a plain `u64`: + /// three independent counters synced by copy reissued live ids. + #[rust] + part_ids: PartIdAllocator, #[rust] selection: Vec, #[rust(ViewMode::ThreeD)] diff --git a/crates/apps/nigig-build/src/construction_frame/pages/workspace/cad/scene_holder.rs b/crates/apps/nigig-build/src/construction_frame/pages/workspace/cad/scene_holder.rs index 384f79d..222b301 100644 --- a/crates/apps/nigig-build/src/construction_frame/pages/workspace/cad/scene_holder.rs +++ b/crates/apps/nigig-build/src/construction_frame/pages/workspace/cad/scene_holder.rs @@ -464,35 +464,84 @@ mod tests { /// This is the first increment of Phase 5.1. `CadViewport` still owns the /// store directly; sharing one document across the three viewports (5.2) /// is a separate change. -/// Reconcile a destination viewport's id allocator with an incoming -/// snapshot. +/// Part-id allocator shared by every `CadViewport`. /// -/// The three `CadViewport`s each own a `next_part_id` counter and are -/// kept in step by copying whole snapshots between them. Assigning the -/// source's counter directly -- which is what the sync used to do -- -/// can move a destination's counter *backwards*, because only one -/// viewport wins per frame and the loser's allocations are not in the -/// winner's snapshot. +/// The three viewports used to hold one `u64` each, kept in step by +/// copying whole snapshots. That cannot be made correct: only one +/// viewport wins a sync frame, so the loser's allocations are absent +/// from the winner's snapshot and a direct assignment moves the +/// allocator backwards, reissuing ids that are still live. Two parts +/// drawn in different views between two syncs got the same `NodeId` -- +/// which keys `MeshCache`, `part_geoms` and every command. /// -/// Two parts drawn in different viewports between two syncs therefore -/// both got the same id, and the loser's part was silently dropped. +/// Reconciling the counters on every sync bounded that damage. Sharing +/// one counter removes it: an id handed out anywhere is never handed +/// out again, with no sync step involved. /// -/// The counter must never retreat, and must never collide with an id -/// already present in the incoming parts, so take the maximum of: -/// what the destination has already handed out, what the source has -/// handed out, and one past the highest id actually in the list. -pub fn reconcile_next_part_id( - destination_next: u64, - source_next: u64, - incoming: &[CadNode], -) -> u64 { - let highest_in_use = incoming - .iter() - .map(|n| n.id.raw()) - .max() - .map(|m| m.saturating_add(1)) - .unwrap_or(0); - destination_next.max(source_next).max(highest_in_use) +/// `Rc>` rather than `Arc`: the viewports live on the UI +/// thread. `Cell` because a `u64` is `Copy` and no borrow can outlive +/// the call, so there is no `RefCell` panic to reason about. +/// +/// This is the first piece of Phase 5.2 to become genuinely shared +/// rather than copied. The parts list itself is next. +#[derive(Clone, Debug)] +pub struct PartIdAllocator { + next: std::rc::Rc>, +} + +impl PartIdAllocator { + /// A fresh allocator starting at `first`. + pub fn new(first: u64) -> Self { + Self { next: std::rc::Rc::new(std::cell::Cell::new(first)) } + } + + /// Hand out the next id. + /// + /// Saturates rather than wrapping: reissuing id 0 after 2^64 parts + /// would alias a live node, which is worse than refusing to + /// allocate. + pub fn allocate(&self) -> u64 { + let id = self.next.get(); + self.next.set(id.saturating_add(1)); + id + } + + /// The id that `allocate` will return next. + pub fn peek(&self) -> u64 { + self.next.get() + } + + /// Ensure no id below `bound` is ever handed out. + /// + /// Used when adopting parts from outside the allocator -- a script + /// rebuild, a file load, an undo restore -- whose ids were not + /// issued by it. Never lowers the counter. + pub fn reserve_up_to(&self, bound: u64) { + if bound > self.next.get() { + self.next.set(bound); + } + } + + /// Reserve past every id present in `nodes`. + pub fn reserve_past_nodes(&self, nodes: &[CadNode]) { + if let Some(max) = nodes.iter().map(|n| n.id.raw()).max() { + self.reserve_up_to(max.saturating_add(1)); + } + } + + /// True when both handles refer to the same counter. + /// + /// Only meaningful in tests: production code shares one allocator by + /// construction, and this is what proves it. + pub fn shares_with(&self, other: &Self) -> bool { + std::rc::Rc::ptr_eq(&self.next, &other.next) + } +} + +impl Default for PartIdAllocator { + fn default() -> Self { + Self::new(1) + } } #[derive(Debug, Default)] @@ -773,51 +822,65 @@ mod scene_cache_generation_tests { ); } - // ---- id allocator reconciliation ---- + // ---- shared id allocator ---- - /// The sync used to assign the source's `next_part_id` directly, - /// which can move a destination's allocator BACKWARDS. - /// - /// Only one viewport wins per sync frame. If the user draws in the - /// 2D view and the 3D view between two syncs, both allocate the - /// same id independently, and the loser's counter is then reset to - /// the winner's -- so the very next part reuses an id that is - /// already live. + /// The defect that motivated sharing: two viewports allocating + /// between syncs used to produce the same id. A shared allocator + /// cannot, because there is one counter. #[test] - fn reconciling_never_moves_the_allocator_backwards() { - // Destination has handed out up to 41; source only up to 20. - let next = reconcile_next_part_id(42, 20, &[]); - assert_eq!( - next, 42, - "the destination's allocator must not retreat to the source's" + fn two_handles_never_hand_out_the_same_id() { + let view_2d = PartIdAllocator::new(1); + let view_3d = view_2d.clone(); + + let a = view_2d.allocate(); + let b = view_3d.allocate(); + let c = view_2d.allocate(); + + assert_eq!((a, b, c), (1, 2, 3), "ids must be globally unique"); + assert!( + view_2d.shares_with(&view_3d), + "clone must alias the counter, not copy it" ); + assert_eq!(view_3d.peek(), 4, "both handles observe every allocation"); } - /// It must also clear every id actually present in the incoming - /// list, even when both counters are stale -- a snapshot restored - /// from disk or from undo can carry ids above both. + /// A separately constructed allocator must NOT share -- otherwise + /// the test above would pass for the wrong reason. #[test] - fn reconciling_clears_the_highest_incoming_id() { - let parts = vec![node(3), node(99), node(7)]; - let next = reconcile_next_part_id(5, 5, &parts); - assert_eq!( - next, 100, - "an id already in the list must never be handed out again" - ); + fn separately_constructed_allocators_do_not_share() { + let a = PartIdAllocator::new(1); + let b = PartIdAllocator::new(1); + assert!(!a.shares_with(&b)); + a.allocate(); + assert_eq!(b.peek(), 1, "an unrelated allocator is unaffected"); } - /// The ordinary case: nothing stale, so the counters agree. + /// Parts arriving from outside the allocator -- a script rebuild, a + /// file load, an undo restore -- carry ids it never issued. It must + /// step past them or the next allocation aliases a live node. #[test] - fn reconciling_is_a_no_op_when_the_viewports_agree() { - let parts = vec![node(1), node(2)]; - assert_eq!(reconcile_next_part_id(3, 3, &parts), 3); + fn reserving_past_adopted_nodes_prevents_reuse() { + let alloc = PartIdAllocator::new(1); + alloc.reserve_past_nodes(&[node(3), node(99), node(7)]); + assert_eq!(alloc.allocate(), 100); } - /// u64::MAX must not wrap into 0 and start reissuing live ids. + /// Reserving must never lower the counter: ids already handed out + /// are live even if the incoming list happens to be shorter. #[test] - fn reconciling_saturates_instead_of_wrapping() { - let parts = vec![node(u64::MAX)]; - assert_eq!(reconcile_next_part_id(0, 0, &parts), u64::MAX); + fn reserving_never_moves_the_counter_backwards() { + let alloc = PartIdAllocator::new(50); + alloc.reserve_up_to(10); + alloc.reserve_past_nodes(&[node(2)]); + assert_eq!(alloc.peek(), 50); + } + + /// Saturate rather than wrap. Wrapping to 0 would reissue live ids. + #[test] + fn allocation_saturates_at_the_top_of_the_range() { + let alloc = PartIdAllocator::new(u64::MAX); + assert_eq!(alloc.allocate(), u64::MAX); + assert_eq!(alloc.peek(), u64::MAX, "must not wrap to 0"); } #[test] diff --git a/crates/apps/nigig-build/src/construction_frame/pages/workspace/cad/viewport.rs b/crates/apps/nigig-build/src/construction_frame/pages/workspace/cad/viewport.rs index a618eb0..e3a21af 100644 --- a/crates/apps/nigig-build/src/construction_frame/pages/workspace/cad/viewport.rs +++ b/crates/apps/nigig-build/src/construction_frame/pages/workspace/cad/viewport.rs @@ -131,12 +131,24 @@ impl CadViewport { cx.redraw_all(); } - pub(crate) fn parts_snapshot(&self) -> (Vec, Vec, u64) { - ( - self.parts.as_slice().to_vec(), - self.selection.clone(), - self.next_part_id, - ) + pub(crate) fn parts_snapshot(&self) -> (Vec, Vec) { + (self.parts.as_slice().to_vec(), self.selection.clone()) + } + + /// Hand this viewport the allocator the others use. + /// + /// Called once per viewport at startup. Until every viewport shares + /// one, ids are only unique per view. + pub(crate) fn adopt_part_id_allocator(&mut self, shared: PartIdAllocator) { + // Anything this viewport already handed out must stay reserved. + shared.reserve_up_to(self.part_ids.peek()); + shared.reserve_past_nodes(self.parts.as_slice()); + self.part_ids = shared; + } + + /// The allocator handle, for sharing with the other viewports. + pub(crate) fn part_id_allocator(&self) -> PartIdAllocator { + self.part_ids.clone() } /// The `PartsStore` generation this viewport is currently showing. @@ -154,7 +166,6 @@ impl CadViewport { cx: &mut Cx, parts: Vec, selection: Vec, - next_part_id: u64, ) { // Which node ids survive this replacement. GPU geometry for a // surviving id is still valid -- the mesh is rebuilt from the @@ -169,15 +180,11 @@ impl CadViewport { parts.iter().map(|p| p.id.raw()).collect(); self.part_geoms.retain(|id, _| incoming.contains(id)); - // Never let the id allocator retreat. Only one viewport wins a - // sync frame, so the loser's allocations are absent from this - // snapshot -- assigning the source's counter directly would - // reissue ids that are already live in the other view. - self.next_part_id = super::scene_holder::reconcile_next_part_id( - self.next_part_id, - next_part_id, - &parts, - ); + // No id reconciliation here any more: the allocator is shared, + // so an id handed out in any viewport is already accounted for + // in all of them. Parts adopted from outside the allocator + // (script rebuild, file load) still need reserving. + self.part_ids.reserve_past_nodes(&parts); self.parts.replace_all(parts); self.selection = selection; // The scene snapshot is rebuilt on demand: `scene_for` compares @@ -454,8 +461,7 @@ impl CadViewport { } pub(crate) fn add_part(&mut self, cx: &mut Cx, kind: PartKind) -> u64 { - let id = self.next_part_id; - self.next_part_id += 1; + let id = self.part_ids.allocate(); let (size, color) = match kind { PartKind::Cube => (vec3(1.0, 1.0, 1.0), vec4(0.34, 0.74, 0.86, 1.0)), PartKind::Cylinder => (vec3(1.0, 1.2, 1.0), vec4(0.55, 0.82, 0.55, 1.0)), @@ -1217,8 +1223,7 @@ impl CadViewport { let seg_len = total_len / n as f32; let mut new_ids = Vec::new(); for i in 0..n { - let new_id = self.next_part_id; - self.next_part_id += 1; + let new_id = self.part_ids.allocate(); let offset = (i as f32 + 0.5) * seg_len - total_len * 0.5; let mut pos = part.pos(); let mut size = part.size(); @@ -1331,8 +1336,7 @@ impl CadViewport { let clipboard = self.clipboard_parts.clone(); let mut new_ids = Vec::new(); for part in &clipboard { - let id = self.next_part_id; - self.next_part_id += 1; + let id = self.part_ids.allocate(); let mut new_part = part.clone(); new_part.id = NodeId(id); let mut p = new_part.pos(); @@ -3263,8 +3267,7 @@ impl CadViewport { } let start = self.drawing.start_world; let end = self.drawing.current_world; - let id = self.next_part_id; - self.next_part_id += 1; + let id = self.part_ids.allocate(); match self.drawing.tool { CadTool::Arc => { if let Some(mid) = self.drawing.arc_mid_world { @@ -3703,8 +3706,7 @@ impl CadViewport { } else { (0.2f32, dist as f32) }; - let id = self.next_part_id; - self.next_part_id += 1; + let id = self.part_ids.allocate(); self.parts.push(CadNode { id: NodeId(id), name: format!("Wall-{}", id), diff --git a/crates/apps/nigig-build/src/construction_frame/pages/workspace/cad/workspace.rs b/crates/apps/nigig-build/src/construction_frame/pages/workspace/cad/workspace.rs index c34ef80..0dd0829 100644 --- a/crates/apps/nigig-build/src/construction_frame/pages/workspace/cad/workspace.rs +++ b/crates/apps/nigig-build/src/construction_frame/pages/workspace/cad/workspace.rs @@ -314,20 +314,20 @@ impl CadWorkspace { .widget(cx, ids!(cad_viewport)) .borrow_mut::() { - let (parts, selected, next_id) = vp.parts_snapshot(); + let (parts, selected) = vp.parts_snapshot(); if let Some(mut vp2d) = self .view .widget(cx, ids!(cad_viewport_2d)) .borrow_mut::() { - vp2d.replace_parts_snapshot(cx, parts.clone(), selected.clone(), next_id); + vp2d.replace_parts_snapshot(cx, parts.clone(), selected.clone()); } if let Some(mut vp3d) = self .view .widget(cx, ids!(cad_viewport_3d)) .borrow_mut::() { - vp3d.replace_parts_snapshot(cx, parts, selected, next_id); + vp3d.replace_parts_snapshot(cx, parts, selected); } } } @@ -617,8 +617,8 @@ impl CadWorkspace { return None; } let script = vp.generate_parts_script(); - let (parts, selected, next_id) = vp.parts_snapshot(); - Some((script, parts, selected, next_id, vp.parts_generation())) + let (parts, selected) = vp.parts_snapshot(); + Some((script, parts, selected, vp.parts_generation())) }); if let Some(Some(taken)) = taken { if snapshot.is_none() { @@ -627,7 +627,7 @@ impl CadWorkspace { } } } - let (Some((script, parts, selected, next_id, source_generation)), Some(source)) = + let (Some((script, parts, selected, source_generation)), Some(source)) = (snapshot, source) else { return None; @@ -651,7 +651,7 @@ impl CadWorkspace { continue; } self.with_viewport(cx, which, |vp, cx| { - vp.replace_parts_snapshot(cx, parts.clone(), selected.clone(), next_id); + vp.replace_parts_snapshot(cx, parts.clone(), selected.clone()); }); } if script.is_empty() { @@ -803,7 +803,27 @@ impl CadWorkspace { self.set_viewport_layout_mode(cx, next); } + /// Give all three viewports the same `PartIdAllocator`. + /// + /// Idempotent and cheap, so it runs on the same path as worker + /// creation rather than needing its own init hook. Until this has + /// run, each viewport allocates from its own counter and ids are + /// only unique per view -- which is the bug this exists to remove. + fn share_part_id_allocator(&mut self, cx: &mut Cx) { + let Some(shared) = self.with_viewport(cx, 0, |vp, _cx| vp.part_id_allocator()) else { + return; + }; + for which in [1usize, 2] { + self.with_viewport(cx, which, |vp, _cx| { + if !vp.part_id_allocator().shares_with(&shared) { + vp.adopt_part_id_allocator(shared.clone()); + } + }); + } + } + fn request_rebuild(&mut self, cx: &mut Cx, force: bool, save_output: bool) { + self.share_part_id_allocator(cx); if self.cad_worker.is_none() { self.cad_worker = Some(CadRebuildWorker::new(cx)); } diff --git a/crates/apps/nigig-build/src/construction_frame/pages/workspace/project/mod.rs b/crates/apps/nigig-build/src/construction_frame/pages/workspace/project/mod.rs index aa307bc..eb04ef1 100644 --- a/crates/apps/nigig-build/src/construction_frame/pages/workspace/project/mod.rs +++ b/crates/apps/nigig-build/src/construction_frame/pages/workspace/project/mod.rs @@ -813,18 +813,17 @@ impl Widget for BuildProjectsPage { if let Some(mut ws) = self.view.widget(cx, ids!(spreadsheet_workspace)).borrow_mut::() { ws.save_to_disk(cx); } - // 2) Load the active sheet's data into the mobile grid + // 2) Point the mobile grid at the saved workbook. + // + // The grid no longer owns a SpreadsheetData; it reads + // through a SharedWorkspaceModel. `load_saved` handles + // both the multi-sheet and the legacy single-sheet + // formats, which this used to open-code. if let Some(mut grid) = self.view.widget(cx, ids!(m_ss_grid)).borrow_mut::() { - if let Some(saved) = spreadsheet_ui::persistence::load_saved_spreadsheet_state() { - if let Some((sheets, active_idx)) = spreadsheet_ui::workbook::deserialize_workbook(&saved) { - if let Some(sheet) = sheets.get(active_idx) { - grid.data = sheet.data.clone(); - } - } else { - // Legacy single-sheet format - grid.data = spreadsheet_ui::SpreadsheetData::default(); - grid.data.deserialize(&saved); - } + if let Some(model) = spreadsheet_ui::model::WorkspaceModel::load_saved() { + grid.attach_workspace_model(std::rc::Rc::new( + std::cell::RefCell::new(model), + )); } grid.selection_anchor = Some((0, 0)); grid.selection_head = Some((0, 0)); @@ -836,19 +835,13 @@ impl Widget for BuildProjectsPage { // Was mobile → going to desktop: // 1) Update the saved workbook with mobile grid's active sheet data // (preserves the other sheets that were saved from desktop) + // The grid edits the shared model in place, so saving it + // preserves every sheet -- no read-modify-write of the + // active sheet against the on-disk workbook. if let Some(grid) = self.view.widget(cx, ids!(m_ss_grid)).borrow::() { - let active_data = grid.data.serialize(); - if let Some(saved) = spreadsheet_ui::persistence::load_saved_spreadsheet_state() { - if let Some((mut sheets, active_idx)) = spreadsheet_ui::workbook::deserialize_workbook(&saved) { - if let Some(sheet) = sheets.get_mut(active_idx) { - sheet.data = spreadsheet_ui::SpreadsheetData::default(); - sheet.data.deserialize(&active_data); - } - let updated = spreadsheet_ui::workbook::serialize_workbook(&sheets, active_idx); - let _ = spreadsheet_ui::persistence::save_spreadsheet_state(&updated); - } else { - // Legacy format — just save the grid data as-is - let _ = spreadsheet_ui::persistence::save_spreadsheet_state(&active_data); + if let Some(model) = &grid.shared_model { + if let Err(e) = model.borrow().save() { + error!("spreadsheet: mobile->desktop save failed: {e}"); } } }