Introduce the (Typed)Box2D type. #219
Conversation
|
|
||
| #[inline] | ||
| pub fn union(&self, other: &Self) -> Self { | ||
| if other.is_empty_or_negative() { |
There was a problem hiding this comment.
Do we need to special case this or can we just rely on the maxes?
There was a problem hiding this comment.
I don't think we can rely on the maxes for union (although it does work for intersection). We could turn this into a debug_assert and force people to use ensure_positive beforehand if this is a performance concern.
src/rect.rs
Outdated
| /// A 2d Rectangle represented using two points, optionally tagged with a unit. | ||
| #[repr(C)] | ||
| pub struct TypedBox2D<T, Unit = UnknownUnit> { | ||
| min: TypedPoint2D<T, Unit>, |
src/rect.rs
Outdated
| impl<T, U> TypedBox2D<T, U> { | ||
| pub fn new(min: TypedPoint2D<T, U>, max: TypedPoint2D<T, U>) -> Self { | ||
| TypedBox2D { | ||
| min: min, |
src/rect.rs
Outdated
| } | ||
|
|
||
| impl<T, U> TypedBox2D<T, U> | ||
| where T: Copy + Clone + Zero + One + PartialOrd + PartialEq + Add<T, Output=T> + Sub<T, Output=T> + Mul<Output=T> { |
There was a problem hiding this comment.
can be Add<Output=T> and Sub<Output=T>
src/rect.rs
Outdated
|
|
||
| Some(TypedRect { | ||
| origin: self.min, | ||
| size: self.size(), |
There was a problem hiding this comment.
we shouldn't compute size() twice (previously done in is_empty_or_negative)
src/rect.rs
Outdated
|
|
||
| #[inline] | ||
| pub fn intersects(&self, other: &Self) -> bool { | ||
| self.min.x < other.max.x && other.min.x < self.max.x && |
There was a problem hiding this comment.
appears to return true even when one of the boxes is zero/negative
src/rect.rs
Outdated
| let min_point = point2(max(self.min.x, other.min.x), max(self.min.y, other.min.y)); | ||
| let max_point = point2(min(self.max.x, other.max.x), min(self.max.y, other.max.y)); | ||
|
|
||
| Self::new(min_point, max_point) |
There was a problem hiding this comment.
nit: any reason you don't just do
TypedBox {
min: ..
max: ..
}
src/rect.rs
Outdated
| /// `t` is expected to be between zero and one. | ||
| #[inline] | ||
| pub fn lerp(&self, other: Self, t: T) -> Self { | ||
| Self::new( |
There was a problem hiding this comment.
needs debug_assert! (as discussed) to be sound
src/rect.rs
Outdated
| let s = Rect::new(point2(20, -15), size2(250, 200)).to_box(); | ||
|
|
||
| let pq = p.union(&q); | ||
| assert!(pq.min == point2(0, 0)); |
There was a problem hiding this comment.
let's use assert_eq whenever possible
src/rect.rs
Outdated
| let r = Rect::new(point2(-5, -5), size2(8, 8)).to_box(); | ||
|
|
||
| let pq = p.intersection(&q); | ||
| assert!(!pq.is_empty_or_negative()); |
There was a problem hiding this comment.
need a test for is_empty_or_negative() == true
| @@ -133,3 +135,10 @@ pub type Matrix4D<T> = Transform3D<T>; | |||
| #[deprecated] | |||
| pub type TypedMatrix4D<T, Src, Dst> = TypedTransform3D<T, Src, Dst>; | |||
|
|
|||
| fn min<T: PartialOrd>(x: T, y: T) -> T { | |||
There was a problem hiding this comment.
that seems rather sad that we need those functions
There was a problem hiding this comment.
Yeah, that's a more general rust std sadness topic. They were already there, I just moved them up so that Point::min/max can use that instead of depending on the Float trait.
src/rect.rs
Outdated
| /// `t` is expected to be between zero and one. | ||
| #[inline] | ||
| pub fn lerp(&self, other: Self, t: T) -> Self { | ||
| debug_assert!(!self.is_positive()); |
There was a problem hiding this comment.
shouldn't we assert that debug_assert!(self.is_positive()); instead?
src/rect.rs
Outdated
| let s = Rect::new(point2(20, -15), size2(250, 200)).to_box(); | ||
|
|
||
| let pq = p.union(&q); | ||
| assert!(pq.min == point2(0, 0)); |
|
r=me if @jrmuizel is also happy |
|
Yeah, this is fine to land. We can iterate on it as we see fit. |
|
As I think about this more I'm leaning towards wanting a separate type for NonEmpty boxes. i.e. |
|
Also, size() can overflow. I think it should return unsigned values. |
|
☔ The latest upstream changes (presumably #225) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Closing this for now, we can reopen if/when need it again. |
This representation can improve performance when dealing with lots of rectangle intersections. It's a tradeoff so this type won't replace TypedRect which should probably remain the canonical representation for rectangles while code can opt into using boxes where it makes sense.
This change is