Make a large selection scale with the drawing

Selecting a whole drawing was unusable on a 186 468-entity file. Four costs
scaled with the selection where they did not have to.

Every selected handle triggered a walk of the entire document to find LEADERs
pointing at it. On the file this came from, selecting 471 583 entities in a
585 786-entity drawing did not finish inside eight minutes. A reverse index
built once per geometry epoch answers the same question; a test asserts it
returns exactly what the walk returned, including the case of two leaders on
one annotation.

Group expansion asked the selection about every group: for each selected
handle it scanned each group's own member list. Asking each group once
whether it holds anything selected turns that around.

Selecting and deselecting went one entity at a time, and `deselect_entity`
rebuilt the ordered list with a `retain` per handle, so removing N of M
walked the order list N times. Both now take a slice.

Grips were built for every selected entity. AutoCAD stops at GRIPOBJLIMIT,
and the DWG header does not carry the variable, so its default of 100 stands
in for it.

The remaining two commits are measurement: `select-commit` reports the phases
of a selection separately, which is what showed the bulk paths had been added
to the press-drag branch while a click-move-click window goes through a
different one entirely.
This commit is contained in:
Panos committed 2026-09-09 10:29:24 +03:00
1 parent 56e09cd3f5
commit d02ce08349
5 files changed
+424 -42

No files matched your search

+69
View File
@@ -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",
);
}
}
+50 -18
View File
@@ -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;
+64 -15
View File
@@ -183,21 +183,22 @@ impl Scene {
handles: &[Handle],
) -> HashSet<Handle> {
let mut expanded: HashSet<Handle> = 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<Handle> = 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::<HashSet<_>>(),
"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::<HashSet<_>>(),
);
// 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::<HashSet<_>>());
assert!(!both.contains(&lonely));
}
}
+10
View File
@@ -1877,6 +1877,13 @@ pub struct Scene {
entity_block_map_cache: RefCell<Option<(u64, HashMap<Handle, Handle>)>>,
/// Candidate handles per block, in document order and keyed by geometry epoch.
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>>)>>,
/// Insert/Viewport/Block/BlockEnd handles omitted by the spatial index.
unindexable_cache: RefCell<Option<(u64, Vec<Handle>)>>,
/// `(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;
+231 -9
View File
@@ -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<Handle, Vec<Handle>>)> {
{
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<Handle, Vec<Handle>> = 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<Handle> {
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<Handle> =
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<Handle> {
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<Handle> {
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<Handle> {
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};