From 1490fe4ea91dd83498bc7c280e124a07f194c37f Mon Sep 17 00:00:00 2001 From: Hakan Seven Date: Wed, 15 Jul 2026 11:11:17 +0300 Subject: [PATCH] perf(undo): record entity deltas instead of cloning the whole document MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Frequent entity-only edits (MOVE/ROTATE/SCALE/MIRROR/ALIGN, COPY/ARRAY, draw-add, ERASE) captured a full ~800k-entity CadDocument clone (~46ms) on every edit — the last big per-edit cost after the incremental-render work. They now record only the touched entities' before/after images: - Scene self-records through the five mutation primitives when a recording is open (UndoRecording + is_recording_undo guard, zero cost off-path). - apply_entity_delta installs one side per handle (in-place / re-insert with the original handle / remove) plus a per-handle cache reseed, then a single bump_entities so the incremental caches patch instead of a full re-tess — undo/redo of a small edit is fast too, not just the edit. - HistorySnapshot is now Full | Delta; a Delta is self-inverting and moves between the undo/redo stacks without re-capturing. Commands that touch non-entity state (new layer, group, *D block, viewport) fall back to the full snapshot via per-command predicates, and a poison flag debug-asserts any mis-scope. remove_entity leaves the handle dangling in its owner block record's entity_handles (harmless on its own, a lookup miss is skipped), so a re-insert strips stale entries first — otherwise add_entity would list the handle twice and the DXF/DWG writer would emit the entity twice. Round-trip tests cover transform/erase/add/copy (do->undo->redo equality, handle preservation, no duplication). Co-Authored-By: Claude Opus 4.8 --- src/app/command_driver.rs | 54 ++++++- src/app/commands/draw.rs | 6 +- src/app/document.rs | 43 ++++- src/app/history.rs | 228 ++++++++++++++++++++++++-- src/app/update/mod.rs | 8 +- src/scene/entity.rs | 85 ++++++++++ src/scene/mod.rs | 329 ++++++++++++++++++++++++++++++++++++++ src/scene/modify.rs | 19 +++ src/scene/selection.rs | 15 ++ 9 files changed, 761 insertions(+), 26 deletions(-) diff --git a/src/app/command_driver.rs b/src/app/command_driver.rs index 322fe0e6b..535552f0d 100644 --- a/src/app/command_driver.rs +++ b/src/app/command_driver.rs @@ -230,17 +230,27 @@ impl OpenCADStudio { self.update_cont_anchor(&entity); } let label = self.history_label_from_active_cmd(i, "ENTITY"); - self.push_undo_snapshot(i, label); + // A plain drawable added on an existing layer touches only the + // one new entity; a block/image/dimension/viewport add also + // mutates layers/objects/blocks → keep the full snapshot. + let delta_safe = self.delta_add_safe(i, &entity); + let pending = self.begin_undo(i, label, 1, delta_safe); self.commit_entity(entity); self.tabs[i].dirty = true; let prompt = self.tabs[i].active_cmd.as_ref().map(|c| c.prompt()); if let Some(p) = prompt { self.command_line.push_info(&p); } + if let Some(pd) = pending { + self.commit_undo_delta(i, pd); + } } CmdResult::TransformSelected(handles, transform) => { let label = self.history_label_from_active_cmd(i, "MOVE"); - self.push_undo_snapshot(i, label); + // A move/rotate/scale/mirror mutates only the selected entities + // (and their baked dimension sub-entities) through + // transform_entities — always delta-safe. + let pending = self.begin_undo(i, label, handles.len(), true); self.tabs[i].scene.transform_entities(&handles, &transform); // ACIS solids render from a cached mesh, so a move/rotate/ // scale/mirror needs the mesh re-tessellated from the now-moved @@ -255,10 +265,16 @@ impl OpenCADStudio { self.tabs[i].snap_result = None; self.restore_pre_cmd_tangent(); self.refresh_properties(); + if let Some(p) = pending { + self.commit_undo_delta(i, p); + } } CmdResult::CopySelected(handles, transform) => { let label = self.history_label_from_active_cmd(i, "COPY"); - self.push_undo_snapshot(i, label); + // Copying a dimension clones a *D block record, so gate delta on + // the selection being dimension-free. + let delta_safe = self.delta_copy_safe(i, &handles); + let pending = self.begin_undo(i, label, handles.len(), delta_safe); let new_handles = self.tabs[i].scene.copy_entities(&handles, &transform); if self.tabs[i].scene.any_solid(&new_handles) { self.tabs[i].scene.populate_meshes_from_document(); @@ -274,6 +290,9 @@ impl OpenCADStudio { self.command_line.push_info(&p); } self.refresh_properties(); + if let Some(pd) = pending { + self.commit_undo_delta(i, pd); + } } CmdResult::CommitAndExit(entity) => { // For XATTACH: ensure the xref block definition exists before @@ -301,13 +320,17 @@ impl OpenCADStudio { // (before `entity` is moved into commit_entity). self.update_cont_anchor(&entity); let label = self.history_label_from_active_cmd(i, "ENTITY"); - self.push_undo_snapshot(i, label); + let delta_safe = self.delta_add_safe(i, &entity); + let pending = self.begin_undo(i, label, 1, delta_safe); self.commit_entity(entity); self.tabs[i].dirty = true; self.tabs[i].scene.clear_preview_wire(); self.tabs[i].active_cmd = None; self.tabs[i].snap_result = None; self.restore_pre_cmd_tangent(); + if let Some(pd) = pending { + self.commit_undo_delta(i, pd); + } } CmdResult::CommitSolid { entity, solid } => { let label = self.history_label_from_active_cmd(i, "SOLID"); @@ -321,7 +344,8 @@ impl OpenCADStudio { } CmdResult::CommitAndEditText(entity) => { let label = self.history_label_from_active_cmd(i, "ENTITY"); - self.push_undo_snapshot(i, label); + let delta_safe = self.delta_add_safe(i, &entity); + let pending = self.begin_undo(i, label, 1, delta_safe); let handle = self.commit_entity_handle(entity); self.tabs[i].dirty = true; self.tabs[i].scene.clear_preview_wire(); @@ -329,6 +353,9 @@ impl OpenCADStudio { self.tabs[i].snap_result = None; self.restore_pre_cmd_tangent(); self.ribbon.deactivate_tool(); + if let Some(pd) = pending { + self.commit_undo_delta(i, pd); + } if let Some(h) = handle { return self.begin_text_edit(h); } @@ -419,8 +446,12 @@ impl OpenCADStudio { } CmdResult::BatchCopy(handles, transforms) => { let label = self.history_label_from_active_cmd(i, "ARRAY"); - self.push_undo_snapshot(i, label.clone()); let count = transforms.len(); + // Same gate as COPY (dimension-free), sized by the total number + // of copies the array will add. + let delta_safe = self.delta_copy_safe(i, &handles); + let pending = + self.begin_undo(i, label.clone(), handles.len() * count, delta_safe); for t in &transforms { self.tabs[i].scene.copy_entities(&handles, t); } @@ -433,6 +464,9 @@ impl OpenCADStudio { self.command_line .push_output(&format!("{label}: {count} {noun} created.")); self.refresh_properties(); + if let Some(pd) = pending { + self.commit_undo_delta(i, pd); + } } CmdResult::ReplaceMany(replacements, additions) => { let label = self.history_label_from_active_cmd(i, "FILLET"); @@ -962,7 +996,10 @@ impl OpenCADStudio { self.restore_pre_cmd_tangent(); } else { let label = self.history_label_from_active_cmd(i, "ALIGN"); - self.push_undo_snapshot(i, label); + // A chain of transform_entities on the same handles — the + // recording keeps each entity's first (pre-align) image, so + // it's delta-safe like a single transform. + let pending = self.begin_undo(i, label, handles.len(), true); // Step 1: translate so src1 is at origin self.tabs[i].scene.transform_entities( &handles, @@ -1004,6 +1041,9 @@ impl OpenCADStudio { self.restore_pre_cmd_tangent(); self.command_line.push_output("ALIGN: applied."); self.refresh_properties(); + if let Some(pd) = pending { + self.commit_undo_delta(i, pd); + } } } CmdResult::LengthenEntity { diff --git a/src/app/commands/draw.rs b/src/app/commands/draw.rs index 9b67e69c9..c8ca72813 100644 --- a/src/app/commands/draw.rs +++ b/src/app/commands/draw.rs @@ -696,7 +696,8 @@ impl OpenCADStudio { self.tabs[i].active_cmd = Some(Box::new(cmd)); } else { let n = handles.len(); - self.push_undo_snapshot(i, "ERASE"); + let delta_safe = self.delta_erase_safe(i, &handles); + let pending = self.begin_undo(i, "ERASE", handles.len(), delta_safe); // Stash the erased entities so OOPS can restore them. self.oops_cache = handles .iter() @@ -707,6 +708,9 @@ impl OpenCADStudio { self.refresh_properties(); self.command_line .push_output(&format!("{n} object(s) erased.")); + if let Some(pd) = pending { + self.commit_undo_delta(i, pd); + } } } diff --git a/src/app/document.rs b/src/app/document.rs index df181602f..1696a3694 100644 --- a/src/app/document.rs +++ b/src/app/document.rs @@ -8,7 +8,7 @@ use crate::scene::Scene; use crate::snap::SnapResult; use crate::ui::{LayerPanel, PropertiesPanel}; use acadrust::tables::Ucs; -use acadrust::{CadDocument, Handle}; +use acadrust::{CadDocument, EntityType, Handle}; use iced; use std::any::Any; use std::collections::HashMap; @@ -472,8 +472,30 @@ impl DocumentTab { } } +/// One undo/redo entry. Most edits touch the whole drawing's state and store a +/// [`FullSnapshot`] (a document clone); frequent entity-only edits (move / copy / +/// draw / erase) instead store a cheap [`DeltaSnapshot`] naming just the entities +/// they changed, so an 800k-entity drawing doesn't pay a ~46 ms document clone +/// per edit. #[derive(Clone)] -pub(super) struct HistorySnapshot { +pub(super) enum HistorySnapshot { + Full(FullSnapshot), + Delta(DeltaSnapshot), +} + +impl HistorySnapshot { + pub(super) fn label(&self) -> &str { + match self { + HistorySnapshot::Full(f) => &f.label, + HistorySnapshot::Delta(d) => &d.label, + } + } +} + +/// A whole-document undo entry: the safe fallback for any command that may touch +/// layers, objects, block records, styles or tables. +#[derive(Clone)] +pub(super) struct FullSnapshot { pub(super) document: CadDocument, pub(super) current_layout: String, pub(super) selected: Vec, @@ -481,6 +503,23 @@ pub(super) struct HistorySnapshot { pub(super) label: String, } +/// An entity-only undo entry: for each touched handle, its before-image and +/// after-image (`None` = the entity was absent on that side, i.e. an add or an +/// erase). Symmetric — undo applies the before side, redo the after side — so +/// one delta moves between the undo and redo stacks without re-capturing. The +/// command by construction changes no layers / objects / blocks, so only the +/// cheap scalars (`current_layout`, selection, `dirty`) ride alongside. +#[derive(Clone)] +pub(super) struct DeltaSnapshot { + pub(super) entities: Vec<(Handle, Option, Option)>, + pub(super) current_layout: String, + pub(super) selected_before: Vec, + pub(super) selected_after: Vec, + pub(super) dirty_before: bool, + pub(super) dirty_after: bool, + pub(super) label: String, +} + #[derive(Default)] pub(super) struct HistoryState { pub(super) undo_stack: Vec, diff --git a/src/app/history.rs b/src/app/history.rs index 35230bc69..9b95eb071 100644 --- a/src/app/history.rs +++ b/src/app/history.rs @@ -1,6 +1,26 @@ -use super::{document::HistorySnapshot, OpenCADStudio}; +use super::{ + document::{DeltaSnapshot, FullSnapshot, HistorySnapshot}, + OpenCADStudio, +}; +use acadrust::{EntityType, Handle}; use rustc_hash::FxHashSet as HashSet; +/// Above this many touched entities a "delta-safe" command falls back to a full +/// snapshot: deep-cloning that many before-images would cost more than the (now +/// Arc-cheap) document clone, and huge bulk edits are rare. Below it, the delta +/// keeps a per-edit undo at roughly zero cost. +const DELTA_UNDO_MAX_ENTITIES: usize = 5000; + +/// Pre-command state captured by [`OpenCADStudio::begin_undo`] and handed back to +/// [`OpenCADStudio::commit_undo_delta`] to close a delta entry. Lives on the +/// stack across the (synchronous) command body — no per-tab field needed. +pub(super) struct PendingDelta { + label: String, + current_layout: String, + selected_before: Vec, + dirty_before: bool, +} + impl OpenCADStudio { pub(super) fn history_label_from_active_cmd(&self, i: usize, fallback: &'static str) -> String { self.tabs[i] @@ -15,13 +35,13 @@ impl OpenCADStudio { i: usize, label: impl Into, ) -> HistorySnapshot { - HistorySnapshot { + HistorySnapshot::Full(FullSnapshot { document: self.tabs[i].scene.document.clone(), current_layout: self.tabs[i].scene.current_layout.clone(), selected: self.tabs[i].scene.selected.iter().copied().collect(), dirty: self.tabs[i].dirty, label: label.into(), - } + }) } pub(super) fn push_undo_snapshot(&mut self, i: usize, label: impl Into) { @@ -30,7 +50,168 @@ impl OpenCADStudio { self.tabs[i].history.redo_stack.clear(); } - pub(super) fn restore_history_snapshot(&mut self, i: usize, snapshot: HistorySnapshot) { + /// Begin undo capture for an entity edit that will touch `touched` entities. + /// When `delta_safe` (the caller's per-command predicate guarantees the edit + /// mutates only entities, through the five Scene primitives) and the edit is + /// small enough, starts a cheap Scene delta recording and returns the + /// pre-command state to pass to [`OpenCADStudio::commit_undo_delta`] after the + /// mutation. Otherwise pushes a full snapshot and returns `None`. + pub(super) fn begin_undo( + &mut self, + i: usize, + label: impl Into, + touched: usize, + delta_safe: bool, + ) -> Option { + let label = label.into(); + if delta_safe && touched <= DELTA_UNDO_MAX_ENTITIES { + self.tabs[i].scene.begin_undo_recording(); + Some(PendingDelta { + label, + current_layout: self.tabs[i].scene.current_layout.clone(), + selected_before: self.tabs[i].scene.selected.iter().copied().collect(), + dirty_before: self.tabs[i].dirty, + }) + } else { + self.push_undo_snapshot(i, label); + None + } + } + + /// Copy is delta-safe only when no target is a Dimension: copying a + /// dimension clones a fresh anonymous `*D` block record (non-entity state a + /// pure-entity delta can't restore). + pub(super) fn delta_copy_safe(&self, i: usize, handles: &[Handle]) -> bool { + let doc = &self.tabs[i].scene.document; + !handles + .iter() + .any(|h| matches!(doc.get_entity(*h), Some(EntityType::Dimension(_)))) + } + + /// Erase is delta-safe only when no target belongs to a group: erasing a + /// grouped entity rewrites the group's membership in `document.objects`. + pub(super) fn delta_erase_safe(&self, i: usize, handles: &[Handle]) -> bool { + use acadrust::objects::ObjectType; + let doc = &self.tabs[i].scene.document; + !doc.objects.values().any(|o| match o { + ObjectType::Group(g) => g.entities.iter().any(|h| handles.contains(h)), + _ => false, + }) + } + + /// Add is delta-safe only for a plain drawable on an already-existing layer: + /// an insert / block / image / dimension add also creates block records, + /// image definitions or layers (non-entity state). + pub(super) fn delta_add_safe(&self, i: usize, entity: &EntityType) -> bool { + if matches!( + entity, + EntityType::Insert(_) + | EntityType::Block(_) + | EntityType::BlockEnd(_) + | EntityType::RasterImage(_) + | EntityType::Dimension(_) + // A viewport commit routes through add_entity_to_layout + + // bump_geometry_no_blocks, bypassing the recorded Scene::add_entity + // — nothing would be captured, so keep the full snapshot. + | EntityType::Viewport(_) + ) { + return false; + } + let layer = entity.common().layer.clone(); + layer.trim().is_empty() || self.tabs[i].scene.document.layers.contains(&layer) + } + + /// Close the delta transaction opened by [`OpenCADStudio::begin_undo`]: + /// harvests the recorded before-images, pairs each with the entity's current + /// (after) state, and pushes a symmetric [`DeltaSnapshot`] onto the undo + /// stack. Called after the command's mutations (and after `dirty`/selection + /// reach their final values). + pub(super) fn commit_undo_delta(&mut self, i: usize, pending: PendingDelta) { + let Some(rec) = self.tabs[i].scene.take_undo_recording() else { + return; + }; + // The predicate should have kept the command entity-only; if a primitive + // still poisoned the recording we've already mutated and the pre-command + // full state is gone, so the entity delta is the best we can do (its + // entity part stays correct; a leaked layer/group/block just won't + // revert). Loud in debug, a warning in release — never a silent wrong. + debug_assert!( + !rec.is_poisoned(), + "delta command '{}' mutated non-entity state", + pending.label + ); + if rec.is_empty() { + // Nothing actually changed (e.g. every target was on a locked + // layer). Leave the undo/redo stacks untouched. + return; + } + if rec.is_poisoned() { + eprintln!( + "[undo] delta '{}' touched non-entity state; undo may be incomplete", + pending.label + ); + } + let entities: Vec<(Handle, Option, Option)> = rec + .into_before_images() + .into_iter() + .map(|(h, before)| { + let after = self.tabs[i].scene.document.get_entity(h).cloned(); + (h, before, after) + }) + .collect(); + let selected_after = self.tabs[i].scene.selected.iter().copied().collect(); + let dirty_after = self.tabs[i].dirty; + let delta = DeltaSnapshot { + entities, + current_layout: pending.current_layout, + selected_before: pending.selected_before, + selected_after, + dirty_before: pending.dirty_before, + dirty_after, + label: pending.label, + }; + self.tabs[i] + .history + .undo_stack + .push(HistorySnapshot::Delta(delta)); + self.tabs[i].history.redo_stack.clear(); + } + + /// Apply one side of a delta entry in place: `undo` restores each entity's + /// before-image, `!undo` (redo) its after-image. `None` on the chosen side + /// means the entity is absent there (erase it). Reports the exact touched + /// handles through `bump_entities` so the incremental caches patch per-handle + /// — no full re-tessellation, no `populate_*` document walk. + fn apply_delta(&mut self, i: usize, d: &DeltaSnapshot, undo: bool) { + // Install the chosen side of every entity image (Scene handles the + // in-place / re-insert / remove, the derived-cache reseed and the + // block-record dedup); we report the touched handles incrementally. + let changes = self.tabs[i].scene.apply_entity_delta(&d.entities, undo); + let scene = &mut self.tabs[i].scene; + scene.bump_entities(&changes); + let (sel, dirty) = if undo { + (&d.selected_before, d.dirty_before) + } else { + (&d.selected_after, d.dirty_after) + }; + let restored: HashSet = sel + .iter() + .copied() + .filter(|h| scene.document.get_entity(*h).is_some()) + .collect(); + scene.selected = restored; + // Delta commands never change the layout; this is a no-op unless a + // future caller widens the delta scope, in which case it stays correct. + scene.set_current_layout(d.current_layout.clone()); + scene.clear_preview_wire(); + self.tabs[i].dirty = dirty; + self.tabs[i].active_cmd = None; + self.tabs[i].snap_result = None; + self.tabs[i].active_grip = None; + self.refresh_properties(); + } + + pub(super) fn restore_history_snapshot(&mut self, i: usize, snapshot: FullSnapshot) { self.tabs[i].scene.document = snapshot.document; self.tabs[i].scene.set_current_layout(snapshot.current_layout); // Force a re-tessellation: the cached wires were keyed against the @@ -85,11 +266,21 @@ impl OpenCADStudio { let Some(snapshot) = self.tabs[i].history.undo_stack.pop() else { break; }; - let label = snapshot.label.clone(); - let current = self.capture_history_snapshot(i, label.clone()); - self.tabs[i].history.redo_stack.push(current); - self.restore_history_snapshot(i, snapshot); - last_label = label; + last_label = snapshot.label().to_string(); + match snapshot { + HistorySnapshot::Full(f) => { + let current = self.capture_history_snapshot(i, f.label.clone()); + self.tabs[i].history.redo_stack.push(current); + self.restore_history_snapshot(i, f); + } + HistorySnapshot::Delta(d) => { + // Symmetric: undo applies the before side, then the same + // delta rides to the redo stack (it still holds the after + // side) — no current-state capture needed. + self.apply_delta(i, &d, true); + self.tabs[i].history.redo_stack.push(HistorySnapshot::Delta(d)); + } + } } self.command_line .push_output(&format!("Undo: {last_label}")); @@ -109,11 +300,18 @@ impl OpenCADStudio { let Some(snapshot) = self.tabs[i].history.redo_stack.pop() else { break; }; - let label = snapshot.label.clone(); - let current = self.capture_history_snapshot(i, label.clone()); - self.tabs[i].history.undo_stack.push(current); - self.restore_history_snapshot(i, snapshot); - last_label = label; + last_label = snapshot.label().to_string(); + match snapshot { + HistorySnapshot::Full(f) => { + let current = self.capture_history_snapshot(i, f.label.clone()); + self.tabs[i].history.undo_stack.push(current); + self.restore_history_snapshot(i, f); + } + HistorySnapshot::Delta(d) => { + self.apply_delta(i, &d, false); + self.tabs[i].history.undo_stack.push(HistorySnapshot::Delta(d)); + } + } } self.command_line .push_output(&format!("Redo: {last_label}")); @@ -124,6 +322,6 @@ pub(super) fn history_dropdown_labels(stack: &[HistorySnapshot]) -> Vec stack .iter() .rev() - .map(|snapshot| snapshot.label.clone()) + .map(|snapshot| snapshot.label().to_string()) .collect() } diff --git a/src/app/update/mod.rs b/src/app/update/mod.rs index 101388ea0..f6204e890 100644 --- a/src/app/update/mod.rs +++ b/src/app/update/mod.rs @@ -2047,7 +2047,10 @@ impl OpenCADStudio { self.tabs[i].scene.selection.borrow_mut().context_menu = None; let handles: Vec<_> = self.tabs[i].scene.selected.iter().cloned().collect(); if !handles.is_empty() { - self.push_undo_snapshot(i, "ERASE"); + // Erase is delta-safe unless a target is in a group (group + // cleanup rewrites document.objects). + let delta_safe = self.delta_erase_safe(i, &handles); + let pending = self.begin_undo(i, "ERASE", handles.len(), delta_safe); // Stash the erased entities so OOPS can restore them. self.oops_cache = handles .iter() @@ -2056,6 +2059,9 @@ impl OpenCADStudio { self.tabs[i].scene.erase_entities(&handles); self.tabs[i].dirty = true; self.refresh_properties(); + if let Some(pd) = pending { + self.commit_undo_delta(i, pd); + } } Task::none() } diff --git a/src/scene/entity.rs b/src/scene/entity.rs index c8aa9c7b0..34a21924a 100644 --- a/src/scene/entity.rs +++ b/src/scene/entity.rs @@ -106,6 +106,12 @@ impl Scene { // (e.g. a plugin-supplied layer) so it survives a DWG save instead of // collapsing to layer 0 in the reopened file (#252). let layer = entity.common().layer.clone(); + // Delta-undo poison inputs (captured before the mutations below): an + // add that also creates a new layer, adds a block, or inserts an image + // definition mutates non-entity state a pure-entity delta can't undo. + let creates_layer = + self.is_recording_undo() && !layer.trim().is_empty() && !self.document.layers.contains(&layer); + let is_image = matches!(&entity, EntityType::RasterImage(_)); self.ensure_layer(&layer); // Route to the correct block based on current editing mode: @@ -135,6 +141,16 @@ impl Scene { if let Some(model) = mesh_seed { self.meshes.insert(handle, model); } + // Delta-undo: the new handle's before-image is "nothing" (it did not + // exist). Poison the recording if this add also mutated non-entity + // state (a new layer / block / image definition) so the app knows a + // pure-entity delta would be incomplete. + if self.is_recording_undo() { + self.record_undo_before(handle, None); + if creates_layer || affects_blocks || is_image { + self.poison_undo_recording(); + } + } if affects_blocks { self.bump_geometry(); } else { @@ -210,6 +226,9 @@ impl Scene { // so the edited entity keeps that layer on save instead of collapsing // to layer 0 in the reopened file (#252). let new_layer = entity.common().layer.clone(); + let creates_layer = self.is_recording_undo() + && !new_layer.trim().is_empty() + && !self.document.layers.contains(&new_layer); self.ensure_layer(&new_layer); // Rebuild the derived-model seeds from the new entity (as add_entity). @@ -239,6 +258,17 @@ impl Scene { None }; + // Delta-undo: capture the entity's pre-edit image (before the slot is + // overwritten) so an undo can restore it, and poison if this replace + // also created a layer or crossed a block boundary. + if self.is_recording_undo() { + let before = self.document.get_entity(handle).cloned(); + self.record_undo_before(handle, before); + if creates_layer || affects_blocks { + self.poison_undo_recording(); + } + } + // Write the new entity into the live slot. let Some(slot) = self.document.get_entity_mut(handle) else { return false; @@ -273,6 +303,61 @@ impl Scene { true } + /// Rebuild the per-entity derived caches (hatch fill / raster image / solid + /// mesh) for a single handle from whatever entity currently lives at it — + /// or drop them all if the handle is now absent. Mirrors the reseed block in + /// [`Scene::update_entity`]; used by delta-undo when it re-applies an + /// entity's before / after image so the fills and meshes follow. + pub(crate) fn reseed_derived_caches(&mut self, handle: Handle) { + let (hatch_seed, image_seed, mesh_seed) = match self.document.get_entity(handle) { + None => (None, None, None), + Some(entity) => { + let hatch_seed = if let EntityType::Hatch(dxf) = entity { + let color = self.render_style(entity).0; + Self::hatch_model_from_dxf(dxf, color) + } else if let EntityType::Solid(solid) = entity { + let color = self.render_style(entity).0; + Some(Self::solid_hatch_model(solid, color)) + } else { + None + }; + let image_seed = if let EntityType::RasterImage(img) = entity { + ImageModel::from_raster_image(img) + } else { + None + }; + let facet_res = self.document.header.facet_resolution; + let mesh_seed = if matches!( + entity, + EntityType::Solid3D(_) + | EntityType::Region(_) + | EntityType::Body(_) + | EntityType::Surface(_) + ) { + let color = self.render_style(entity).0; + crate::entities::solid3d::tessellate_volume(entity, color, facet_res) + .map(offset_mesh_lod_set) + } else { + None + }; + (hatch_seed, image_seed, mesh_seed) + } + }; + self.hatches.remove(&handle); + self.images.remove(&handle); + self.meshes.remove(&handle); + self.solid_models.remove(&handle); + if let Some(model) = hatch_seed { + self.hatches.insert(handle, model); + } + if let Some(model) = image_seed { + self.images.insert(handle, model); + } + if let Some(model) = mesh_seed { + self.meshes.insert(handle, model); + } + } + /// Returns the RGBA color for the given layer name. pub fn layer_color(&self, layer: &str) -> [f32; 4] { let layer_entry = self.document.layers.get(layer); diff --git a/src/scene/mod.rs b/src/scene/mod.rs index 9a3e788dd..083904cd5 100644 --- a/src/scene/mod.rs +++ b/src/scene/mod.rs @@ -105,6 +105,48 @@ pub enum ChangeKind { Modified, } +/// While an entity-only undoable command runs, this captures the pre-mutation +/// image of every entity the five mutation primitives (add / update / erase / +/// transform / copy) touch, so the app can build a cheap **delta**-undo entry +/// instead of cloning the whole ~800k-entity document (~46 ms). `poisoned` is +/// set when a primitive also mutated non-entity state (a brand-new layer, a +/// group, or a `*D` block record) that a pure-entity delta cannot restore — the +/// app's per-command predicate keeps that from happening, and a debug assertion +/// fires if it ever does. +#[derive(Default)] +pub struct UndoRecording { + /// First-touch before-image per handle (the first write for a handle wins, + /// so repeated touches within one command keep the true pre-command state). + /// A value of `None` marks an entity *added* by the command (no prior + /// state). `order` preserves first-touch order for a deterministic delta. + before: HashMap>, + order: Vec, + poisoned: bool, +} + +impl UndoRecording { + /// The command touched non-entity state a pure-entity delta can't restore. + pub fn is_poisoned(&self) -> bool { + self.poisoned + } + + /// No entity was recorded (nothing to undo). + pub fn is_empty(&self) -> bool { + self.order.is_empty() + } + + /// Consume the recording into `(handle, before-image)` pairs in first-touch + /// order. A `None` before-image means the entity was added by the command. + /// The app pairs each with the entity's current (after) state to build the + /// invertible delta entry. + pub fn into_before_images(mut self) -> Vec<(Handle, Option)> { + self.order + .drain(..) + .map(|h| (h, self.before.remove(&h).flatten())) + .collect() + } +} + /// One geometry mutation's delta record. `changes` names the exact handles that /// changed; `full` marks a mutation that can't enumerate its handles (undo/redo, /// file open, style/display change, any bulk op) and forces every consumer that @@ -1019,6 +1061,12 @@ pub struct Scene { /// render layer matches `new_gen` against the viewport's content id and /// `prev_gen` against what the GPU currently holds; a mismatch just rebuilds. model_wire_gpu_patch: RefCell>)>>, + /// Active delta-undo recording, or `None` when no entity-only command is + /// capturing. Populated by the five mutation primitives via + /// [`Scene::record_undo_before`]; consumed by the app (`take_undo_recording`) + /// to build a cheap history delta instead of a full document clone. See + /// [`UndoRecording`]. + undo_recording: Option, } impl Scene { @@ -1105,6 +1153,7 @@ impl Scene { geometry_journal_floor: std::cell::Cell::new(0), resident_patch_hits: std::cell::Cell::new(0), model_wire_gpu_patch: RefCell::new(None), + undo_recording: None, } } @@ -1342,6 +1391,136 @@ impl Scene { /// changed — the delta lets every derived cache patch per-handle instead of /// re-walking the whole drawing. Blocks are untouched (`block_epoch` stays), /// so this is for top-level entity add / edit / erase, not block-defn edits. + /// Begin capturing before-images for a delta-undo entry. Pair with + /// [`Scene::take_undo_recording`]. Only entity-only commands — whose + /// mutations flow through the five primitives (add / update / erase / + /// transform / copy) and that touch no layers / objects / block records — + /// may use this; anything else must fall back to a full history snapshot. + pub fn begin_undo_recording(&mut self) { + self.undo_recording = Some(UndoRecording::default()); + } + + /// Whether a delta-undo recording is currently open. + pub fn is_recording_undo(&self) -> bool { + self.undo_recording.is_some() + } + + /// Finish and return the open recording (if any) for the app to build a + /// history delta from. + pub fn take_undo_recording(&mut self) -> Option { + self.undo_recording.take() + } + + /// Record the pre-mutation image of `handle` (first touch wins). `before` + /// is `None` for a freshly added entity. No-op when not recording — the + /// callers guard with [`Scene::is_recording_undo`] so the clone is skipped + /// entirely on the common (no-recording) path. + pub(crate) fn record_undo_before(&mut self, handle: Handle, before: Option) { + if let Some(rec) = self.undo_recording.as_mut() { + if !rec.before.contains_key(&handle) { + rec.order.push(handle); + rec.before.insert(handle, before); + } + } + } + + /// Flag the open recording as touching non-entity state (a new layer, a + /// group edit, a `*D` block record) that a pure-entity delta cannot + /// restore. The app's per-command predicate keeps this from firing. + pub(crate) fn poison_undo_recording(&mut self) { + if let Some(rec) = self.undo_recording.as_mut() { + rec.poisoned = true; + } + } + + /// Re-apply one side of an entity delta to the document. For each + /// `(handle, before, after)`, install the chosen `target` — `before` when + /// `undo`, `after` on redo: overwrite the entity in place, re-insert it with + /// its original handle, or remove it — reseeding the per-handle derived + /// caches so fills/meshes follow. Returns the exact per-handle change list + /// for the caller to report via [`Scene::bump_entities`]; it does not bump, + /// touch the selection, or rebuild any whole-document cache. + /// + /// `remove_entity` leaves a handle dangling in its owner block record's + /// `entity_handles` (harmless — a render/save lookup miss is skipped), so a + /// re-insert's `add_entity` push would make it appear twice and emit the + /// entity twice on save. Every to-be-re-inserted handle is therefore + /// stripped from the block records in one pass first, leaving exactly one + /// entry after the re-insert. + pub(crate) fn apply_entity_delta( + &mut self, + entities: &[(Handle, Option, Option)], + undo: bool, + ) -> Vec<(Handle, ChangeKind)> { + let reinsert: HashSet = entities + .iter() + .filter_map(|(h, before, after)| { + let target = if undo { before } else { after }; + (target.is_some() && self.document.get_entity(*h).is_none()).then_some(*h) + }) + .collect(); + if !reinsert.is_empty() { + for br in self.document.block_records.iter_mut() { + br.entity_handles.retain(|h| !reinsert.contains(h)); + } + } + + let mut changes: Vec<(Handle, ChangeKind)> = Vec::with_capacity(entities.len()); + for (h, before, after) in entities { + let target = if undo { before } else { after }; + let existed = self.document.get_entity(*h).is_some(); + match target { + Some(ent) => { + if existed { + if let Some(slot) = self.document.get_entity_mut(*h) { + *slot = ent.clone(); + } + changes.push((*h, ChangeKind::Modified)); + } else { + // Re-insert with the original handle: the stored image + // keeps it, and document.add_entity honours a preset, + // non-null handle (routing to its owner block record). + let _ = self.document.add_entity(ent.clone()); + changes.push((*h, ChangeKind::Added)); + } + self.reseed_derived_caches(*h); + } + None => { + if existed { + self.document.remove_entity(*h); + // Drops the now-absent entity's hatch/image/mesh caches. + self.reseed_derived_caches(*h); + changes.push((*h, ChangeKind::Removed)); + } + } + } + } + + // Integrity net: no re-inserted handle may appear more than once across + // the block records (a duplicate would emit the entity twice on save). + // Debug-only, and skipped entirely when nothing was re-inserted (the + // common transform/property-edit case). + #[cfg(debug_assertions)] + if !reinsert.is_empty() { + let mut counts: HashMap = HashMap::default(); + for br in self.document.block_records.iter() { + for h in &br.entity_handles { + if reinsert.contains(h) { + *counts.entry(*h).or_default() += 1; + } + } + } + for (h, c) in counts { + debug_assert!( + c <= 1, + "delta re-insert duplicated handle {h:?} in block records ({c}×)" + ); + } + } + + changes + } + pub fn bump_entities(&mut self, changes: &[(Handle, ChangeKind)]) { let epoch = GEOMETRY_EPOCH.fetch_add(1, Ordering::Relaxed); self.geometry_epoch = epoch; @@ -4988,3 +5167,153 @@ mod journal_tests { } } +/// Delta-undo round-trips: a recorded entity-only edit must be exactly +/// invertible (undo restores the pre-edit images) and re-appliable (redo +/// restores the post-edit images), including handle preservation and no +/// duplication of a re-inserted handle in its owning block record. +#[cfg(test)] +mod delta_undo_tests { + use super::*; + use crate::command::EntityTransform; + use acadrust::entities::Line; + use acadrust::types::Vector3; + use glam::DVec3; + + fn line(x1: f64, y1: f64, x2: f64, y2: f64) -> EntityType { + EntityType::Line(Line::from_points( + Vector3::new(x1, y1, 0.0), + Vector3::new(x2, y2, 0.0), + )) + } + + /// Build the delta the way `commit_undo_delta` does: pair each recorded + /// before-image with the entity's current (after) state. + fn build_delta( + scene: &Scene, + rec: UndoRecording, + ) -> Vec<(Handle, Option, Option)> { + rec.into_before_images() + .into_iter() + .map(|(h, before)| { + let after = scene.document.get_entity(h).cloned(); + (h, before, after) + }) + .collect() + } + + /// Occurrences of a handle in the *Model_Space block record's entity list. + fn ms_occurrences(scene: &Scene, handle: Handle) -> usize { + scene + .document + .block_records + .get("*Model_Space") + .map(|br| br.entity_handles.iter().filter(|&&x| x == handle).count()) + .unwrap_or(0) + } + + #[test] + fn transform_delta_round_trips() { + let mut scene = Scene::new(); + let h = scene.add_entity(line(0.0, 0.0, 10.0, 0.0)); + let orig = scene.document.get_entity(h).cloned().unwrap(); + + scene.begin_undo_recording(); + scene.transform_entities(&[h], &EntityTransform::Translate(DVec3::new(5.0, 3.0, 0.0))); + let rec = scene.take_undo_recording().unwrap(); + assert!(!rec.is_poisoned(), "a plain move must not poison"); + let delta = build_delta(&scene, rec); + let moved = scene.document.get_entity(h).cloned().unwrap(); + assert_ne!(orig, moved, "the move must actually change the entity"); + + // Undo restores the pre-move image exactly, in place (same handle). + scene.apply_entity_delta(&delta, true); + assert_eq!(scene.document.get_entity(h).cloned().unwrap(), orig); + assert_eq!(ms_occurrences(&scene, h), 1); + + // Redo re-applies the post-move image. + scene.apply_entity_delta(&delta, false); + assert_eq!(scene.document.get_entity(h).cloned().unwrap(), moved); + assert_eq!(ms_occurrences(&scene, h), 1); + } + + #[test] + fn erase_delta_round_trips_with_handle_and_no_dup() { + let mut scene = Scene::new(); + let h1 = scene.add_entity(line(0.0, 0.0, 1.0, 0.0)); + let h2 = scene.add_entity(line(2.0, 2.0, 3.0, 3.0)); + let orig1 = scene.document.get_entity(h1).cloned().unwrap(); + let orig2 = scene.document.get_entity(h2).cloned().unwrap(); + let count = scene.document.entities().count(); + + scene.begin_undo_recording(); + scene.erase_entities(&[h2]); + let rec = scene.take_undo_recording().unwrap(); + assert!(!rec.is_poisoned(), "erasing an ungrouped entity must not poison"); + let delta = build_delta(&scene, rec); + assert!(scene.document.get_entity(h2).is_none(), "h2 must be erased"); + + // Undo re-inserts h2 with its ORIGINAL handle and image, exactly once in + // the block record (the strip prevents a duplicated dangling entry), + // leaving h1 untouched. + scene.apply_entity_delta(&delta, true); + assert_eq!(scene.document.get_entity(h2).cloned().unwrap(), orig2); + assert_eq!(scene.document.get_entity(h1).cloned().unwrap(), orig1); + assert_eq!(scene.document.entities().count(), count); + assert_eq!(ms_occurrences(&scene, h2), 1, "h2 must appear once, not duplicated"); + + // Redo erases h2 again. (remove_entity leaves the handle dangling in + // the block record — harmless, a lookup miss is skipped on render/save + // — so we assert absence from the document, not from entity_handles.) + scene.apply_entity_delta(&delta, false); + assert!(scene.document.get_entity(h2).is_none()); + } + + #[test] + fn add_delta_round_trips() { + let mut scene = Scene::new(); + scene.begin_undo_recording(); + let h = scene.add_entity(line(0.0, 0.0, 4.0, 4.0)); + let rec = scene.take_undo_recording().unwrap(); + assert!(!rec.is_poisoned(), "a plain add on an existing layer must not poison"); + let added = scene.document.get_entity(h).cloned().unwrap(); + let delta = build_delta(&scene, rec); + + // Undo removes the freshly added entity from the document (a dangling + // block-record entry may remain — see erase test — that's fine). + scene.apply_entity_delta(&delta, true); + assert!(scene.document.get_entity(h).is_none()); + + // Redo re-adds it with the same handle, exactly once (the strip clears + // any dangling entry so add_entity doesn't duplicate it). + scene.apply_entity_delta(&delta, false); + assert_eq!(scene.document.get_entity(h).cloned().unwrap(), added); + assert_eq!(ms_occurrences(&scene, h), 1); + } + + #[test] + fn copy_delta_round_trips() { + let mut scene = Scene::new(); + let src = scene.add_entity(line(0.0, 0.0, 1.0, 0.0)); + + scene.begin_undo_recording(); + let new_handles = + scene.copy_entities(&[src], &EntityTransform::Translate(DVec3::new(10.0, 0.0, 0.0))); + let rec = scene.take_undo_recording().unwrap(); + assert!(!rec.is_poisoned(), "copying a plain entity must not poison"); + assert_eq!(new_handles.len(), 1); + let copy_h = new_handles[0]; + let copy_img = scene.document.get_entity(copy_h).cloned().unwrap(); + let delta = build_delta(&scene, rec); + + // Undo erases the copy from the document, leaving the source intact. + scene.apply_entity_delta(&delta, true); + assert!(scene.document.get_entity(copy_h).is_none()); + assert!(scene.document.get_entity(src).is_some()); + + // Redo restores the copy with its handle, once. + scene.apply_entity_delta(&delta, false); + assert_eq!(scene.document.get_entity(copy_h).cloned().unwrap(), copy_img); + assert_eq!(ms_occurrences(&scene, copy_h), 1); + } +} + diff --git a/src/scene/modify.rs b/src/scene/modify.rs index 0193585e5..938e6893a 100644 --- a/src/scene/modify.rs +++ b/src/scene/modify.rs @@ -132,6 +132,11 @@ impl Scene { .flatten() .collect(); for &h in handles { + // Delta-undo: capture the pre-transform image before mutating. + if self.is_recording_undo() { + let before = self.document.get_entity(h).cloned(); + self.record_undo_before(h, before); + } if let Some(entity) = self.document.get_entity_mut(h) { view::dispatch::apply_transform(entity, t); if mirror_true { @@ -164,7 +169,13 @@ impl Scene { } } // Move the baked dimension-block sub-entities too (collected above). + // These are entities not named in the reported change set, so a delta + // must capture their before-images here or a dimension move won't undo. for h in &dim_block_subs { + if self.is_recording_undo() { + let before = self.document.get_entity(*h).cloned(); + self.record_undo_before(*h, before); + } if let Some(entity) = self.document.get_entity_mut(*h) { view::dispatch::apply_transform(entity, t); } @@ -317,6 +328,10 @@ impl Scene { let bn = d.base().block_name.clone(); if !bn.trim().is_empty() { if let Some(new_bn) = self.clone_transformed_block(&bn, t) { + // Delta-undo: a copied dimension adds a fresh anonymous + // *D block record — non-entity state a pure-entity delta + // can't restore. Poison so the app keeps a full snapshot. + self.poison_undo_recording(); if let EntityType::Dimension(d) = &mut entity { d.base_mut().block_name = new_bn; } @@ -327,6 +342,10 @@ impl Scene { entity.common_mut().handle = Handle::NULL; let h = self.document.add_entity(entity).unwrap_or(Handle::NULL); if !h.is_null() { + // Delta-undo: a copy's before-image is "nothing" (undo erases it). + if self.is_recording_undo() { + self.record_undo_before(h, None); + } let new_model = match self.document.get_entity(h) { Some(EntityType::Hatch(dxf)) => { let color = convert::tess_util::aci_to_rgba(&dxf.common.color); diff --git a/src/scene/selection.rs b/src/scene/selection.rs index 1e06f9d2a..c87d0e3f2 100644 --- a/src/scene/selection.rs +++ b/src/scene/selection.rs @@ -335,6 +335,11 @@ impl Scene { if self.is_layer_locked(h) { continue; } + // Delta-undo: capture the removed entity so an undo can re-insert it. + if self.is_recording_undo() { + let before = self.document.get_entity(h).cloned(); + self.record_undo_before(h, before); + } self.document.remove_entity(h); self.selected.remove(&h); self.hatches.remove(&h); @@ -344,13 +349,18 @@ impl Scene { } // Remove erased handles from all groups; delete groups that become empty. let group_dict_handle = self.document.header.acad_group_dict_handle; + let mut groups_changed = false; let to_remove: Vec = self .document .objects .values_mut() .filter_map(|obj| match obj { ObjectType::Group(g) => { + let before = g.entities.len(); g.entities.retain(|h| !handles.contains(h)); + if g.entities.len() != before { + groups_changed = true; + } if g.entities.is_empty() { Some(g.handle) } else { @@ -360,6 +370,11 @@ impl Scene { _ => None, }) .collect(); + // Delta-undo: editing a group (or deleting an emptied one) mutates + // document.objects — non-entity state a pure-entity delta can't restore. + if groups_changed { + self.poison_undo_recording(); + } for gh in &to_remove { if let Some(ObjectType::Dictionary(dict)) = self.document.objects.get_mut(&group_dict_handle)