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.

Sign up for GitHub

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

Jump to bottom

Implement some missing functionalities that would be useful in WebRender. #170

Merged
merged 4 commits into from
Dec 9, 2016
Merged

Implement some missing functionalities that would be useful in WebRender. #170

merged 4 commits into from
Dec 9, 2016

Conversation

nical
Copy link
Contributor

@nical nical commented Nov 18, 2016 •
edited by larsbergstrom
Loading

to_untyped and from_untyped ease the conversion between TypedRect/Size/Point/Matrix objects and their UnknownUnit default counterparts. Most types already have these methods but it is currently missing from Matrix4D.

r? @glennw

This change is Reviewable

@nical nical changed the title Implement to_untyped and from_untyped for Matrix4D. Implement some missing functionalities that would be useful in WebRender. Nov 18, 2016
@nical
Copy link
Contributor Author

nical commented Dec 9, 2016

I added rect(x, y, w, h) and a few other shorthands for ergonomic purposes, I know that the general convention is to use Foo::new, but typing Rect::new(Point2D::new(x, y), Size2D::new(x, y)) all over the place is annoying. There are plenty of such places in the WebRender repo like: https://github.com/servo/webrender/blob/28bc050fe739e178cc8d55a484ffb396410ec26d/wrench/src/yaml_frame_reader.rs#L89
and some other projects I'm doing with euclid also suffer from that.

Ms2ger
Ms2ger reviewed Dec 9, 2016
View reviewed changes
src/rect.rs
@@ -215,6 +215,31 @@ where T: Copy + Clone + Zero + PartialOrd + PartialEq + Add<T, Output=T> + Sub<T
pub fn translate_by_size(&self, size: &TypedSize2D<T, U>) -> TypedRect<T, U> {
self.translate(&TypedPoint2D::new(size.width, size.height))
}

/// Returns the smallest rectangle containing the four points.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Four?

src/point.rs
@@ -760,6 +760,18 @@ impl<T: ApproxEq<T>, U> ApproxEq<T> for TypedPoint4D<T, U> {
}
}

pub fn point2<T: Copy, U>(x: T, y: T) -> TypedPoint2D<T, U> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Meh.

@glennw
Copy link
Member

glennw commented Dec 9, 2016

@bors-servo r+

@bors-servo
Copy link
Contributor

📌 Commit 48096cd has been approved by glennw

@bors-servo
Copy link
Contributor

⚡ Test exempted - status

@bors-servo bors-servo merged commit 48096cd into servo:master Dec 9, 2016
bors-servo pushed a commit that referenced this pull request Dec 9, 2016
Auto merge of #170 - nical:mat4-to_untyped, r=glennw
a56bf44 
Implement some missing functionalities that would be useful in WebRender.

to_untyped and from_untyped ease the conversion between TypedRect/Size/Point/Matrix objects and their UnknownUnit default counterparts. Most types already have these methods but it is currently missing from Matrix4D.

r? @glennw

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/euclid/170)
<!-- Reviewable:end -->
@bors-servo bors-servo mentioned this pull request Dec 9, 2016
Merged
@staktrace
Copy link
Contributor

This PR added a new TypedRect::from_points function, and should have caused a non-minor version bump. Instead it got included in the 0.10.2 -> 0.10.3 euclid update, and causes webrender compile bustage (because webrender has its own from_points in a helper trait, I think).

@glennw @nical FYI ^

@staktrace
Copy link
Contributor

Oh I guess webrender was updated to use the new from_points, but the one in our quantum branch is still using the old webrender. Updating everything together should fix the issue I'm having. (But still, I think this PR deserved a non-minor version bump as it can cause bustage)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ms2ger Ms2ger Ms2ger left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@nical @glennw @bors-servo @staktrace @Ms2ger