2010/8/17 Hauke Heibel
<hauke.heibel@xxxxxxxxxxxxxx>
I looked into the remaining transformations and there seem to be some
bugs hidden which are not exposed by our unit tests. Maybe some of
them or only because of my changes.
First, on thing I recognized. You consider any fixed size (Dim)x(Dim)
or (Dim+1)x(Dim+1) matrix as a linear and generic transformation
respectively. Therefore, we have the following special product paths
for Transform
1) T * linear matrix => T
2) T * generic matrix => Projective
Ah, thanks for this summary.
Both are violated in case the RHS matrices are not invertible.
This would be the only place where I don't agree: linear/affine/whatever transformations don't necessarily have to be invertible.
Rule 1) is on top violated, in case the "linear" matrix is not
orthogonal and applied to an Isometry.
Indeed, multiplying by a non-isometry (i.e. non-orthogonal) destroys being an isometry.. Elsewhere in Eigen we make this sort of thing the responsibility of the user, but I agree that it's unreasonable here as the user didn't _ask_ for the result to be considered an isometry.
For these paths, my suggestion is to prohibit case 1) completely. I
think the functionality is well covered by
T.linear() *= linear matrix
For case 2), I would choose to simply return a matrix.
I understand that's useful when you want to apply a transformation to a homogeneous vector, or to a batch of homogeneous vectors stored as the columns of a matrix.
However, if we do that, then I would also suggest that we do that in the non-homogeneous case for affine transforms, that is, in case 1) above:
Affine3d * Vector3d ---> Vector3d
more generally:
Affine3d * MatrixWith3Rows ---> MatrixWith3Rows
This is just for consistency. If we get the user used to the convenience that transform can be applied to a matrix, we should be consistent. Also, if we agree that the ability to do these products is useful (which I believe) then they are also useful in the affine case with non-homogeneous vectors.
This is
mathematically more sane and in case your are dead sure, what you are
doing, you can still do
T * Projective(generic matrix)
T * Affine(generic matrix)
etc.
right.
This is even better because it will not necessarily end up with a
projective transformation. It is also more explicit (the ctors are
explicit) and leaves no option for interpretations as to what is
actually happening.
right.
The next issue - and this maybe newly introduced by me - is that currently
Transform<Scalar,Dim,Projective> * Matrix<Scalar,Dim,Dynamic>
compiles. The chosen implementation path is wrong because it assumes
Affine or Isometric times set of points and even if we agreed upon
extending the RHS matrix by 1s as their homogeneous coordinates, we
were suddenly required to return something homogeneous.
Let's quickly take a look at what kind of point transformations we
currently allow for Affine and Isometry:
a) T * Matrix<Scalar,Dim,Dynamic>
b) T * Matrix<Scalar,Dim,1>
do we really special-case 1 and Dynamic? I think, if we agree on the plan outlined above, we should be generic in the number of columns.
c) T * Matrix<Scalar,Dim+1,1>
On top, we allow for Projective
d) T * Matrix<Scalar,Dim+1,Dynamic>
I would propose to allow d) for Affine and Isometry too. It is
mathematically perfectly valid and would simply leave the homogeneous
coordinates untouched.
Right. An affine transform is just a special case of projective transform.
Currently, the only issue is that we allow
T * Matrix<Scalar,Dim,Dim> --> T
T * Matrix<Scalar,Dim+1,Dim+1> --> Projective
as discussed above. In case we do what I proposed, the ambiguity were
resolved and we could allow d) for all transformations.
Exactly!
Here is the summary of allowed transforms as I suggest to offer them -
the asterisk is a wildcard for anything.
// this is all for projective
Matrix<Scalar,Dim+1,*> = Transform<Scalar,Dim,Projective> *
Matrix<Scalar,Dim+1,*>
// these for Affine, AffineCompact and Isometry
Matrix<Scalar,Dim+1,*> = Transform<Scalar,Dim,Affine> * Matrix<Scalar,Dim+1,*>
Matrix<Scalar,Dim,*> = Transform<Scalar,Dim,Affine> * Matrix<Scalar,Dim,*>
Exactly what I had in mind! So actually, we do agree about the point 1) above! I thought you wanted to disallow the latter product and suggested that it does what you just described.
What do you say? The changes are easy and they will reduce code and
template instantiations, make our code more safe and more easy to
understand -- for the user and devloper.
Yep, it looks all good to me. Can you just clarify the apparent contradiction between what you said at the top (disallow 1)) and what you just said (allow the latter product).
Cheers,
Benoit
- Hauke