Re: [eigen] Transform products

2010/8/19 Gael Guennebaud <gael.guennebaud@xxxxxxxxx>

Hi,

first of al let me say l I agree with the changes proposed in the other thread.

However, DiagonalMatrix is used to represent non uniform scaling. For
instance Scaling(1,2) returns a 2x2 DiagonalMatrix. So this time I
think that

T * DiagonalMatrix

should really be interpreted as T * scaling, whence the following rules:

Affine = Isometry * diagonal
Affine = Affine * diagonal
Projective = Projective * diagonal

Who want to represent points using a diagonal matrix....

Otherwise we would have to add an explicit NonUniformScaling class,
but this is exactly what a DiagonalMatrix is....

Also, the old (current) code in Transform is taking any EigenBase, i.e. either a dense matrix or another kind of special matrix, in particular a DiagonalMatrix.

That was nice when products were interpreted in the old way (interpreting the EigenBase as a Transform) but frankly not necessary, especially not with the new interpretation.

So I think that you can just change that produce code to only take MatrixBase objects. So we drop support for special matrix classes. In the new interpretation of Transform products that we're coming to, it's not useful anyway. Then you don't have any conflict anymore with DiagonalMatrix, so you can add a separate operator* taking (Transform, DiagonalMatrix).

Benoit

gael

On Thu, Aug 19, 2010 at 1:00 PM, Hauke Heibel

<hauke.heibel@xxxxxxxxxxxxxx> wrote:

> Ok, I am starting a new thread.. The other one was way too long.
>
> I started with the RHS product refactoring. Everything went well,
> until I hit the case, where the RHS is a DiagonalMatrix. The patch is
> attached but I will anyways past the most important piece of code over
> here because it is hard to find in the diff.
>
> The function causing troubles is ei_transform_right_product_impl::run
> for Affine and Isometry. I tried to make a one fits all implementation
> and in theory, it works well. Even the inlining on MSVC seems to be
> working for fixed size types. For DiagonalMatrix, I am hitting the
> issue, that there is now way to extract a block.
>
> Here is how I tried to implement run:
>
> EIGEN_STRONG_INLINE static ResultType run(const
> Transform<Scalar,Dim,Mode>& T, const Other& other)
> {
> EIGEN_STATIC_ASSERT(OtherRows==Dim || OtherRows==HDim,
> YOU_MIXED_MATRICES_OF_DIFFERENT_SIZES);
>
> typedef Block<ResultType, Dim, OtherCols> TopLeftLhs;
> typedef Block<Other, Dim, OtherCols> TopLeftRhs;
>
> ResultType res(other.rows(),other.cols());
>
> TopLeftLhs(res, 0, 0, Dim, other.cols()) =
> ( T.linear() * TopLeftRhs(other, 0, 0, Dim, other.cols()) )..colwise() +
> T.translation();
>
> // This is a hack to circumvent the fact that DiagonalMatrix does
> not expose ::row(int)
> // In fact, the code below will not even be used in release mode,
> but it will still be compiled.
> if (OtherRows==HDim)
> Block<ResultType, 1, OtherCols>(res,0,0,1,other.cols()) =
> Block<Other, 1, OtherCols>(other.derived(),0,0,1,other.cols());
>
> return res;
> }
>
> I used Block in the end, because row() does not exist for
> DiagonalMatrix. Any Ideas? I would be too sad, if I were required to
> write a specialization just because of this since in general, the if
> clause is already removed by the compiler -- though it is still
> compiled.
>
> On the other hand side, who want to do something like this because now
> everything on the RHS is interpreted as points. If the RHS is 3x3 it
> is not any more interpreted as [S 0; 0 1]. The correct move would be
>
> A.linear() *= S;
>
> and even this works
>
> A * (S * pts_33)
>
> though this does not
>
> (A * S) * pts_33
>
> I could even go back to row() instead of block().
>
> I am pausing the patching for now, until I get feedback.
>
> - Hauke
>