fix edge-case in bvh object removal and creation logic (#367)

* fix edge-case in bvh object removal logic

* chore: add regression test for bvh add/remove

* fix bug in bvh creation from exactly two leaves

* chore: update changelog

Author: sebcrozet

CHANGELOG.md

@@ -1,3 +1,10 @@
+## Unreleased
+
+### Fixed
+
+- Fix bug in BVH tree node removal.
+- Fix bug in BVH tree building from exactly two leaves.
+
 ## 0.22.0-beta.1
 
 ### Fixed

src/partitioning/bvh/bvh_tests.rs

@@ -0,0 +1,91 @@
+use crate::bounding_volume::Aabb;
+use crate::math::{Real, Vector};
+use crate::partitioning::{Bvh, BvhBuildStrategy};
+
+fn make_test_aabb(i: usize) -> Aabb {
+    Aabb::from_half_extents(Vector::repeat(i as Real).into(), Vector::repeat(1.0))
+}
+
+#[test]
+fn bvh_build_and_removal() {
+    // Check various combination of building pattern and removal pattern.
+    // The tree validity is asserted at every step.
+    #[derive(Copy, Clone, Debug)]
+    enum BuildPattern {
+        Ploc,
+        Binned,
+        Insert,
+    }
+
+    #[derive(Copy, Clone, Debug)]
+    enum RemovalPattern {
+        InOrder,
+        RevOrder,
+        EvenOdd,
+    }
+
+    for build_pattern in [
+        BuildPattern::Ploc,
+        BuildPattern::Binned,
+        BuildPattern::Insert,
+    ] {
+        for removal_pattern in [
+            RemovalPattern::InOrder,
+            RemovalPattern::RevOrder,
+            RemovalPattern::EvenOdd,
+        ] {
+            for len in 1..=100 {
+                std::println!(
+                    "Testing build: {:?}, removal: {:?}, len: {}",
+                    build_pattern,
+                    removal_pattern,
+                    len
+                );
+                let leaves: std::vec::Vec<_> = (0..len).map(make_test_aabb).collect();
+
+                let mut bvh = match build_pattern {
+                    BuildPattern::Binned => Bvh::from_leaves(BvhBuildStrategy::Binned, &leaves),
+                    BuildPattern::Ploc => Bvh::from_leaves(BvhBuildStrategy::Ploc, &leaves),
+                    BuildPattern::Insert => {
+                        let mut bvh = Bvh::new();
+                        for i in 0..len {
+                            bvh.insert(make_test_aabb(i), i as u32);
+                            bvh.assert_well_formed();
+                        }
+                        bvh
+                    }
+                };
+
+                bvh.assert_well_formed();
+
+                match removal_pattern {
+                    RemovalPattern::InOrder => {
+                        // Remove in insertion order.
+                        for i in 0..len {
+                            bvh.remove(i as u32);
+                            bvh.assert_well_formed();
+                        }
+                    }
+                    RemovalPattern::RevOrder => {
+                        // Remove in reverse insertion order.
+                        for i in (0..len).rev() {
+                            bvh.remove(i as u32);
+                            bvh.assert_well_formed();
+                        }
+                    }
+                    RemovalPattern::EvenOdd => {
+                        // Remove even indices first, then odd.
+                        for i in (0..len).filter(|i| i % 2 == 0) {
+                            bvh.remove(i as u32);
+                            bvh.assert_well_formed();
+                        }
+                        for i in (0..len).filter(|i| i % 2 != 0) {
+                            bvh.remove(i as u32);
+                            bvh.assert_well_formed();
+                        }
+                    }
+                }
+            }
+        }
+    }
+}

src/partitioning/bvh/bvh_tree.rs

@@ -557,13 +557,16 @@ impl Bvh {
                     right: BvhNode::zeros(),
                 });
                 result.parents.push(BvhNodeIndex::default());
+                result.leaf_node_indices[0] = BvhNodeIndex::left(0);
             }
             2 => {
                 result.nodes.push(BvhNodeWide {
                     left: workspace.rebuild_leaves[0],
                     right: workspace.rebuild_leaves[1],
                 });
                 result.parents.push(BvhNodeIndex::default());
+                result.leaf_node_indices[0] = BvhNodeIndex::left(0);
+                result.leaf_node_indices[1] = BvhNodeIndex::right(0);
             }
             _ => {
                 result.nodes.reserve(capacity);
@@ -712,7 +715,7 @@ impl Bvh {
                     self.nodes[0].right = BvhNode::zeros();
                 } else {
                     // The sibling isn’t a leaf. It becomes the new root at index 0.
-                    self.nodes[0] = self.nodes[sibling.decompose().0];
+                    self.nodes[0] = self.nodes[self.nodes[sibling].children as usize];
                     // Both parent pointers need to be updated since both nodes moved to the root.
                     let new_root = &mut self.nodes[0];
                     if new_root.left.is_leaf() {

src/partitioning/bvh/mod.rs

@@ -13,3 +13,6 @@ mod bvh_traverse;
 mod bvh_traverse_bvtt;
 mod bvh_tree;
 mod bvh_validation;
+
+#[cfg(test)]
+mod bvh_tests;