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

Rename Matrix2D/4D into Transform2D/3D #194

Merged
merged 1 commit into from
May 21, 2017
Merged

Conversation

nical
Copy link
Contributor

@nical nical commented May 19, 2017

Fixes #160.


This change is Reviewable

@nical
Copy link
Contributor Author

nical commented May 19, 2017

r? @nox

src/lib.rs Outdated
@@ -110,3 +110,13 @@ pub type Radians<T> = Length<T, Rad>;

/// A value in Degrees.
pub type Degrees<T> = Length<T, Deg>;

/// Temporary alias to facilitate the transition to the new naming scheme
pub type Matrix2D<T> = Transform2D<T>;
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps, we should put #[deprecated] on top? See rust-lang/rust#29935

@@ -18,51 +18,51 @@ use trig::Trig;
use std::fmt;

define_matrix! {
/// A 2d transform stored as a 2 by 3 matrix in row-major order in memory,
/// A 2d transform stored as a 2 by 3 transform in row-major order in memory,
Copy link
Member

Choose a reason for hiding this comment

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

I think this should stay as matrix

@@ -18,51 +18,51 @@ use trig::Trig;
use std::fmt;

define_matrix! {
Copy link
Member

Choose a reason for hiding this comment

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

define_transform!?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's the same macro used with points and other euclid types, so I think it's fine to leave it as matrix.

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we even still need this, given now serde_derive is stable and comes with knobs to change the default deriving behaviour, we should look into simplifying it before 1.0.

/// useful to represent 2d transformations.
///
/// Matrices can be parametrized over the source and destination units, to describe a
/// transformation from a space to another.
/// For example, `TypedMatrix2D<f32, WordSpace, ScreenSpace>::transform_point4d`
/// For example, `TypedTransform2D<f32, WordSpace, ScreenSpace>::transform_point4d`
/// takes a `TypedPoint2D<f32, WordSpace>` and returns a `TypedPoint2D<f32, ScreenSpace>`.
///
/// Matrices expose a set of convenience methods for pre- and post-transformations.
Copy link
Member

Choose a reason for hiding this comment

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

Transforms expose a set of ...

pub fn row_major(m11: T, m12: T, m21: T, m22: T, m31: T, m32: T) -> TypedMatrix2D<T, Src, Dst> {
TypedMatrix2D {
impl<T: Copy, Src, Dst> TypedTransform2D<T, Src, Dst> {
/// Create a transform specifying its components in row-major order.
Copy link
Member

Choose a reason for hiding this comment

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

specifying its matrix elements in row-major mode

@nical
Copy link
Contributor Author

nical commented May 20, 2017

Thanks @kvark, I addressed the review comments (except for the macro name).

Copy link
Member

@kvark kvark left a comment

Choose a reason for hiding this comment

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

Thanks!
If we don't really need define_matrix!, we can deal with it as a follow-up.

@nox
Copy link
Contributor

nox commented May 21, 2017

@bors-servo r=kvark

@bors-servo
Copy link
Contributor

📌 Commit c409c9d has been approved by kvark

@bors-servo
Copy link
Contributor

⌛ Testing commit c409c9d with merge 94799b6...

bors-servo pushed a commit that referenced this pull request May 21, 2017
Rename Matrix2D/4D into Transform2D/3D

Fixes #160.

<!-- 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/194)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

☀️ Test successful - status-travis
Approved by: kvark
Pushing 94799b6 to master...

@bors-servo bors-servo merged commit c409c9d into servo:master May 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants