Port 6 audit-found bugs from the upstream OSNAP fix branch

Ports the fixes from fix/osnap-circle-arc-ellipse-intersection (and
this session's audit follow-up) onto this branch. All are independent
correctness/reliability fixes, unrelated to the parametric-constraint-
system work also in progress here.

1. OSNAP Intersection snap used a tessellated approximation instead of
   exact geometry (GitHub discussion #1052): a circle/arc always
   tessellates at a fixed 48 segments in the resident wire set
   regardless of radius or zoom, so every circle/arc intersection snap
   was off by construction. Added exact_circle_intersections, a
   closed-form, frame-independent circle/arc-vs-circle/arc solver,
   wired into both Intersection-snap code paths ahead of the
   tessellated sweep. This branch's TangentGeom predates the
   PlanarEllipse variant added upstream, so the ellipse/line
   generalization from the upstream fix isn't ported here — this
   covers the literal reported bug (a circle-circle case) using only
   variants this branch already has.

2. Unbounded MINSERT allocation: array_offsets() reserved one Vec
   element per array instance with row/column counts parsed as
   unchecked u16s — a corrupt 65535x65535 pair requests ~96 GiB.
   Fixed with a documented instance budget (20,000), enforced at both
   the load-time corrupt-entity filter and a render-graph failsafe.

3. MINSERT spacing was scaled by the block's own scale:
   insert_instance_transform composed the offset translation before
   the instance transform, so x/y block scale multiplied the row/
   column spacing too. Fixed by orienting the offset with rotation +
   OCS only, then translating the already-scaled instance transform.

4. Automation "open" skipped the finalization (block-origin
   normalization, raster-path resolution, source_path) and corrupt-
   entity purge a UI open always runs. Added
   io::load_bytes_finalized(path, bytes), used by automation's "open"
   handler, which now also reports the purge count.

5. Material maps were uploaded without checking
   device.limits().max_texture_dimension_2d, unlike raster-image
   rendering (which already tiles to that limit). A mesh material is
   sampled by UV, so it can't use the same tiling trick — instead,
   upload_rgba_texture now downscales an oversized image before
   upload, falling back to the 1x1 material color if malformed.

6. Sketch-constraint persistence/undo, audit-found while reviewing
   this branch's own constraint system:
   - Clearing every constraint in a scope after a prior non-empty save
     left the stale XRecord in place, resurrecting the deleted
     constraints on the next load. materialize_sketch_constraints_for_
     save now actively removes the XRecord when a scope goes back to
     empty.
   - ERASE removing an entity took its constraints with it (design
     doc's deletion policy), but sketch_constraints lives on Scene,
     outside document/document.objects, so nothing captured that
     mutation for undo — the erased geometry came back on undo, but
     its constraints didn't. refresh_sketch_constraints's deletion
     pass now records the touched scope's before-image into the same
     UndoRecording the entity removal rides in, threaded through a new
     sketch_constraints channel on DeltaSnapshot, so one undo restores
     both together. HistorySnapshot::Delta is now boxed (clippy
     flagged the resulting size skew across undo/redo stack variants).

31 new/adapted tests across all six fixes. Full workspace suite passes
except the one pre-existing, unrelated font-test failure also present
on main.

Co-Authored-By: Claude Sonnet 5 <[email protected]>
This commit is contained in:
Felix RiestraandClaude Sonnet 5 committed 2026-09-09 10:46:28 +02:00
1 parent 63a74b19a9
commit 44deee2ac9
11 files changed
+953 -41

No files matched your search

+63 -9
View File
@@ -282,16 +282,19 @@ impl OpenCADStudio {
let Some(path) = req["path"].as_str() else {
return err("open: missing \"path\"");
};
let bytes = match self.read_drawing(std::path::Path::new(path)) {
let path_buf = PathBuf::from(path);
let bytes = match self.read_drawing(&path_buf) {
Ok(b) => b,
Err(e) => return err(format!("open: {e}")),
};
let name = PathBuf::from(path)
.file_name()
.map(|n| n.to_string_lossy().into_owned())
.unwrap_or_else(|| path.to_string());
match crate::io::load_bytes(&name, bytes) {
Ok(doc) => {
// Runs the same finalization (block-origin normalization,
// raster-path resolution, source_path) and corrupt-entity
// purge the UI open path always gets, using the real path —
// not just its basename — so an automation open behaves the
// same as opening the identical file from the UI instead of
// silently skipping that safety/normalization net.
match crate::io::load_bytes_finalized(&path_buf, bytes) {
Ok((doc, dropped)) => {
let i = self.active_tab;
self.tabs[i].scene.document = doc;
self.tabs[i].scene.load_sketch_constraints_from_document();
@@ -300,10 +303,16 @@ impl OpenCADStudio {
&mut self.tabs[i].scene.document,
);
self.tabs[i].adopt_active_ucs_from_header();
self.tabs[i].current_path = Some(PathBuf::from(path));
self.tabs[i].current_path = Some(path_buf);
self.tabs[i].is_start = false;
self.tabs[i].scene.bump_geometry();
self.entity_summary()
let mut summary = self.entity_summary();
if dropped > 0 {
if let Some(obj) = summary.as_object_mut() {
obj.insert("purged".to_string(), json!(dropped));
}
}
summary
}
Err(e) => err(format!("open: {e}")),
}
@@ -1267,4 +1276,49 @@ mod tests {
let _ = std::fs::remove_file(sidecar);
let _ = std::fs::remove_file(&path);
}
/// Audit finding: automation's `"open"` used to call `io::load_bytes`
/// directly, skipping the finalization and corrupt-entity purge every UI
/// open runs — so the same file opened two ways behaved differently.
/// Proves both halves of the fix: `source_path` (only ever set by
/// finalization, never by `load_bytes` alone) is populated, and a
/// zero-radius circle — `io::is_entity_corrupt`'s own rejection case —
/// is purged and reported, exactly like a UI open of the same file.
#[test]
fn open_finalizes_and_purges_like_the_ui_open_path() {
let mut app = OpenCADStudio::new_for_test();
let path = std::env::temp_dir().join(format!(
"ocs_automation_finalize_test_{}.dxf",
std::process::id()
));
let _ = std::fs::remove_file(&path);
let mut doc = acadrust::CadDocument::new();
let mut good = acadrust::entities::Circle::new();
good.center = acadrust::types::Vector3::new(5.0, 5.0, 0.0);
good.radius = 2.0;
doc.add_entity(acadrust::EntityType::Circle(good)).unwrap();
let mut corrupt = acadrust::entities::Circle::new();
corrupt.center = acadrust::types::Vector3::new(1.0, 1.0, 0.0);
corrupt.radius = 0.0; // io::is_entity_corrupt rejects a zero-radius circle
doc.add_entity(acadrust::EntityType::Circle(corrupt)).unwrap();
let bytes = crate::io::save_to_bytes(&doc, "dxf", doc.version)
.expect("save a document containing a corrupt entity");
std::fs::write(&path, bytes).unwrap();
let p = path.to_string_lossy().replace('\\', "\\\\");
let result = app.automation_op(&format!(r#"{{"op":"open","path":"{p}"}}"#));
assert_eq!(result["ok"], true, "{}", result["error"]);
assert_eq!(result["total"], 1, "the corrupt circle must not survive the open");
assert_eq!(result["purged"], 1, "the purge count must be reported, matching the UI open path's diagnostics");
let i = app.active_tab;
assert!(
app.tabs[i].scene.document.source_path.is_some(),
"automation open must run the same finalization as a path-based open, which sets source_path (load_bytes alone never does)"
);
drop(app);
let _ = std::fs::remove_file(&path);
}
}
+35 -1
View File
@@ -655,7 +655,11 @@ impl DocumentTab {
/// full entity store.
#[derive(Clone)]
pub(super) enum HistorySnapshot {
Delta(DeltaSnapshot),
// Boxed: `DeltaSnapshot` grew past the other variants once
// `sketch_constraints` (ERASE-undo fix) was added, and every undo/redo
// stack slot costs as much as this enum's largest variant regardless of
// which one it actually holds.
Delta(Box<DeltaSnapshot>),
ObjectVisibility(ObjectVisibilitySnapshot),
SketchConstraints(SketchConstraintsSnapshot),
}
@@ -690,6 +694,13 @@ impl HistorySnapshot {
.as_ref()
.map_or(0, |(before, after)| before.len().saturating_add(after.len())),
)
.saturating_add(
d.sketch_constraints
.iter()
.map(|entry| entry.before.constraints.len().saturating_add(entry.after.constraints.len()))
.sum::<usize>()
.saturating_mul(96), // matches HistorySnapshot::SketchConstraints's own rough per-constraint estimate
)
.saturating_add(d.label.len()),
HistorySnapshot::ObjectVisibility(v) => v
.before
@@ -776,6 +787,12 @@ pub(super) struct DeltaSnapshot {
/// Opposite non-entity document state. `apply_delta_state` swaps this with
/// the live structure, so the same allocation shuttles between undo/redo.
pub(super) structure: Option<StructureSnapshot>,
/// Sketch-constraint scopes this same command changed alongside its
/// entities (e.g. ERASE removing a constraint set's touched constraints)
/// — `sketch_constraints` lives on `Scene`, not in `document`/
/// `document.objects`, so it needs its own channel here rather than
/// riding along with `entities`/`structure`. Almost always empty.
pub(super) sketch_constraints: Vec<SketchConstraintsEntryDelta>,
pub(super) label: String,
}
@@ -836,6 +853,23 @@ pub(super) struct ObjectEntryDelta {
pub(super) after: Option<acadrust::objects::ObjectType>,
}
/// One sketch-constraint scope's before/after image within an entity
/// `DeltaSnapshot` — e.g. ERASE removing an entity's constraints along with
/// it (`Scene::refresh_sketch_constraints`'s deletion policy). Distinct from
/// [`SketchConstraintsSnapshot`]: that type is its own separate
/// `HistorySnapshot` entry for a dedicated constraint add/remove command
/// (still a deliberate two-step undo, design doc's own scope-down); this one
/// rides inside the *same* delta as the entity change that caused it, so one
/// undo restores both together. `before`/`after` are always `Some` scope
/// state (never absent) — a scope's `Vec` entry, once created, is never
/// removed, so there is no "scope didn't exist" case to represent here.
#[derive(Clone)]
pub(super) struct SketchConstraintsEntryDelta {
pub(super) scope: crate::scene::sketch_constraints::SketchScope,
pub(super) before: crate::scene::sketch_constraints::SketchConstraintSet,
pub(super) after: crate::scene::sketch_constraints::SketchConstraintSet,
}
#[derive(Default)]
pub(super) struct HistoryState {
pub(super) undo_stack: Vec<HistorySnapshot>,
+38 -10
View File
@@ -1,7 +1,8 @@
use super::{
document::{
DeltaSnapshot, HistorySnapshot, ObjectEntryDelta, ObjectVisibilitySnapshot,
PendingHistorySnapshot, SketchConstraintsSnapshot, StructureSnapshot, TableEntryDelta,
PendingHistorySnapshot, SketchConstraintsEntryDelta, SketchConstraintsSnapshot,
StructureSnapshot, TableEntryDelta,
},
OpenCADStudio,
};
@@ -259,9 +260,10 @@ impl OpenCADStudio {
dirty_after: true,
active_layer: None,
structure: (!objects.is_empty()).then_some(StructureSnapshot::Objects(objects)),
sketch_constraints: Vec::new(),
label: label.into(),
};
self.push_undo_entry(i, HistorySnapshot::Delta(delta));
self.push_undo_entry(i, HistorySnapshot::Delta(Box::new(delta)));
}
pub(super) fn defer_live_entity_history_after(&mut self, i: usize, handle: Handle) {
@@ -368,9 +370,10 @@ impl OpenCADStudio {
active_layer: (pending.active_layer != active_layer_after)
.then_some((pending.active_layer, active_layer_after)),
structure: structure_changed.then_some(StructureSnapshot::Full(pending.structure_before)),
sketch_constraints: Vec::new(),
label: pending.label,
};
self.push_undo_entry(i, HistorySnapshot::Delta(delta));
self.push_undo_entry(i, HistorySnapshot::Delta(Box::new(delta)));
}
pub(super) fn finish_all_pending_history(&mut self) {
@@ -475,9 +478,10 @@ impl OpenCADStudio {
dirty_after: self.tabs[i].dirty,
active_layer: None,
structure: Some(StructureSnapshot::Layers(entries)),
sketch_constraints: Vec::new(),
label: pending.label,
};
self.push_undo_entry(i, HistorySnapshot::Delta(delta));
self.push_undo_entry(i, HistorySnapshot::Delta(Box::new(delta)));
}
pub(super) fn begin_text_style_undo(
@@ -530,9 +534,10 @@ impl OpenCADStudio {
dirty_after: self.tabs[i].dirty,
active_layer: None,
structure: Some(StructureSnapshot::TextStyles(entries)),
sketch_constraints: Vec::new(),
label: pending.label,
};
self.push_undo_entry(i, HistorySnapshot::Delta(delta));
self.push_undo_entry(i, HistorySnapshot::Delta(Box::new(delta)));
}
pub(super) fn begin_dim_style_undo(
@@ -585,9 +590,10 @@ impl OpenCADStudio {
dirty_after: self.tabs[i].dirty,
active_layer: None,
structure: Some(StructureSnapshot::DimStyles(entries)),
sketch_constraints: Vec::new(),
label: pending.label,
};
self.push_undo_entry(i, HistorySnapshot::Delta(delta));
self.push_undo_entry(i, HistorySnapshot::Delta(Box::new(delta)));
}
fn group_object_state(&self, i: usize) -> FxHashMap<Handle, acadrust::objects::ObjectType> {
@@ -648,9 +654,10 @@ impl OpenCADStudio {
dirty_after: self.tabs[i].dirty,
active_layer: None,
structure: Some(StructureSnapshot::Objects(entries)),
sketch_constraints: Vec::new(),
label: pending.label,
};
self.push_undo_entry(i, HistorySnapshot::Delta(delta));
self.push_undo_entry(i, HistorySnapshot::Delta(Box::new(delta)));
}
pub(super) fn commit_style_undo(
@@ -681,9 +688,10 @@ impl OpenCADStudio {
dim_names,
object_handles,
}),
sketch_constraints: Vec::new(),
label: "STYLE".to_string(),
};
self.push_undo_entry(i, HistorySnapshot::Delta(delta));
self.push_undo_entry(i, HistorySnapshot::Delta(Box::new(delta)));
}
/// Dimension copies still need a full snapshot because they clone a fresh
@@ -757,7 +765,7 @@ impl OpenCADStudio {
pending.label
);
}
let (entity_before, object_before) = rec.into_recorded_images();
let (entity_before, object_before, sketch_constraints_before) = rec.into_recorded_images();
let entities: Vec<(Handle, Option<Arc<EntityType>>, Option<Arc<EntityType>>)> = entity_before
.into_iter()
.map(|(h, before)| {
@@ -776,6 +784,21 @@ impl OpenCADStudio {
})
})
.collect();
let sketch_constraints: Vec<SketchConstraintsEntryDelta> = sketch_constraints_before
.into_iter()
.filter_map(|(scope, before)| {
let after = self.tabs[i]
.scene
.sketch_constraint_set(scope)
.cloned()
.unwrap_or_else(|| crate::scene::sketch_constraints::SketchConstraintSet::new(scope));
(before.constraints != after.constraints).then_some(SketchConstraintsEntryDelta {
scope,
before,
after,
})
})
.collect();
let selected_after = self.tabs[i].scene.selected.iter().copied().collect();
let dirty_after = self.tabs[i].dirty;
let mut structure = pending.structure_before.map(StructureSnapshot::Full);
@@ -811,9 +834,10 @@ impl OpenCADStudio {
dirty_after,
active_layer: None,
structure,
sketch_constraints,
label: pending.label,
};
self.push_undo_entry(i, HistorySnapshot::Delta(delta));
self.push_undo_entry(i, HistorySnapshot::Delta(Box::new(delta)));
}
/// Apply one side of a delta entry in place: `undo` restores each entity's
@@ -939,6 +963,10 @@ impl OpenCADStudio {
}
let changes = self.tabs[i].scene.apply_entity_delta(&d.entities, undo);
let scene = &mut self.tabs[i].scene;
for entry in &d.sketch_constraints {
let value = if undo { &entry.before } else { &entry.after };
*scene.sketch_constraint_set_mut(entry.scope) = value.clone();
}
let (sel, dirty) = if undo {
(&d.selected_before, d.dirty_before)
} else {