From 416b0d4dc3bab2d9465830d7215c3a3752aa928f Mon Sep 17 00:00:00 2001 From: Hakan Seven Date: Sun, 17 May 2026 19:28:57 +0300 Subject: [PATCH] fix(quadtree): split() on internal node was orphaning the entire subtree MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Symptom: open a non-UTM drawing, most lines/polylines/hatches/etc. vanish — only ~5% of indexed entities reachable from a huge query (walking the nodes flat showed all 2975 items present, but recursing through `node.children` from root reached only 161). Root cause: when straddler items piled up at an INTERNAL node and the count exceeded LEAF_CAPACITY, `insert()` re-invoked `split()` on that node. `split()` allocates four fresh children and assigns them to `node.children = Some([...])` — overwriting the existing children pointer and orphaning the entire previous subtree (the descendant nodes still hold their items; nothing reaches them). Splitting an internal node is futile anyway: straddlers don't fit smaller children either. Fix: gate the split on `children.is_none()` so only leaves split. Internal nodes accumulate straddlers as a linear-scan list — slower per-query for big strad- dler counts, but correct. Adds a regression test (`many_straddlers_dont_orphan_subtree`) that floods an internal node with straddlers and verifies a huge query still returns every inserted handle. Bonus: `examples/dump_index_stats.rs` — standalone tool that loads a DWG/DXF and dumps per-type counts (indexed / unbounded / top-level / block-internal) plus a sample of bboxes. Useful for future bug triage; built and ran cleanly against the file that exposed this. Co-Authored-By: Claude Opus 4.7 --- examples/dump_index_stats.rs | 171 +++++++++++++++++++++++++++++++++++ src/scene/quadtree.rs | 31 ++++++- 2 files changed, 201 insertions(+), 1 deletion(-) create mode 100644 examples/dump_index_stats.rs diff --git a/examples/dump_index_stats.rs b/examples/dump_index_stats.rs new file mode 100644 index 000000000..482edfa63 --- /dev/null +++ b/examples/dump_index_stats.rs @@ -0,0 +1,171 @@ +// Diagnostic: simulate Scene::entity_index() build on a DXF/DWG file +// and report per-type stats for indexed / unbounded / unindexable +// entities, plus how many would survive a hypothetical view query. +// +// cargo run --release --example dump_index_stats -- + +use acadrust::EntityType; +use std::collections::BTreeMap; + +fn entity_kind(e: &EntityType) -> &'static str { + match e { + EntityType::Line(_) => "Line", + EntityType::LwPolyline(_) => "LwPolyline", + EntityType::Polyline2D(_) => "Polyline2D", + EntityType::Polyline3D(_) => "Polyline3D", + EntityType::Polyline(_) => "Polyline", + EntityType::Circle(_) => "Circle", + EntityType::Arc(_) => "Arc", + EntityType::Ellipse(_) => "Ellipse", + EntityType::Spline(_) => "Spline", + EntityType::Text(_) => "Text", + EntityType::MText(_) => "MText", + EntityType::Hatch(_) => "Hatch", + EntityType::Wipeout(_) => "Wipeout", + EntityType::Insert(_) => "Insert", + EntityType::Viewport(_) => "Viewport", + EntityType::Block(_) => "Block", + EntityType::BlockEnd(_) => "BlockEnd", + EntityType::Dimension(_) => "Dimension", + EntityType::Leader(_) => "Leader", + EntityType::MLine(_) => "MLine", + EntityType::Point(_) => "Point", + EntityType::Solid(_) => "Solid", + EntityType::Face3D(_) => "Face3D", + EntityType::RasterImage(_) => "RasterImage", + EntityType::Solid3D(_) => "Solid3D", + EntityType::Region(_) => "Region", + EntityType::Body(_) => "Body", + EntityType::Ole2Frame(_) => "Ole2Frame", + EntityType::Ray(_) => "Ray", + EntityType::XLine(_) => "XLine", + _ => "Other", + } +} + +fn is_unindexable(e: &EntityType) -> bool { + matches!( + e, + EntityType::Insert(_) + | EntityType::Viewport(_) + | EntityType::Block(_) + | EntityType::BlockEnd(_) + ) +} + +fn world_aabb_f64(e: &EntityType) -> Option<[f64; 4]> { + let bb = e.as_entity().bounding_box(); + let (xmin, ymin, xmax, ymax) = (bb.min.x, bb.min.y, bb.max.x, bb.max.y); + if xmin == xmax && ymin == ymax { + return None; + } + if !xmin.is_finite() || !ymin.is_finite() || !xmax.is_finite() || !ymax.is_finite() { + return None; + } + Some([xmin, ymin, xmax, ymax]) +} + +fn main() -> Result<(), Box> { + let path = std::env::args().nth(1).expect("usage: dump_index_stats "); + let mut doc = if path.to_lowercase().ends_with(".dwg") { + acadrust::io::dwg::DwgReader::from_file(&path)?.read()? + } else { + acadrust::io::dxf::DxfReader::from_file(&path)?.read()? + }; + let base_dir = std::path::Path::new(&path) + .parent() + .unwrap_or(std::path::Path::new(".")); + let _ = H7CAD::io::xref::resolve_xrefs(&mut doc, base_dir); + + let model_block = doc.header.model_space_block_handle; + println!("file: {}", path); + println!("model_space_block_handle: {:?}", model_block); + println!("total entities: {}", doc.entities().count()); + println!(); + + let mut total: BTreeMap<&str, usize> = BTreeMap::new(); + let mut indexed: BTreeMap<&str, usize> = BTreeMap::new(); + let mut unbounded: BTreeMap<&str, usize> = BTreeMap::new(); + let mut unindexable_count: BTreeMap<&str, usize> = BTreeMap::new(); + let mut top_level: BTreeMap<&str, usize> = BTreeMap::new(); + let mut block_internal: BTreeMap<&str, usize> = BTreeMap::new(); + let mut union: Option<[f64; 4]> = None; + + for e in doc.entities() { + let k = entity_kind(e); + *total.entry(k).or_default() += 1; + let owner = e.common().owner_handle; + if owner == model_block { + *top_level.entry(k).or_default() += 1; + } else { + *block_internal.entry(k).or_default() += 1; + } + if is_unindexable(e) { + *unindexable_count.entry(k).or_default() += 1; + continue; + } + match world_aabb_f64(e) { + Some(ab) => { + *indexed.entry(k).or_default() += 1; + union = Some(match union { + None => ab, + Some(u) => [ + u[0].min(ab[0]), + u[1].min(ab[1]), + u[2].max(ab[2]), + u[3].max(ab[3]), + ], + }); + } + None => *unbounded.entry(k).or_default() += 1, + } + } + + println!("──── per type ────────────────────────────────────────────────"); + println!( + "{:<14} {:>6} {:>6} {:>6} {:>6} {:>10} {:>10}", + "Type", "total", "idx", "unb", "skip", "top-lvl", "blk-int" + ); + let mut all_keys: std::collections::BTreeSet<&str> = std::collections::BTreeSet::new(); + all_keys.extend(total.keys()); + for k in &all_keys { + println!( + "{:<14} {:>6} {:>6} {:>6} {:>6} {:>10} {:>10}", + k, + total.get(k).copied().unwrap_or(0), + indexed.get(k).copied().unwrap_or(0), + unbounded.get(k).copied().unwrap_or(0), + unindexable_count.get(k).copied().unwrap_or(0), + top_level.get(k).copied().unwrap_or(0), + block_internal.get(k).copied().unwrap_or(0), + ); + } + + println!(); + println!("union of indexed AABBs: {:?}", union); + + // Sample top-level entity bboxes by type + println!(); + println!("──── sample top-level entity bboxes (first 5 per type) ────────"); + let mut shown: BTreeMap<&str, usize> = BTreeMap::new(); + for e in doc.entities() { + let k = entity_kind(e); + if e.common().owner_handle != model_block { + continue; + } + let c = shown.entry(k).or_insert(0); + if *c >= 5 { + continue; + } + *c += 1; + let bb = e.as_entity().bounding_box(); + println!( + " {:<12} h={:?} bb=({:.3},{:.3})→({:.3},{:.3})", + k, + e.common().handle, + bb.min.x, bb.min.y, bb.max.x, bb.max.y, + ); + } + + Ok(()) +} diff --git a/src/scene/quadtree.rs b/src/scene/quadtree.rs index c0be210f0..a80b26fcc 100644 --- a/src/scene/quadtree.rs +++ b/src/scene/quadtree.rs @@ -99,9 +99,14 @@ impl QuadTree { .push(Item { handle, aabb }); self.locator.insert(handle, (node_idx, item_idx)); - // Split if the leaf overflowed and we still have depth budget. + // Split only LEAF nodes. Internal nodes accumulate straddlers + // (items whose AABB crosses a child boundary); calling split on + // an internal node would overwrite its `children` pointer and + // orphan the entire existing subtree. Straddlers can't fit + // smaller children anyway, so splitting is futile here. if self.nodes[node_idx as usize].items.len() > LEAF_CAPACITY && self.nodes[node_idx as usize].depth < MAX_DEPTH + && self.nodes[node_idx as usize].children.is_none() { self.split(node_idx); } @@ -336,6 +341,30 @@ mod tests { assert_eq!(far, vec![h(1)]); } + #[test] + fn many_straddlers_dont_orphan_subtree() { + // Regression: previously, when straddlers piled up at an + // internal node and exceeded LEAF_CAPACITY, split() was called + // again — overwriting the node's `children` and orphaning every + // descendant. Symptom: thousands of items inserted, only ~5% + // reachable from root via a huge_query. + let mut t = QuadTree::new([0.0, 0.0, 100.0, 100.0]); + // First, fill one leaf so it splits. + for i in 0..40u64 { + t.insert(h(i), [1.0, 1.0, 2.0, 2.0]); + } + // Now flood the root with straddlers (AABBs that cross center). + for i in 100..200u64 { + t.insert(h(i), [10.0, 10.0, 90.0, 90.0]); + } + // After the regression-trigger inserts, every handle must still + // come back from a huge query. + let huge = [-1e9, -1e9, 1e9, 1e9]; + let hits = t.query_rect(huge); + let expected = 40 + 100; + assert_eq!(hits.len(), expected, "lost items: tree.len={} hits.len={}", t.len(), hits.len()); + } + #[test] fn straddler_stays_at_parent() { let mut t = QuadTree::new([0.0, 0.0, 100.0, 100.0]);