diff --git a/src/app/properties.rs b/src/app/properties.rs index d203b9f51..39d6e29d6 100644 --- a/src/app/properties.rs +++ b/src/app/properties.rs @@ -2426,6 +2426,11 @@ impl OpenCADStudio { } /// 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) { let i = self.active_tab; let locked_active_grip = self.tabs[i].active_grip.as_ref().is_some_and(|grip| { @@ -2453,6 +2458,22 @@ impl OpenCADStudio { .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 { + selected + }; for (handle, entity) in selected { if self.tabs[i].scene.is_layer_locked(handle) { continue; @@ -4027,3 +4048,51 @@ mod chprop_integration_tests { assert_eq!(app.ribbon.active_lineweight, new_lw); } } + +#[cfg(test)] +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}; + use acadrust::types::Vector3; + + let select_n = |n: usize| -> usize { + let mut app = OpenCADStudio::new_for_test(); + app.automation_op(r#"{"op":"new"}"#); + let i = app.active_tab; + let handles: Vec<_> = (0..n) + .map(|k| { + let x = k as f64; + app.tabs[i].scene.add_entity(EntityType::Line( + Line::from_points( + Vector3::new(x, 0.0, 0.0), + Vector3::new(x + 1.0, 1.0, 0.0), + ), + )) + }) + .collect(); + for handle in handles { + app.tabs[i].scene.select_entity(handle, false); + } + app.refresh_selected_grips(); + app.tabs[i].selected_grips.len() + }; + + assert!( + select_n(OpenCADStudio::GRIP_OBJECT_LIMIT) > 0, + "a selection at the limit must still show its grips", + ); + assert_eq!( + select_n(OpenCADStudio::GRIP_OBJECT_LIMIT + 1), + 0, + "one past the limit must show none at all", + ); + } +} diff --git a/src/app/update/viewport.rs b/src/app/update/viewport.rs index 12c339a01..534618de0 100644 --- a/src/app/update/viewport.rs +++ b/src/app/update/viewport.rs @@ -4093,6 +4093,10 @@ impl OpenCADStudio { aabb }, ); + // A 186 k-entity box selection sits at ~800 ms in this + // handler and two guesses about which step owns it have + // both been wrong. Split it. + let t_sel = crate::perf::enabled().then(Instant::now); let area_candidates = self.tabs[i].scene.interaction_candidates_in_aabb( all_wires, world_aabb, @@ -4101,6 +4105,9 @@ impl OpenCADStudio { eye, bounds, ); + let cand_ms = t_sel + .map_or(0.0, |t| t.elapsed().as_secs_f64() * 1000.0); + let t_hit = crate::perf::enabled().then(Instant::now); let candidate_handles = self.tabs[i] .scene .interaction_candidate_handles(&area_candidates); @@ -4161,20 +4168,30 @@ impl OpenCADStudio { // selection, Shift+box removes the boxed // entities. Esc / empty-space click still clears. // PICKADD 0 (#226): a plain box REPLACES. + let hit_ms = t_hit + .map_or(0.0, |t| t.elapsed().as_secs_f64() * 1000.0); + let t_apply = crate::perf::enabled().then(Instant::now); if self.shift_down || self.select_remove_mode { - for h in &handles { - self.tabs[i].scene.deselect_entity(*h); - } + self.tabs[i].scene.deselect_entities(&handles); } else { if !selection_pick_add && !handles.is_empty() { self.tabs[i].scene.deselect_all(); } - for h in &handles { - self.tabs[i].scene.select_entity(*h, false); - } + self.tabs[i].scene.select_entities(&handles); self.tabs[i].scene.expand_selection_for_groups(&handles); } + let apply_ms = t_apply + .map_or(0.0, |t| t.elapsed().as_secs_f64() * 1000.0); + let t_props = crate::perf::enabled().then(Instant::now); self.refresh_properties(); + if crate::perf::enabled() { + crate::perf_record!( + "[perf] select-commit kind=drag-box candidates={cand_ms:.1}ms \ +hit={hit_ms:.1}ms apply={apply_ms:.1}ms properties={:.1}ms picked={}", + t_props.map_or(0.0, |t| t.elapsed().as_secs_f64() * 1000.0), + handles.len(), + ); + } selection_just_completed = true; } } else { @@ -4208,9 +4225,7 @@ impl OpenCADStudio { // leaves the current selection untouched so a stray // drag never discards hard-won picks. if self.shift_down || self.select_remove_mode { - for h in &handles { - self.tabs[i].scene.deselect_entity(*h); - } + self.tabs[i].scene.deselect_entities(&handles); } else { // PICKADD 0 (#226): a plain marquee REPLACES // the selection (empty results still leave it @@ -4218,9 +4233,7 @@ impl OpenCADStudio { if !selection_pick_add && !handles.is_empty() { self.tabs[i].scene.deselect_all(); } - for h in &handles { - self.tabs[i].scene.select_entity(*h, false); - } + self.tabs[i].scene.select_entities(&handles); self.tabs[i].scene.expand_selection_for_groups(&handles); } self.refresh_properties(); @@ -4454,6 +4467,10 @@ impl OpenCADStudio { aabb }, ); + // This is the path a click-move-click window takes, as + // opposed to a press-drag; it is the one a large selection + // actually goes through. + let t_sel = crate::perf::enabled().then(Instant::now); let area_candidates = self.tabs[i].scene.interaction_candidates_in_aabb( all_wires, world_aabb, @@ -4462,6 +4479,9 @@ impl OpenCADStudio { eye, bounds, ); + let cand_ms = + t_sel.map_or(0.0, |t| t.elapsed().as_secs_f64() * 1000.0); + let t_hit = crate::perf::enabled().then(Instant::now); let candidate_handles = self.tabs[i] .scene .interaction_candidate_handles(&area_candidates); @@ -4517,27 +4537,39 @@ impl OpenCADStudio { bounds, candidate_handles.as_ref(), )); + let hit_ms = t_hit.map_or(0.0, |t| t.elapsed().as_secs_f64() * 1000.0); + let t_filter = crate::perf::enabled().then(Instant::now); // Selection filter: keep only allowed types. handles.retain(|&h| self.tabs[i].scene.passes_selection_filter(h)); + let filter_ms = + t_filter.map_or(0.0, |t| t.elapsed().as_secs_f64() * 1000.0); + let t_apply = crate::perf::enabled().then(Instant::now); // Accumulate (issue #83): a plain box adds to the // current selection, Shift+box removes the boxed // entities. An empty box leaves the selection alone // so an accidental empty drag never discards it. // PICKADD 0 (#226): a plain box REPLACES instead. if self.shift_down || self.select_remove_mode { - for h in &handles { - self.tabs[i].scene.deselect_entity(*h); - } + self.tabs[i].scene.deselect_entities(&handles); } else { if !selection_pick_add && !handles.is_empty() { self.tabs[i].scene.deselect_all(); } - for h in &handles { - self.tabs[i].scene.select_entity(*h, false); - } + self.tabs[i].scene.select_entities(&handles); self.tabs[i].scene.expand_selection_for_groups(&handles); } + let apply_ms = + t_apply.map_or(0.0, |t| t.elapsed().as_secs_f64() * 1000.0); + let t_props = crate::perf::enabled().then(Instant::now); self.refresh_properties(); + if crate::perf::enabled() { + crate::perf_record!( + "[perf] select-commit kind=window candidates={cand_ms:.1}ms \ +hit={hit_ms:.1}ms filter={filter_ms:.1}ms apply={apply_ms:.1}ms properties={:.1}ms picked={}", + t_props.map_or(0.0, |t| t.elapsed().as_secs_f64() * 1000.0), + handles.len(), + ); + } let mut sel = self.tabs[i].scene.selection.borrow_mut(); sel.box_last = Some((a, p)); sel.box_last_crossing = crossing; diff --git a/src/scene/group_layer.rs b/src/scene/group_layer.rs index 2c3aa78e3..a70fca518 100644 --- a/src/scene/group_layer.rs +++ b/src/scene/group_layer.rs @@ -183,21 +183,22 @@ impl Scene { handles: &[Handle], ) -> HashSet { let mut expanded: HashSet = handles.iter().copied().collect(); - expanded.extend( - self.document - .objects - .values() - .filter_map(|obj| match obj { - ObjectType::Group(g) - if g.selectable - && handles.iter().any(|handle| g.contains(*handle)) => - { - Some(g.entities.clone()) - } - _ => None, - }) - .flatten(), - ); + // 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 { + if g.selectable && g.entities.iter().any(|e| wanted.contains(e)) { + expanded.extend(g.entities.iter().copied()); + } + } + } expanded } @@ -211,3 +212,51 @@ impl Scene { } } } + +#[cfg(test)] +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}; + use acadrust::types::Vector3; + + let mut scene = Scene::new(); + let mut line = |x: f64| { + scene.add_entity(EntityType::Line(Line::from_points( + Vector3::new(x, 0.0, 0.0), + Vector3::new(x + 1.0, 0.0, 0.0), + ))) + }; + let (a, b, c) = (line(0.0), line(1.0), line(2.0)); + let (d, e) = (line(3.0), line(4.0)); + let lonely = line(9.0); + scene.create_group("GRUPPO".to_string(), vec![a, b, c]); + scene.create_group("ALTRO".to_string(), vec![d, e]); + + // One member pulls in its siblings, and only its own group's. + let from_a = scene.handles_expanded_for_selectable_groups(&[a]); + assert_eq!( + from_a, + [a, b, c].into_iter().collect::>(), + "selecting a member must select that group and no other", + ); + + // An entity in no group expands to itself. + assert_eq!( + scene.handles_expanded_for_selectable_groups(&[lonely]), + [lonely].into_iter().collect::>(), + ); + + // Touching both groups pulls in both, and nothing outside them. + let both = scene.handles_expanded_for_selectable_groups(&[a, e]); + assert_eq!(both, [a, b, c, d, e].into_iter().collect::>()); + assert!(!both.contains(&lonely)); + } +} diff --git a/src/scene/mod.rs b/src/scene/mod.rs index 6aba6b27b..04b0f7bb7 100644 --- a/src/scene/mod.rs +++ b/src/scene/mod.rs @@ -1877,6 +1877,13 @@ 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)>>, /// `(epoch, layout block, present type names, list exposed to the UI)`. @@ -2128,6 +2135,7 @@ impl Scene { model_extents_cache: RefCell::new(None), entity_block_map_cache: RefCell::new(None), block_members_cache: RefCell::new(None), + leaders_by_annotation_cache: RefCell::new(None), unindexable_cache: RefCell::new(None), layout_type_names_cache: RefCell::new(None), dependency_index_cache: RefCell::new(None), @@ -10797,6 +10805,8 @@ mod journal_tests { assert_eq!(folded.len(), before.len() + 1); } + // Differential oracle: the incrementally-patched entity index must always + // equal a from-scratch rebuild after any add / move / erase. #[test] fn block_member_index_matches_the_full_scan() { use acadrust::entities::Line; diff --git a/src/scene/selection.rs b/src/scene/selection.rs index d77e6d88a..9457136da 100644 --- a/src/scene/selection.rs +++ b/src/scene/selection.rs @@ -4,11 +4,110 @@ impl Scene { // ── Selection ───────────────────────────────────────────────────────── /// Treat a classic LEADER and its attached annotation as one logical object. /// Clicking/copying/deleting either side expands to the complete pair. + /// Every LEADER that points at `annotation`, resolved once per + /// `geometry_epoch` rather than by walking the document per handle. + fn leaders_by_annotation( + &self, + ) -> std::cell::Ref<'_, (u64, HashMap>)> { + { + let cache = self.leaders_by_annotation_cache.borrow(); + if cache.as_ref().is_some_and(|(epoch, _)| *epoch == self.geometry_epoch) { + drop(cache); + return std::cell::Ref::map( + self.leaders_by_annotation_cache.borrow(), + |c| c.as_ref().unwrap(), + ); + } + } + let mut by_annotation: HashMap> = HashMap::default(); + for entity in self.document.entities() { + if let EntityType::Leader(leader) = entity { + if !leader.annotation_handle.is_null() { + by_annotation + .entry(leader.annotation_handle) + .or_default() + .push(entity.common().handle); + } + } + } + *self.leaders_by_annotation_cache.borrow_mut() = + Some((self.geometry_epoch, by_annotation)); + std::cell::Ref::map(self.leaders_by_annotation_cache.borrow(), |c| { + c.as_ref().unwrap() + }) + } + + /// 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()); + for &handle in handles { + expanded.push(handle); + if let Some(EntityType::Leader(leader)) = self.document.get_entity(handle) { + if !leader.annotation_handle.is_null() { + expanded.push(leader.annotation_handle); + } + } + if let Some(pointing) = leaders.1.get(&handle) { + expanded.extend(pointing.iter().copied()); + } + } + 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; + } + let expanded = self.expanded_with_leaders(handles); + let mut changed = false; + for handle in expanded { + if self.selected.insert(handle) { + self.selected_order.push(handle); + changed = true; + } + } + if changed { + self.bump_selection_set(); + } + } + + /// 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; + } + let doomed: HashSet = + self.expanded_with_leaders(handles).into_iter().collect(); + let mut changed = false; + for handle in &doomed { + changed |= self.selected.remove(handle); + } + if changed { + self.selected_order.retain(|handle| !doomed.contains(handle)); + self.bump_selection_set(); + } + } + pub(crate) fn handles_expanded_for_leader_annotations( &self, handles: &[Handle], ) -> Vec { let mut expanded = handles.to_vec(); + let leaders = self.leaders_by_annotation(); for &handle in handles { // LEADER -> annotation. @@ -19,15 +118,9 @@ impl Scene { } // Annotation -> LEADER. - expanded.extend(self.document.entities().filter_map(|entity| match entity { - EntityType::Leader(leader) - if !leader.annotation_handle.is_null() - && leader.annotation_handle == handle => - { - Some(entity.common().handle) - } - _ => None, - })); + if let Some(pointing) = leaders.1.get(&handle) { + expanded.extend(pointing.iter().copied()); + } } expanded.sort_unstable_by_key(|handle| handle.value()); @@ -1003,6 +1096,135 @@ 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; + use acadrust::types::Vector3; + + let build = || { + let mut scene = Scene::new(); + let handles: Vec<_> = (0..40) + .map(|k| { + let x = k as f64; + scene.add_entity(EntityType::Line(Line::from_points( + Vector3::new(x, 0.0, 0.0), + Vector3::new(x + 1.0, 0.0, 0.0), + ))) + }) + .collect(); + (scene, handles) + }; + + // Picked in an order that is neither creation nor handle order. + let picked = |handles: &[Handle]| -> Vec { + let mut order: Vec<_> = handles.iter().copied().collect(); + order.reverse(); + order.retain(|h| h.value() % 3 != 0); + order + }; + + let (mut one_at_a_time, handles) = build(); + let order = picked(&handles); + for &handle in &order { + one_at_a_time.select_entity(handle, false); + } + + let (mut in_bulk, handles) = build(); + in_bulk.select_entities(&picked(&handles)); + + assert_eq!( + in_bulk.selected_handles_in_order(), + one_at_a_time.selected_handles_in_order(), + "the bulk path must select the same entities in the same order", + ); + + // And removing a subset must agree too. + let drop: Vec<_> = order.iter().copied().take(7).collect(); + for &handle in &drop { + one_at_a_time.deselect_entity(handle); + } + in_bulk.deselect_entities(&drop); + assert_eq!( + in_bulk.selected_handles_in_order(), + one_at_a_time.selected_handles_in_order(), + "the bulk removal must leave the same selection in the same order", + ); + } + + /// 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; + use acadrust::types::Vector3; + + let mut scene = Scene::new(); + let line = |x: f64| { + EntityType::Line(acadrust::entities::Line::from_points( + Vector3::new(x, 0.0, 0.0), + Vector3::new(x + 1.0, 0.0, 0.0), + )) + }; + let annotation = scene.add_entity(line(0.0)); + let unrelated = scene.add_entity(line(5.0)); + let mut leader_on = |target: Handle| { + let mut leader = Leader::default(); + leader.annotation_handle = target; + scene.add_entity(EntityType::Leader(leader)) + }; + // Two leaders share one annotation; a third points nowhere. + let first = leader_on(annotation); + let second = leader_on(annotation); + let dangling = leader_on(Handle::NULL); + + // The walk this replaced, written out so the index is compared against + // behaviour rather than against itself. + let by_walk = |handle: Handle| -> Vec { + let mut out = vec![handle]; + if let Some(EntityType::Leader(leader)) = scene.document.get_entity(handle) { + if !leader.annotation_handle.is_null() { + out.push(leader.annotation_handle); + } + } + out.extend(scene.document.entities().filter_map(|entity| match entity { + EntityType::Leader(leader) + if !leader.annotation_handle.is_null() + && leader.annotation_handle == handle => + { + Some(entity.common().handle) + } + _ => None, + })); + out.sort_unstable_by_key(Handle::value); + out.dedup(); + out + }; + + for handle in [annotation, unrelated, first, second, dangling] { + assert_eq!( + scene.handles_expanded_for_leader_annotations(&[handle]), + by_walk(handle), + "the index must answer exactly what the walk answered", + ); + } + let expanded = scene.handles_expanded_for_leader_annotations(&[annotation]); + assert!( + expanded.contains(&first) && expanded.contains(&second), + "both leaders on one annotation must come back, not just one", + ); + } + + /// 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};