Re: [eigen] Transform products

[Hauke Heibel <hauke.heibel@xxxxxxxxxxxxxx>]

Ok, all good. Here is a clean patch - in case you want to double check.

All geo_* tests are passing. I am just missing the 'make check' result.

In theory, I am also taking any expression on the RHS, as I said

A * (S * pts_3X)

is working and here the RHS is a product expression and now also

(A * S) * pts_3X

is working again.

One question. In Transform, the speciaization for the RHS product. Do
you prefer to use an

EIGEN_STATIC_ASSERT(OtherRows==Dim || OtherRows==HDim, ...)

or an

typename ei_enable_if<OtherRows==Dim || OtherRows==HDim,void>::type* = 0

- Hauke

On Thu, Aug 19, 2010 at 2:47 PM, Benoit Jacob <jacob.benoit.1@xxxxxxxxx> wrote:
> 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
>> >
>>
>>
>
>

Attachment: geo_prod.patch
Description: Binary data