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 cf8966b..d78bf5a 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,26 +171,19 @@ compile error. wipe redundant. It previously did both on every sync, for all three viewports, on every frame of a drag. - **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. + 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. - 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. + 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. 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 9916e6d..7299538 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::{PartIdAllocator, PartsStore, SceneCache}; +pub use scene_holder::{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,11 +1659,8 @@ pub struct CadViewport { /// `mark_dirty()` + `invalidate_node(id)`. #[rust] scene_cache: SceneCache, - /// 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(1u64)] + next_part_id: u64, #[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 222b301..384f79d 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,84 +464,35 @@ 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. -/// Part-id allocator shared by every `CadViewport`. +/// Reconcile a destination viewport's id allocator with an incoming +/// 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. +/// 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. /// -/// 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. +/// Two parts drawn in different viewports between two syncs therefore +/// both got the same id, and the loser's part was silently dropped. /// -/// `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) - } +/// 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) } #[derive(Debug, Default)] @@ -822,65 +773,51 @@ mod scene_cache_generation_tests { ); } - // ---- shared id allocator ---- + // ---- id allocator reconciliation ---- - /// 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. + /// 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. #[test] - 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" + 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" ); - assert_eq!(view_3d.peek(), 4, "both handles observe every allocation"); } - /// A separately constructed allocator must NOT share -- otherwise - /// the test above would pass for the wrong reason. + /// 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. #[test] - 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"); + 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" + ); } - /// 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. + /// The ordinary case: nothing stale, so the counters agree. #[test] - 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); + 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); } - /// Reserving must never lower the counter: ids already handed out - /// are live even if the incoming list happens to be shorter. + /// u64::MAX must not wrap into 0 and start reissuing live ids. #[test] - 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"); + fn reconciling_saturates_instead_of_wrapping() { + let parts = vec![node(u64::MAX)]; + assert_eq!(reconcile_next_part_id(0, 0, &parts), u64::MAX); } #[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 e3a21af..a618eb0 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,24 +131,12 @@ impl CadViewport { cx.redraw_all(); } - 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() + pub(crate) fn parts_snapshot(&self) -> (Vec, Vec, u64) { + ( + self.parts.as_slice().to_vec(), + self.selection.clone(), + self.next_part_id, + ) } /// The `PartsStore` generation this viewport is currently showing. @@ -166,6 +154,7 @@ 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 @@ -180,11 +169,15 @@ impl CadViewport { parts.iter().map(|p| p.id.raw()).collect(); self.part_geoms.retain(|id, _| incoming.contains(id)); - // 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); + // 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, + ); self.parts.replace_all(parts); self.selection = selection; // The scene snapshot is rebuilt on demand: `scene_for` compares @@ -461,7 +454,8 @@ impl CadViewport { } pub(crate) fn add_part(&mut self, cx: &mut Cx, kind: PartKind) -> u64 { - let id = self.part_ids.allocate(); + let id = self.next_part_id; + self.next_part_id += 1; 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)), @@ -1223,7 +1217,8 @@ 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.part_ids.allocate(); + let new_id = self.next_part_id; + self.next_part_id += 1; let offset = (i as f32 + 0.5) * seg_len - total_len * 0.5; let mut pos = part.pos(); let mut size = part.size(); @@ -1336,7 +1331,8 @@ impl CadViewport { let clipboard = self.clipboard_parts.clone(); let mut new_ids = Vec::new(); for part in &clipboard { - let id = self.part_ids.allocate(); + let id = self.next_part_id; + self.next_part_id += 1; let mut new_part = part.clone(); new_part.id = NodeId(id); let mut p = new_part.pos(); @@ -3267,7 +3263,8 @@ impl CadViewport { } let start = self.drawing.start_world; let end = self.drawing.current_world; - let id = self.part_ids.allocate(); + let id = self.next_part_id; + self.next_part_id += 1; match self.drawing.tool { CadTool::Arc => { if let Some(mid) = self.drawing.arc_mid_world { @@ -3706,7 +3703,8 @@ impl CadViewport { } else { (0.2f32, dist as f32) }; - let id = self.part_ids.allocate(); + let id = self.next_part_id; + self.next_part_id += 1; 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 0dd0829..c34ef80 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) = vp.parts_snapshot(); + let (parts, selected, next_id) = 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()); + vp2d.replace_parts_snapshot(cx, parts.clone(), selected.clone(), next_id); } if let Some(mut vp3d) = self .view .widget(cx, ids!(cad_viewport_3d)) .borrow_mut::() { - vp3d.replace_parts_snapshot(cx, parts, selected); + vp3d.replace_parts_snapshot(cx, parts, selected, next_id); } } } @@ -617,8 +617,8 @@ impl CadWorkspace { return None; } let script = vp.generate_parts_script(); - let (parts, selected) = vp.parts_snapshot(); - Some((script, parts, selected, vp.parts_generation())) + let (parts, selected, next_id) = vp.parts_snapshot(); + Some((script, parts, selected, next_id, vp.parts_generation())) }); if let Some(Some(taken)) = taken { if snapshot.is_none() { @@ -627,7 +627,7 @@ impl CadWorkspace { } } } - let (Some((script, parts, selected, source_generation)), Some(source)) = + let (Some((script, parts, selected, next_id, 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()); + vp.replace_parts_snapshot(cx, parts.clone(), selected.clone(), next_id); }); } if script.is_empty() { @@ -803,27 +803,7 @@ 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 eb04ef1..aa307bc 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,17 +813,18 @@ impl Widget for BuildProjectsPage { if let Some(mut ws) = self.view.widget(cx, ids!(spreadsheet_workspace)).borrow_mut::() { ws.save_to_disk(cx); } - // 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. + // 2) Load the active sheet's data into the mobile grid if let Some(mut grid) = self.view.widget(cx, ids!(m_ss_grid)).borrow_mut::() { - if let Some(model) = spreadsheet_ui::model::WorkspaceModel::load_saved() { - grid.attach_workspace_model(std::rc::Rc::new( - std::cell::RefCell::new(model), - )); + 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); + } } grid.selection_anchor = Some((0, 0)); grid.selection_head = Some((0, 0)); @@ -835,13 +836,19 @@ 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::() { - if let Some(model) = &grid.shared_model { - if let Err(e) = model.borrow().save() { - error!("spreadsheet: mobile->desktop save failed: {e}"); + 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); } } }