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

The from_points for Box2D and Box3D does not work quite as advertised. #519

Open
kyp44 opened this issue Mar 4, 2024 · 2 comments
Open

Comments

@kyp44
Copy link
Contributor

kyp44 commented Mar 4, 2024

I get that the max of the box structs are non-inclusive, and that is all fine. However, consider this code:

use euclid::default::{Box2D, Box3D, Point2D, Point3D};

fn main() {
    let p2 = Point2D::new(1, 1);
    let b2 = Box2D::from_points(vec![p2]);
    assert!(!b2.is_empty());
    assert!(b2.contains(p2));

    let p3 = Point3D::new(1, 1, 1);
    let b3 = Box3D::from_points(vec![p3]);
    assert!(!b3.is_empty());
    assert!(b3.contains(p3));
}

All of the above assertions fail, which seems to contradiction the documentation of the from_points method, according to which it "Returns the smallest box containing all of the provided points." Basically the maximum point passed in will never actually be contained in the box.

This should be a very simple fix, and I'm happy to implement it once this has been confirmed as a genuine bug.

@nical
Copy link
Contributor

nical commented Mar 6, 2024

You are right, the documentation is misleading here. The behavior of contains purposefully excludes points on the right and bottom edges, downstream users rely on points being contained by a single box in an arrangement of side-by-side touching but non overlapping ones.

@kyp44
Copy link
Contributor Author

kyp44 commented Mar 6, 2024

No problem, this is something I can easily work around if it's the intended behavior. I'll leave it up to you whether to change the documentation to reflect the fact that not all points will be contained in the box.

Really liking the crate by the way! I am migrating my codebase to use euclid instead of cgmath, since it has some nice features (boxes being one of them!) that cgmath does not have.

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

No branches or pull requests

2 participants