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

(breaking change) Some Rect and Box adjustments #459

Merged
merged 4 commits into from Jul 23, 2020

Conversation

@nical
Copy link
Collaborator

nical commented Jul 23, 2020

A few rect/box related adjustments:

  • Change Rect::intersection -> Option<Self> into Rect::try_intersection -> Option<NonEmpty<Self> to match Box2D's API and get the extra typeness of NonEmpty. That's main ly for consistency but I would understand if we feel like it's too annoying a change becase webrender (and perhaps servo layout?) code is riddled with intersection checks. Let me know.
  • Make Box2D::try_intersection check that the box is empty instead of just negative. That was an oversight, we'd previously get Some(intersection) with intersection being an empty rect (or a bad rect with NaNs) which I believe isn't what we want.
  • Express Box3D::try_intersection the same way as Box2D::try_intersection: That shouldn't affect the behavior but it removes a bit of redundant computation.
nical added 3 commits Jul 23, 2020
This shouldn't change the behavior but it's a bit more efficient since calculating the intersection is redundant after having computed whether the boxes intersect.
@nical
Copy link
Collaborator Author

nical commented Jul 23, 2020

Let me know if the Rect::try_intersection -> Option<NonEmpty<Self>> change is too annoying to deal with downstream. I can live with the inconsistency.

For WebRender I want to eventually replace most uses of Rect with Box2D anyway because it has less precision loss and is faster for the majority of what we do in WebRender (anything except translating boxes).

@nical nical requested review from jrmuizel, kvark and Manishearth Jul 23, 2020
src/rect.rs Outdated
@@ -215,13 +215,13 @@ where
T: Copy + PartialOrd + Add<T, Output = T> + Sub<T, Output = T>,
{
#[inline]
pub fn intersection(&self, other: &Self) -> Option<Self> {
pub fn try_intersection(&self, other: &Self) -> Option<NonEmpty<Self>> {

This comment has been minimized.

@kvark

kvark Jul 23, 2020

Member

I don't think try_ returning Option is idiomatic. Perhaps, we could call it just intersect?

@nical
Copy link
Collaborator Author

nical commented Jul 23, 2020

As discussed, changed the PR so that we have

  • fn intersection(&self, other: &Self) -> Option<NonEmpty<Self>>
  • fn intersection_unchecked(&self, other: &Self) -> Self

I don't feel like rebasing the other two commits over the last change, but if having the back-and-forth in the commit log bothers you I can squash it into one.

With this Rect::intersection keeps the same name so it won't cause as much breakage downstream.

@kvark
kvark approved these changes Jul 23, 2020
@nical
Copy link
Collaborator Author

nical commented Jul 23, 2020

@bors-servo
Copy link
Contributor

bors-servo commented Jul 23, 2020

📌 Commit a7b648a has been approved by nical

@bors-servo
Copy link
Contributor

bors-servo commented Jul 23, 2020

Testing commit a7b648a with merge de9e4cd...

@nical
Copy link
Collaborator Author

nical commented Jul 23, 2020

Oh noes I meant r=kvark. It's one of these days.

@bors-servo
Copy link
Contributor

bors-servo commented Jul 23, 2020

☀️ Test successful - checks-travis
Approved by: nical
Pushing de9e4cd to master...

@bors-servo bors-servo merged commit de9e4cd into servo:master Jul 23, 2020
2 checks passed
2 checks passed
Travis CI - Pull Request Build Passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.