implement Transform for Matrix3 and Matrix4

[mhintz]

Pursuant to conversations in #337, #302, and #324, I've implemented Transform for these two matrices. This makes working with the matrices as fully-fledged transformations much easier.

Along the lines of #302 (comment), I've also implemented Transform2 and Transform3 for Matrix3, to reflect its potential use as both a 3D rotation matrix and a 2D affine transform.

Transform3 is implemented for Matrix4 as well, and I moved the From impls down with the rest of the From impls, in an unrelated edit which I think nonetheless improves readability of what is now a rather large file.

I'm not positive I've gotten the Transform2 implementation for Matrix3 correct, would appreciate a sanity check :D

[mhintz]

Well, two things. One: I should note that both of my implementations of look_at for Matrix3 might be a little strange, seeing as how the function takes three parameters, and I'm condensing those down to two.

Second, I see that I should have run the tests before submitting the PR! All of the test failures are due to the fact that both SquareMatrix and Transform define an invert (or invert_self) method, and those methods are now clashing. Of course, it's possible to use Universal Function Call Syntax to disambiguate (as described here rust-lang/rust#17151), but that's silly for a method as commonplace as invert.

I think having Transform implemented for Matrix3 and Matrix4 is a good thing, so I would propose either changing the name of one of the inverts (to, say, inverse_transform on Transform), or removing it from the Transform trait altogether (it feels to me like it belongs more naturally in SquareMatrix)

[brendanzab]

I agree. Sorry, I have been tackling this privately in my spare time. Also note that one also conflicts.

I was thinking something like this:

      Mul<Self, Output = Self>
                 ^
                 |
                One
                 ^
                 |
             Transform
                 ^
                 |
    *------------*-----------*
    |            |           |
 Rotation   UniformScale  Translation
                 ^
                 |
          NonUniformScale

[brendanzab]

Also, make sure you run cargo test :)

[mhintz]

That trait hierarchy looks good to me, but as far as I can tell, it doesn't solve the problem of clashing implementations of invert? Or am I mistaken?

[brendanzab]

Yeah, you're right. We could always have a trait for orthogonal transforms that are guaranteed to have inverses (matrices would not implement this, but quaternions for example would). I dunno. Perhaps leaving it out for now would be easier.

[mhintz]

Hm yeah I suppose leaving it out would be the easiest approach. What's the argument against changing the name to something that doesn't clash? Just duplication? I can imagine a world where not every struct that implements Transform is also a Matrix, and you might want an inverse function just for that Transform.

Then again, I can also imagine a world in which there are non-invertible transforms (not sure what this would be, but hey maybe it's possible)

[brendanzab]

By "guaranteed to have inverses" I mean that, for example, not all matrices have a corresponding inverse value. But I see now... we are actually returning an Option<T> from Transform::invert.

[mhintz]

Oh right! I wasn't even thinking about a transform that, for example, just collapses the x-axis to zero and wouldn't be invertible. I'm starting to see your point of view - I agree that invert should be taken out of Transform.

invert is also defined on Rotation and SquareMatrix, but the Rotation version returns a plain Matrix whereas the SquareMatrix version returns an Option. Maybe it's a good idea to remove it from those as well? Each struct would define its own version.

[kvark]

I don't see how removing invert is gonna do any good. People working with generic transformations should still be able to invert their stuff. Renaming it is fine though, to avoid collisions.

[mhintz]

In the interest of moving forward temporarily (at least until a possible trait reorganization lands), I added changes that resolve the clashing names: Transform::invert -> Transform::inverse_transform, and Transform::invert_self -> Transform::to_inverse

[mhintz]

Thanks!

[brendanzab]

Sorry for the lateness >_<

I'm wondering if we should just remove the SquareMatrix::invert_self and Transform::to_inverse methods. They don't really do much right now...

[mhintz]

Yeah, I would agree with that decision. I think there's a subtlety there which could be confusing to someone coming new to the library. Generally I think it's a good idea to have most of your operations be immutable, except when there's a real performance cost when doing it that way...