diff --git a/src/app/automation.rs b/src/app/automation.rs index a3d4cd418..0f2a380ef 100644 --- a/src/app/automation.rs +++ b/src/app/automation.rs @@ -319,12 +319,6 @@ impl OpenCADStudio { Ok(b) => b, 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) { Ok((doc, dropped)) => { let i = self.active_tab; @@ -1404,13 +1398,6 @@ mod tests { 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(); diff --git a/src/app/properties.rs b/src/app/properties.rs index b5e13bcdc..f3b455fa8 100644 --- a/src/app/properties.rs +++ b/src/app/properties.rs @@ -113,10 +113,6 @@ impl OpenCADStudio { // unit context, the layer and linetype lists. 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); - // `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 = self.tabs[i] .scene .selected_handles_in_order() @@ -2192,10 +2188,6 @@ impl OpenCADStudio { ..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 groups = build_selection_groups(&selected); 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()) .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 .as_ref() .map(|group| { @@ -2227,11 +2214,6 @@ impl OpenCADStudio { } else { 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>)> = filtered .iter() @@ -2449,10 +2431,6 @@ handles={handles_ms:.1} panel={:.1} ribbon={ribbon_ms:.1} tail={:.1} selected={} let mut lineweight_mixed = false; 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 { 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; 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); let mut grips = 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 { Vec::new() } else { @@ -3176,11 +3139,6 @@ pub(super) fn aggregate_sections( 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 Some((handle, entity)) = entities.next() else { return vec![]; @@ -4144,11 +4102,6 @@ mod chprop_integration_tests { mod grip_limit_tests { 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] fn a_selection_past_the_limit_gets_no_grips() { use acadrust::entities::{EntityType, Line}; @@ -4210,10 +4163,6 @@ mod aggregation_tests { .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] fn shared_values_survive_the_fold_and_differing_ones_do_not() { let entities = [line("WALLS", 1), line("WALLS", 3), line("WALLS", 1)]; diff --git a/src/app/update/viewport.rs b/src/app/update/viewport.rs index f3f842a19..431093ef2 100644 --- a/src/app/update/viewport.rs +++ b/src/app/update/viewport.rs @@ -4251,10 +4251,6 @@ properties={:.1}ms picked={}", if box_anchor.is_none() { 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); - // 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 prior_selection = self.tabs[i].scene.selected.len(); let click_candidates = self.tabs[i].scene.interaction_pick_candidates_near( diff --git a/src/io/mod.rs b/src/io/mod.rs index 0a49833ac..04e8de258 100644 --- a/src/io/mod.rs +++ b/src/io/mod.rs @@ -744,18 +744,6 @@ 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 @@ -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 // 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 @@ -2451,11 +2429,11 @@ mod corrupt_guard_tests { // 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() { + fn preserves_large_minsert_data_while_rendering_is_bounded() { 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))); + assert!(!is_entity_corrupt(&EntityType::Insert(i))); } // An ordinary array insert, well under the budget, is valid source data. diff --git a/src/scene/entity.rs b/src/scene/entity.rs index af068e6ad..159598d43 100644 --- a/src/scene/entity.rs +++ b/src/scene/entity.rs @@ -298,10 +298,6 @@ impl Scene { }; 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() { keep.insert(handle); } diff --git a/src/scene/group_layer.rs b/src/scene/group_layer.rs index a70fca518..e636bde8f 100644 --- a/src/scene/group_layer.rs +++ b/src/scene/group_layer.rs @@ -183,14 +183,6 @@ impl Scene { handles: &[Handle], ) -> HashSet { let mut expanded: HashSet = 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 = handles.iter().copied().collect(); for obj in self.document.objects.values() { if let ObjectType::Group(g) = obj { @@ -218,10 +210,6 @@ mod group_expansion_tests { use super::*; 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] fn group_expansion_pulls_in_siblings_and_nothing_else() { use acadrust::entities::{EntityType, Line}; diff --git a/src/scene/mod.rs b/src/scene/mod.rs index 2aa89c864..49feddc94 100644 --- a/src/scene/mod.rs +++ b/src/scene/mod.rs @@ -1877,12 +1877,6 @@ pub struct Scene { entity_block_map_cache: RefCell)>>, /// Candidate handles per block, in document order and keyed by geometry epoch. block_members_cache: RefCell>, - /// 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>)>>, /// Insert/Viewport/Block/BlockEnd handles omitted by the spatial index. unindexable_cache: RefCell)>>, @@ -7865,12 +7859,6 @@ impl Scene { (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( &self, wires: Arc>, diff --git a/src/scene/pick/hit_test.rs b/src/scene/pick/hit_test.rs index 4e694837c..0a71ca48d 100644 --- a/src/scene/pick/hit_test.rs +++ b/src/scene/pick/hit_test.rs @@ -1271,28 +1271,6 @@ fn indexed_box_crossing_hits<'a, W: WireSource + ?Sized>( 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 = Vec::new(); fn mark(wire_hit: &mut Vec, wire: u32) { 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. 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 || !wire.fill_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() { - // 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 || !wire.fill_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 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 { (0..count) .map(|i| { @@ -2804,10 +2767,6 @@ mod parallel_selection_tests { .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] #[ignore = "measurement, not a check"] fn selection_scaling_numbers() { diff --git a/src/scene/pick/interaction_index.rs b/src/scene/pick/interaction_index.rs index e470fddd2..5a2cd30fe 100644 --- a/src/scene/pick/interaction_index.rs +++ b/src/scene/pick/interaction_index.rs @@ -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(&self, entries: &[Entry3], query: [f64; 4]) -> Vec { let mut indices = Vec::new(); self.oversized.query(entries, query, &mut indices); @@ -1211,13 +1197,6 @@ impl InteractionIndex { .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( &self, wires: Arc>, @@ -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( &self, wires: Arc>, aabb: [f64; 4], ) -> 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)) = crate::par::join( || { @@ -1703,16 +1659,6 @@ impl InteractionCandidates { .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) { 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] fn area_query_matches_full_query_where_it_is_read() { let wires = sample(); @@ -2096,13 +2037,6 @@ mod area_query_tests { let full = index.query_xy(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(v: &Option>) -> Option> { v.as_ref().map(|items| { let mut items = items.clone(); @@ -2137,11 +2071,6 @@ mod area_query_tests { 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] fn the_area_merge_dedups_wire_indices_and_nothing_else() { let arc = Arc::new(sample()); diff --git a/src/scene/pipeline/mesh_gpu.rs b/src/scene/pipeline/mesh_gpu.rs index 391cb139e..dcd586732 100644 --- a/src/scene/pipeline/mesh_gpu.rs +++ b/src/scene/pipeline/mesh_gpu.rs @@ -568,12 +568,15 @@ 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 valid_rgba_size(width: u32, height: u32, pixels: &[u8]) -> bool { + width > 0 && height > 0 + && (width as usize).checked_mul(height as usize).and_then(|size| size.checked_mul(4)) == Some(pixels.len()) +} + fn downscale_rgba_to_limit(width: u32, height: u32, rgba: &[u8], limit: u32) -> Option<(u32, u32, Vec)> { + if limit == 0 || !valid_rgba_size(width, height, rgba) { + return None; + } 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); @@ -590,14 +593,9 @@ fn upload_rgba_texture( fallback: [u8; 4], srgb: bool, ) -> 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 resized; + let image = image.filter(|image| valid_rgba_size(image.width, image.height, &image.rgba)); 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) { diff --git a/src/scene/pipeline/mod.rs b/src/scene/pipeline/mod.rs index 422c8f305..7cd070c5a 100644 --- a/src/scene/pipeline/mod.rs +++ b/src/scene/pipeline/mod.rs @@ -2701,13 +2701,6 @@ impl Pipeline { // O(highlighted), no per-wire string parse or deep geometry clone. let mut selected_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 { if let Some(idxs) = self.wire_handle_index.get(&h.value()) { debug_assert!( @@ -2747,25 +2740,6 @@ impl Pipeline { } } 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) = Self::classify_highlight_wires( &selected_wires, @@ -2857,12 +2831,6 @@ impl Pipeline { if let Some(started) = perf_started { let elapsed_ms = started.elapsed().as_secs_f64() * 1000.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 classify = t_shapes.unwrap_or(0.0); let analytic = t_split.unwrap_or(0.0); @@ -5725,12 +5693,6 @@ mod highlight_classification_tests { 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] fn each_wire_lands_in_the_bucket_the_old_predicate_chose() { let wires = vec![plain("1"), circle("2"), ellipse("3"), plain("4")]; diff --git a/src/scene/pipeline/wire_gpu.rs b/src/scene/pipeline/wire_gpu.rs index 80761302d..e12578375 100644 --- a/src/scene/pipeline/wire_gpu.rs +++ b/src/scene/pipeline/wire_gpu.rs @@ -1148,11 +1148,6 @@ impl WireGpu { let _ = const_bgl; let max_packed_instances = super::gpu_budget::max_elements::(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::*; let per: Vec> = wires .par_iter() diff --git a/src/scene/render_graph.rs b/src/scene/render_graph.rs index 5ecbc67d0..0f8c7abb5 100644 --- a/src/scene/render_graph.rs +++ b/src/scene/render_graph.rs @@ -1042,15 +1042,6 @@ 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]> { @@ -1093,16 +1084,6 @@ pub fn insert_instance_transform( if offset == [0.0; 3] { transform } 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)) .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])); @@ -1201,10 +1182,6 @@ mod tests { #[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; @@ -1230,10 +1207,6 @@ mod tests { #[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); diff --git a/src/scene/selection.rs b/src/scene/selection.rs index 792eb2b6c..ced148433 100644 --- a/src/scene/selection.rs +++ b/src/scene/selection.rs @@ -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 { let leaders = self.leaders_by_annotation(); let mut expanded = Vec::with_capacity(handles.len()); @@ -62,11 +56,6 @@ impl Scene { 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]) { if handles.is_empty() { 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]) { if handles.is_empty() { return; @@ -1082,10 +1067,6 @@ impl Scene { mod tests { 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] fn bulk_selection_matches_selecting_one_at_a_time() { 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] fn the_leader_index_matches_a_document_walk() { 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] fn adding_a_type_already_present_reuses_the_list() { use acadrust::entities::{Circle, EntityType, Line}; diff --git a/src/snap.rs b/src/snap.rs index 9d0f9db90..d59c9bbc6 100644 --- a/src/snap.rs +++ b/src/snap.rs @@ -1369,31 +1369,20 @@ impl Snapper { && (local_segments.is_some() || allow_unindexed_pairwise) { if let Some(segments) = &local_segments { - // Curved pairs (circle/arc/ellipse, and a line paired with - // one) are solved exactly up front (bug #1052 — see - // `exact_curve_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. Every - // distinct wire pair touching the cursor-local segment set - // is tried — cheap even for a plain line-vs-line pair, - // which `exact_curve_intersections` rejects immediately — - // rather than pre-filtering to "known curved" wires, so a - // line paired with a circle/arc/ellipse is covered too. - 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_curve_intersections(wire_i, wire_j) { - for pt in pts { - try_pt(pt, SnapType::Intersection); + let local_wires: Vec<_> = wires.iter().filter(|wire| wire_in_range(wire)).collect(); + let mut resolved_pairs = rustc_hash::FxHashSet::default(); + let pair_key = |a: &WireModel, b: &WireModel| { + let a = a as *const WireModel as usize; + let b = b as *const WireModel as usize; + (a.min(b), a.max(b)) + }; + for (idx, &wire_i) in local_wires.iter().enumerate() { + for &wire_j in &local_wires[idx + 1..] { + if let Some(points) = exact_curve_intersections(wire_i, wire_j) { + for point in points { + try_pt(point, 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() { 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; } 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 -/// 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 { - 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> { - 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) -} - -/// 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). +// Coordinates for adapting wire geometry to the kernel intersection solver. struct WirePlane { origin: 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 { let [geom] = wire.tangent_geoms.as_slice() else { return None; @@ -2611,13 +2387,6 @@ fn wire_plane(wire: &WireModel) -> Option { 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 { 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 { 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 } => { 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 tol || !frame.contains(c, tol) { return None; } - let boundary = |angle: f64| c + *radius * (angle.cos() * ax + angle.sin() * ay); - let (raw_start, raw_end) = (angle_in_frame(boundary(*start_angle), c), angle_in_frame(boundary(*end_angle), c)); - // The arc's own (ax, ay, n) frame and the target frame's (axis_x, axis_y, normal) - // agree on the plane but may disagree on which way is "up": when n is anti-parallel - // 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 - // physical arc instead of its complement. - 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 { centre: frame.to_2d(c), radius: *radius, start_angle, end_angle })) + let arc = KArc { centre: frame.to_2d(c), radius: *radius, start_angle: *start_angle, end_angle: *end_angle }; + let sweep = arc.sweep(); + if (sweep - std::f64::consts::TAU).abs() <= 1e-12 { + return Some(Curve::Circle(KCircle { centre: arc.centre, radius: *radius })); + } + let boundary = if n.dot(frame.normal) < 0.0 { *end_angle } else { *start_angle }; + let start_angle = angle_in_frame(c + *radius * (boundary.cos() * ax + boundary.sin() * ay), c); + Some(Curve::Arc(KArc { start_angle, end_angle: start_angle + sweep, ..arc })) } + TangentGeom::PlanarEllipse { center, major_axis, normal, minor_axis_ratio, start_param, end_param } => { let c = DVec3::new(center[0], center[1], center[2]); 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 Option Option> { - if let Some(pts) = exact_circle_intersections(wire_a, 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 }) - })?; + let frame = wire_plane(wire_a).or_else(|| wire_plane(wire_b))?; const PLANE_TOL: f64 = 1e-7; 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 = crossings.into_iter().map(|c| frame.to_3d(c.point)).collect(); - (!points.is_empty()).then_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 { - wire_plane(wire).map(|p| p.normal) -} - -fn wp_f64_first(wire: &WireModel) -> Option { - (!wire.points.is_empty()).then(|| wp_f64(wire, 0)) + Some(points) } /// 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] - fn exact_circle_intersections_matches_the_3_4_5_report() { + fn exact_curve_intersections_matches_the_3_4_5_report() { let c1 = WireModel { tangent_geoms: vec![TangentGeom::PlanarCircle { center: [0.0, 0.0, 0.0], @@ -3365,7 +3088,7 @@ mod ext_tests { }], ..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"); 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:?}"); @@ -3376,11 +3099,7 @@ mod ext_tests { } #[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. + fn exact_curve_intersections_respects_an_arcs_own_sweep() { let arc = WireModel { tangent_geoms: vec![TangentGeom::Arc { 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) // [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!((pts[0] - DVec3::new(0.0, 4.0, 0.0)).length() < 1e-9); } #[test] - fn exact_circle_intersections_is_none_for_non_coplanar_circles() { + fn exact_curve_intersections_is_none_for_non_coplanar_circles() { let flat = WireModel { tangent_geoms: vec![TangentGeom::PlanarCircle { center: [0.0, 0.0, 0.0], @@ -3429,13 +3148,13 @@ mod ext_tests { ..Default::default() }; 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" ); } #[test] - fn exact_circle_intersections_is_none_when_circles_dont_meet() { + fn exact_curve_intersections_resolves_disjoint_circles() { 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() @@ -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 }], ..Default::default() }; - assert!(exact_circle_intersections(&near, &far).is_none()); + assert!(exact_curve_intersections(&near, &far).unwrap().is_empty()); } #[test] - fn wire_circle_is_none_for_a_plain_line_wire() { - // 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() { + fn exact_curve_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 { @@ -3487,12 +3180,6 @@ mod ext_tests { }], ..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], @@ -3504,14 +3191,7 @@ mod ext_tests { }], ..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()); + assert!(exact_curve_intersections(&arc_a, &arc_b).unwrap().is_empty()); // 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 @@ -3527,7 +3207,7 @@ mod ext_tests { }], ..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!((pts[0] - DVec3::new(0.0, -4.0, 0.0)).length() < 1e-9, "got {:?}", pts[0]); } @@ -3555,10 +3235,6 @@ mod ext_tests { #[test] 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 { tangent_geoms: vec![TangentGeom::Arc { 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] fn exact_curve_intersections_ellipse_arc_respects_normal_direction() { // 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); } - /// 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] fn exact_curve_intersections_arc_respects_normal_direction_against_an_ellipse_frame() { let ellipse = WireModel { diff --git a/src/ui/window/options.rs b/src/ui/window/options.rs index cf73444c3..4d3484255 100644 --- a/src/ui/window/options.rs +++ b/src/ui/window/options.rs @@ -761,9 +761,9 @@ pub fn view_window<'a>( .height(sizing.height); container(body) - .width(540) - .height(560) .style(container::rounded_box) .padding([16, 18]) + .width(sizing.width) + .height(sizing.height) .into() }