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

Apply skew transformation for alpha and beta angles. #100

Merged
merged 1 commit into from Sep 11, 2015
Merged

Conversation

@nox
Copy link
Member

nox commented Aug 27, 2015

This patch is to fix servo/servo#6238.

Review on Reviewable

pub fn create_skew(sx: f32, sy: f32) -> Matrix4 {
/// https://drafts.csswg.org/css-transforms/#funcdef-skew
pub fn create_skew(alpha: f32, beta: f32) -> Matrix4 {
let (sx, sy) = (beta.tan(), alpha.tan());

This comment has been minimized.

Copy link
@mbrubeck

mbrubeck Sep 11, 2015

Contributor

Aren't these parameters swapped?

This comment has been minimized.

Copy link
@nox

nox Sep 11, 2015

Author Member

Well… #77

This comment has been minimized.

Copy link
@mbrubeck

mbrubeck Sep 11, 2015

Contributor

I see this was addressed previously.

/// Create a 2d skew matrix
pub fn create_skew(sx: f32, sy: f32) -> Matrix4 {
/// https://drafts.csswg.org/css-transforms/#funcdef-skew
pub fn create_skew(alpha: f32, beta: f32) -> Matrix4 {

This comment has been minimized.

Copy link
@mbrubeck

mbrubeck Sep 11, 2015

Contributor

We should make sure to bump the version number to 0.2 for this breaking change. It might not matter in this case, but it's a good habit.

This comment has been minimized.

Copy link
@nox

nox Sep 11, 2015

Author Member

Ok.

This comment has been minimized.

Copy link
@mbrubeck

mbrubeck Sep 11, 2015

Contributor

I'm having second thoughts about this. No crate outside of Servo uses this function, so it'd create a bunch of busywork for a purely theoretical breakage. Perhaps we can just fudge this as a "bug fix".

@@ -303,8 +303,9 @@ impl Matrix4 {
)
}

/// Create a 2d skew matrix
pub fn create_skew(sx: f32, sy: f32) -> Matrix4 {
/// https://drafts.csswg.org/css-transforms/#funcdef-skew

This comment has been minimized.

Copy link
@mbrubeck

mbrubeck Sep 11, 2015

Contributor

I'd prefer to keep the descriptive doc string, and add the link to the body of the comment

This comment has been minimized.

Copy link
@nox

nox Sep 11, 2015

Author Member

Ok.

@mbrubeck
Copy link
Contributor

mbrubeck commented Sep 11, 2015

See above for review comments.

@nox nox force-pushed the nox:skew branch from 8bc9b18 to 680fcb5 Sep 11, 2015
This patch is to fix #6237 in servo.
@nox nox force-pushed the nox:skew branch from 680fcb5 to 1f1c5eb Sep 11, 2015
@mbrubeck
Copy link
Contributor

mbrubeck commented Sep 11, 2015

@bors-servo
Copy link
Contributor

bors-servo commented Sep 11, 2015

📌 Commit 1f1c5eb has been approved by mbrubeck

@bors-servo
Copy link
Contributor

bors-servo commented Sep 11, 2015

Testing commit 1f1c5eb with merge 985b58d...

bors-servo pushed a commit that referenced this pull request Sep 11, 2015
bors-servo
Apply skew transformation for alpha and beta angles.

This patch is to fix servo/servo#6238.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/euclid/100)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Sep 11, 2015

☀️ Test successful - travis

@bors-servo bors-servo merged commit 1f1c5eb into servo:master Sep 11, 2015
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI 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.

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