Add RigidTransform3D type #328

Manishearth · 2019-03-26T21:13:14Z

I need this for implementing WebXR in Servo. The type can be implemented out-of-tree as well, but it feels appropriate to have it here.

I wasn't quite sure what to name it or if I should have called it TypedRigidTransform3D. I haven't written the 2D version because I don't need it, but it should actually be a straightforward copy-paste with the 3s changed to 2s (since the matrix math doesn't change). I'll add it on request.

The tests do help assure me that the math is right, but I would like to have some scrutiny there.

If this is going to take a while to review, let me know and I can try landing a temporary version of this in Servo so I can continue with my work.

r? @pcwalton @nical

This change is

src/rigid.rs

pcwalton · 2019-03-28T00:13:50Z

src/rigid.rs

-    /// Construct a new rigid transformation, where the `rotation` applies first
-    pub fn new(rotation: TypedRotation3D<T, U, U>, translation: TypedVector3D<T, U>) -> Self {
+impl<T: Float + ApproxEq<T>, Src, Dst> TypedRigidTransform3D<T, Src, Dst> {
+    /// Construct a new rigid transformation, where the `rotation` applies first\


Typo at end of line?

pcwalton · 2019-03-28T00:20:41Z

src/rigid.rs

+impl<T: Float + ApproxEq<T>, Src, Dst> TypedRigidTransform3D<T, Src, Dst> {
+    /// Construct a new rigid transformation, where the `rotation` applies first\
+    #[inline]
+    pub fn new(rotation: TypedRotation3D<T, Src, Dst>, translation: TypedVector3D<T, Dst>) -> Self {


I kind of wonder if it might be easier if we had from_translated_rotation for this and from_rotated_transform for "reversed"? Up to you. I'm not sure whether the wordiness would help.

pcwalton · 2019-03-28T00:25:18Z

src/rigid.rs

+        }
+    }
+
+    /// Construct an identity transform\


Typo at end of line?

pcwalton · 2019-03-28T00:31:02Z

src/rigid.rs

+        let rigid = RigidTransform3D::new(rotation, translation);
+        assert!(rigid
+            .to_transform()
+            .approx_eq(&translation.to_transform().pre_mul(&rotation.to_transform())));


Wait, it says in your comment above that RigidTransform3D is Translation * Rotation. But you have Rotation * Translation written here. translation.pre_mul(rotation) is rotation.post_mul(translation) and A.post_mul(B) is AB. See the definition of post_mul here: https://github.com/servo/euclid/blob/master/src/transform3d.rs#L259 (each entry is row of A times column of B).

Am I missing something or is this backwards?

The type signatures of pre_mul and post_mul are backwards, then:

pub fn post_mul<NewDst>(&self, mat: &TypedTransform3D<T, Dst, NewDst>) -> TypedTransform3D<T, Src, NewDst>

i.e. Transform<Src, Dst>.post_mul(Transform<Dst, NewDst>) = Transform<Src, Dst> * Transform<Dst, NewDst> = Transform<Src, NewDst>. This doesn't make sense, if we were to transform a vector V the transformation would be Transform<Src, Dst> * Transform<Dst, NewDst> * Vector<?> , which forces the vector to be Dst, transforming it to NewDst and then somehow transforming it to Dst from Src, which it is not.

This all makes sense if the multiplication is backwards.

aside: post_mul and pre_mul are confusing method names because it's not clear if a.post_mul(b) means "a postmultiplied by b" or "postmultiply a to b" 😄

I think I got confused enough times with pre_mul and post_mul that I've verified what they do multiple times (we really should write the matrix math down in the docs) and I may have made a mistake with this, but also the typed units seem to work

Just for laughs, Pathfinder and Euclid have pre_mul and post_mul swapped.

So are the units wrong or are the names wrong? AB is each row of A times each column of B. That corresponds to A.post_mul(B) as written in the code fragment I linked, right?

I think I'm going to abolish the pre and post stuff in Pathfinder. It's so confusing.

Typically I multiply matrices by column vectors in that order. i.e. Ma = b, where a and b are column vectors.

Yes, that is what everyone does, but this is not what Euclid is doing, assuming it's using row-major order. Euclid is doing aM=b with row vectors.

Also please don't take my word for anything I say about euclid's code here, I've been looking at this enough that it's quite possible that I'm missing something and just reading things wrong.

Oh the documentation of post_mul matches its units (and the way I use it, i.e. a.post_mul(b) = ba) but not the code:

/// Returns the multiplication of the two matrices such that mat's transformation /// applies after self's transformation. pub fn post_mul<NewDst>(&self, mat: &TypedTransform3D<T, Dst, NewDst>) -> TypedTransform3D<T, Src, NewDst> { TypedTransform3D::row_major(

"mat's transform applies after self's transform" means mat * self, which matches the units, and my interpretation. But not the code.

Increasingly convinced the code is column major

pub fn create_translation(x: T, y: T, z: T) -> Self { let (_0, _1): (T, T) = (Zero::zero(), One::one()); TypedTransform3D::row_major( _1, _0, _0, _0, _0, _1, _0, _0, _0, _0, _1, _0, x, y, z, _1 ) }

oh hey the translation matrices are wrong too :)

Manishearth · 2019-03-28T07:26:40Z

Amusingly, even though the transforms code is wrong, this code is correct, because it doesn't use any of the affected APIs, and deals purely in transforms.

(which is why it typechecks and passes tests)

nical · 2019-03-28T10:28:59Z

aside: post_mul and pre_mul are confusing method names because it's not clear if a.post_mul(b) means "a postmultiplied by b" or "postmultiply a to b" smile

In retrospect I think that pre_transform/post_transform would have been better names to help avoid thinking in terms of matrix multiplication which is the root a lot of grief.
a.post_mul(b) here means, the effect of transformation b is applied after the effect of a, so:

/// This is equivalent to first translating and then rotating p.
let transform = translation.post_transform(rotation);
let result = transform.transform_point3d(p);

Manishearth · 2019-03-28T15:37:33Z

Honestly that doesn't clear things up too much since you have to know that this is talking in terms of transforms. I and @pcwalton discussed that with Gankro on twitter yesterday -- both of us are used to using matrices for graphics and find those method names (as used in Gecko) confusing.

pre_transform and post_transform also have the same problem as pre_mul/post_mul -- you can read it as an active or passive sentence, and it means opposite things.

Gankro's suggestion .then() is much much nicer, though still a little bit confusing.

The problem here is really the documentation.

nical · 2019-03-28T15:57:20Z

There's also append_transform and prepend_transform which maybe better underline the notion of a transform being built upon of an ordered chain of transforms.

.then() works too, after all we don't necessarily need a method in both directions.

I'm very keen on improving the documentation. Don't hesitate to point me to the places where it falls short (having written a big chunk of this stuff I'm not necessarily in a good spot to see where the confusing areas are). Are you thinking about a paragraph on the structs, or more examples in the methods ?

Manishearth · 2019-03-28T17:38:53Z

Updated with row-vector math

pcwalton

Almost done, just had a couple more questions.

pcwalton · 2019-03-29T01:31:35Z

src/rigid.rs

+        }
+    }
+
+    /// Decompose this into a position and an orientation to be applied in the opposite order


Change "position" to "translation" and "orientation" to "rotation" for consistency?

pcwalton · 2019-03-29T01:35:09Z

src/rigid.rs

+        //        = (R' * R'^-1) * T' * R' * T2
+        //        = R' * (R'^-1 * T' * R') * T2
+        //        = R' * T'' * T2
+        //        = R * T''


How do you get this last step from the above one?

er, typo, it should be R'

Manishearth · 2019-03-29T01:59:57Z

I kinda want to pick method names like from_translated_rotation but I don't think those are particularly clearer, and the introduced ambiguity may in fact be more confusing. Whereas with ::new you get encouraged to stick to one of the two.

Manishearth · 2019-03-29T02:00:04Z

(addressed)

src/rigid.rs

pcwalton · 2019-03-29T04:11:57Z

OK, r=me. This is obviously correct code and I'm tired of fighting GitHub reviews :)

Manishearth · 2019-03-29T04:12:21Z

hah

@bors-servo r=pcwalton

bors-servo · 2019-03-29T04:12:22Z

📌 Commit e941644 has been approved by pcwalton

bors-servo · 2019-03-29T04:12:26Z

⌛ Testing commit e941644 with merge f5552ad...

@pcwalton

Add RigidTransform3D type I need this for implementing WebXR in Servo. The type can be implemented out-of-tree as well, but it feels appropriate to have it here. I wasn't quite sure what to name it or if I should have called it `TypedRigidTransform3D`. I haven't written the 2D version because I don't need it, but it should actually be a straightforward copy-paste with the 3s changed to 2s (since the matrix math doesn't change). I'll add it on request. The tests do help assure me that the math is right, but I would like to have some scrutiny there. If this is going to take a while to review, let me know and I can try landing a temporary version of this in Servo so I can continue with my work. r? @pcwalton @nical  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/euclid/328)

Manishearth · 2019-03-29T04:14:28Z

@bors-servo r=pcwalton

bors-servo · 2019-03-29T04:14:29Z

📌 Commit 60a6fe8 has been approved by pcwalton

bors-servo · 2019-03-29T04:14:32Z

⌛ Testing commit 60a6fe8 with merge 58a50ff...

@pcwalton

Add RigidTransform3D type I need this for implementing WebXR in Servo. The type can be implemented out-of-tree as well, but it feels appropriate to have it here. I wasn't quite sure what to name it or if I should have called it `TypedRigidTransform3D`. I haven't written the 2D version because I don't need it, but it should actually be a straightforward copy-paste with the 3s changed to 2s (since the matrix math doesn't change). I'll add it on request. The tests do help assure me that the math is right, but I would like to have some scrutiny there. If this is going to take a while to review, let me know and I can try landing a temporary version of this in Servo so I can continue with my work. r? @pcwalton @nical  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/euclid/328)

bors-servo · 2019-03-29T04:23:18Z

☀️ Test successful - checks-travis
Approved by: pcwalton
Pushing 58a50ff to master...

@jdm

Update XR code to use rigid transforms and new pose/transform stuff from the spec This updates our XR code to use euclid's new [RigidTransform3D type](servo/euclid#328), which is more efficent and convenient to work with. It additionally brings us up to speed with the spec: - `XRViewerPose` was made a subclass of `XRPose` (immersive-web/webxr#496) - `XRView.viewMatrix` was removed in favor of `XRRigidTransform.inverse.matrix` (immersive-web/webxr#531) - `XRRigidTransform.inverse` is an attribute (immersive-web/webxr#560) - `XRRigidTransform` now validates positions in its constructor (immersive-web/webxr#568) Furthermore, it adds support for `XRRigidTransform.matrix`. While fixing this I also noticed that our view matrix code was incorrect, we calculated view matrices as `pose.to_column_major_array()`, whereas it *should* be `pose.inverse().to_row_major_array()` (since Euclid uses row vectors, whenever the spec says it wants a column major array we should use `.to_row_major_array()` since all web specs implicitly use column vectors). For 3DOF devices poses are mostly rotations anyway, so the effective transpose behaved _like_ an inversion, but was incorrect. This PR gets rid of `view.viewMatrix` anyway, however I felt like I should mention this discrepancy, since otherwise the replacement of `view.viewMatrix` with `view.transform.inverse.matrix` doesn't make sense r? @jdm  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/23159)

@jdm

Update XR code to use rigid transforms and new pose/transform stuff from the spec This updates our XR code to use euclid's new [RigidTransform3D type](servo/euclid#328), which is more efficent and convenient to work with. It additionally brings us up to speed with the spec: - `XRViewerPose` was made a subclass of `XRPose` (immersive-web/webxr#496) - `XRView.viewMatrix` was removed in favor of `XRRigidTransform.inverse.matrix` (immersive-web/webxr#531) - `XRRigidTransform.inverse` is an attribute (immersive-web/webxr#560) - `XRRigidTransform` now validates positions in its constructor (immersive-web/webxr#568) Furthermore, it adds support for `XRRigidTransform.matrix`. While fixing this I also noticed that our view matrix code was incorrect, we calculated view matrices as `pose.to_column_major_array()`, whereas it *should* be `pose.inverse().to_row_major_array()` (since Euclid uses row vectors, whenever the spec says it wants a column major array we should use `.to_row_major_array()` since all web specs implicitly use column vectors). For 3DOF devices poses are mostly rotations anyway, so the effective transpose behaved _like_ an inversion, but was incorrect. This PR gets rid of `view.viewMatrix` anyway, however I felt like I should mention this discrepancy, since otherwise the replacement of `view.viewMatrix` with `view.transform.inverse.matrix` doesn't make sense r? @jdm  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/23159)

@jdm

Update XR code to use rigid transforms and new pose/transform stuff from the spec This updates our XR code to use euclid's new [RigidTransform3D type](servo/euclid#328), which is more efficent and convenient to work with. It additionally brings us up to speed with the spec: - `XRViewerPose` was made a subclass of `XRPose` (immersive-web/webxr#496) - `XRView.viewMatrix` was removed in favor of `XRRigidTransform.inverse.matrix` (immersive-web/webxr#531) - `XRRigidTransform.inverse` is an attribute (immersive-web/webxr#560) - `XRRigidTransform` now validates positions in its constructor (immersive-web/webxr#568) Furthermore, it adds support for `XRRigidTransform.matrix`. While fixing this I also noticed that our view matrix code was incorrect, we calculated view matrices as `pose.to_column_major_array()`, whereas it *should* be `pose.inverse().to_row_major_array()` (since Euclid uses row vectors, whenever the spec says it wants a column major array we should use `.to_row_major_array()` since all web specs implicitly use column vectors). For 3DOF devices poses are mostly rotations anyway, so the effective transpose behaved _like_ an inversion, but was incorrect. This PR gets rid of `view.viewMatrix` anyway, however I felt like I should mention this discrepancy, since otherwise the replacement of `view.viewMatrix` with `view.transform.inverse.matrix` doesn't make sense r? @jdm  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/23159)

@jdm

Update XR code to use rigid transforms and new pose/transform stuff from the spec This updates our XR code to use euclid's new [RigidTransform3D type](servo/euclid#328), which is more efficent and convenient to work with. It additionally brings us up to speed with the spec: - `XRViewerPose` was made a subclass of `XRPose` (immersive-web/webxr#496) - `XRView.viewMatrix` was removed in favor of `XRRigidTransform.inverse.matrix` (immersive-web/webxr#531) - `XRRigidTransform.inverse` is an attribute (immersive-web/webxr#560) - `XRRigidTransform` now validates positions in its constructor (immersive-web/webxr#568) Furthermore, it adds support for `XRRigidTransform.matrix`. While fixing this I also noticed that our view matrix code was incorrect, we calculated view matrices as `pose.to_column_major_array()`, whereas it *should* be `pose.inverse().to_row_major_array()` (since Euclid uses row vectors, whenever the spec says it wants a column major array we should use `.to_row_major_array()` since all web specs implicitly use column vectors). For 3DOF devices poses are mostly rotations anyway, so the effective transpose behaved _like_ an inversion, but was incorrect. This PR gets rid of `view.viewMatrix` anyway, however I felt like I should mention this discrepancy, since otherwise the replacement of `view.viewMatrix` with `view.transform.inverse.matrix` doesn't make sense r? @jdm  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/23159)

Manishearth mentioned this pull request Mar 26, 2019

Properly support transforms in WebXR servo/servo#23097

Merged

Manishearth commented Mar 26, 2019

View reviewed changes

src/rigid.rs Outdated Show resolved Hide resolved

Manishearth force-pushed the rigid branch from 2d76230 to 3cee190 Compare March 26, 2019 22:41

pcwalton reviewed Mar 28, 2019

View reviewed changes

Manishearth mentioned this pull request Mar 28, 2019

Transform is column-major, not row major #329

Closed

Manishearth force-pushed the rigid branch from 7fd1bc5 to 166d5fd Compare March 28, 2019 17:38

Manishearth added 4 commits March 28, 2019 10:39

Add Vector2D::to_transform()

6737532

Add Vector3D::to_transform()

713b593

Add simple RigidTransform3D type

3f0f345

Add tests for rigid transformations

002d5f2

Manishearth force-pushed the rigid branch from 166d5fd to bee5eef Compare March 28, 2019 17:39

pcwalton reviewed Mar 29, 2019

View reviewed changes

Manishearth force-pushed the rigid branch from bee5eef to eff7338 Compare March 29, 2019 01:55

Add second type parameter to TypedRigidTransform

74c79e0

Manishearth force-pushed the rigid branch from eff7338 to f5222ba Compare March 29, 2019 01:59

pcwalton reviewed Mar 29, 2019

View reviewed changes

src/rigid.rs Outdated Show resolved Hide resolved

src/rigid.rs Outdated Show resolved Hide resolved

Manishearth force-pushed the rigid branch from f5222ba to e941644 Compare March 29, 2019 03:54

Swap matrix math to use row-vector notation

60a6fe8

Manishearth force-pushed the rigid branch from e941644 to 60a6fe8 Compare March 29, 2019 04:13

bors-servo merged commit 60a6fe8 into servo:master Mar 29, 2019

Manishearth deleted the rigid branch March 29, 2019 04:28

Manishearth mentioned this pull request Apr 3, 2019

Update XR code to use rigid transforms and new pose/transform stuff from the spec servo/servo#23159

Merged

Add RigidTransform3D type #328

Add RigidTransform3D type #328

Uh oh!

Conversation

Manishearth commented Mar 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Manishearth Mar 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Manishearth commented Mar 28, 2019

Uh oh!

nical commented Mar 28, 2019

Uh oh!

Manishearth commented Mar 28, 2019

Uh oh!

nical commented Mar 28, 2019

Uh oh!

Manishearth commented Mar 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pcwalton left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Manishearth commented Mar 29, 2019

Uh oh!

Manishearth commented Mar 29, 2019

Uh oh!

Uh oh!

Uh oh!

pcwalton commented Mar 29, 2019

Uh oh!

Manishearth commented Mar 29, 2019

Uh oh!

bors-servo commented Mar 29, 2019

Uh oh!

bors-servo commented Mar 29, 2019

Uh oh!

Manishearth commented Mar 29, 2019

Uh oh!

bors-servo commented Mar 29, 2019

Uh oh!

bors-servo commented Mar 29, 2019

Uh oh!

bors-servo commented Mar 29, 2019

Uh oh!

Reviewers

Assignees

Manishearth commented Mar 26, 2019 •

edited

Loading

Manishearth Mar 28, 2019 •

edited

Loading

Manishearth commented Mar 28, 2019 •

edited

Loading