georust / rstar

R*-tree spatial index for the Rust ecosystem
https://docs.rs/rstar
Apache License 2.0
384 stars 67 forks source link

Initial `AABB` can cause `from_points`/others to be wrong #170

Closed grovesNL closed 1 month ago

grovesNL commented 1 month ago

The lower bounds are wrong in this example on master:

fn main() {
    let aabb = AABB::from_points(&[(3., 3., 3.), (4., 4., 4.)]);
    println!("{:?}", aabb); // AABB { lower: (1.0, 1.0, 1.0), upper: (4.0, 4.0, 4.0) }
}
grovesNL commented 1 month ago

I think the problem is with recent changes to new_empty default bounds https://github.com/georust/rstar/blob/0139255a78ada92277ce0d1025c009254ea5b298/rstar/src/aabb.rs#L222

If 1 is always lower than the values provided the components of any points provided, then it will show up here. The same thing happens if the points are always lower than the default upper bound of 0:

let aabb = AABB::from_points(&[(-0.5, -0.5)]);
println!("{:?}", aabb); // AABB { lower: (-0.5, -0.5), upper: (0.0, 0.0) }
michaelkirk commented 1 month ago

Good catch! That might be due to #162 (as of yet unreleased)

adamreichold commented 1 month ago

Sorry for introducing that regression. I do think we should fix from_points though instead of reverting as the current implementation does rely on the initial having min/max coordinates instead of just being empty.

I think the most straight-forward implementation would be something like

i.into_iter().map(Self::from_point).reduce(|mut lhs, rhs| {
  lhs.merge(&rhs);
  lhs
).unwrap_or(Self::new_empty())

or we could change the initial value of the fold to really use max/min-valued points?