Expose pre/post operations and transpositions explicitly, and add documentation. #149

nical · 2016-07-29T18:13:35Z

This is a big PR, beware that it has a lot of breaking changes in the matrix types.
It implements what I propose in issue #145 and issue #146, namely:

Remove mul for matrices in favor of explicit pre_mul and post_mul (similar to what we have in gecko).
Add Row-major and column-major serializers and constructors to matrices to help interfacing with dom matrices and external libs such as skia (the actual layout of the matrix types stays row-major but that's an implementation detail that only matters to the people looking at the memory in a debugger).
Add a whole bunch of methods that are used a lot in gecko.
Add a lot of documentation.

This is still a work in progress but it's almost ready, so interested parties can start looking at it give feedback.

This change is

nical · 2016-08-02T22:46:13Z

The PR is in a good state now (modulo making the git history pretty once eventual review comments have been addressed). Here is a summary of what's in there:

make pre and post multiplication explicit (removing mul in favor of pre_mul/post_mul
implement the common pre/post operations (translation, scale, rotation), using translated instead of translate in the terminology to avoid immutable methods looking like they are in-place transformations (and let the names available for in-place operations if we ever find out that it helps with code-gen in the future). see post_translated, pre_rotated, etc.
add explicit row-major/column-major constructors and serializers (row_major replaces new, column_major is added, to_array becomes to_row_major_array and to_column_major_array complements it. With this, the internal layout is completely irrelevant unless you are looking at your stack in a debugger.

The above changes make it hard to fall into any trap by habit of using a convention or another.
Additionally, I added methods that are used all over the place in gecko:

float to integer conversion for points/sizes/etc. without these people tend to just use casts but truncating decimals towards zero doesn't always make sense from a geometrical point of view. See floor, ceil and round.
Similar methds for rects, see round_in and round_out. They are used so much in gecko that I am surprised servo doesn't have equivalents already, or perhaps it is outside of euclid.
Lots and lots of documentation all over euclid.

And last but not least, I made a lot of code that was relying on Clone, rely on Copy instead. This simplifies things a great deal and I don't think anyone will ever use euclid with scalar types that are not Copy, although that's debatable. A lot of code was already only implemented for Copy types, especially in matrices, so I just generalized it to the rest of the library.

nical · 2016-08-02T22:56:21Z

I realize that's a lot for one PR, sorry. We can discuss, the different aspects of it here and I'll remove things if need be.

samlh · 2016-08-03T07:38:52Z

src/matrix2d.rs

-impl<T: Copy, Src, Dst> TypedMatrix2D<T, Src, Dst> {
-    pub fn to_array(&self) -> [T; 6] {
+    /// Create a matrix specifying its components in column-major order.
+    pub fn colum_major(m11: T, m21: T, m31: T, m12: T, m22: T, m32: T) -> TypedMatrix2D<T, Src, Dst> {


samlh · 2016-08-03T07:41:51Z

src/point.rs

-        match (NumCast::from(self.x.clone()), NumCast::from(self.y.clone())) {
+    ///
+    /// When casting from floating point to integer coordinates, the decimals are truncated
+    /// as one would expect from a simple cast, but this behavior does not always marke sense


bors-servo · 2016-08-03T08:13:21Z

☔ The latest upstream changes (presumably #150) made this pull request unmergeable. Please resolve the merge conflicts.

peterjoel · 2016-08-07T16:34:47Z

I'm curious why the change to make various types implement Copy. Is that being tracked or discussed in an issue?

peterjoel · 2016-08-07T16:54:11Z

Never mind: somehow I thought you had made the matrix types implement Copy. I see now that it's just a constraint on the scalar type parameter. It's hard to imagine how that would be a problem.

Comments from Reviewable

peterjoel · 2016-08-07T17:11:54Z

Is there any reason not to also change the Clone constraint to Copy for Length?

peterjoel · 2016-08-07T17:13:10Z

src/point.rs

    /// Constructor taking properly typed Lengths instead of scalar values.
    #[inline]
    pub fn from_lengths(x: Length<T, U>, y: Length<T, U>) -> TypedPoint2D<T, U> {
-        TypedPoint2D::new(x.get(), y.get())
+        TypedPoint2D::new(x.0, y.0)


x.get() actually feels cleaner to me.

nical · 2016-08-08T07:42:20Z

Since a lot of methods were already implemented on top of Copy scalar types, having the other half of the library implemented on top of (the less constrained) Clone trait was not particularly useful. Basing everything on top of Copy simplified the code quite a bit.

No particular reason for Length's Clone constraint being left to Clone instead of Copy. For vector/matrix types the incentive was to make the methods easier to write and more readable while Length doesn't do anything fancy with its value. I can change it to use Copy as well If you prefer.

peterjoel · 2016-08-08T19:20:01Z

It might be a bit neater though to have the same constraints everywhere. But it probably doesn't matter too much either way.

bors-servo · 2016-08-10T17:15:29Z

☔ The latest upstream changes (presumably #153) made this pull request unmergeable. Please resolve the merge conflicts.

peterjoel · 2016-08-13T15:55:58Z

I tried applying this to servo. The only thing missing is that TypedMatrix2D needs Copy and Clone impls for T: Copy.

peterjoel · 2016-08-13T17:41:53Z

When this is merged, I have made the relevant changes to Servo, which I'll make follow-on PRs for:

peterjoel · 2016-08-13T20:32:27Z

src/matrix4d.rs

    /// Create a 2d skew matrix.
-    /// https://drafts.csswg.org/css-transforms/#funcdef-skew
+    ///
+    /// See https://drafts.csswg.org/css-transforms/#funcdef-skew
    pub fn create_skew(alpha: T, beta: T) -> TypedMatrix4D<T, Src, Dst> {


Shouldn't alpha and beta be Radians<T> here?

nical · 2016-08-16T16:17:29Z

Hi, I'm just back from holiday, I'll catch up with this asap, probably tomorrow, address comments, and rebase the PR.
@peterjoel thanks a lot for the followup PRs! I can take care of the rest if you want (I don't mean to have you spend your time cleaning after my breaking changes).

peterjoel · 2016-08-16T16:51:18Z

@nical No worries. I need to keep my unmerged DOMMatrix branch up to date, so I'm applying those changes anyway.


        Expose pre/post operations and transpositions explicitly.

nical · 2016-08-17T18:03:44Z

I just rebased the branch, squashed it into a single commit fixed the skew parameters to use radians explicitly. I also rolled back the part where Point::zero() was renamed into Point::origin() which had landed in this branch accidentally.

nical · 2016-08-17T18:25:06Z

r? @pcwalton

pcwalton · 2016-08-18T17:22:55Z

@bors-servo: r+

Very nice to see more work being done on euclid :)

bors-servo · 2016-08-18T17:22:55Z

📌 Commit fe20f19 has been approved by pcwalton

bors-servo · 2016-08-18T17:25:54Z

⚡ Test exempted - status


        Auto merge of #149 - nical:pre-post, r=pcwalton

Expose pre/post operations and transpositions explicitly, and add documentation. This is a big PR, beware that it has a lot of breaking changes in the matrix types. It implements what I propose in issue #145 and issue #146, namely: * Remove mul for matrices in favor of explicit pre_mul and post_mul (similar to what we have in gecko). * Add Row-major and column-major serializers and constructors to matrices to help interfacing with dom matrices and external libs such as skia (the actual layout of the matrix types stays row-major but that's an implementation detail that only matters to the people looking at the memory in a debugger). * Add a whole bunch of methods that are used a lot in gecko. * Add a lot of documentation. This is still a work in progress but it's almost ready, so interested parties can start looking at it give feedback.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/euclid/149)

nical force-pushed the nical:pre-post branch from 5a625ca to e2f55f7 Jul 31, 2016

samlh reviewed Aug 3, 2016
View changes

nical force-pushed the nical:pre-post branch from beecec7 to 0c8bee0 Aug 3, 2016

peterjoel reviewed Aug 7, 2016
View changes

peterjoel reviewed Aug 13, 2016
View changes

Expose pre/post operations and transpositions explicitly.

Loading status checks…

fe20f19

nical force-pushed the nical:pre-post branch from 0c8bee0 to fe20f19 Aug 17, 2016

bors-servo merged commit fe20f19 into servo:master Aug 18, 2016
1 of 2 checks passed

1 of 2 checks passed

code-review/reviewable 10 files, 3 discussions left (peterjoel, samlh)
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

bors-servo mentioned this pull request Aug 18, 2016

Associativity issue because w is not multiplied #157

Merged

nical mentioned this pull request Aug 19, 2016

Document why Matrix4 and other types are reversed with regard to the WhatWG specs #85

Closed

servo / euclid

Expose pre/post operations and transpositions explicitly, and add documentation. #149

Expose pre/post operations and transpositions explicitly, and add documentation. #149

nical commented Jul 29, 2016 •

edited by larsbergstrom

nical commented Aug 2, 2016

nical commented Aug 2, 2016

This comment has been minimized.

This comment has been minimized.

bors-servo commented Aug 3, 2016

peterjoel commented Aug 7, 2016

peterjoel commented Aug 7, 2016

peterjoel commented Aug 7, 2016

This comment has been minimized.

nical commented Aug 8, 2016

peterjoel commented Aug 8, 2016

bors-servo commented Aug 10, 2016

peterjoel commented Aug 13, 2016

peterjoel commented Aug 13, 2016 •

edited

This comment has been minimized.

nical commented Aug 16, 2016

peterjoel commented Aug 16, 2016

nical commented Aug 17, 2016

nical commented Aug 17, 2016

pcwalton commented Aug 18, 2016

bors-servo commented Aug 18, 2016

bors-servo commented Aug 18, 2016

servo / euclid

Join GitHub today

Expose pre/post operations and transpositions explicitly, and add documentation. #149

Expose pre/post operations and transpositions explicitly, and add documentation. #149

Conversation

nical commented Jul 29, 2016 • edited by larsbergstrom

nical commented Aug 2, 2016

nical commented Aug 2, 2016

This comment has been minimized.

samlh Aug 3, 2016

This comment has been minimized.

samlh Aug 3, 2016

bors-servo commented Aug 3, 2016

peterjoel commented Aug 7, 2016

peterjoel commented Aug 7, 2016

peterjoel commented Aug 7, 2016

This comment has been minimized.

peterjoel Aug 7, 2016

nical commented Aug 8, 2016

peterjoel commented Aug 8, 2016

bors-servo commented Aug 10, 2016

peterjoel commented Aug 13, 2016

peterjoel commented Aug 13, 2016 • edited

This comment has been minimized.

peterjoel Aug 13, 2016

nical commented Aug 16, 2016

peterjoel commented Aug 16, 2016

nical commented Aug 17, 2016

nical commented Aug 17, 2016

pcwalton commented Aug 18, 2016

bors-servo commented Aug 18, 2016

bors-servo commented Aug 18, 2016

nical commented Jul 29, 2016 •

edited by larsbergstrom

peterjoel commented Aug 13, 2016 •

edited