diff --git a/src/app/automation.rs b/src/app/automation.rs index 5545abda1..cc3c4c748 100644 --- a/src/app/automation.rs +++ b/src/app/automation.rs @@ -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); + } } diff --git a/src/app/document.rs b/src/app/document.rs index 333072934..afc634527 100644 --- a/src/app/document.rs +++ b/src/app/document.rs @@ -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), 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::() + .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, + /// 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, pub(super) label: String, } @@ -836,6 +853,23 @@ pub(super) struct ObjectEntryDelta { pub(super) after: Option, } +/// 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, diff --git a/src/app/history.rs b/src/app/history.rs index d40870a7a..27919f0c7 100644 --- a/src/app/history.rs +++ b/src/app/history.rs @@ -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 { @@ -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>, Option>)> = entity_before .into_iter() .map(|(h, before)| { @@ -776,6 +784,21 @@ impl OpenCADStudio { }) }) .collect(); + let sketch_constraints: Vec = 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 { diff --git a/src/io/mod.rs b/src/io/mod.rs index b672b5879..6cc943eb5 100644 --- a/src/io/mod.rs +++ b/src/io/mod.rs @@ -746,6 +746,32 @@ pub fn load_bytes(name: &str, bytes: Vec) -> Result { } } +/// [`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) -> 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))); + } } diff --git a/src/scene/mod.rs b/src/scene/mod.rs index 5d32267c2..844d6f5c2 100644 --- a/src/scene/mod.rs +++ b/src/scene/mod.rs @@ -330,6 +330,15 @@ pub struct UndoRecording { /// by the command. `None` denotes a newly-created object. object_before: HashMap>, object_order: Vec, + /// 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_order: Vec, 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>)>, Vec<(Handle, Option)>, + 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); diff --git a/src/scene/pipeline/mesh_gpu.rs b/src/scene/pipeline/mesh_gpu.rs index f8eb33808..391cb139e 100644 --- a/src/scene/pipeline/mesh_gpu.rs +++ b/src/scene/pipeline/mesh_gpu.rs @@ -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)> { + 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()); + } +} diff --git a/src/scene/render_graph.rs b/src/scene/render_graph.rs index db73f1f56..5ecbc67d0 100644 --- a/src/scene/render_graph.rs +++ b/src/scene/render_graph.rs @@ -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 { 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); + } } diff --git a/src/scene/sketch_constraints.rs b/src/scene/sketch_constraints.rs index 6e7032dd8..a5bd58246 100644 --- a/src/scene/sketch_constraints.rs +++ b/src/scene/sketch_constraints.rs @@ -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, diff --git a/src/scene/sketch_persist.rs b/src/scene/sketch_persist.rs index 6fb97c6f6..4cfe0f7bd 100644 --- a/src/scene/sketch_persist.rs +++ b/src/scene/sketch_persist.rs @@ -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)]; diff --git a/src/scene/sketch_solve.rs b/src/scene/sketch_solve.rs index 565c76a94..c807d2443 100644 --- a/src/scene/sketch_solve.rs +++ b/src/scene/sketch_solve.rs @@ -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 diff --git a/src/snap.rs b/src/snap.rs index 950b29615..5955ffedc 100644 --- a/src/snap.rs +++ b/src/snap.rs @@ -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 = 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 { + 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> { + 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 = 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]); + } }