Add TypedBox2D.

[nical]

This is the 2D equivalent of TypedBox3D introduced in #311.

TypedBox2D's endpoint representation is better suited for performing inclusion and intersection tests than TypedRect among other things.

TypedRect's origin + size representation might still be better for layout-ish things.

---

This change is 

[nical]

r? @kvark

[kvark]

Reviewed 5 of 5 files at r1.
Reviewable status: all files reviewed, 14 unresolved discussions (waiting on @nical)

---

src/box2d.rs, line 39 at r1 (raw file):

/// The default box 2d type with no unit.
pub type Box2D<T> = TypedBox2D<T, UnknownUnit>;

understanding that this is following existing logic in other modules, it seems unnecessary to have U = UnknownUnit default in TypedBox2D if we define a Box2D alias.

---

src/box2d.rs, line 47 at r1 (raw file):

        D: Deserializer<'de>,
    {
        let (min, max) = try!(Deserialize::deserialize(deserializer));

I'm a little bit confused by the serialization here. Why does euclid have so much custom serialization code? Why is TypedRect serialized the same way as TypedBox2D (i.e via a tuple)?

---

src/box2d.rs, line 62 at r1 (raw file):

}

impl<T: Hash, U> Hash for TypedBox2D<T, U> {

All of that noise could be removed if we just say that U has to impl those traits. It doesn't cost anything to the user (other than this being a breaking change) to add derive(Hash) to the spaces it defines.

---

src/box2d.rs, line 141 at r1 (raw file):

    /// Returns true if the two boxes intersect.
    #[inline]
    pub fn intersects(&self, other: &Self) -> bool {

huh, it just hit me that we can get x.contains_rect(&y) == true and x.intersects(&y) == false at the same time... which isn't obvious from afar

---

src/box2d.rs, line 171 at r1 (raw file):

        if intersection.is_negative() {
            return None;

nit: no need to return early if the rest of the body is so small

---

src/box2d.rs, line 199 at r1 (raw file):

    pub fn contains(&self, other: &TypedPoint2D<T, U>) -> bool {
        self.min.x <= other.x && other.x < self.max.x
            && self.min.y < other.y && other.y <= self.max.y

this looks inconsistent, shouldn't it be self.min.y <= other.y && other.y < self.max.y instead?

---

src/box2d.rs, line 212 at r1 (raw file):

    #[inline]
    pub fn contains_box(&self, other: &Self) -> bool {
        other.is_empty()

This may need some clarity around negative box handling. Currently it's written as a negative box would contain an empty box

---

src/box2d.rs, line 224 at r1 (raw file):

    #[inline]
    pub fn size(&self)-> TypedSize2D<T, U> {
        (self.max - self.min).to_size()

is it fine to be negative here?

---

src/box2d.rs, line 265 at r1 (raw file):

        };

        debug_assert!(b.size().width >= T::zero());

could this use is_empty_or_negative ? also, we assert here but not handle negative result in other cases

---

src/box2d.rs, line 326 at r1 (raw file):

            match points.next() {
                Some(second) => assign_min_max(second),
                None => return TypedBox2D::zero(),

is that really zero? is there a semantic difference between zero and a single-point box that is not zero?

---

src/box2d.rs, line 329 at r1 (raw file):

            }

            for point in points {

could also be written with a fold

---

src/box2d.rs, line 369 at r1 (raw file):

impl<T, U> TypedBox2D<T, U>
where
    T: Copy + Clone + PartialOrd + Add<T, Output = T> + Sub<T, Output = T> + Zero,

looks like there are too many constraints here

---

src/box2d.rs, line 561 at r1 (raw file):

    #[cfg_attr(feature = "unstable", must_use)]
    pub fn round_out(&self) -> Self {
        let min_x = self.min.x.floor();

why is this written differently from round_in? i.e. not using the floor() and ceil on points

---

src/box2d.rs, line 631 at r1 (raw file):

#[cfg(test)]
mod tests {

can we have any tests that must_fail due to debug assertions in the code?

[nical]

I'm a little bit confused by the serialization here. Why does euclid have so much custom serialization code? Why is TypedRect serialized the same way as TypedBox2D (i.e via a tuple)?

I merely followed what was done with rect, so I don't know the answer for sure. The git history points to this commit: c50aa4f

All of that noise could be removed if we just say that U has to impl those traits. It doesn't cost anything to the user (other than this being a breaking change) to add derive(Hash) to the spaces it defines.

I prefer to handle this in euclid at the cost of this boilerplate, although it's not a huge deal for me. That said I'd like to not make a breaking change if I can avoid it. We can always come back to this later if we feel it's worth the pain of breaking changes.

huh, it just hit me that we can get x.contains_rect(&y) == true and x.intersects(&y) == false at the same time... which isn't obvious from afar

Unless I made a mistake the semantics are the same as TypedRect's. I agree it's not obvious but I'm not sure what the original motivations were.

    pub fn contains(&self, other: &TypedPoint2D<T, U>) -> bool {
        self.min.x <= other.x && other.x < self.max.x
            && self.min.y < other.y && other.y <= self.max.y

this looks inconsistent, shouldn't it be self.min.y <= other.y && other.y < self.max.y instead?

Good catch, fixed in the next revision.

    #[inline]
    pub fn contains_box(&self, other: &Self) -> bool {
        other.is_empty()

This may need some clarity around negative box handling. Currently it's written as a negative box would contain an empty box

Good point. I think that this should use is_empty_or_negative

    #[inline]
    pub fn size(&self)-> TypedSize2D<T, U> {
        (self.max - self.min).to_size()

is it fine to be negative here?

I'd say "yes", but it's up for debate.

Negative rects are a thing mostly to be able to chain intersections with less overhead than if we had to juggle with the Option at each step. Beyond that I don't think we should make efforts to paper over weirdness of negative rects (just like we don't check that the size of a TypedRect is positive).

        };

        debug_assert!(b.size().width >= T::zero());

could this use is_empty_or_negative ? also, we assert here but not handle negative result in other cases

I removed this assertion. It was there by symmetry with TypedRect but I think that it would make sense to let the user deal with the negative case just like with intersections.

We could add try_inner_box and try_outer_box for the checked case returning an option.

            match points.next() {
                Some(second) => assign_min_max(second),
                None => return TypedBox2D::zero(),

is that really zero? is there a semantic difference between zero and a single-point box that is not zero?

Sorry, my answer is again that it's the same as TypedRect. I'm not sure what else we could do. Maybe assert that there is at least a point? In our code empty rects tend to have the same effect whatever their position is. I couldn't say for sure that all empty rects have the exact same semantics but it's close I think.

can we have any tests that must_fail due to debug assertions in the code?

I ended up removing the debug assertions you are referring to (it was only inner and outer box AFAICT).

[kvark]

Y U no reviewable?

I merely followed what was done with rect, so I don't know the answer for sure. The git history points to this commit:

Thanks, I filed #322 to investigate.
Everything else seems to be addressed 👍

[kvark]

@bors-servo r+

[bors-servo]

📌 Commit 5479baa has been approved by kvark

[bors-servo]

⌛ Testing commit 5479baa with merge e278e16...

[bors-servo]

☀️ Test successful - checks-travis
Approved by: kvark
Pushing e278e16 to master...