forked from 1b-agent/opencadstudio-upstream
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 <noreply@anthropic.com>
This commit is contained in:
Felix RiestraandClaude Sonnet 5 committed
1 parent
63a74b19a9
commit
44deee2ac9
11 files changed
+953
-41
No files matched your search
+63
-9
@@ -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
@@ -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
@@ -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 {
|
||||
|
||||
@@ -746,6 +746,32 @@ pub fn load_bytes(name: &str, bytes: Vec<u8>) -> Result<CadDocument, String> {
|
||||
}
|
||||
}
|
||||
|
||||
/// [`load_bytes`] plus the same finalization and corruption handling the
|
||||
/// canonical path-based open (`load_file_for_open`/`finalize_loaded_outcome`)
|
||||
/// already runs — for a byte-based open that nonetheless has a real
|
||||
/// filesystem path behind it (automation's `"open"` op reads the file into
|
||||
/// memory itself, e.g. to go through an edit lease, but the drawing still
|
||||
/// lives at `path`). Without this, automation-opened documents kept
|
||||
/// legacy non-zero block origins un-normalized, left relative raster/material
|
||||
/// paths unresolved, carried no `source_path`, and skipped the corrupt-entity
|
||||
/// purge the UI open path always runs — silently behaving differently from a
|
||||
/// UI open of the exact same file. `load_bytes` itself is unchanged (and
|
||||
/// still used directly by the byte round-trip tests, which have no real path
|
||||
/// and don't want this finalization).
|
||||
#[cfg(not(target_arch = "wasm32"))]
|
||||
pub fn load_bytes_finalized(path: &Path, bytes: Vec<u8>) -> Result<(CadDocument, usize), String> {
|
||||
let name = path
|
||||
.file_name()
|
||||
.map(|n| n.to_string_lossy().into_owned())
|
||||
.unwrap_or_else(|| path.to_string_lossy().into_owned());
|
||||
let mut doc = load_bytes(&name, bytes)?;
|
||||
normalize_block_origins(&mut doc);
|
||||
resolve_raster_image_paths(&mut doc, path.parent());
|
||||
doc.source_path = Some(path.to_string_lossy().into_owned());
|
||||
let dropped = purge_corrupt_entities(&mut doc);
|
||||
Ok((doc, dropped))
|
||||
}
|
||||
|
||||
/// Load a DWG or DXF file directly from a path (auto-detect by extension).
|
||||
/// Peek at a file's leading bytes to tell a DWG (version tag "AC10xx") from a
|
||||
/// DXF. Used for `.bak` copies, whose extension hides the real format.
|
||||
@@ -2037,7 +2063,17 @@ pub(crate) fn is_entity_corrupt(e: &EntityType) -> bool {
|
||||
// rarely use this many — and parser desync produces exactly-100_000-vertex
|
||||
// junk records.
|
||||
const MAX_VERTS: usize = 100_000;
|
||||
// A MINSERT's row/column counts are parsed `u16`s whose unchecked product
|
||||
// (`Insert::instance_count`) drives a per-instance allocation in
|
||||
// `render_graph::array_offsets` and a full per-instance render-graph walk
|
||||
// for every one — a corrupt or adversarial 65535x65535 pair requests over
|
||||
// four billion instances (tens of GiB) long before any GPU-side chunking
|
||||
// limit could help. Kept in sync with the identical failsafe clamp in
|
||||
// `render_graph::array_offsets`, which guards inserts that reach the
|
||||
// render graph some other way (e.g. built at runtime, not loaded).
|
||||
const MAX_MINSERT_INSTANCES: usize = 20_000;
|
||||
match e {
|
||||
E::Insert(i) => i.instance_count() > MAX_MINSERT_INSTANCES,
|
||||
E::LwPolyline(p) => {
|
||||
!finite_unit_normal(&p.normal)
|
||||
|| p.vertices.len() >= MAX_VERTS
|
||||
@@ -2452,4 +2488,33 @@ mod corrupt_guard_tests {
|
||||
let s = Spline::from_control_points(3, pts);
|
||||
assert!(!is_entity_corrupt(&EntityType::Spline(s)));
|
||||
}
|
||||
|
||||
// A corrupt or adversarial MINSERT row/column pair (u16, so its unchecked
|
||||
// product can reach into the billions) must be rejected before it can
|
||||
// drive the render graph's per-instance allocation and expansion.
|
||||
#[test]
|
||||
fn rejects_pathological_minsert_counts() {
|
||||
let mut i = acadrust::entities::Insert::new("BLOCK", Vector3::ZERO);
|
||||
i.row_count = u16::MAX;
|
||||
i.column_count = u16::MAX;
|
||||
assert!(is_entity_corrupt(&EntityType::Insert(i)));
|
||||
}
|
||||
|
||||
// An ordinary array insert, well under the budget, is valid source data.
|
||||
#[test]
|
||||
fn keeps_a_reasonable_minsert() {
|
||||
let mut i = acadrust::entities::Insert::new("BLOCK", Vector3::ZERO);
|
||||
i.row_count = 10;
|
||||
i.column_count = 10;
|
||||
i.row_spacing = 5.0;
|
||||
i.column_spacing = 5.0;
|
||||
assert!(!is_entity_corrupt(&EntityType::Insert(i)));
|
||||
}
|
||||
|
||||
// A plain (non-array) INSERT is never treated as a MINSERT-count problem.
|
||||
#[test]
|
||||
fn keeps_a_plain_insert() {
|
||||
let i = acadrust::entities::Insert::new("BLOCK", Vector3::ZERO);
|
||||
assert!(!is_entity_corrupt(&EntityType::Insert(i)));
|
||||
}
|
||||
}
|
||||
+45
-7
@@ -330,6 +330,15 @@ pub struct UndoRecording {
|
||||
/// by the command. `None` denotes a newly-created object.
|
||||
object_before: HashMap<Handle, Option<ObjectType>>,
|
||||
object_order: Vec<Handle>,
|
||||
/// First-touch before-images of every sketch-constraint scope a command
|
||||
/// changed (e.g. ERASE removing an entity's constraints along with it via
|
||||
/// `refresh_sketch_constraints`'s deletion policy). `sketch_constraints`
|
||||
/// lives on `Scene`, not in `document`/`document.objects`, so neither
|
||||
/// directory above ever sees this change — it needs its own directory.
|
||||
/// Unlike the two above, there is no "didn't exist before" case: a
|
||||
/// scope's `Vec` entry, once created, is never removed.
|
||||
sketch_constraints_before: HashMap<sketch_constraints::SketchScope, sketch_constraints::SketchConstraintSet>,
|
||||
sketch_constraints_order: Vec<sketch_constraints::SketchScope>,
|
||||
poisoned: bool,
|
||||
}
|
||||
|
||||
@@ -339,18 +348,22 @@ impl UndoRecording {
|
||||
self.poisoned
|
||||
}
|
||||
|
||||
/// No entity or object entry was recorded (nothing to undo).
|
||||
/// No entity, object, or sketch-constraint entry was recorded (nothing to undo).
|
||||
pub fn is_empty(&self) -> bool {
|
||||
self.order.is_empty() && self.object_order.is_empty()
|
||||
self.order.is_empty() && self.object_order.is_empty() && self.sketch_constraints_order.is_empty()
|
||||
}
|
||||
|
||||
/// Consume both recording directories in deterministic first-touch order.
|
||||
/// A `None` image means the entity/object was added by the command.
|
||||
/// Consume all three recording directories in deterministic first-touch
|
||||
/// order. A `None` image means the entity/object was added by the
|
||||
/// command; every sketch-constraint scope's before-image is a real
|
||||
/// `SketchConstraintSet` (its `Vec` entry, once created, is never
|
||||
/// removed, so there is no "didn't exist before" case there).
|
||||
pub fn into_recorded_images(
|
||||
mut self,
|
||||
) -> (
|
||||
Vec<(Handle, Option<Arc<EntityType>>)>,
|
||||
Vec<(Handle, Option<ObjectType>)>,
|
||||
Vec<(sketch_constraints::SketchScope, sketch_constraints::SketchConstraintSet)>,
|
||||
) {
|
||||
let entities = self
|
||||
.order
|
||||
@@ -362,7 +375,12 @@ impl UndoRecording {
|
||||
.drain(..)
|
||||
.map(|h| (h, self.object_before.remove(&h).flatten()))
|
||||
.collect();
|
||||
(entities, objects)
|
||||
let sketch_constraints = self
|
||||
.sketch_constraints_order
|
||||
.drain(..)
|
||||
.filter_map(|scope| self.sketch_constraints_before.remove(&scope).map(|before| (scope, before)))
|
||||
.collect();
|
||||
(entities, objects, sketch_constraints)
|
||||
}
|
||||
|
||||
/// Entity-only convenience used by the focused Scene delta tests.
|
||||
@@ -2538,6 +2556,26 @@ impl Scene {
|
||||
}
|
||||
}
|
||||
|
||||
/// Record one sketch-constraint scope's whole-set image before its first
|
||||
/// mutation within the open recording (first touch wins) — e.g. ERASE
|
||||
/// about to remove some of a scope's constraints via
|
||||
/// `refresh_sketch_constraints`'s deletion policy. `sketch_constraints`
|
||||
/// lives on `Scene`, outside `document`/`document.objects`, so neither
|
||||
/// `record_undo_before` nor `record_undo_object_before` ever sees this
|
||||
/// change on their own.
|
||||
pub(crate) fn record_undo_sketch_constraints_before(
|
||||
&mut self,
|
||||
scope: sketch_constraints::SketchScope,
|
||||
before: sketch_constraints::SketchConstraintSet,
|
||||
) {
|
||||
if let Some(rec) = self.undo_recording.as_mut() {
|
||||
if !rec.sketch_constraints_before.contains_key(&scope) {
|
||||
rec.sketch_constraints_order.push(scope);
|
||||
rec.sketch_constraints_before.insert(scope, 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.
|
||||
@@ -11007,7 +11045,7 @@ mod delta_undo_tests {
|
||||
let handle = scene.add_entity(EntityType::RasterImage(image));
|
||||
let rec = scene.take_undo_recording().unwrap();
|
||||
assert!(!rec.is_poisoned());
|
||||
let (entities, objects) = rec.into_recorded_images();
|
||||
let (entities, objects, _sketch_constraints) = rec.into_recorded_images();
|
||||
assert_eq!(entities.len(), 1);
|
||||
assert_eq!(entities[0].0, handle);
|
||||
assert_eq!(objects.len(), 1);
|
||||
@@ -11031,7 +11069,7 @@ mod delta_undo_tests {
|
||||
scene.erase_entities(&[h1]);
|
||||
let rec = scene.take_undo_recording().unwrap();
|
||||
assert!(!rec.is_poisoned());
|
||||
let (entities, objects) = rec.into_recorded_images();
|
||||
let (entities, objects, _sketch_constraints) = rec.into_recorded_images();
|
||||
assert_eq!(entities.len(), 1);
|
||||
assert_eq!(entities[0].0, h1);
|
||||
assert_eq!(objects.len(), 1);
|
||||
|
||||
@@ -568,6 +568,20 @@ struct MeshSurfaceParams {
|
||||
flags: [u32; 4],
|
||||
}
|
||||
|
||||
/// Downscale a decoded RGBA buffer so neither dimension exceeds `limit`,
|
||||
/// preserving aspect ratio (scaled by whichever dimension overshoots more).
|
||||
/// `None` only for genuinely malformed input (`rgba`'s length doesn't match
|
||||
/// `width * height * 4`) — the caller falls back to the 1x1 material color
|
||||
/// in that case, same as it already does for a missing image entirely.
|
||||
fn downscale_rgba_to_limit(width: u32, height: u32, rgba: &[u8], limit: u32) -> Option<(u32, u32, Vec<u8>)> {
|
||||
let buffer = image::RgbaImage::from_raw(width, height, rgba.to_vec())?;
|
||||
let scale = (limit as f64 / width.max(height) as f64).min(1.0);
|
||||
let new_width = ((width as f64 * scale).round() as u32).clamp(1, limit);
|
||||
let new_height = ((height as f64 * scale).round() as u32).clamp(1, limit);
|
||||
let resized = image::imageops::resize(&buffer, new_width, new_height, image::imageops::FilterType::Triangle);
|
||||
Some((new_width, new_height, resized.into_raw()))
|
||||
}
|
||||
|
||||
fn upload_rgba_texture(
|
||||
device: &wgpu::Device,
|
||||
queue: &wgpu::Queue,
|
||||
@@ -576,9 +590,27 @@ fn upload_rgba_texture(
|
||||
fallback: [u8; 4],
|
||||
srgb: bool,
|
||||
) -> wgpu::TextureView {
|
||||
let (width, height, pixels) = image.map_or((1, 1, fallback.as_slice()), |image| {
|
||||
(image.width, image.height, image.rgba.as_slice())
|
||||
});
|
||||
// A material map's decoded dimensions come straight from the source file
|
||||
// (`material_model::load_map_image`), uncapped — an oversized image would
|
||||
// otherwise fail `wgpu` texture-size validation and leave the material
|
||||
// unusable (audit: raster-image rendering already tiles to this same
|
||||
// limit in `image_gpu.rs`; a mesh material sampled by UV can't use that
|
||||
// per-quad tiling trick, so downscale instead of tiling).
|
||||
let limit = device.limits().max_texture_dimension_2d.max(1);
|
||||
let resized;
|
||||
let (width, height, pixels): (u32, u32, &[u8]) = match image {
|
||||
Some(image) if image.width > limit || image.height > limit => {
|
||||
match downscale_rgba_to_limit(image.width, image.height, &image.rgba, limit) {
|
||||
Some((w, h, buf)) => {
|
||||
resized = buf;
|
||||
(w, h, resized.as_slice())
|
||||
}
|
||||
None => (1, 1, fallback.as_slice()),
|
||||
}
|
||||
}
|
||||
Some(image) => (image.width, image.height, image.rgba.as_slice()),
|
||||
None => (1, 1, fallback.as_slice()),
|
||||
};
|
||||
let texture = device.create_texture(&wgpu::TextureDescriptor {
|
||||
label: Some(label),
|
||||
size: wgpu::Extent3d {
|
||||
@@ -2426,3 +2458,40 @@ pub fn build_mesh_batch_filtered(
|
||||
}
|
||||
(chunks, total_tris)
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod texture_limit_tests {
|
||||
use super::downscale_rgba_to_limit;
|
||||
|
||||
#[test]
|
||||
fn downscale_keeps_both_dimensions_within_the_limit() {
|
||||
// A wide-and-short image: the limit is only crossed by width, but
|
||||
// both dimensions must come out <= limit and aspect ratio preserved.
|
||||
let (w, h) = (500u32, 10u32);
|
||||
let rgba = vec![0u8; (w * h * 4) as usize];
|
||||
let (new_w, new_h, buf) = downscale_rgba_to_limit(w, h, &rgba, 200).expect("well-formed input must resize");
|
||||
assert!(new_w <= 200 && new_h <= 200, "both dimensions must respect the limit, got {new_w}x{new_h}");
|
||||
assert_eq!(buf.len(), (new_w * new_h * 4) as usize, "the returned buffer must match its reported dimensions");
|
||||
// Aspect ratio 50:1 should be preserved within rounding.
|
||||
let ratio = new_w as f64 / new_h as f64;
|
||||
assert!((ratio - 50.0).abs() < 1.0, "expected aspect ratio near 50:1, got {ratio}");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn downscale_handles_a_square_image_over_the_limit() {
|
||||
let (w, h) = (300u32, 300u32);
|
||||
let rgba = vec![255u8; (w * h * 4) as usize];
|
||||
let (new_w, new_h, buf) = downscale_rgba_to_limit(w, h, &rgba, 128).expect("well-formed input must resize");
|
||||
assert_eq!(new_w, 128);
|
||||
assert_eq!(new_h, 128);
|
||||
assert_eq!(buf.len(), (128 * 128 * 4) as usize);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn downscale_rejects_a_buffer_whose_length_does_not_match_its_dimensions() {
|
||||
// Corrupt/mismatched input (audit's "validate decoded byte length" ask):
|
||||
// must fail cleanly, not panic or silently misread the buffer.
|
||||
let rgba = vec![0u8; 10];
|
||||
assert!(downscale_rgba_to_limit(1000, 1000, &rgba, 512).is_none());
|
||||
}
|
||||
}
|
||||
+121
-4
@@ -7,7 +7,7 @@
|
||||
//! producing their normal wire, hatch, image, wipeout, or mesh model.
|
||||
|
||||
use acadrust::entities::Insert;
|
||||
use acadrust::types::{Color, Transform, Vector3};
|
||||
use acadrust::types::{Color, Matrix3, Matrix4, Transform, Vector3};
|
||||
use acadrust::{CadDocument, EntityType, Handle};
|
||||
use rustc_hash::{FxHashMap, FxHashSet};
|
||||
|
||||
@@ -1042,11 +1042,34 @@ pub fn document_block_uses(document: &CadDocument) -> Vec<BlockUse> {
|
||||
uses
|
||||
}
|
||||
|
||||
/// A MINSERT's row/column counts are parsed `u16`s whose unchecked product
|
||||
/// (`Insert::instance_count`) would otherwise drive this function's
|
||||
/// allocation and a full per-instance render-graph walk after it — a corrupt
|
||||
/// or adversarial 65535x65535 pair requests over four billion instances (tens
|
||||
/// of GiB) long before any GPU-side chunking limit could help. The primary
|
||||
/// defense is the load-time filter in `io::is_entity_corrupt` (kept in sync
|
||||
/// with this constant), which drops such an insert before it ever reaches a
|
||||
/// `Scene`; this is the failsafe for an insert built or edited some other way
|
||||
/// (API/scripting) that skips that path.
|
||||
const MAX_MINSERT_INSTANCES: usize = 20_000;
|
||||
|
||||
pub fn array_offsets(insert: &Insert) -> Vec<[f64; 3]> {
|
||||
if !insert.is_minsert() {
|
||||
return vec![[0.0; 3]];
|
||||
}
|
||||
let mut offsets = Vec::with_capacity(insert.instance_count());
|
||||
let instance_count = insert.instance_count();
|
||||
if instance_count > MAX_MINSERT_INSTANCES {
|
||||
eprintln!(
|
||||
"MINSERT '{}' requests {} instances ({}x{}), exceeding the {} budget; rendering a single instance instead",
|
||||
insert.block_name,
|
||||
instance_count,
|
||||
insert.row_count,
|
||||
insert.column_count,
|
||||
MAX_MINSERT_INSTANCES
|
||||
);
|
||||
return vec![[0.0; 3]];
|
||||
}
|
||||
let mut offsets = Vec::with_capacity(instance_count);
|
||||
for row in 0..insert.row_count {
|
||||
for column in 0..insert.column_count {
|
||||
offsets.push([
|
||||
@@ -1070,8 +1093,20 @@ pub fn insert_instance_transform(
|
||||
if offset == [0.0; 3] {
|
||||
transform
|
||||
} else {
|
||||
Transform::from_translation(Vector3::new(offset[0], offset[1], offset[2]))
|
||||
.then(&transform)
|
||||
// Row/column spacing is defined in the insert's rotated/OCS-oriented
|
||||
// space but is NOT subject to the block's x/y/z scale — this matches
|
||||
// the pinned CAD model's own `Insert::array_points`, which rotates
|
||||
// the offset but never scales it. Composing the raw offset ahead of
|
||||
// the full (scaled) transform, as this used to do, let the block
|
||||
// scale multiply the spacing too (e.g. an x-scale-2 insert with
|
||||
// column spacing 10 would render 20 units between columns). Instead,
|
||||
// orient the offset by rotation + OCS only, then translate the
|
||||
// already-composed (scaled) instance transform by that world-space
|
||||
// vector, applied after — never scaled.
|
||||
let orientation = Transform::from_matrix(Matrix4::rotation_z(insert.rotation))
|
||||
.then(&Transform::from_matrix(Matrix4::from_matrix3(Matrix3::arbitrary_axis(insert.normal))));
|
||||
let world_offset = orientation.apply_rotation(Vector3::new(offset[0], offset[1], offset[2]));
|
||||
transform.then(&Transform::from_translation(world_offset))
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1163,4 +1198,86 @@ mod tests {
|
||||
assert!((from_insert.x - 12.0).abs() < 1.0e-9);
|
||||
assert!((applied.x - 11.0).abs() < 1.0e-9);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn array_offsets_clamps_pathological_minsert_counts() {
|
||||
// A corrupt or adversarial row/column pair (parsed as u16, so this is
|
||||
// reachable from a malformed file) must not drive a multi-billion-
|
||||
// element allocation; the primary defense is the load-time filter in
|
||||
// `io::is_entity_corrupt`, this is the render-graph-side failsafe.
|
||||
let mut insert = Insert::new("BLOCK", Vector3::ZERO);
|
||||
insert.row_count = u16::MAX;
|
||||
insert.column_count = u16::MAX;
|
||||
insert.row_spacing = 1.0;
|
||||
insert.column_spacing = 1.0;
|
||||
let offsets = array_offsets(&insert);
|
||||
assert_eq!(offsets, vec![[0.0; 3]], "pathological instance count must clamp to a single instance");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn array_offsets_is_unaffected_for_a_reasonable_minsert() {
|
||||
let mut insert = Insert::new("BLOCK", Vector3::ZERO);
|
||||
insert.row_count = 3;
|
||||
insert.column_count = 4;
|
||||
insert.row_spacing = 5.0;
|
||||
insert.column_spacing = 2.0;
|
||||
let offsets = array_offsets(&insert);
|
||||
assert_eq!(offsets.len(), 12);
|
||||
assert_eq!(offsets[0], [0.0, 0.0, 0.0]);
|
||||
assert_eq!(offsets[1], [2.0, 0.0, 0.0]);
|
||||
assert_eq!(offsets[4], [0.0, 5.0, 0.0]);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn insert_instance_transform_does_not_scale_the_row_column_spacing() {
|
||||
// github discussion-derived report: an INSERT with x-scale 2 and
|
||||
// column spacing 10 was rendering 20 drawing units between columns
|
||||
// instead of 10, because the offset translation was composed ahead
|
||||
// of (and thus multiplied by) the block scale.
|
||||
let document = CadDocument::new();
|
||||
let mut insert = Insert::new("BLOCK", Vector3::new(100.0, 0.0, 0.0));
|
||||
insert.set_x_scale(2.0);
|
||||
insert.row_count = 1;
|
||||
insert.column_count = 2;
|
||||
insert.column_spacing = 10.0;
|
||||
|
||||
let transform = insert_instance_transform(
|
||||
&document,
|
||||
&insert,
|
||||
[10.0, 0.0, 0.0],
|
||||
1.0,
|
||||
BlockScalePolicy::Applied,
|
||||
);
|
||||
// The block's own origin (local point 0,0,0) placed at this instance
|
||||
// must land 10 units from the insert point, not 20.
|
||||
let world = transform.apply(Vector3::ZERO);
|
||||
assert!((world.x - 110.0).abs() < 1.0e-9, "expected spacing 10, got offset {}", world.x - 100.0);
|
||||
|
||||
// Block-local geometry must still pick up the block scale normally.
|
||||
let block_point = transform.apply(Vector3::new(1.0, 0.0, 0.0));
|
||||
assert!((block_point.x - 112.0).abs() < 1.0e-9, "block-local geometry should still be scaled by x_scale 2");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn insert_instance_transform_rotates_the_spacing_with_the_insert() {
|
||||
// Matches the pinned CAD model's own `Insert::array_points`: spacing
|
||||
// is rotated with the insert but never scaled.
|
||||
let document = CadDocument::new();
|
||||
let mut insert = Insert::new("BLOCK", Vector3::ZERO);
|
||||
insert.rotation = std::f64::consts::FRAC_PI_2;
|
||||
insert.row_count = 1;
|
||||
insert.column_count = 2;
|
||||
insert.column_spacing = 10.0;
|
||||
|
||||
let transform = insert_instance_transform(
|
||||
&document,
|
||||
&insert,
|
||||
[10.0, 0.0, 0.0],
|
||||
1.0,
|
||||
BlockScalePolicy::Applied,
|
||||
);
|
||||
let world = transform.apply(Vector3::ZERO);
|
||||
assert!(world.x.abs() < 1.0e-9, "a 90deg rotation should turn column spacing into Y, got x={}", world.x);
|
||||
assert!((world.y - 10.0).abs() < 1.0e-9, "expected the spacing rotated onto Y, got y={}", world.y);
|
||||
}
|
||||
}
|
||||
@@ -85,7 +85,7 @@ pub type ConstraintId = u32;
|
||||
/// One persisted constraint record: a friendly [`ConstraintKind`], the
|
||||
/// entities/points it relates, and — for a dimensional kind — the value
|
||||
/// driving it.
|
||||
#[derive(Debug, Clone, Serialize, Deserialize)]
|
||||
#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)]
|
||||
pub struct SketchConstraint {
|
||||
pub id: ConstraintId,
|
||||
pub kind: ConstraintKind,
|
||||
|
||||
@@ -17,9 +17,31 @@
|
||||
|
||||
use super::sketch_constraints::SketchConstraintSet;
|
||||
use super::Scene;
|
||||
use acadrust::{CadDocument, Handle};
|
||||
|
||||
const XRECORD_KEY: &str = "OCS_SKETCH_CONSTRAINTS";
|
||||
|
||||
/// Removes a named XRecord from `owner`'s extension dictionary, if present.
|
||||
/// The dictionary itself is left in place even if now empty — other code may
|
||||
/// already reference it via `xdictionary_handle`, and an empty extension
|
||||
/// dictionary is harmless bookkeeping, unlike a stale XRecord that would
|
||||
/// resurrect deleted data on the next load.
|
||||
fn remove_xrecord(document: &mut CadDocument, owner: Handle, key: &str) {
|
||||
let Some(dictionary_handle) = document.extension_dictionary_handle(owner) else {
|
||||
return;
|
||||
};
|
||||
let Some(acadrust::objects::ObjectType::Dictionary(dictionary)) =
|
||||
document.objects.get_mut(&dictionary_handle)
|
||||
else {
|
||||
return;
|
||||
};
|
||||
let Some(index) = dictionary.entries.iter().position(|(name, _)| name.eq_ignore_ascii_case(key)) else {
|
||||
return;
|
||||
};
|
||||
let (_, record_handle) = dictionary.entries.remove(index);
|
||||
document.objects.remove(&record_handle);
|
||||
}
|
||||
|
||||
/// Prefixed onto every serialized blob so a future schema change can be
|
||||
/// detected and gracefully skipped (dropping just that one scope's
|
||||
/// constraints, not the whole load) rather than silently misreading bytes
|
||||
@@ -53,13 +75,20 @@ impl Scene {
|
||||
/// edits ride the ordinary object-delta undo path per §5.1(b), but this
|
||||
/// lazy model was the doc's own recommended first step).
|
||||
///
|
||||
/// A scope with an empty constraint set is skipped rather than writing
|
||||
/// an empty XRecord — an unconstrained drawing (the common case) should
|
||||
/// not gain persisted-but-empty bookkeeping on every save.
|
||||
/// A scope with an empty constraint set writes no XRecord — an
|
||||
/// unconstrained drawing (the common case) should not gain
|
||||
/// persisted-but-empty bookkeeping on every save. If a *previous* save
|
||||
/// already materialized a non-empty XRecord for that scope, the now-empty
|
||||
/// set actively removes it instead: leaving the stale blob in place would
|
||||
/// resurrect the deleted constraints the next time the document loads.
|
||||
pub(crate) fn materialize_sketch_constraints_for_save(&mut self) {
|
||||
for index in 0..self.sketch_constraints.len() {
|
||||
let set = &self.sketch_constraints[index];
|
||||
if set.constraints.is_empty() {
|
||||
let owner = set.scope.owner_handle(&self.document);
|
||||
if !owner.is_null() {
|
||||
remove_xrecord(&mut self.document, owner, XRECORD_KEY);
|
||||
}
|
||||
continue;
|
||||
}
|
||||
let owner = set.scope.owner_handle(&self.document);
|
||||
@@ -239,6 +268,38 @@ mod tests {
|
||||
assert!(scene.document.xrecord(owner, XRECORD_KEY).is_none(), "an empty constraint set should not create an XRecord");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn clearing_a_previously_materialized_set_removes_its_stale_xrecord() {
|
||||
// Regression for the audit-flagged case `an_empty_constraint_set_is_not_materialized`
|
||||
// doesn't cover: a scope that already had a *non-empty* XRecord from a
|
||||
// prior save must have that XRecord actively removed once its
|
||||
// in-memory set goes back to empty — otherwise the stale blob
|
||||
// survives the save and resurrects the deleted constraints on the
|
||||
// next load.
|
||||
let mut scene = Scene::new();
|
||||
let a = scene.add_entity(EntityType::Line(acadrust::entities::Line::from_points(
|
||||
Vector3::new(0.0, 0.0, 0.0),
|
||||
Vector3::new(10.0, 0.0, 0.0),
|
||||
)));
|
||||
scene.sketch_constraint_set_mut(SketchScope::ModelSpace).add(ConstraintKind::Horizontal, vec![SketchRef::whole(a)], None);
|
||||
scene.materialize_sketch_constraints_for_save();
|
||||
|
||||
let owner = scene.document.header.model_space_block_handle;
|
||||
assert!(scene.document.xrecord(owner, XRECORD_KEY).is_some(), "sanity: the first save should have materialized an XRecord");
|
||||
|
||||
// Clear every constraint (mirrors deleting the constrained entity, or
|
||||
// removing the last constraint by hand) and save again.
|
||||
scene.sketch_constraint_set_mut(SketchScope::ModelSpace).constraints.clear();
|
||||
scene.materialize_sketch_constraints_for_save();
|
||||
|
||||
assert!(scene.document.xrecord(owner, XRECORD_KEY).is_none(), "the stale XRecord from the earlier non-empty save must be removed, not left behind");
|
||||
|
||||
// And the load path must agree: nothing resurrects on reload.
|
||||
scene.sketch_constraints.clear();
|
||||
scene.load_sketch_constraints_from_document();
|
||||
assert!(scene.sketch_constraint_set(SketchScope::ModelSpace).is_none(), "no constraints should come back after the set was cleared and re-saved");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn decode_rejects_a_mismatched_format_version() {
|
||||
let mut bytes = vec![FORMAT_VERSION.wrapping_add(1)];
|
||||
|
||||
@@ -383,12 +383,24 @@ impl Scene {
|
||||
// and unconditionally over every scope (not just ones a `touched`
|
||||
// check would catch), so a scope left with zero constraints after
|
||||
// this doesn't attempt a pointless resolve below.
|
||||
//
|
||||
// Recorded into the *same* undo transaction as the entity removal
|
||||
// that caused it (audit finding: ERASE undo silently lost constraint
|
||||
// state, since `sketch_constraints` lives outside `document`/
|
||||
// `document.objects` and neither of those directories ever saw this
|
||||
// mutation) — one undo press now restores both the entity and its
|
||||
// constraints together.
|
||||
for (handle, kind) in changes {
|
||||
if *kind != ChangeKind::Removed {
|
||||
continue;
|
||||
}
|
||||
for set in &mut self.sketch_constraints {
|
||||
set.remove_all_touching(*handle);
|
||||
for i in 0..self.sketch_constraints.len() {
|
||||
if self.sketch_constraints[i].constraints_touching(*handle).next().is_some() {
|
||||
let scope = self.sketch_constraints[i].scope;
|
||||
let before = self.sketch_constraints[i].clone();
|
||||
self.record_undo_sketch_constraints_before(scope, before);
|
||||
self.sketch_constraints[i].remove_all_touching(*handle);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -504,12 +516,70 @@ mod tests {
|
||||
scene.bump_entities(&[(a, super::ChangeKind::Modified)]);
|
||||
let recording = scene.take_undo_recording().expect("an undo recording should still be open");
|
||||
|
||||
let (entities, _objects) = recording.into_recorded_images();
|
||||
let (entities, _objects, _sketch_constraints) = recording.into_recorded_images();
|
||||
let touched: std::collections::HashSet<_> = entities.iter().map(|(h, _)| *h).collect();
|
||||
assert!(touched.contains(&a), "the directly-edited line must be in the undo delta");
|
||||
assert!(touched.contains(&b), "the constraint-solved neighbor must ride the same undo delta");
|
||||
}
|
||||
|
||||
/// Audit-flagged gap: ERASE removing an entity takes its constraints with
|
||||
/// it (design doc's deletion policy, above), but `sketch_constraints`
|
||||
/// lives on `Scene`, outside `document`/`document.objects`, so nothing
|
||||
/// used to capture that mutation for undo at all — the erased geometry
|
||||
/// came back on undo, but its constraints didn't. This proves the fix at
|
||||
/// the level it actually lives: `refresh_sketch_constraints`'s deletion
|
||||
/// pass must record the scope's whole before-image into the same
|
||||
/// `UndoRecording` the entity removal itself rides in, so one recovered
|
||||
/// image is enough to restore both together (the app-level wiring that
|
||||
/// turns this into one committed, one-undo-press `DeltaSnapshot` is
|
||||
/// `OpenCADStudio::commit_undo_delta`/`apply_delta_state`, exercised by
|
||||
/// the app, not `Scene`, so it's out of this test's reach — this covers
|
||||
/// the root cause, not that outer plumbing).
|
||||
#[test]
|
||||
fn erasing_a_constrained_entity_records_its_scope_for_undo() {
|
||||
let mut scene = Scene::new();
|
||||
let a = scene.add_entity(EntityType::Line(acadrust::entities::Line::from_points(
|
||||
Vector3::new(0.0, 0.0, 0.0),
|
||||
Vector3::new(10.0, 0.0, 0.0),
|
||||
)));
|
||||
let b = scene.add_entity(EntityType::Line(acadrust::entities::Line::from_points(
|
||||
Vector3::new(10.0, 0.0, 0.0),
|
||||
Vector3::new(20.0, 5.0, 0.0),
|
||||
)));
|
||||
scene.sketch_constraint_set_mut(SketchScope::ModelSpace).add(
|
||||
ConstraintKind::Coincident,
|
||||
vec![SketchRef::point(a, 1), SketchRef::point(b, 0)],
|
||||
None,
|
||||
);
|
||||
assert_eq!(scene.sketch_constraint_set(SketchScope::ModelSpace).unwrap().constraints.len(), 1);
|
||||
|
||||
scene.begin_undo_recording();
|
||||
scene.erase_entities(&[a]);
|
||||
|
||||
// The live scope must already have lost the constraint (design doc's
|
||||
// deletion policy) — this ensures the test actually exercises restore,
|
||||
// not a no-op.
|
||||
assert_eq!(
|
||||
scene.sketch_constraint_set(SketchScope::ModelSpace).map_or(0, |s| s.constraints.len()),
|
||||
0,
|
||||
"the constraint touching the erased line should be gone from the live set"
|
||||
);
|
||||
|
||||
let recording = scene.take_undo_recording().expect("an undo recording should still be open");
|
||||
let (_entities, _objects, sketch_constraints) = recording.into_recorded_images();
|
||||
assert_eq!(sketch_constraints.len(), 1, "the touched scope's before-image must be captured");
|
||||
let (scope, before) = &sketch_constraints[0];
|
||||
assert_eq!(*scope, SketchScope::ModelSpace);
|
||||
assert_eq!(before.constraints.len(), 1, "the before-image must still hold the constraint as it was before the erase");
|
||||
assert_eq!(before.constraints[0].kind, ConstraintKind::Coincident);
|
||||
assert_eq!(before.constraints[0].refs, vec![SketchRef::point(a, 1), SketchRef::point(b, 0)]);
|
||||
|
||||
// What `apply_delta_state` does with this on undo: install the
|
||||
// before-image back as the live scope state.
|
||||
*scene.sketch_constraint_set_mut(*scope) = before.clone();
|
||||
assert_eq!(scene.sketch_constraint_set(SketchScope::ModelSpace).unwrap().constraints.len(), 1, "restoring the before-image must bring the constraint back");
|
||||
}
|
||||
|
||||
/// Design doc §7 open question 6 (live-drag re-solve):
|
||||
/// `solve_sketch_constraints_preview` must report the constrained
|
||||
/// neighbor's solved position without writing anything into the
|
||||
|
||||
+376
@@ -1367,6 +1367,29 @@ impl Snapper {
|
||||
&& (local_segments.is_some() || allow_unindexed_pairwise)
|
||||
{
|
||||
if let Some(segments) = &local_segments {
|
||||
// Circle/arc pairs are solved exactly up front (bug #1052 —
|
||||
// see `exact_circle_intersections`'s doc comment); the
|
||||
// segment sweep below then skips any pair already resolved
|
||||
// this way, so the stale tessellated approximation never
|
||||
// competes with the true point as a candidate.
|
||||
let mut local_wires: Vec<u32> = segments.iter().map(|s| s.wire).collect();
|
||||
local_wires.sort_unstable();
|
||||
local_wires.dedup();
|
||||
let mut resolved_pairs: Vec<(u32, u32)> = Vec::new();
|
||||
for (idx, &wi) in local_wires.iter().enumerate() {
|
||||
for &wj in &local_wires[idx + 1..] {
|
||||
let (Some(wire_i), Some(wire_j)) = (wires.source_wire(wi), wires.source_wire(wj)) else {
|
||||
continue;
|
||||
};
|
||||
if let Some(pts) = exact_circle_intersections(wire_i, wire_j) {
|
||||
for pt in pts {
|
||||
try_pt(pt, SnapType::Intersection);
|
||||
}
|
||||
resolved_pairs.push((wi, wj));
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// Exact cursor-local sweep: never discard a valid intersection
|
||||
// in dense geometry. Min-X ordering plus Y overlap avoids
|
||||
// comparing segment pairs whose bounds cannot meet.
|
||||
@@ -1382,6 +1405,9 @@ impl Snapper {
|
||||
if a.wire == b.wire || a.max_y() < b.min_y() || a.min_y() > b.max_y() {
|
||||
continue;
|
||||
}
|
||||
if resolved_pairs.contains(&(a.wire.min(b.wire), a.wire.max(b.wire))) {
|
||||
continue;
|
||||
}
|
||||
if let Some(pt) = seg_intersect_3d(a.a, a.b, b.a, b.b) {
|
||||
let exact_cursor = dist2(
|
||||
world_to_screen(pt, view_rot, eye, bounds),
|
||||
@@ -1410,6 +1436,14 @@ impl Snapper {
|
||||
if !wire_in_range(wire_j) {
|
||||
continue;
|
||||
}
|
||||
// Circle/arc pairs are solved exactly (bug #1052);
|
||||
// see `exact_circle_intersections`'s doc comment.
|
||||
if let Some(pts) = exact_circle_intersections(wire_i, wire_j) {
|
||||
for pt in pts {
|
||||
try_pt(pt, SnapType::Intersection);
|
||||
}
|
||||
continue;
|
||||
}
|
||||
for ai in 0..wire_i.points.len().saturating_sub(1) {
|
||||
// S: pre-convert outside inner loop
|
||||
let a0 = wp_f64(wire_i, ai);
|
||||
@@ -2299,6 +2333,155 @@ fn ray_segment_intersect_3d(
|
||||
))
|
||||
}
|
||||
|
||||
/// A full circle recovered from a wire's `tangent_geoms` — the entity's real
|
||||
/// definition, in world space — used to solve Intersection snap exactly for
|
||||
/// circle/arc pairs instead of falling back to `seg_intersect_3d` against
|
||||
/// their tessellated approximation.
|
||||
///
|
||||
/// Why this exists (GitHub discussion #1052): a chord of *any* tessellated
|
||||
/// circle sits strictly inside the true circle, so the "intersection" of two
|
||||
/// tessellated circles is never quite where the two real circles actually
|
||||
/// cross — the error is small but real, and does not shrink with zoom: the
|
||||
/// resident wire set OSNAP searches is built once per geometry edit with a
|
||||
/// fixed 48-segment tessellation for every circle regardless of radius, by
|
||||
/// design (`Scene::resident_wires_for`'s own doc comment: zoom-independent,
|
||||
/// so hit-testing/snap don't re-tessellate on every camera move). A r=100
|
||||
/// circle's chord sagitta at 48 segments is already ≈0.21 world units.
|
||||
struct WireCircle {
|
||||
center: DVec3,
|
||||
radius: f64,
|
||||
/// Unit normal of the plane the circle lies in.
|
||||
normal: DVec3,
|
||||
/// `Some((axis_x, axis_y, start_angle, end_angle))` for an arc — used to
|
||||
/// keep only the full circle's crossing points that actually fall
|
||||
/// within the arc's own sweep. `None` for a full circle: every point
|
||||
/// the circle-circle solve finds is valid. Angles follow this
|
||||
/// codebase's own convention (`src/entities/arc.rs`): measured from
|
||||
/// `axis_x` toward `axis_y`, equal start/end meaning a full turn.
|
||||
arc_span: Option<(DVec3, DVec3, f64, f64)>,
|
||||
}
|
||||
|
||||
/// The wire's underlying circle, if it's simple enough to have one: a single
|
||||
/// Circle or Arc entity as a whole, not a compound chain. Lines return
|
||||
/// `None` here and fall back to the ordinary tessellated sweep unchanged
|
||||
/// (line-vs-line is already exact via the segment sweep). This branch's
|
||||
/// `TangentGeom` has no `PlanarEllipse` variant yet (it predates that
|
||||
/// addition on `main`), so ellipse intersections aren't addressed by this
|
||||
/// port — see the equivalent fix on `main` (and the upstream PR) for the
|
||||
/// general `exact_curve_intersections` path once that variant lands here
|
||||
/// too. A polyline with a bulge segment still goes through the tessellated
|
||||
/// sweep: it isn't a single recognized curve.
|
||||
fn wire_circle(wire: &WireModel) -> Option<WireCircle> {
|
||||
let [geom] = wire.tangent_geoms.as_slice() else {
|
||||
return None;
|
||||
};
|
||||
match geom {
|
||||
TangentGeom::PlanarCircle { center, axis_x, axis_y, radius } => {
|
||||
let (axis_x, axis_y) = (DVec3::new(axis_x[0], axis_x[1], axis_x[2]), DVec3::new(axis_y[0], axis_y[1], axis_y[2]));
|
||||
Some(WireCircle {
|
||||
center: DVec3::new(center[0], center[1], center[2]),
|
||||
radius: *radius,
|
||||
normal: axis_x.cross(axis_y).normalize(),
|
||||
arc_span: None,
|
||||
})
|
||||
}
|
||||
TangentGeom::Circle { center, radius } => Some(WireCircle {
|
||||
center: DVec3::new(center[0] as f64, center[1] as f64, center[2] as f64),
|
||||
radius: *radius as f64,
|
||||
normal: DVec3::Z,
|
||||
arc_span: None,
|
||||
}),
|
||||
TangentGeom::Arc { center, axis_x, axis_y, radius, start_angle, end_angle } => {
|
||||
let (axis_x, axis_y) = (DVec3::new(axis_x[0], axis_x[1], axis_x[2]), DVec3::new(axis_y[0], axis_y[1], axis_y[2]));
|
||||
Some(WireCircle {
|
||||
center: DVec3::new(center[0], center[1], center[2]),
|
||||
radius: *radius,
|
||||
normal: axis_x.cross(axis_y).normalize(),
|
||||
arc_span: Some((axis_x, axis_y, *start_angle, *end_angle)),
|
||||
})
|
||||
}
|
||||
TangentGeom::Line { .. } => None,
|
||||
}
|
||||
}
|
||||
|
||||
impl WireCircle {
|
||||
/// Whether world point `p` — assumed already on this circle — falls
|
||||
/// within the arc's actual angular sweep. Always true for a full circle.
|
||||
fn contains_point(&self, p: DVec3) -> bool {
|
||||
let Some((axis_x, axis_y, start, end)) = self.arc_span else {
|
||||
return true;
|
||||
};
|
||||
let rel = p - self.center;
|
||||
let tau = std::f64::consts::TAU;
|
||||
let angle = rel.dot(axis_y).atan2(rel.dot(axis_x)).rem_euclid(tau);
|
||||
let span = (end - start).rem_euclid(tau);
|
||||
let from_start = (angle - start).rem_euclid(tau);
|
||||
// Equal start/end means a full turn (this codebase's own Arc
|
||||
// convention), not zero sweep.
|
||||
span == 0.0 || from_start <= span + 1e-9
|
||||
}
|
||||
}
|
||||
|
||||
/// Exact circle/arc intersection for one wire pair, bypassing their
|
||||
/// tessellated approximation entirely. `None` when neither side is a circle
|
||||
/// or arc (the segment sweep is already exact for pure lines/polylines), the
|
||||
/// two aren't coplanar (a genuinely-3D case the sweep already handles no
|
||||
/// worse than before), or they don't actually meet — in every `None` case
|
||||
/// the caller falls back to the ordinary segment sweep unchanged.
|
||||
///
|
||||
/// Correct for any circle/arc-vs-circle/arc pair regardless of whether the
|
||||
/// two sides' own local frames agree in orientation: the candidate points
|
||||
/// come from a frame-agnostic 3D construction (`a.normal`/`dir`/`perp`
|
||||
/// below), and each side's `WireCircle::contains_point` then checks a
|
||||
/// candidate against *that side's own* axis_x/axis_y independently — there
|
||||
/// is no step that re-expresses one arc's angle range in the other's frame,
|
||||
/// so this handles arc-vs-arc (even two arcs with differently-rotated
|
||||
/// frames — see the test below) exactly as directly as circle-vs-circle.
|
||||
///
|
||||
/// Scoped to circle/arc pairs: a line-vs-circle/arc pair and anything
|
||||
/// involving an ellipse are left to the existing sweep here (this branch's
|
||||
/// `TangentGeom` has no ellipse variant yet — see this function's sibling
|
||||
/// `wire_circle`'s doc comment).
|
||||
fn exact_circle_intersections(wire_a: &WireModel, wire_b: &WireModel) -> Option<Vec<DVec3>> {
|
||||
let a = wire_circle(wire_a)?;
|
||||
let b = wire_circle(wire_b)?;
|
||||
|
||||
let d_vec = b.center - a.center;
|
||||
let d = d_vec.length();
|
||||
if d < 1e-9 {
|
||||
return None; // concentric: coincident or no isolated crossing either way
|
||||
}
|
||||
|
||||
// Coplanarity: both circles' planes must agree (normals parallel, up to
|
||||
// sign), and the vector between centers must actually lie in that plane
|
||||
// — otherwise this is a genuinely-3D case the tessellated sweep already
|
||||
// handles (via `seg_intersect_3d`'s own height-match check) no worse
|
||||
// than an exact-but-wrong-plane answer would.
|
||||
const PLANE_TOL: f64 = 1e-7;
|
||||
if a.normal.cross(b.normal).length() > PLANE_TOL {
|
||||
return None;
|
||||
}
|
||||
if d_vec.dot(a.normal).abs() > PLANE_TOL * d.max(1.0) {
|
||||
return None;
|
||||
}
|
||||
|
||||
let (r1, r2) = (a.radius, b.radius);
|
||||
if d > r1 + r2 + PLANE_TOL || d < (r1 - r2).abs() - PLANE_TOL {
|
||||
return None; // circles don't meet
|
||||
}
|
||||
|
||||
let dir = d_vec / d;
|
||||
let a_dist = ((d * d + r1 * r1 - r2 * r2) / (2.0 * d)).clamp(-r1, r1);
|
||||
let h = (r1 * r1 - a_dist * a_dist).max(0.0).sqrt();
|
||||
let mid = a.center + dir * a_dist;
|
||||
let perp = a.normal.cross(dir).normalize();
|
||||
|
||||
let candidates = if h < 1e-9 { vec![mid] } else { vec![mid + perp * h, mid - perp * h] };
|
||||
let points: Vec<DVec3> = candidates.into_iter().filter(|&p| a.contains_point(p) && b.contains_point(p)).collect();
|
||||
|
||||
(!points.is_empty()).then_some(points)
|
||||
}
|
||||
|
||||
/// XY-plane segment-segment intersection. Returns `None` if parallel or outside.
|
||||
/// True 3D intersection of two segments: the point where their plan (XY)
|
||||
/// projections cross **and** both segments are at the same height there. Returns
|
||||
@@ -2878,4 +3061,197 @@ mod ext_tests {
|
||||
hi.z
|
||||
);
|
||||
}
|
||||
|
||||
/// GitHub discussion #1052's own reproduction: a 3-unit base line, a
|
||||
/// radius-4 circle at one end, a radius-5 circle at the other. The true
|
||||
/// intersection is exactly (0, 4, 0) (a 3-4-5 right triangle) — the
|
||||
/// tessellated segment sweep misses this by a small but real amount
|
||||
/// (confirmed separately against `seg_intersect_3d` on the actual
|
||||
/// tessellated wire); the exact solver must hit it exactly.
|
||||
#[test]
|
||||
fn exact_circle_intersections_matches_the_3_4_5_report() {
|
||||
let c1 = WireModel {
|
||||
tangent_geoms: vec![TangentGeom::PlanarCircle {
|
||||
center: [0.0, 0.0, 0.0],
|
||||
axis_x: [1.0, 0.0, 0.0],
|
||||
axis_y: [0.0, 1.0, 0.0],
|
||||
radius: 4.0,
|
||||
}],
|
||||
..Default::default()
|
||||
};
|
||||
let c2 = WireModel {
|
||||
tangent_geoms: vec![TangentGeom::PlanarCircle {
|
||||
center: [3.0, 0.0, 0.0],
|
||||
axis_x: [1.0, 0.0, 0.0],
|
||||
axis_y: [0.0, 1.0, 0.0],
|
||||
radius: 5.0,
|
||||
}],
|
||||
..Default::default()
|
||||
};
|
||||
let pts = exact_circle_intersections(&c1, &c2).expect("two overlapping circles must intersect");
|
||||
assert_eq!(pts.len(), 2, "two distinct circles crossing at two points");
|
||||
let upper = pts.iter().copied().find(|p| p.y > 0.0).expect("an upper intersection");
|
||||
assert!((upper - DVec3::new(0.0, 4.0, 0.0)).length() < 1e-9, "expected exactly (0,4,0), got {upper:?}");
|
||||
for p in &pts {
|
||||
assert!(((*p - DVec3::ZERO).length() - 4.0).abs() < 1e-9, "must be exactly radius 4 from c1's centre, got {p:?}");
|
||||
assert!(((*p - DVec3::new(3.0, 0.0, 0.0)).length() - 5.0).abs() < 1e-9, "must be exactly radius 5 from c2's centre, got {p:?}");
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn exact_circle_intersections_respects_an_arcs_own_sweep() {
|
||||
// A quarter-circle arc from 0° to 90° (so it only covers the upper-
|
||||
// right quadrant) against a full circle whose two true intersection
|
||||
// points straddle that boundary: only the one actually on the arc's
|
||||
// sweep should come back.
|
||||
let arc = WireModel {
|
||||
tangent_geoms: vec![TangentGeom::Arc {
|
||||
center: [0.0, 0.0, 0.0],
|
||||
axis_x: [1.0, 0.0, 0.0],
|
||||
axis_y: [0.0, 1.0, 0.0],
|
||||
radius: 4.0,
|
||||
start_angle: 0.0,
|
||||
end_angle: std::f64::consts::FRAC_PI_2,
|
||||
}],
|
||||
..Default::default()
|
||||
};
|
||||
let circle = WireModel {
|
||||
tangent_geoms: vec![TangentGeom::PlanarCircle {
|
||||
center: [3.0, 0.0, 0.0],
|
||||
axis_x: [1.0, 0.0, 0.0],
|
||||
axis_y: [0.0, 1.0, 0.0],
|
||||
radius: 5.0,
|
||||
}],
|
||||
..Default::default()
|
||||
};
|
||||
// Full-circle math gives (0,4,0) [on the 0..90° arc] and (0,-4,0)
|
||||
// [not on it].
|
||||
let pts = exact_circle_intersections(&arc, &circle).expect("the circles still cross");
|
||||
assert_eq!(pts.len(), 1, "only the point on the arc's own sweep");
|
||||
assert!((pts[0] - DVec3::new(0.0, 4.0, 0.0)).length() < 1e-9);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn exact_circle_intersections_is_none_for_non_coplanar_circles() {
|
||||
let flat = WireModel {
|
||||
tangent_geoms: vec![TangentGeom::PlanarCircle {
|
||||
center: [0.0, 0.0, 0.0],
|
||||
axis_x: [1.0, 0.0, 0.0],
|
||||
axis_y: [0.0, 1.0, 0.0],
|
||||
radius: 4.0,
|
||||
}],
|
||||
..Default::default()
|
||||
};
|
||||
let tilted = WireModel {
|
||||
tangent_geoms: vec![TangentGeom::PlanarCircle {
|
||||
center: [0.0, 0.0, 0.0],
|
||||
axis_x: [1.0, 0.0, 0.0],
|
||||
axis_y: [0.0, 0.0, 1.0],
|
||||
radius: 4.0,
|
||||
}],
|
||||
..Default::default()
|
||||
};
|
||||
assert!(
|
||||
exact_circle_intersections(&flat, &tilted).is_none(),
|
||||
"a genuinely-3D pair must fall back to the ordinary sweep, not guess a plane"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn exact_circle_intersections_is_none_when_circles_dont_meet() {
|
||||
let near = WireModel {
|
||||
tangent_geoms: vec![TangentGeom::PlanarCircle { center: [0.0, 0.0, 0.0], axis_x: [1.0, 0.0, 0.0], axis_y: [0.0, 1.0, 0.0], radius: 1.0 }],
|
||||
..Default::default()
|
||||
};
|
||||
let far = WireModel {
|
||||
tangent_geoms: vec![TangentGeom::PlanarCircle { center: [100.0, 0.0, 0.0], axis_x: [1.0, 0.0, 0.0], axis_y: [0.0, 1.0, 0.0], radius: 1.0 }],
|
||||
..Default::default()
|
||||
};
|
||||
assert!(exact_circle_intersections(&near, &far).is_none());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn wire_circle_is_none_for_a_plain_line_wire() {
|
||||
// A line wire carries no `TangentGeom::Circle`/`Arc` entry, so
|
||||
// `wire_circle` (and `exact_circle_intersections` built on it) must
|
||||
// not panic or, worse, silently misidentify a line as a circle — it
|
||||
// should just decline, leaving a line-vs-circle pair to the ordinary
|
||||
// tessellated sweep unchanged (this port doesn't add a general
|
||||
// line-vs-round exact path — see `wire_circle`'s doc comment).
|
||||
let line = WireModel {
|
||||
points: vec![[0.0, 0.0, 0.0], [10.0, 0.0, 0.0]],
|
||||
..Default::default()
|
||||
};
|
||||
assert!(wire_circle(&line).is_none());
|
||||
let circle = WireModel {
|
||||
tangent_geoms: vec![TangentGeom::PlanarCircle { center: [0.0, 0.0, 0.0], axis_x: [1.0, 0.0, 0.0], axis_y: [0.0, 1.0, 0.0], radius: 4.0 }],
|
||||
..Default::default()
|
||||
};
|
||||
assert!(exact_circle_intersections(&line, &circle).is_none(), "line-vs-circle falls back to the tessellated sweep in this port");
|
||||
}
|
||||
|
||||
/// Each side's `contains_point` checks the candidate world point against
|
||||
/// *that side's own* native axis_x/axis_y independently — there is no
|
||||
/// step that re-expresses one arc's angle range in the other's frame —
|
||||
/// so two arcs whose local frames are rotated relative to each other
|
||||
/// must still resolve correctly, not just the common case where both
|
||||
/// happen to share the same axis_x.
|
||||
#[test]
|
||||
fn exact_circle_intersections_handles_two_arcs_in_differently_rotated_frames() {
|
||||
// Arc A: quarter circle 0..90°, axis_x along world +X.
|
||||
let arc_a = WireModel {
|
||||
tangent_geoms: vec![TangentGeom::Arc {
|
||||
center: [0.0, 0.0, 0.0],
|
||||
axis_x: [1.0, 0.0, 0.0],
|
||||
axis_y: [0.0, 1.0, 0.0],
|
||||
radius: 4.0,
|
||||
start_angle: 0.0,
|
||||
end_angle: std::f64::consts::FRAC_PI_2,
|
||||
}],
|
||||
..Default::default()
|
||||
};
|
||||
// Arc B: same true geometry as the earlier "full circle" partner,
|
||||
// but described in a frame rotated 90° from world axes (axis_x
|
||||
// along world +Y, axis_y along world -X) — its own start/end angles
|
||||
// are re-expressed accordingly so the arc still covers the same
|
||||
// true half of the circle (the lower half, i.e. world angle
|
||||
// 180..360°, which in this rotated frame is local angle 90..270°).
|
||||
let arc_b = WireModel {
|
||||
tangent_geoms: vec![TangentGeom::Arc {
|
||||
center: [3.0, 0.0, 0.0],
|
||||
axis_x: [0.0, 1.0, 0.0],
|
||||
axis_y: [-1.0, 0.0, 0.0],
|
||||
radius: 5.0,
|
||||
start_angle: std::f64::consts::FRAC_PI_2,
|
||||
end_angle: 3.0 * std::f64::consts::FRAC_PI_2,
|
||||
}],
|
||||
..Default::default()
|
||||
};
|
||||
// True full-circle crossings are (0,4,0) [world angle 90° on B, so
|
||||
// NOT on B's lower-half sweep] and (0,-4,0) [world angle 270°, IS on
|
||||
// B's sweep, and also not on A's 0..90° sweep]. Independently
|
||||
// filtering each side's own frame should therefore find nothing —
|
||||
// proving the two arcs' angle checks aren't accidentally sharing or
|
||||
// confusing each other's frame (a bug here would likely either
|
||||
// wrongly accept one of the two points or wrongly accept both).
|
||||
assert!(exact_circle_intersections(&arc_a, &arc_b).is_none());
|
||||
|
||||
// Flip A to cover the lower-right quadrant (270..360°) instead: now
|
||||
// (0,-4,0) is on both A's and B's own sweep, independently checked
|
||||
// in each one's own (differently rotated) frame.
|
||||
let arc_a_lower = WireModel {
|
||||
tangent_geoms: vec![TangentGeom::Arc {
|
||||
center: [0.0, 0.0, 0.0],
|
||||
axis_x: [1.0, 0.0, 0.0],
|
||||
axis_y: [0.0, 1.0, 0.0],
|
||||
radius: 4.0,
|
||||
start_angle: 3.0 * std::f64::consts::FRAC_PI_2,
|
||||
end_angle: std::f64::consts::TAU,
|
||||
}],
|
||||
..Default::default()
|
||||
};
|
||||
let pts = exact_circle_intersections(&arc_a_lower, &arc_b).expect("both sweeps cover (0,-4,0)");
|
||||
assert_eq!(pts.len(), 1);
|
||||
assert!((pts[0] - DVec3::new(0.0, -4.0, 0.0)).length() < 1e-9, "got {:?}", pts[0]);
|
||||
}
|
||||
}
|
||||
Reference in new issue
Block a user