servo / euclid

Geometry primitives (basic linear algebra) for Rust
Other
462 stars 103 forks source link

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

Open kyp44 opened 4 months ago

kyp44 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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.