From 8a2820fbd160f5d93faf01a6fd3292f896d82cc9 Mon Sep 17 00:00:00 2001 From: Adam Reichold Date: Tue, 5 Nov 2024 21:46:44 +0100 Subject: [PATCH] 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 where that the caller controls and where we can reasonably ask them to control for overflow or use a custom `Point` impl based on saturating arithmetic. --- rstar-benches/Cargo.toml | 1 + rstar/src/aabb.rs | 37 +++++++++++++++++++++++--------- rstar/src/algorithm/iterators.rs | 30 +++++++++++++++----------- rstar/src/node.rs | 1 + rstar/src/point.rs | 24 ++++++++++----------- 5 files changed, 58 insertions(+), 35 deletions(-) diff --git a/rstar-benches/Cargo.toml b/rstar-benches/Cargo.toml index d3da8fc..72a2814 100644 --- a/rstar-benches/Cargo.toml +++ b/rstar-benches/Cargo.toml @@ -1,5 +1,6 @@ [package] name = "rstar-benches" +edition = "2015" version = "0.1.1" authors = ["Stefan Altmayer ", "The Georust Developers "] diff --git a/rstar/src/aabb.rs b/rstar/src/aabb.rs index 092ff51..09cde52 100644 --- a/rstar/src/aabb.rs +++ b/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() -> AABB

{ - 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. diff --git a/rstar/src/algorithm/iterators.rs b/rstar/src/algorithm/iterators.rs index 5a241f0..0000289 100644 --- a/rstar/src/algorithm/iterators.rs +++ b/rstar/src/algorithm/iterators.rs @@ -53,11 +53,12 @@ where Func: SelectionFunction, { pub(crate) fn new(root: &'a ParentNode, 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, { pub(crate) fn new(root: &'a mut ParentNode, 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); } } diff --git a/rstar/src/node.rs b/rstar/src/node.rs index 3498a26..f455752 100644 --- a/rstar/src/node.rs +++ b/rstar/src/node.rs @@ -103,6 +103,7 @@ where } #[cfg(test)] + #[allow(missing_docs)] pub fn sanity_check(&self, check_max_size: bool) -> Option where Params: RTreeParams, diff --git a/rstar/src/point.rs b/rstar/src/point.rs index b162a4f..a6839dd 100644 --- a/rstar/src/point.rs +++ b/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 {