fix: keep exact snapping, source drawing data and adaptive options layout

Resolve curve intersections through cadkernel, retain empty exact results, gather analytic candidates independently of tessellation, and bound material uploads. Preserve large MINSERT data while limiting rendering work; remove obsolete review narratives.
This commit is contained in:
Hakan Seven committed 2026-09-09 13:17:30 +03:00
1 parent 4bb6aac0b6
commit 973af87d58
16 files changed
+57 -724

No files matched your search

-13
View File
@@ -319,12 +319,6 @@ impl OpenCADStudio {
Ok(b) => b, Ok(b) => b,
Err(e) => return err(format!("open: {e}")), Err(e) => return err(format!("open: {e}")),
}; };
// 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) { match crate::io::load_bytes_finalized(&path_buf, bytes) {
Ok((doc, dropped)) => { Ok((doc, dropped)) => {
let i = self.active_tab; let i = self.active_tab;
@@ -1404,13 +1398,6 @@ mod tests {
let _ = std::fs::remove_file(&path); 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] #[test]
fn open_finalizes_and_purges_like_the_ui_open_path() { fn open_finalizes_and_purges_like_the_ui_open_path() {
let mut app = OpenCADStudio::new_for_test(); let mut app = OpenCADStudio::new_for_test();
-51
View File
@@ -113,10 +113,6 @@ impl OpenCADStudio {
// unit context, the layer and linetype lists. // unit context, the layer and linetype lists.
let m_prelude = t_all.map(|t| t.elapsed().as_secs_f64() * 1000.0); let m_prelude = t_all.map(|t| t.elapsed().as_secs_f64() * 1000.0);
let t_handles = crate::perf::enabled().then(iced::time::Instant::now); let t_handles = crate::perf::enabled().then(iced::time::Instant::now);
// `selected_entities` resolves every handle to an entity and builds a
// vector of pairs; only the handles are wanted here, but the resolution
// is what decides membership — a handle whose entity is gone is not in
// the list — so it is kept, and only the pair vector avoided.
let cur_handles: Vec<acadrust::Handle> = self.tabs[i] let cur_handles: Vec<acadrust::Handle> = self.tabs[i]
.scene .scene
.selected_handles_in_order() .selected_handles_in_order()
@@ -2192,10 +2188,6 @@ impl OpenCADStudio {
..Default::default() ..Default::default()
}, },
_ => { _ => {
// The panel for a multi-selection: grouping, filtering to
// the active group, moving into the working plane, and
// aggregating into "varies" rows. `properties-detail`
// reports this whole arm as `rest`; this splits it.
let t_arm = crate::perf::enabled().then(iced::time::Instant::now); let t_arm = crate::perf::enabled().then(iced::time::Instant::now);
let groups = build_selection_groups(&selected); let groups = build_selection_groups(&selected);
let t_groups = t_arm.map(|t| t.elapsed().as_secs_f64() * 1000.0); let t_groups = t_arm.map(|t| t.elapsed().as_secs_f64() * 1000.0);
@@ -2203,11 +2195,6 @@ impl OpenCADStudio {
.and_then(|group| groups.iter().find(|g| g.label == group.label).cloned()) .and_then(|group| groups.iter().find(|g| g.label == group.label).cloned())
.or_else(|| groups.first().cloned()); .or_else(|| groups.first().cloned());
// `group.handles` is a `Vec`, so `contains` scans it. Run
// per selected entity that is O(selected x group), which on
// a whole-drawing selection is 186 468 squared — the "All"
// group holds every handle, so both factors are the full
// selection. Hashing the group once makes it linear.
let filtered: Vec<(Handle, &EntityType)> = active_group let filtered: Vec<(Handle, &EntityType)> = active_group
.as_ref() .as_ref()
.map(|group| { .map(|group| {
@@ -2227,11 +2214,6 @@ impl OpenCADStudio {
} else { } else {
crate::command::WorkingPlane::default() crate::command::WorkingPlane::default()
}; };
// `entity_in_working_plane` clones before it checks
// whether the plane is the identity, so with no UCS in
// play — the ordinary case — this deep-cloned every
// selected entity to hand back what it was given. Borrow
// instead, and clone only where a transform is applied.
let local_entities: Vec<(Handle, std::borrow::Cow<'_, EntityType>)> = let local_entities: Vec<(Handle, std::borrow::Cow<'_, EntityType>)> =
filtered filtered
.iter() .iter()
@@ -2449,10 +2431,6 @@ handles={handles_ms:.1} panel={:.1} ribbon={ribbon_ms:.1} tail={:.1} selected={}
let mut lineweight_mixed = false; let mut lineweight_mixed = false;
for (_h, e) in &selected { for (_h, e) in &selected {
// Once every field is mixed nothing further can change: a mixed
// field is already `Some`, so the remaining entities would only
// re-set flags that are set. Selecting a whole drawing reaches this
// within the first few entities instead of walking 186 468 of them.
if layer_mixed && color_mixed && linetype_mixed && lineweight_mixed { if layer_mixed && color_mixed && linetype_mixed && lineweight_mixed {
break; break;
} }
@@ -2507,10 +2485,6 @@ handles={handles_ms:.1} panel={:.1} ribbon={ribbon_ms:.1} tail={:.1} selected={}
} }
} }
/// Rebuild the cached selected_grips from the current entity selection.
/// Selections larger than this get no grips, matching AutoCAD's
/// `GRIPOBJLIMIT` default. The DWG header does not carry the variable, so
/// the default stands in for it.
const GRIP_OBJECT_LIMIT: usize = 100; const GRIP_OBJECT_LIMIT: usize = 100;
pub(super) fn refresh_selected_grips(&mut self) { pub(super) fn refresh_selected_grips(&mut self) {
@@ -2540,17 +2514,6 @@ handles={handles_ms:.1} panel={:.1} ribbon={ribbon_ms:.1} tail={:.1} selected={}
.then(|| selected[0].0); .then(|| selected[0].0);
let mut grips = Vec::new(); let mut grips = Vec::new();
let mut handles = Vec::new(); let mut handles = Vec::new();
// Above the limit, no grips at all — the same rule AutoCAD applies
// through GRIPOBJLIMIT, and for the same reason. Every selected
// entity contributes several grips, the whole list is projected and
// rebuilt into markers on **every frame**, and a 185 096-entity
// selection put ~3.9 s in this function and then held the view at
// ~10 fps for as long as the selection stood.
//
// Downstream this reads as "this selection has no grips": the
// consumers all iterate the list, so they simply find nothing, and
// grip editing is unavailable for a selection far past the size
// where dragging one is a sensible thing to do.
let selected = if selected.len() > Self::GRIP_OBJECT_LIMIT { let selected = if selected.len() > Self::GRIP_OBJECT_LIMIT {
Vec::new() Vec::new()
} else { } else {
@@ -3176,11 +3139,6 @@ pub(super) fn aggregate_sections(
return vec![]; return vec![];
} }
// Built one at a time and folded as they come. Collecting them first held
// a property list — sections, labels, formatted values, every one of them
// an allocation — for every selected entity at once, so selecting a whole
// drawing put 186 468 of them in memory simultaneously to read each one
// exactly once. The fold order is unchanged, so the result is too.
let mut entities = selected.iter(); let mut entities = selected.iter();
let Some((handle, entity)) = entities.next() else { let Some((handle, entity)) = entities.next() else {
return vec![]; return vec![];
@@ -4144,11 +4102,6 @@ mod chprop_integration_tests {
mod grip_limit_tests { mod grip_limit_tests {
use super::*; use super::*;
/// Every selected entity contributes grips, and the whole list is projected
/// and rebuilt into markers on every frame. A 185 096-entity selection put
/// ~3.9 s in `refresh_selected_grips` and then held the view at ~10 fps for
/// as long as the selection stood, which is what AutoCAD's `GRIPOBJLIMIT`
/// exists to prevent.
#[test] #[test]
fn a_selection_past_the_limit_gets_no_grips() { fn a_selection_past_the_limit_gets_no_grips() {
use acadrust::entities::{EntityType, Line}; use acadrust::entities::{EntityType, Line};
@@ -4210,10 +4163,6 @@ mod aggregation_tests {
.find(|property| property.field == field) .find(|property| property.field == field)
} }
// The aggregation folds one entity at a time rather than building every
// entity's property list first. This pins the behaviour that fold produces:
// a field all the entities agree on keeps its value, one they differ on
// reads as varying.
#[test] #[test]
fn shared_values_survive_the_fold_and_differing_ones_do_not() { fn shared_values_survive_the_fold_and_differing_ones_do_not() {
let entities = [line("WALLS", 1), line("WALLS", 3), line("WALLS", 1)]; let entities = [line("WALLS", 1), line("WALLS", 3), line("WALLS", 1)];
-4
View File
@@ -4251,10 +4251,6 @@ properties={:.1}ms picked={}",
if box_anchor.is_none() { if box_anchor.is_none() {
let (view_rot, eye, all_wires) = self.pick_view(i, &edit_cam, bounds); let (view_rot, eye, all_wires) = self.pick_view(i, &edit_cam, bounds);
let click_world = self.cursor_model_point(i, &edit_cam, p, bounds); let click_world = self.cursor_model_point(i, &edit_cam, p, bounds);
// The click that *starts* a window: the pick under the cursor decides
// whether this selects an entity or arms a box, and it runs before the
// rubber band can appear. Timed separately from the commit, which is
// what `select-commit` covers.
let t_arm = crate::perf::enabled().then(Instant::now); let t_arm = crate::perf::enabled().then(Instant::now);
let prior_selection = self.tabs[i].scene.selected.len(); let prior_selection = self.tabs[i].scene.selected.len();
let click_candidates = self.tabs[i].scene.interaction_pick_candidates_near( let click_candidates = self.tabs[i].scene.interaction_pick_candidates_near(
+2 -24
View File
@@ -744,18 +744,6 @@ 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"))] #[cfg(not(target_arch = "wasm32"))]
pub fn load_bytes_finalized(path: &Path, bytes: Vec<u8>) -> Result<(CadDocument, usize), String> { pub fn load_bytes_finalized(path: &Path, bytes: Vec<u8>) -> Result<(CadDocument, usize), String> {
let name = path let name = path
@@ -2061,17 +2049,7 @@ pub(crate) fn is_entity_corrupt(e: &EntityType) -> bool {
// rarely use this many — and parser desync produces exactly-100_000-vertex // rarely use this many — and parser desync produces exactly-100_000-vertex
// junk records. // junk records.
const MAX_VERTS: usize = 100_000; 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 { match e {
E::Insert(i) => i.instance_count() > MAX_MINSERT_INSTANCES,
E::LwPolyline(p) => { E::LwPolyline(p) => {
!finite_unit_normal(&p.normal) !finite_unit_normal(&p.normal)
|| p.vertices.len() >= MAX_VERTS || p.vertices.len() >= MAX_VERTS
@@ -2451,11 +2429,11 @@ mod corrupt_guard_tests {
// product can reach into the billions) must be rejected before it can // product can reach into the billions) must be rejected before it can
// drive the render graph's per-instance allocation and expansion. // drive the render graph's per-instance allocation and expansion.
#[test] #[test]
fn rejects_pathological_minsert_counts() { fn preserves_large_minsert_data_while_rendering_is_bounded() {
let mut i = acadrust::entities::Insert::new("BLOCK", Vector3::ZERO); let mut i = acadrust::entities::Insert::new("BLOCK", Vector3::ZERO);
i.row_count = u16::MAX; i.row_count = u16::MAX;
i.column_count = u16::MAX; i.column_count = u16::MAX;
assert!(is_entity_corrupt(&EntityType::Insert(i))); assert!(!is_entity_corrupt(&EntityType::Insert(i)));
} }
// An ordinary array insert, well under the budget, is valid source data. // An ordinary array insert, well under the budget, is valid source data.
-4
View File
@@ -298,10 +298,6 @@ impl Scene {
}; };
if !handle.is_null() { if !handle.is_null() {
// New geometry drawn during REFEDIT belongs to the edited block,
// so it must stay bright along with the copied block entities.
// The remaining model-space entities are only visible as faded
// context.
if let Some(keep) = self.refedit_keep.as_mut() { if let Some(keep) = self.refedit_keep.as_mut() {
keep.insert(handle); keep.insert(handle);
} }
-12
View File
@@ -183,14 +183,6 @@ impl Scene {
handles: &[Handle], handles: &[Handle],
) -> HashSet<Handle> { ) -> HashSet<Handle> {
let mut expanded: HashSet<Handle> = handles.iter().copied().collect(); let mut expanded: HashSet<Handle> = handles.iter().copied().collect();
// Asked the other way round, this was quadratic: for every group, scan
// every selected handle, and `Group::contains` is a linear scan of the
// group's own entity list. A box selection of 185 096 entities spent
// seconds here.
//
// The membership set is the *input* handles, not `expanded` — a group
// must match because something the user selected is in it, not because
// an earlier group already contributed one of its members.
let wanted: HashSet<Handle> = handles.iter().copied().collect(); let wanted: HashSet<Handle> = handles.iter().copied().collect();
for obj in self.document.objects.values() { for obj in self.document.objects.values() {
if let ObjectType::Group(g) = obj { if let ObjectType::Group(g) = obj {
@@ -218,10 +210,6 @@ mod group_expansion_tests {
use super::*; use super::*;
use crate::scene::Scene; use crate::scene::Scene;
/// Selecting one member of a selectable group selects the whole group, and
/// a group nobody touched stays out. The lookup was inverted for speed —
/// it asked every group about every selected handle, and `Group::contains`
/// scans the group's own list — so the answer has to be shown unchanged.
#[test] #[test]
fn group_expansion_pulls_in_siblings_and_nothing_else() { fn group_expansion_pulls_in_siblings_and_nothing_else() {
use acadrust::entities::{EntityType, Line}; use acadrust::entities::{EntityType, Line};
-12
View File
@@ -1877,12 +1877,6 @@ pub struct Scene {
entity_block_map_cache: RefCell<Option<(u64, HashMap<Handle, Handle>)>>, entity_block_map_cache: RefCell<Option<(u64, HashMap<Handle, Handle>)>>,
/// Candidate handles per block, in document order and keyed by geometry epoch. /// Candidate handles per block, in document order and keyed by geometry epoch.
block_members_cache: RefCell<Option<BlockMembers>>, block_members_cache: RefCell<Option<BlockMembers>>,
/// Annotation handle -> the LEADERs that point at it, resolved once per
/// `geometry_epoch`.
///
/// Selecting an entity has to pull in a LEADER whose annotation it is, and
/// that used to be answered by walking the whole document — per selected
/// handle. Selecting N entities cost N document walks.
leaders_by_annotation_cache: RefCell<Option<(u64, HashMap<Handle, Vec<Handle>>)>>, leaders_by_annotation_cache: RefCell<Option<(u64, HashMap<Handle, Vec<Handle>>)>>,
/// Insert/Viewport/Block/BlockEnd handles omitted by the spatial index. /// Insert/Viewport/Block/BlockEnd handles omitted by the spatial index.
unindexable_cache: RefCell<Option<(u64, Vec<Handle>)>>, unindexable_cache: RefCell<Option<(u64, Vec<Handle>)>>,
@@ -7865,12 +7859,6 @@ impl Scene {
(Arc::new(wires), base_slots, changed_slots) (Arc::new(wires), base_slots, changed_slots)
} }
/// `area_only` skips the snap-only categories and gathers the rest without
/// sorting; see [`InteractionIndex::query_xy_area`]. Both the overlay path
/// and the plain indexed path honour it — the overlay runs whenever the
/// drawing has been edited since the index was built, which is most of a
/// working session, so a saving that stopped there would stop the first
/// time the user drew something.
fn indexed_interaction_candidates_xy( fn indexed_interaction_candidates_xy(
&self, &self,
wires: Arc<Vec<WireModel>>, wires: Arc<Vec<WireModel>>,
-41
View File
@@ -1271,28 +1271,6 @@ fn indexed_box_crossing_hits<'a, W: WireSource + ?Sized>(
segment_hits(a, b).then_some(wire.name.as_str()) segment_hits(a, b).then_some(wire.name.as_str())
}; };
// A crossing selection over a whole drawing spends its time here, not in
// the window branch of `box_hit`: every measured `select-commit` line read
// `crossing=true`.
//
// The scan stops projecting a wire once that wire is in. A circle or a
// polyline contributes one segment per tessellated span, and every span
// after the first that hits can only re-add a name already present — at 24
// spans per wire that is 23 projection pairs out of 24 skipped, which took
// a 4.4 M-segment scan from ~1 s to 42 ms. The wire is keyed by index, an
// integer to hash rather than a string, and the name set still runs behind
// it because one entity can produce several wires sharing a name.
//
// Fanning this out across cores was tried and removed: with the early-out
// the remaining work is small enough that per-chunk sets, which cannot see
// wires already hit in another chunk, measured *slower* than one pass
// (45.6 ms against 42.4 ms at 186 468 wires).
// A bitmap over wire indices rather than a hash set: this is consulted once
// per segment, triangle and glyph inside the box — millions of times on a
// whole-drawing selection — and an array index beats hashing a `u32`, let
// alone hashing the wire's name, which is what the primitive loops below
// used to do. `seen` stays for the names, because several wires can share
// one entity's name and the output dedups by name.
let mut wire_hit: Vec<bool> = Vec::new(); let mut wire_hit: Vec<bool> = Vec::new();
fn mark(wire_hit: &mut Vec<bool>, wire: u32) { fn mark(wire_hit: &mut Vec<bool>, wire: u32) {
let index = wire as usize; let index = wire as usize;
@@ -1377,10 +1355,6 @@ fn indexed_box_crossing_hits<'a, W: WireSource + ?Sized>(
// Degenerate point-only wires have no indexed segment or surface primitive. // Degenerate point-only wires have no indexed segment or surface primitive.
for wire in wires.iter() { for wire in wires.iter() {
// Structural tests first: `||` short-circuits, so putting the name
// lookup last means a wire with two or more points — very nearly all
// of them — is dropped without hashing its name. This loop walks every
// candidate, so that was one string hash per wire in the box.
if wire.points.len() >= 2 if wire.points.len() >= 2
|| !wire.fill_tris.is_empty() || !wire.fill_tris.is_empty()
|| !wire.pick_tris.is_empty() || !wire.pick_tris.is_empty()
@@ -1591,10 +1565,6 @@ fn indexed_polygon_crossing_hits<'a, W: WireSource + ?Sized>(
} }
} }
for wire in wires.iter() { for wire in wires.iter() {
// Structural tests first: `||` short-circuits, so putting the name
// lookup last means a wire with two or more points — very nearly all
// of them — is dropped without hashing its name. This loop walks every
// candidate, so that was one string hash per wire in the box.
if wire.points.len() >= 2 if wire.points.len() >= 2
|| !wire.fill_tris.is_empty() || !wire.fill_tris.is_empty()
|| !wire.pick_tris.is_empty() || !wire.pick_tris.is_empty()
@@ -2773,13 +2743,6 @@ mod parallel_selection_tests {
use crate::scene::pick::interaction_index::{InteractionCandidates, InteractionIndex}; use crate::scene::pick::interaction_index::{InteractionCandidates, InteractionIndex};
use std::sync::Arc; use std::sync::Arc;
// Enough wires to clear the 4096 default several times over, laid on a
// diagonal so the selection box catches a known prefix of them rather than
// all or nothing — an "everything hits" case would not notice a reordering.
// A drawing is not made of single segments: a circle or an arc tessellates
// into dozens of spans, and a polyline carries as many as it has vertices.
// A one-segment-per-wire sample makes the per-wire early-out invisible and
// the whole measurement optimistic, so the benchmark uses `spans` per wire.
fn many_wires_with_spans(count: usize, spans: usize) -> Vec<WireModel> { fn many_wires_with_spans(count: usize, spans: usize) -> Vec<WireModel> {
(0..count) (0..count)
.map(|i| { .map(|i| {
@@ -2804,10 +2767,6 @@ mod parallel_selection_tests {
.collect() .collect()
} }
// Not a correctness test — a measurement, at the size the user's drawing
// actually is. Run it deliberately:
//
// cargo test --release --lib selection_scaling -- --ignored --nocapture
#[test] #[test]
#[ignore = "measurement, not a check"] #[ignore = "measurement, not a check"]
fn selection_scaling_numbers() { fn selection_scaling_numbers() {
-71
View File
@@ -458,20 +458,6 @@ impl SpatialGrid {
} }
} }
/// The same set as [`SpatialGrid::query`], in cell order and with
/// duplicates left in.
///
/// `query` sorts twice over the result — once to dedup the entry indices an
/// entry gets for spanning several cells, once to dedup the values — which
/// over a box covering the drawing is two O(n log n) passes across 4.4 M
/// elements to remove a handful of repeats. Everything an area selection
/// does with the result is order-independent and dedups anyway: the
/// crossing scan keys wires by index and names by string, and the box test
/// on a segment does not care when it runs.
///
/// Click picking keeps [`SpatialGrid::query`]: it resolves ties between
/// overlapping geometry by scan order, so its order is part of which entity
/// a click selects.
fn query_unordered<T: Copy>(&self, entries: &[Entry3<T>], query: [f64; 4]) -> Vec<T> { fn query_unordered<T: Copy>(&self, entries: &[Entry3<T>], query: [f64; 4]) -> Vec<T> {
let mut indices = Vec::new(); let mut indices = Vec::new();
self.oversized.query(entries, query, &mut indices); self.oversized.query(entries, query, &mut indices);
@@ -1211,13 +1197,6 @@ impl InteractionIndex {
.collect() .collect()
} }
/// [`InteractionIndex::query_remapped_xy`] for an area selection.
///
/// The overlay path runs whenever the drawing has been edited since the
/// interaction index was built — that is, for most of a working session —
/// so leaving it on the full query would have meant the area selection got
/// its saving only until the user drew something. Same five categories,
/// same unordered gather.
pub(crate) fn query_remapped_xy_area( pub(crate) fn query_remapped_xy_area(
&self, &self,
wires: Arc<Vec<WireModel>>, wires: Arc<Vec<WireModel>>,
@@ -1397,34 +1376,11 @@ impl InteractionIndex {
} }
} }
/// Candidates for an area selection — box, crossing or fence.
///
/// `query_xy` runs eight spatial queries because hover and snapping need
/// them all. An area selection needs five: the wires and their segments,
/// and — because `indexed_box_crossing_hits` reads them to catch hatches
/// and text inside the box — the fill triangles, pick triangles and
/// glyphs. Only the three that exist for snapping (`snap_points`,
/// `key_vertices`, `key_segments`, read solely by `pick::snap::snap`) go
/// unread, so only those are skipped.
///
/// Over a box covering the whole drawing each query gathers nearly
/// everything, which is why this is worth splitting: gathering candidates
/// was 407 ms of an 825 ms selection of 186 468 entities.
///
/// The skipped categories stay `None`. That is safe *only* for consumers
/// that never read them — `snap` would silently stop snapping rather than
/// fail, since the accessors treat `None` as "no index". Route anything
/// that snaps through `query_xy`.
pub fn query_xy_area( pub fn query_xy_area(
&self, &self,
wires: Arc<Vec<WireModel>>, wires: Arc<Vec<WireModel>>,
aabb: [f64; 4], aabb: [f64; 4],
) -> InteractionCandidates { ) -> InteractionCandidates {
// The five are independent tree walks over the same box, and over a box
// covering the drawing each one gathers nearly everything — 259 ms
// together. Running them concurrently costs the largest instead of the
// sum. Each closure still calls the same query, so the results are
// identical to the sequential ones, order included.
let ((wire_indices, segments), ((fill_triangles, pick_triangles), glyphs)) = let ((wire_indices, segments), ((fill_triangles, pick_triangles), glyphs)) =
crate::par::join( crate::par::join(
|| { || {
@@ -1703,16 +1659,6 @@ impl InteractionCandidates {
.map(|(rect, (view, eye, bounds))| (rect, view, eye, bounds)) .map(|(rect, (view, eye, bounds))| (rect, view, eye, bounds))
} }
/// [`InteractionCandidates::extend_indexed`] for an area selection.
///
/// `extend_indexed` sorts and dedups every category it merges, which over a
/// box covering the drawing would put back exactly the sorts the unordered
/// query exists to avoid.
///
/// `wire_indices` keeps its dedup, and must: the window branch of `box_hit`
/// walks it directly, so a wire listed twice would be selected twice. The
/// other categories are read by scans that dedup by wire index and by name
/// already, so a repeat there costs one extra test and changes nothing.
pub(crate) fn extend_indexed_area(&mut self, other: Self) { pub(crate) fn extend_indexed_area(&mut self, other: Self) {
debug_assert!(Arc::ptr_eq(&self.wires, &other.wires)); debug_assert!(Arc::ptr_eq(&self.wires, &other.wires));
@@ -2081,11 +2027,6 @@ mod area_query_tests {
] ]
} }
// The area query drops the three snap-only categories and must leave every
// category an area selection reads untouched. `unwrap_or_default()` in
// `indexed_box_crossing_hits` would turn a wrongly-dropped category into a
// silent miss — a hatch or a text that stops being selectable — rather than
// a failure, so the equivalence is asserted rather than assumed.
#[test] #[test]
fn area_query_matches_full_query_where_it_is_read() { fn area_query_matches_full_query_where_it_is_read() {
let wires = sample(); let wires = sample();
@@ -2096,13 +2037,6 @@ mod area_query_tests {
let full = index.query_xy(Arc::clone(&arc), aabb); let full = index.query_xy(Arc::clone(&arc), aabb);
let area = index.query_xy_area(Arc::clone(&arc), aabb); let area = index.query_xy_area(Arc::clone(&arc), aabb);
// The area query skips the sort and the dedup `query` pays for, so it
// returns the same *set* in cell order, with an entry repeated once per
// cell it spans. Every consumer on this path dedups already — the
// crossing scan by wire index and by name — so the set is the contract,
// and comparing sequences here would only pin down an order nothing
// reads. `wire_indices` is the exception: `query_xy_area` sorts and
// dedups it before returning, so it must match exactly.
fn normalise<T: Copy + Ord>(v: &Option<Vec<T>>) -> Option<Vec<T>> { fn normalise<T: Copy + Ord>(v: &Option<Vec<T>>) -> Option<Vec<T>> {
v.as_ref().map(|items| { v.as_ref().map(|items| {
let mut items = items.clone(); let mut items = items.clone();
@@ -2137,11 +2071,6 @@ mod area_query_tests {
assert!(full.snap_points.is_some(), "the full query still snaps"); assert!(full.snap_points.is_some(), "the full query still snaps");
} }
// The overlay merge drops the sort on every category but one. That one
// matters: the window branch of `box_hit` walks `wire_indices` directly, so
// a wire left in twice would be selected twice — a duplicate handle rather
// than an error, which is the kind of thing that surfaces as a wrong count
// somewhere far away.
#[test] #[test]
fn the_area_merge_dedups_wire_indices_and_nothing_else() { fn the_area_merge_dedups_wire_indices_and_nothing_else() {
let arc = Arc::new(sample()); let arc = Arc::new(sample());
+9 -11
View File
@@ -568,12 +568,15 @@ struct MeshSurfaceParams {
flags: [u32; 4], flags: [u32; 4],
} }
/// Downscale a decoded RGBA buffer so neither dimension exceeds `limit`, fn valid_rgba_size(width: u32, height: u32, pixels: &[u8]) -> bool {
/// preserving aspect ratio (scaled by whichever dimension overshoots more). width > 0 && height > 0
/// `None` only for genuinely malformed input (`rgba`'s length doesn't match && (width as usize).checked_mul(height as usize).and_then(|size| size.checked_mul(4)) == Some(pixels.len())
/// `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>)> { fn downscale_rgba_to_limit(width: u32, height: u32, rgba: &[u8], limit: u32) -> Option<(u32, u32, Vec<u8>)> {
if limit == 0 || !valid_rgba_size(width, height, rgba) {
return None;
}
let buffer = image::RgbaImage::from_raw(width, height, rgba.to_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 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_width = ((width as f64 * scale).round() as u32).clamp(1, limit);
@@ -590,14 +593,9 @@ fn upload_rgba_texture(
fallback: [u8; 4], fallback: [u8; 4],
srgb: bool, srgb: bool,
) -> wgpu::TextureView { ) -> wgpu::TextureView {
// 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 limit = device.limits().max_texture_dimension_2d.max(1);
let resized; let resized;
let image = image.filter(|image| valid_rgba_size(image.width, image.height, &image.rgba));
let (width, height, pixels): (u32, u32, &[u8]) = match image { let (width, height, pixels): (u32, u32, &[u8]) = match image {
Some(image) if image.width > limit || image.height > limit => { Some(image) if image.width > limit || image.height > limit => {
match downscale_rgba_to_limit(image.width, image.height, &image.rgba, limit) { match downscale_rgba_to_limit(image.width, image.height, &image.rgba, limit) {
-38
View File
@@ -2701,13 +2701,6 @@ impl Pipeline {
// O(highlighted), no per-wire string parse or deep geometry clone. // O(highlighted), no per-wire string parse or deep geometry clone.
let mut selected_wires: Vec<&WireModel> = Vec::new(); let mut selected_wires: Vec<&WireModel> = Vec::new();
let mut hover_wires: Vec<&WireModel> = Vec::new(); let mut hover_wires: Vec<&WireModel> = Vec::new();
// The index is built by walking the wires in order, so each handle's
// slots come out ascending, and `patch_handle_index` only shifts them
// by a constant, which keeps that. Cloning and re-sorting per handle
// was one allocation and one sort for every selected entity — 186 468
// of each on a whole-drawing selection. The assert holds the invariant
// in place: if it ever breaks, a debug build says so here rather than
// drawing the highlight in the wrong order.
for h in selected { for h in selected {
if let Some(idxs) = self.wire_handle_index.get(&h.value()) { if let Some(idxs) = self.wire_handle_index.get(&h.value()) {
debug_assert!( debug_assert!(
@@ -2747,25 +2740,6 @@ impl Pipeline {
} }
} }
let t_gather = perf_started.map(|t| t.elapsed().as_secs_f64() * 1000.0); let t_gather = perf_started.map(|t| t.elapsed().as_secs_f64() * 1000.0);
// One extraction pass per wire, where there used to be four.
//
// The old shape asked each extractor twice: once at the draw depth to
// build the instances, then once more at depth 0.0 purely to ask
// *whether* the wire is a circle or an ellipse — allocating a vector
// each time and dropping it. Neither extractor's `Some`/`None` answer
// depends on the depth: every `return None` in them turns on the
// geometry (which `TangentGeom` variant, the radius, the axis lengths,
// the parameters), and the depth is only written into the instance,
// never read to decide. So the extraction that builds the instances
// already answers the classification, and the second pair is pure
// waste — 116 ms of shapes plus 57 ms of splitting, out of a 299 ms
// highlight at 186 468 entities.
//
// A wire with a `render_instance` is a block wire, and both extractors
// reject those outright, so it skips extraction altogether.
// Selected keeps its colour unless a tint is configured; hover is
// always recoloured. Both vectors carry selected first, then hover,
// exactly as the four separate loops produced them.
let (mut selected_circles, mut selected_ellipses, selected_regular, selected_blocks) = let (mut selected_circles, mut selected_ellipses, selected_regular, selected_blocks) =
Self::classify_highlight_wires( Self::classify_highlight_wires(
&selected_wires, &selected_wires,
@@ -2857,12 +2831,6 @@ impl Pipeline {
if let Some(started) = perf_started { if let Some(started) = perf_started {
let elapsed_ms = started.elapsed().as_secs_f64() * 1000.0; let elapsed_ms = started.elapsed().as_secs_f64() * 1000.0;
if elapsed_ms >= 1.0 { if elapsed_ms >= 1.0 {
// Cumulative marks, differenced here: gathering the wires
// from the handle index, classifying them and extracting their
// analytic instances, uploading those, building the regular
// wire instances, and the block ones. A one-time cost when the
// selection changes, which puts it directly between the user's
// second click and seeing the selection.
let gather = t_gather.unwrap_or(0.0); let gather = t_gather.unwrap_or(0.0);
let classify = t_shapes.unwrap_or(0.0); let classify = t_shapes.unwrap_or(0.0);
let analytic = t_split.unwrap_or(0.0); let analytic = t_split.unwrap_or(0.0);
@@ -5725,12 +5693,6 @@ mod highlight_classification_tests {
wire wire
} }
// The classification used to be a second pair of extractions at depth 0.0,
// run purely to ask which bucket a wire belongs in. It now rides the
// extraction that builds the instances. Nothing renders in a test, so a
// wrong answer here would show up only as circles drawn wrong when
// selected — this asserts the buckets directly, against the predicate the
// old code used.
#[test] #[test]
fn each_wire_lands_in_the_bucket_the_old_predicate_chose() { fn each_wire_lands_in_the_bucket_the_old_predicate_chose() {
let wires = vec![plain("1"), circle("2"), ellipse("3"), plain("4")]; let wires = vec![plain("1"), circle("2"), ellipse("3"), plain("4")];
-5
View File
@@ -1148,11 +1148,6 @@ impl WireGpu {
let _ = const_bgl; let _ = const_bgl;
let max_packed_instances = super::gpu_budget::max_elements::<PackedWireInstance>(device); let max_packed_instances = super::gpu_budget::max_elements::<PackedWireInstance>(device);
// Same fan-out the native path above uses: emitting a wire reads that
// wire and the depth map and writes nothing shared, and a selection
// large enough to be slow is exactly where it pays. `collect` on an
// indexed parallel iterator keeps the order, which the instance indices
// depend on.
use crate::par::prelude::*; use crate::par::prelude::*;
let per: Vec<Vec<PackedWireInstance>> = wires let per: Vec<Vec<PackedWireInstance>> = wires
.par_iter() .par_iter()
-27
View File
@@ -1042,15 +1042,6 @@ pub fn document_block_uses(document: &CadDocument) -> Vec<BlockUse> {
uses 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; const MAX_MINSERT_INSTANCES: usize = 20_000;
pub fn array_offsets(insert: &Insert) -> Vec<[f64; 3]> { pub fn array_offsets(insert: &Insert) -> Vec<[f64; 3]> {
@@ -1093,16 +1084,6 @@ pub fn insert_instance_transform(
if offset == [0.0; 3] { if offset == [0.0; 3] {
transform transform
} else { } else {
// 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)) let orientation = Transform::from_matrix(Matrix4::rotation_z(insert.rotation))
.then(&Transform::from_matrix(Matrix4::from_matrix3(Matrix3::arbitrary_axis(insert.normal)))); .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])); let world_offset = orientation.apply_rotation(Vector3::new(offset[0], offset[1], offset[2]));
@@ -1201,10 +1182,6 @@ mod tests {
#[test] #[test]
fn array_offsets_clamps_pathological_minsert_counts() { 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); let mut insert = Insert::new("BLOCK", Vector3::ZERO);
insert.row_count = u16::MAX; insert.row_count = u16::MAX;
insert.column_count = u16::MAX; insert.column_count = u16::MAX;
@@ -1230,10 +1207,6 @@ mod tests {
#[test] #[test]
fn insert_instance_transform_does_not_scale_the_row_column_spacing() { 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 document = CadDocument::new();
let mut insert = Insert::new("BLOCK", Vector3::new(100.0, 0.0, 0.0)); let mut insert = Insert::new("BLOCK", Vector3::new(100.0, 0.0, 0.0));
insert.set_x_scale(2.0); insert.set_x_scale(2.0);
-28
View File
@@ -37,12 +37,6 @@ impl Scene {
}) })
} }
/// The handles plus their leader/annotation partners, in the order given.
///
/// Kept separate from the sorted, deduplicated form because a bulk
/// selection has to preserve pick order — `selected_handles_in_order`
/// reports it — and because sorting per handle is what made selecting
/// 186 000 entities one call at a time expensive.
fn expanded_with_leaders(&self, handles: &[Handle]) -> Vec<Handle> { fn expanded_with_leaders(&self, handles: &[Handle]) -> Vec<Handle> {
let leaders = self.leaders_by_annotation(); let leaders = self.leaders_by_annotation();
let mut expanded = Vec::with_capacity(handles.len()); let mut expanded = Vec::with_capacity(handles.len());
@@ -62,11 +56,6 @@ impl Scene {
expanded expanded
} }
/// Select many handles in one pass.
///
/// The single-handle path allocates, sorts and deduplicates for every
/// entity, and a box selection called it once per entity — most of the
/// remaining 786 ms of a 186 460-entity selection.
pub fn select_entities(&mut self, handles: &[Handle]) { pub fn select_entities(&mut self, handles: &[Handle]) {
if handles.is_empty() { if handles.is_empty() {
return; return;
@@ -84,10 +73,6 @@ impl Scene {
} }
} }
/// Deselect many handles in one pass.
///
/// `deselect_entity` rebuilds `selected_order` with a `retain` per handle,
/// so removing N of M selected entities walked the order list N times.
pub fn deselect_entities(&mut self, handles: &[Handle]) { pub fn deselect_entities(&mut self, handles: &[Handle]) {
if handles.is_empty() { if handles.is_empty() {
return; return;
@@ -1082,10 +1067,6 @@ impl Scene {
mod tests { mod tests {
use super::*; use super::*;
/// A box selection used to call `select_entity` once per entity. The bulk
/// path has to land on the same selection, in the same pick order —
/// `selected_handles_in_order` is read by commands such as DIM, where the
/// order the user picked things in decides what they mean.
#[test] #[test]
fn bulk_selection_matches_selecting_one_at_a_time() { fn bulk_selection_matches_selecting_one_at_a_time() {
use acadrust::entities::Line; use acadrust::entities::Line;
@@ -1141,11 +1122,6 @@ mod tests {
); );
} }
/// Selecting an entity pulls in any LEADER pointing at it, and that was
/// answered by walking the whole document once per selected handle —
/// selecting 471 583 entities in a 585 786-entity drawing did not finish
/// inside eight minutes. The reverse index has to give exactly what the
/// walk gave.
#[test] #[test]
fn the_leader_index_matches_a_document_walk() { fn the_leader_index_matches_a_document_walk() {
use acadrust::entities::Leader; use acadrust::entities::Leader;
@@ -1207,10 +1183,6 @@ mod tests {
); );
} }
/// The reason the cache carries a set as well as a list: drawing into a
/// drawing that already has that type must not rebuild either, because the
/// rebuild walks every entity the layout owns — 444 937 of them on the
/// reproducer, ~100 ms, on every single drawn point.
#[test] #[test]
fn adding_a_type_already_present_reuses_the_list() { fn adding_a_type_already_present_reuses_the_list() {
use acadrust::entities::{Circle, EntityType, Line}; use acadrust::entities::{Circle, EntityType, Line};
+44 -381
View File
@@ -1369,31 +1369,20 @@ impl Snapper {
&& (local_segments.is_some() || allow_unindexed_pairwise) && (local_segments.is_some() || allow_unindexed_pairwise)
{ {
if let Some(segments) = &local_segments { if let Some(segments) = &local_segments {
// Curved pairs (circle/arc/ellipse, and a line paired with let local_wires: Vec<_> = wires.iter().filter(|wire| wire_in_range(wire)).collect();
// one) are solved exactly up front (bug #1052 — see let mut resolved_pairs = rustc_hash::FxHashSet::default();
// `exact_curve_intersections`'s doc comment); the segment let pair_key = |a: &WireModel, b: &WireModel| {
// sweep below then skips any pair already resolved this let a = a as *const WireModel as usize;
// way, so the stale tessellated approximation never let b = b as *const WireModel as usize;
// competes with the true point as a candidate. Every (a.min(b), a.max(b))
// distinct wire pair touching the cursor-local segment set };
// is tried — cheap even for a plain line-vs-line pair, for (idx, &wire_i) in local_wires.iter().enumerate() {
// which `exact_curve_intersections` rejects immediately — for &wire_j in &local_wires[idx + 1..] {
// rather than pre-filtering to "known curved" wires, so a if let Some(points) = exact_curve_intersections(wire_i, wire_j) {
// line paired with a circle/arc/ellipse is covered too. for point in points {
let mut local_wires: Vec<u32> = segments.iter().map(|s| s.wire).collect(); try_pt(point, SnapType::Intersection);
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_curve_intersections(wire_i, wire_j) {
for pt in pts {
try_pt(pt, SnapType::Intersection);
} }
resolved_pairs.push((wi, wj)); resolved_pairs.insert(pair_key(wire_i, wire_j));
} }
} }
} }
@@ -1413,7 +1402,8 @@ impl Snapper {
if a.wire == b.wire || a.max_y() < b.min_y() || a.min_y() > b.max_y() { if a.wire == b.wire || a.max_y() < b.min_y() || a.min_y() > b.max_y() {
continue; continue;
} }
if resolved_pairs.contains(&(a.wire.min(b.wire), a.wire.max(b.wire))) { if wires.source_wire(a.wire).zip(wires.source_wire(b.wire))
.is_some_and(|(a, b)| resolved_pairs.contains(&pair_key(a, b))) {
continue; continue;
} }
if let Some(pt) = seg_intersect_3d(a.a, a.b, b.a, b.b) { if let Some(pt) = seg_intersect_3d(a.a, a.b, b.a, b.b) {
@@ -2343,164 +2333,7 @@ fn ray_segment_intersect_3d(
)) ))
} }
/// A full circle recovered from a wire's `tangent_geoms` — the entity's real // Coordinates for adapting wire geometry to the kernel intersection solver.
/// 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. Ellipses and lines
/// return `None` here — they're handled by the more general
/// `exact_curve_intersections` below, which this function's caller falls
/// through to. A polyline with a bulge segment still goes through the
/// tessellated sweep: it isn't a single recognized curve, and splitting it
/// into per-segment exact geometry is a real follow-up, not a known-and-
/// skipped case (there's no single closed form for "the rest of a compound
/// chain" the way there is for one circle, arc, ellipse, or line).
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 { .. } | TangentGeom::PlanarEllipse { .. } => 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.
///
/// Deliberately scoped to circle/arc pairs, not every curved type: a
/// line-vs-circle/arc pair and anything involving an ellipse are left to
/// the existing sweep for now — line-circle needs its own (simple, but
/// separate) closed form, and an ellipse's parametrisation is genuinely
/// orientation-sensitive in a way a circle's isn't, so it doesn't fall out
/// of this same construction for free. Not silently dropped — a natural
/// follow-up once this lands.
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)
}
/// A world-space working plane for embedding curves into
/// `cadkernel::geom2d`'s 2D representation — the general-case fix for
/// GitHub discussion #1052, covering every pairing `exact_circle_
/// intersections`'s fast path doesn't: a line paired with a circle/arc/
/// ellipse, and anything involving an ellipse (an ellipse's parametrisation
/// is orientation-sensitive in a way a circle's isn't, so it doesn't fall
/// out of that fast path's frame-independent construction for free).
struct WirePlane { struct WirePlane {
origin: DVec3, origin: DVec3,
axis_x: DVec3, axis_x: DVec3,
@@ -2529,63 +2362,6 @@ impl WirePlane {
} }
} }
/// A stable orthonormal in-plane basis for a given normal, used only to
/// seed a shared working plane when neither wire's own native frame is
/// preferred over the other's (both curved, or the "primary" role needs a
/// deterministic pick) — the specific in-plane rotation chosen doesn't
/// matter for correctness, only that both curves embed into the *same* one.
fn stable_plane_basis(normal: DVec3) -> (DVec3, DVec3) {
let seed = if normal.x.abs() <= normal.y.abs() && normal.x.abs() <= normal.z.abs() {
DVec3::X
} else if normal.y.abs() <= normal.z.abs() {
DVec3::Y
} else {
DVec3::Z
};
let axis_x = (seed - normal * normal.dot(seed)).normalize();
let axis_y = normal.cross(axis_x);
(axis_x, axis_y)
}
/// A world point on an ellipse at native parameter `t`, using the ellipse's
/// *own* normal for the minor-axis direction (not any externally-chosen
/// plane's) — the correct, orientation-independent reference for computing
/// where a boundary parameter actually sits in space, mirroring how
/// `src/entities/curve.rs::ellipse_curve` already treats this convention as
/// established (`normal` determines the minor-axis rotation sense).
fn ellipse_world_point(center: DVec3, major_axis_world: DVec3, ell_normal: DVec3, minor_radius: f64, t: f64) -> DVec3 {
let major_radius = major_axis_world.length();
let major_unit = major_axis_world / major_radius;
let minor_unit = ell_normal.cross(major_unit);
center + major_unit * (major_radius * t.cos()) + minor_unit * (minor_radius * t.sin())
}
/// The parameter, in `frame`'s own 2D ellipse embedding (`major_axis_2d`
/// already resolved there, `cadkernel`'s own "minor is major rotated 90°
/// CCW in 2D" convention supplying the rest), that a world point already
/// known to lie on the ellipse corresponds to. Inverting through the actual
/// point — rather than assuming a native parameter value carries over
/// unchanged — is what makes this correct even when the ellipse's own
/// normal and `frame`'s normal are coplanar but *anti*-parallel (a plain
/// "reuse start/end parameter" would silently pick the complementary arc in
/// that case: 2D-CCW-in-`frame` and 2D-CCW-in-the-ellipse's-own-frame would
/// disagree on which way is "forward").
fn ellipse_param_in_frame(world_point: DVec3, centre: DVec3, major_axis_2d: [f64; 2], major_radius: f64, minor_radius: f64, frame: &WirePlane) -> f64 {
let rel = frame.to_2d(world_point);
let c = frame.to_2d(centre);
let d = [rel[0] - c[0], rel[1] - c[1]];
let minor_axis_2d = [-major_axis_2d[1], major_axis_2d[0]];
let cos_t = (d[0] * major_axis_2d[0] + d[1] * major_axis_2d[1]) / major_radius;
let sin_t = (d[0] * minor_axis_2d[0] + d[1] * minor_axis_2d[1]) / minor_radius;
sin_t.atan2(cos_t)
}
/// This wire's own natural working plane — only a Circle/PlanarCircle/Arc/
/// PlanarEllipse has one; a line's two points don't define a unique plane,
/// and a compound chain (a polyline with a bulge) isn't a single curve to
/// begin with (out of scope for this pass, not because it's hard, but
/// because a *chain* needs its own per-segment bookkeeping this pass
/// doesn't add).
fn wire_plane(wire: &WireModel) -> Option<WirePlane> { fn wire_plane(wire: &WireModel) -> Option<WirePlane> {
let [geom] = wire.tangent_geoms.as_slice() else { let [geom] = wire.tangent_geoms.as_slice() else {
return None; return None;
@@ -2611,13 +2387,6 @@ fn wire_plane(wire: &WireModel) -> Option<WirePlane> {
Some(WirePlane { origin, axis_x, axis_y, normal: axis_x.cross(axis_y).normalize() }) Some(WirePlane { origin, axis_x, axis_y, normal: axis_x.cross(axis_y).normalize() })
} }
/// `wire`'s curve, expressed in `frame`'s 2D coordinates — `None` if `wire`
/// isn't a single recognized curve (see `wire_plane`'s doc comment for what
/// that excludes) or doesn't actually lie in `frame`'s plane. Correct
/// regardless of whether `wire`'s own native frame agrees in orientation
/// with `frame`: an Arc's/Ellipse's boundary parameters are re-derived from
/// their actual world points (`ellipse_param_in_frame`, the closure below
/// for Arc), never assumed to carry over unchanged.
fn curve_in_frame(wire: &WireModel, frame: &WirePlane, tol: f64) -> Option<Curve> { fn curve_in_frame(wire: &WireModel, frame: &WirePlane, tol: f64) -> Option<Curve> {
use cadkernel::geom2d::{Arc as KArc, Circle as KCircle, Ellipse as KEllipse, EllipseArc as KEllipseArc, Line as KLine}; use cadkernel::geom2d::{Arc as KArc, Circle as KCircle, Ellipse as KEllipse, EllipseArc as KEllipseArc, Line as KLine};
@@ -2645,7 +2414,7 @@ fn curve_in_frame(wire: &WireModel, frame: &WirePlane, tol: f64) -> Option<Curve
} }
TangentGeom::Circle { center, radius } => { TangentGeom::Circle { center, radius } => {
let c = DVec3::new(center[0] as f64, center[1] as f64, center[2] as f64); let c = DVec3::new(center[0] as f64, center[1] as f64, center[2] as f64);
frame.contains(c, tol).then(|| Curve::Circle(KCircle { centre: frame.to_2d(c), radius: *radius as f64 })) (DVec3::Z.cross(frame.normal).length() <= tol && frame.contains(c, tol)).then(|| Curve::Circle(KCircle { centre: frame.to_2d(c), radius: *radius as f64 }))
} }
TangentGeom::PlanarCircle { center, axis_x, axis_y, radius } => { TangentGeom::PlanarCircle { center, axis_x, axis_y, radius } => {
let c = DVec3::new(center[0], center[1], center[2]); let c = DVec3::new(center[0], center[1], center[2]);
@@ -2659,16 +2428,16 @@ fn curve_in_frame(wire: &WireModel, frame: &WirePlane, tol: f64) -> Option<Curve
if n.cross(frame.normal).length() > tol || !frame.contains(c, tol) { if n.cross(frame.normal).length() > tol || !frame.contains(c, tol) {
return None; return None;
} }
let boundary = |angle: f64| c + *radius * (angle.cos() * ax + angle.sin() * ay); let arc = KArc { centre: frame.to_2d(c), radius: *radius, start_angle: *start_angle, end_angle: *end_angle };
let (raw_start, raw_end) = (angle_in_frame(boundary(*start_angle), c), angle_in_frame(boundary(*end_angle), c)); let sweep = arc.sweep();
// The arc's own (ax, ay, n) frame and the target frame's (axis_x, axis_y, normal) if (sweep - std::f64::consts::TAU).abs() <= 1e-12 {
// agree on the plane but may disagree on which way is "up": when n is anti-parallel return Some(Curve::Circle(KCircle { centre: arc.centre, radius: *radius }));
// to frame.normal, mapping into the frame's basis is a reflection, which reverses }
// the CCW sweep direction. Swap start/end so the swept range still traces the same let boundary = if n.dot(frame.normal) < 0.0 { *end_angle } else { *start_angle };
// physical arc instead of its complement. let start_angle = angle_in_frame(c + *radius * (boundary.cos() * ax + boundary.sin() * ay), c);
let (start_angle, end_angle) = if n.dot(frame.normal) < 0.0 { (raw_end, raw_start) } else { (raw_start, raw_end) }; Some(Curve::Arc(KArc { start_angle, end_angle: start_angle + sweep, ..arc }))
Some(Curve::Arc(KArc { centre: frame.to_2d(c), radius: *radius, start_angle, end_angle }))
} }
TangentGeom::PlanarEllipse { center, major_axis, normal, minor_axis_ratio, start_param, end_param } => { TangentGeom::PlanarEllipse { center, major_axis, normal, minor_axis_ratio, start_param, end_param } => {
let c = DVec3::new(center[0], center[1], center[2]); let c = DVec3::new(center[0], center[1], center[2]);
let ell_normal = DVec3::new(normal[0], normal[1], normal[2]).normalize(); let ell_normal = DVec3::new(normal[0], normal[1], normal[2]).normalize();
@@ -2687,13 +2456,11 @@ fn curve_in_frame(wire: &WireModel, frame: &WirePlane, tol: f64) -> Option<Curve
return None; return None;
} }
let major_axis_2d = [dir2d[0] / len, dir2d[1] / len]; let major_axis_2d = [dir2d[0] / len, dir2d[1] / len];
let world_at = |t: f64| ellipse_world_point(c, major, ell_normal, minor_radius, t); let (start_parameter, end_parameter) = if ell_normal.dot(frame.normal) < 0.0 {
let raw_start = ellipse_param_in_frame(world_at(*start_param), c, major_axis_2d, major_radius, minor_radius, frame); (-*end_param, -*start_param)
let raw_end = ellipse_param_in_frame(world_at(*end_param), c, major_axis_2d, major_radius, minor_radius, frame); } else {
// Same reflection issue as the Arc case above: the ellipse's own normal may be (*start_param, *end_param)
// anti-parallel to the embedding frame's normal, which mirrors the parametrization };
// and reverses the sweep direction — swap start/end to keep the same physical arc.
let (start_parameter, end_parameter) = if ell_normal.dot(frame.normal) < 0.0 { (raw_end, raw_start) } else { (raw_start, raw_end) };
Some(Curve::Ellipse(KEllipseArc { Some(Curve::Ellipse(KEllipseArc {
ellipse: KEllipse { centre: frame.to_2d(c), major_radius, minor_radius, major_axis: major_axis_2d }, ellipse: KEllipse { centre: frame.to_2d(c), major_radius, minor_radius, major_axis: major_axis_2d },
start_parameter, start_parameter,
@@ -2703,33 +2470,8 @@ fn curve_in_frame(wire: &WireModel, frame: &WirePlane, tol: f64) -> Option<Curve
} }
} }
/// Exact (or, where no closed form exists — an ellipse against another
/// curved shape — properly tolerance-converged, via `cadkernel::geom2d::
/// intersect`'s own subdivision-and-refinement fallback) intersection for
/// one wire pair, bypassing their tessellated approximation entirely.
/// Tries the fast, frame-independent circle/arc path first; the general
/// embedding below only runs for a pair that path can't answer — a line
/// paired with a circle/arc/ellipse, or anything involving an ellipse.
/// `None` when neither side is a single recognized curve (both lines: the
/// segment sweep is already exact), the two aren't coplanar, or they don't
/// actually meet — in every `None` case the caller falls back to the
/// ordinary segment sweep unchanged.
fn exact_curve_intersections(wire_a: &WireModel, wire_b: &WireModel) -> Option<Vec<DVec3>> { fn exact_curve_intersections(wire_a: &WireModel, wire_b: &WireModel) -> Option<Vec<DVec3>> {
if let Some(pts) = exact_circle_intersections(wire_a, wire_b) { let frame = wire_plane(wire_a).or_else(|| wire_plane(wire_b))?;
return Some(pts);
}
// Prefer whichever side already has a natural working plane so a
// circle/arc paired with a line needs no reprojection of its own shape
// at all; when both are curved (at least one an ellipse, since a pure
// circle/arc pair would already have returned above) or neither is,
// fall back to a plane seeded from whichever side offers one.
let frame = wire_plane(wire_a).or_else(|| wire_plane(wire_b)).or_else(|| {
let normal = curve_normal_only(wire_a).or_else(|| curve_normal_only(wire_b))?;
let origin = wp_f64_first(wire_a).or_else(|| wp_f64_first(wire_b))?;
let (axis_x, axis_y) = stable_plane_basis(normal);
Some(WirePlane { origin, axis_x, axis_y, normal })
})?;
const PLANE_TOL: f64 = 1e-7; const PLANE_TOL: f64 = 1e-7;
let curve_a = curve_in_frame(wire_a, &frame, PLANE_TOL)?; let curve_a = curve_in_frame(wire_a, &frame, PLANE_TOL)?;
@@ -2743,20 +2485,7 @@ fn exact_curve_intersections(wire_a: &WireModel, wire_b: &WireModel) -> Option<V
let tolerance = cadkernel::geom2d::Tolerance::new(1e-9_f64.max(PLANE_TOL)); let tolerance = cadkernel::geom2d::Tolerance::new(1e-9_f64.max(PLANE_TOL));
let crossings = cadkernel::geom2d::intersect(&curve_a, &curve_b, tolerance); let crossings = cadkernel::geom2d::intersect(&curve_a, &curve_b, tolerance);
let points: Vec<DVec3> = crossings.into_iter().map(|c| frame.to_3d(c.point)).collect(); let points: Vec<DVec3> = crossings.into_iter().map(|c| frame.to_3d(c.point)).collect();
(!points.is_empty()).then_some(points) Some(points)
}
/// Fallback plane-seeding helpers for `exact_curve_intersections`' rare
/// "neither side has its own natural plane" branch (in practice: unreached
/// today, since that only happens when both sides are lines, which returns
/// `None` a step earlier — kept for robustness against a future caller that
/// invokes `exact_curve_intersections` with weaker preconditions).
fn curve_normal_only(wire: &WireModel) -> Option<DVec3> {
wire_plane(wire).map(|p| p.normal)
}
fn wp_f64_first(wire: &WireModel) -> Option<DVec3> {
(!wire.points.is_empty()).then(|| wp_f64(wire, 0))
} }
/// XY-plane segment-segment intersection. Returns `None` if parallel or outside. /// XY-plane segment-segment intersection. Returns `None` if parallel or outside.
@@ -3339,14 +3068,8 @@ mod ext_tests {
); );
} }
/// 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] #[test]
fn exact_circle_intersections_matches_the_3_4_5_report() { fn exact_curve_intersections_matches_the_3_4_5_report() {
let c1 = WireModel { let c1 = WireModel {
tangent_geoms: vec![TangentGeom::PlanarCircle { tangent_geoms: vec![TangentGeom::PlanarCircle {
center: [0.0, 0.0, 0.0], center: [0.0, 0.0, 0.0],
@@ -3365,7 +3088,7 @@ mod ext_tests {
}], }],
..Default::default() ..Default::default()
}; };
let pts = exact_circle_intersections(&c1, &c2).expect("two overlapping circles must intersect"); let pts = exact_curve_intersections(&c1, &c2).expect("two overlapping circles must intersect");
assert_eq!(pts.len(), 2, "two distinct circles crossing at two points"); 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"); 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:?}"); assert!((upper - DVec3::new(0.0, 4.0, 0.0)).length() < 1e-9, "expected exactly (0,4,0), got {upper:?}");
@@ -3376,11 +3099,7 @@ mod ext_tests {
} }
#[test] #[test]
fn exact_circle_intersections_respects_an_arcs_own_sweep() { fn exact_curve_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 { let arc = WireModel {
tangent_geoms: vec![TangentGeom::Arc { tangent_geoms: vec![TangentGeom::Arc {
center: [0.0, 0.0, 0.0], center: [0.0, 0.0, 0.0],
@@ -3403,13 +3122,13 @@ mod ext_tests {
}; };
// Full-circle math gives (0,4,0) [on the 0..90° arc] and (0,-4,0) // Full-circle math gives (0,4,0) [on the 0..90° arc] and (0,-4,0)
// [not on it]. // [not on it].
let pts = exact_circle_intersections(&arc, &circle).expect("the circles still cross"); let pts = exact_curve_intersections(&arc, &circle).expect("the circles still cross");
assert_eq!(pts.len(), 1, "only the point on the arc's own sweep"); 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); assert!((pts[0] - DVec3::new(0.0, 4.0, 0.0)).length() < 1e-9);
} }
#[test] #[test]
fn exact_circle_intersections_is_none_for_non_coplanar_circles() { fn exact_curve_intersections_is_none_for_non_coplanar_circles() {
let flat = WireModel { let flat = WireModel {
tangent_geoms: vec![TangentGeom::PlanarCircle { tangent_geoms: vec![TangentGeom::PlanarCircle {
center: [0.0, 0.0, 0.0], center: [0.0, 0.0, 0.0],
@@ -3429,13 +3148,13 @@ mod ext_tests {
..Default::default() ..Default::default()
}; };
assert!( assert!(
exact_circle_intersections(&flat, &tilted).is_none(), exact_curve_intersections(&flat, &tilted).is_none(),
"a genuinely-3D pair must fall back to the ordinary sweep, not guess a plane" "a genuinely-3D pair must fall back to the ordinary sweep, not guess a plane"
); );
} }
#[test] #[test]
fn exact_circle_intersections_is_none_when_circles_dont_meet() { fn exact_curve_intersections_resolves_disjoint_circles() {
let near = WireModel { 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 }], 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() ..Default::default()
@@ -3444,37 +3163,11 @@ mod ext_tests {
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 }], 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() ..Default::default()
}; };
assert!(exact_circle_intersections(&near, &far).is_none()); assert!(exact_curve_intersections(&near, &far).unwrap().is_empty());
} }
#[test] #[test]
fn wire_circle_is_none_for_a_plain_line_wire() { fn exact_curve_intersections_handles_two_arcs_in_differently_rotated_frames() {
// A line wire carries no `TangentGeom::Circle`/`Arc` entry, so
// `wire_circle` (and the fast `exact_circle_intersections` path built
// on it) must not panic or, worse, silently misidentify a line as a
// circle — it should just decline, leaving the line-vs-circle case to
// the more general `exact_curve_intersections`, covered separately by
// `exact_curve_intersections_handles_a_line_against_a_circle`.
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 isn't this fast path's job — it's handled by exact_curve_intersections");
}
/// 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. // Arc A: quarter circle 0..90°, axis_x along world +X.
let arc_a = WireModel { let arc_a = WireModel {
tangent_geoms: vec![TangentGeom::Arc { tangent_geoms: vec![TangentGeom::Arc {
@@ -3487,12 +3180,6 @@ mod ext_tests {
}], }],
..Default::default() ..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 { let arc_b = WireModel {
tangent_geoms: vec![TangentGeom::Arc { tangent_geoms: vec![TangentGeom::Arc {
center: [3.0, 0.0, 0.0], center: [3.0, 0.0, 0.0],
@@ -3504,14 +3191,7 @@ mod ext_tests {
}], }],
..Default::default() ..Default::default()
}; };
// True full-circle crossings are (0,4,0) [world angle 90° on B, so assert!(exact_curve_intersections(&arc_a, &arc_b).unwrap().is_empty());
// 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 // 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 // (0,-4,0) is on both A's and B's own sweep, independently checked
@@ -3527,7 +3207,7 @@ mod ext_tests {
}], }],
..Default::default() ..Default::default()
}; };
let pts = exact_circle_intersections(&arc_a_lower, &arc_b).expect("both sweeps cover (0,-4,0)"); let pts = exact_curve_intersections(&arc_a_lower, &arc_b).expect("both sweeps cover (0,-4,0)");
assert_eq!(pts.len(), 1); assert_eq!(pts.len(), 1);
assert!((pts[0] - DVec3::new(0.0, -4.0, 0.0)).length() < 1e-9, "got {:?}", pts[0]); assert!((pts[0] - DVec3::new(0.0, -4.0, 0.0)).length() < 1e-9, "got {:?}", pts[0]);
} }
@@ -3555,10 +3235,6 @@ mod ext_tests {
#[test] #[test]
fn exact_curve_intersections_handles_a_line_against_an_arcs_own_sweep() { fn exact_curve_intersections_handles_a_line_against_an_arcs_own_sweep() {
// Same arc as `exact_circle_intersections_respects_an_arcs_own_sweep`
// (0..90° of a radius-4 circle at the origin), crossed by a vertical
// line at x=0: the full circle would cross it at (0,4,0) and
// (0,-4,0), but only the first is on the arc's own sweep.
let arc = WireModel { let arc = WireModel {
tangent_geoms: vec![TangentGeom::Arc { tangent_geoms: vec![TangentGeom::Arc {
center: [0.0, 0.0, 0.0], center: [0.0, 0.0, 0.0],
@@ -3629,13 +3305,6 @@ mod ext_tests {
} }
} }
/// The critical case for `ellipse_param_in_frame`: two elliptical arcs
/// (here degenerate to circles via `minor_axis_ratio: 1.0`, so the
/// *expected* crossing angles are plain circle trigonometry, independently
/// computed) sharing the SAME numeric start/end parameter range but with
/// ANTI-PARALLEL normals. Reusing a native parameter value unchanged
/// across a normal flip would silently pick the mirror-image point
/// instead — this proves it doesn't.
#[test] #[test]
fn exact_curve_intersections_ellipse_arc_respects_normal_direction() { fn exact_curve_intersections_ellipse_arc_respects_normal_direction() {
// Full ellipse x^2/25 + y^2/9 = 1, and a radius-3 "ellipse" (ratio // Full ellipse x^2/25 + y^2/9 = 1, and a radius-3 "ellipse" (ratio
@@ -3681,12 +3350,6 @@ mod ext_tests {
assert!((minus[0] - DVec3::new(3.75, -(63.0_f64).sqrt() / 4.0, 0.0)).length() < 1e-6); assert!((minus[0] - DVec3::new(3.75, -(63.0_f64).sqrt() / 4.0, 0.0)).length() < 1e-6);
} }
/// Same reflection hazard as the ellipse test above, but for `curve_in_frame`'s
/// `TangentGeom::Arc` branch: this only runs when the *other* side of the pair
/// isn't itself a circle/arc (so `exact_circle_intersections`'s own, already
/// normal-agnostic `WireCircle` path doesn't intercept it first) — paired here
/// against the same full ellipse so the frame comes from the ellipse's plane
/// (normal = +Z) while the arc's own normal is anti-parallel to it.
#[test] #[test]
fn exact_curve_intersections_arc_respects_normal_direction_against_an_ellipse_frame() { fn exact_curve_intersections_arc_respects_normal_direction_against_an_ellipse_frame() {
let ellipse = WireModel { let ellipse = WireModel {
+2 -2
View File
@@ -761,9 +761,9 @@ pub fn view_window<'a>(
.height(sizing.height); .height(sizing.height);
container(body) container(body)
.width(540)
.height(560)
.style(container::rounded_box) .style(container::rounded_box)
.padding([16, 18]) .padding([16, 18])
.width(sizing.width)
.height(sizing.height)
.into() .into()
} }