Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
grovesNL opened this issue Jun 27, 2024 · 3 comments · Fixed by #171
Closed

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

grovesNL opened this issue Jun 27, 2024 · 3 comments · Fixed by #171

Comments

@grovesNL
Copy link
Contributor

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
Copy link
Contributor Author

grovesNL commented Jun 27, 2024

I think the problem is with recent changes to new_empty default bounds

fn new_empty<P: Point>() -> AABB<P> {

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
Copy link
Member

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

@adamreichold
Copy link
Member

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?

@grovesNL grovesNL mentioned this issue Jun 27, 2024
2 tasks
github-merge-queue bot pushed a commit that referenced this issue Jun 28, 2024
…:new_empty (#171)

- [x] I agree to follow the project's [code of
conduct](https://github.com/georust/geo/blob/master/CODE_OF_CONDUCT.md).
- [x] I added an entry to `rstar/CHANGELOG.md` if knowledge of this
change could be valuable to users.
---

This method does not just need an empty AABB, but actually requires that
the lower/upper corners use max/min values of the point coordinates.

This change therefore open-codes this to keep these invariants and the
code that relies on them close together.

A changelog entry should not be necessary as we luckily did not yet
release the regression.

Closes #170
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants