Add RigidTransform3D type

[Manishearth]

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 

[pcwalton]

Typo at end of line?

[pcwalton]

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]

Typo at end of line?

[pcwalton]

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?

[Manishearth]

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" 😄

[Manishearth]

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

[asajeffrey]

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

[pcwalton]

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.

[pcwalton]

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

[Manishearth]

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.

[Manishearth]

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.

[Manishearth]

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

[Manishearth]

    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]

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]

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]

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]

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]

Updated with row-vector math

[pcwalton]

Almost done, just had a couple more questions.

[pcwalton]

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

[pcwalton]

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

[Manishearth]

er, typo, it should be R'

[Manishearth]

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]

(addressed)

[pcwalton]

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

[Manishearth]

hah

@bors-servo r=pcwalton

[bors-servo]

📌 Commit e941644 has been approved by pcwalton

[bors-servo]

⌛ Testing commit e941644 with merge f5552ad...

[Manishearth]

@bors-servo r=pcwalton

[bors-servo]

📌 Commit 60a6fe8 has been approved by pcwalton

[bors-servo]

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

[bors-servo]

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