Revert back to min/max representation of empty AABB

This reverts back to the min/max representation of empty AABB
due to regressions introduced by the numerically more tame but
not invariant for merging representation.

To avoid regressing #161, it also adds a single check to avoid
computing the distance to an empty envelope (of the root node
of an empty tree). Integer coordinates are always prone to overflow
but in the empty case we are forcing this onto the caller whereas
every non-empty tree will have AABB that the caller supplied and
where we can reasonably ask them to control for overflow or
use a custom `Point` impl based on saturating arithmetic.

Author: adamreichold

rstar-benches/Cargo.toml

@@ -1,5 +1,6 @@
 [package]
 name = "rstar-benches"
+edition = "2015"
 version = "0.1.1"
 authors = ["Stefan Altmayer <[email protected]>", "The Georust Developers <[email protected]>"]
 

rstar/CHANGELOG.md

@@ -1,3 +1,8 @@
+# Unreleased
+
+## Changed
+- Reverted the change to `AABB::new_empty` while still avoiding overflow panics applying selections on empty trees ([PR](https://github.com/georust/rstar/pull/184)
+
 # 0.12.1
 
 ## Added

rstar/src/aabb.rs

@@ -106,7 +106,12 @@ where
     type Point = P;
 
     fn new_empty() -> Self {
-        new_empty()
+        let max = P::Scalar::max_value();
+        let min = P::Scalar::min_value();
+        Self {
+            lower: P::from_value(max),
+            upper: P::from_value(min),
+        }
     }
 
     fn contains_point(&self, point: &P) -> bool {
@@ -219,21 +224,33 @@ where
     }
 }
 
-fn new_empty<P: Point>() -> AABB<P> {
-    let one = P::Scalar::one();
-    let zero = P::Scalar::zero();
-    AABB {
-        lower: P::from_value(one),
-        upper: P::from_value(zero),
-    }
-}
-
 #[cfg(test)]
 mod test {
     use super::AABB;
     use crate::envelope::Envelope;
     use crate::object::PointDistance;
 
+    #[test]
+    fn empty_rect() {
+        let empty = AABB::<[f32; 2]>::new_empty();
+
+        let other = AABB::from_corners([1.0, 1.0], [1.0, 1.0]);
+        let subject = empty.merged(&other);
+        assert_eq!(other, subject);
+
+        let other = AABB::from_corners([0.0, 0.0], [0.0, 0.0]);
+        let subject = empty.merged(&other);
+        assert_eq!(other, subject);
+
+        let other = AABB::from_corners([0.5, 0.5], [0.5, 0.5]);
+        let subject = empty.merged(&other);
+        assert_eq!(other, subject);
+
+        let other = AABB::from_corners([-0.5, -0.5], [-0.5, -0.5]);
+        let subject = empty.merged(&other);
+        assert_eq!(other, subject);
+    }
+
     /// Test that min_max_dist_2 is identical to distance_2 for the equivalent
     /// min max corner of the AABB. This is necessary to prevent optimizations
     /// from inadvertently changing floating point order of operations.

rstar/src/algorithm/iterators.rs

@@ -53,11 +53,12 @@ where
     Func: SelectionFunction<T>,
 {
     pub(crate) fn new(root: &'a ParentNode<T>, func: Func) -> Self {
-        let current_nodes = if func.should_unpack_parent(&root.envelope()) {
-            root.children.iter().collect()
-        } else {
-            SmallVec::new()
-        };
+        let current_nodes =
+            if !root.children.is_empty() && func.should_unpack_parent(&root.envelope()) {
+                root.children.iter().collect()
+            } else {
+                SmallVec::new()
+            };
 
         SelectionIterator {
             func,
@@ -135,7 +136,7 @@ where
         ControlFlow::Continue(())
     }
 
-    if func.should_unpack_parent(&root.envelope()) {
+    if !root.children.is_empty() && func.should_unpack_parent(&root.envelope()) {
         inner(root, &mut Args { func, visitor })?;
     }
 
@@ -158,11 +159,12 @@ where
     Func: SelectionFunction<T>,
 {
     pub(crate) fn new(root: &'a mut ParentNode<T>, func: Func) -> Self {
-        let current_nodes = if func.should_unpack_parent(&root.envelope()) {
-            root.children.iter_mut().collect()
-        } else {
-            SmallVec::new()
-        };
+        let current_nodes =
+            if !root.children.is_empty() && func.should_unpack_parent(&root.envelope()) {
+                root.children.iter_mut().collect()
+            } else {
+                SmallVec::new()
+            };
 
         SelectionIteratorMut {
             func,
@@ -240,7 +242,7 @@ where
         ControlFlow::Continue(())
     }
 
-    if func.should_unpack_parent(&root.envelope()) {
+    if !root.children.is_empty() && func.should_unpack_parent(&root.envelope()) {
         inner(root, &mut Args { func, visitor })?;
     }
 
@@ -408,8 +410,10 @@ mod test {
 
     #[test]
     fn test_locate_within_distance_on_empty_tree() {
-        let tree: RTree<[i64; 3]> = RTree::new();
+        let tree: RTree<[f64; 3]> = RTree::new();
+        tree.locate_within_distance([0.0, 0.0, 0.0], 10.0);
 
+        let tree: RTree<[i64; 3]> = RTree::new();
         tree.locate_within_distance([0, 0, 0], 10);
     }
 }

rstar/src/node.rs

@@ -103,6 +103,7 @@ where
     }
 
     #[cfg(test)]
+    #[allow(missing_docs)]
     pub fn sanity_check<Params>(&self, check_max_size: bool) -> Option<usize>
     where
         Params: RTreeParams,

rstar/src/point.rs

@@ -14,20 +14,16 @@ use num_traits::{Bounded, Num, Signed, Zero};
 /// # Example
 /// ```
 /// # extern crate num_traits;
-/// use num_traits::{Num, One, Signed, Zero};
+/// use num_traits::{Bounded, Num, Signed};
 ///
 /// #[derive(Clone, Copy, PartialEq, PartialOrd, Debug)]
 /// struct MyFancyNumberType(f32);
 ///
-/// impl Zero for MyFancyNumberType {
+/// impl num_traits::Bounded for MyFancyNumberType {
 ///   // ... details hidden ...
-/// # fn zero() -> Self { MyFancyNumberType(Zero::zero()) }
-/// # fn is_zero(&self) -> bool { unimplemented!() }
-/// }
-///
-/// impl One for MyFancyNumberType {
-///   // ... details hidden ...
-/// # fn one() -> Self { MyFancyNumberType(One::one()) }
+/// # fn min_value() -> Self { Self(Bounded::min_value()) }
+/// #
+/// # fn max_value() -> Self { Self(Bounded::max_value()) }
 /// }
 ///
 /// impl Signed for MyFancyNumberType {
@@ -58,9 +54,13 @@ use num_traits::{Bounded, Num, Signed, Zero};
 /// rtree.insert([MyFancyNumberType(0.0), MyFancyNumberType(0.0)]);
 /// # }
 ///
-/// # impl num_traits::Bounded for MyFancyNumberType {
-/// #   fn min_value() -> Self { unimplemented!() }
-/// #   fn max_value() -> Self { unimplemented!() }
+/// # impl num_traits::Zero for MyFancyNumberType {
+/// #   fn zero() -> Self { unimplemented!() }
+/// #   fn is_zero(&self) -> bool { unimplemented!() }
+/// # }
+/// #
+/// # impl num_traits::One for MyFancyNumberType {
+/// #   fn one() -> Self { unimplemented!() }
 /// # }
 /// #
 /// # impl core::ops::Mul for MyFancyNumberType {