Re: [eigen] Mapping array of scalars into quaternions

[Benoit Jacob <jacob.benoit.1@xxxxxxxxx>]

2009/10/22 Gael Guennebaud <gael.guennebaud@xxxxxxxxx>:
>
>
> On Wed, Oct 21, 2009 at 4:41 PM, Benoit Jacob <jacob.benoit.1@xxxxxxxxx>
> wrote:
>>
>> Hi,
>>
>> this is really good stuff, i can confirm that you've correctly
>> understood the various issues about which you were unsure.
>> Incidentally we had been talking about something like this with Rohit
>> recently.
>>
>> There are so many things to do at the moment, could you please address
>> these points before we apply your patch, that would help a lot:
>> 1) need to extend the quaternion unit tests to cover that. The file is
>> test/geo_quaternion.cpp. No need to adapt all of it, just append a few
>> sanity checks for Quaternion<Map...> at the end, e.g. build a
>> quaternion that maps the array of a plain quaternion, do some
>> arithmetic on both and check (VERIFY_IS_APPROX) that the results are
>> the same.
>> 2) i've made a few comments inline below.
>>
>> 2009/10/21 Mathieu Gautier <mathieu.gautier@xxxxxx>:
>> > Hi,
>> >
>> > I want to implement a class which hold both a quaternion and a vector3
>> > to
>> > discribe a position of a frame in respect to an other frame (Something
>> > similar to Frame in KDL). For some historic reason, the memory layout
>> > must
>> > be a continuous array of scalar (Scalar[7]), the 3 first scalars are the
>> > vector3 and the other are the quaternion. Besides I have to interface
>> > this
>> > structure with other libraries which return an array of 7 scalars. I
>> > also
>> > have to map array of 4 scalars into the Quaternion class.
>>
>> OK. So indeed with the current quaternion class you were out of luck
>> because its array is 16-byte aligned.
>>
>> >
>> > I though of something like that :
>> >
>> > template<typename _Scalars>
>> > struct Frame {
>> >    _Scalars[7] data;
>> >    Map<Matrix<3,1,_Scalars>> v;
>> >    Map<Quaternion<_Scalars>> q;
>> > };
>> >
>> > and setting the Map accordingly. However, there's no such concept of Map
>> > for
>> > the Quaternion class or RotationBase.
>>
>> Indeed.
>>
>> >
>> > So, I modified the Quaternion class to use either a Matrix<Scalar,4,1>
>> > to
>> > store the quaternion coefficients or a Map<Matrix<Scalar,4,1>> to map an
>> > array of scalars. I added a argument to the template prototype of the
>> > quaternion class which I call StorageType which is either the Matrix or
>> > Map<Matrix>.
>>
>> Good.
>> In the Quaternion constructor, you could add some sanity checks for that
>> type:
>> EIGEN_STATIC_ASSERT_VECTOR_ONLY(StorageType)
>> EIGEN_STATIC_ASSERT(
>>  int(StorageType::Flags)&DirectAccessBit,
>>
>>  THIS_METHOD_IS_ONLY_FOR_EXPRESSIONS_WITH_DIRECT_MEMORY_ACCESS_SUCH_AS_MAP_OR_PLAIN_MATRICES
>> )
>>
>> > Since I have almost no experience with Eigen, I don't know if it's the
>> > better choice and if it could cause alignement issues. I have attached
>> > my
>> > modifications as a patch. This implementation may be incomplete.
>>
>> That's good and there's no alignment issue, because the StorageType
>> would only claim to be aligned if you explicitly tell it that the
>> pointer is aligned.
>> Currently we have a little performance trouble: there's no way to tell
>> Map that the pointer is aligned, but see other new thread, this is
>> taken care of.
>>
>> >
>> > So, to summarize, the Frame class would be:
>> >
>> > template<typename _Scalars>
>> > struct Frame {
>> >    _Scalars[7] data;
>> >    Map<Matrix<3,1,_Scalars>> v;
>> >    QuaternionMap<_Scalars> q;
>> > };
>> >
>> > where v is a mapping of data and q a mapping of data+3.
>>
>> Looks good.
>>
>> Some more comments inline in the patch:
>>
>> >
>> > --> -template<typename _Scalar> struct ei_traits<Quaternion<_Scalar> >
>> > +template<typename _Scalar, class StorageType> struct
>> > ei_traits<Quaternion<_Scalar, StorageType> >
>> >  {
>> >   typedef _Scalar Scalar;
>> >  };
>> >
>> > -template<typename _Scalar>
>> > -class Quaternion : public RotationBase<Quaternion<_Scalar>,3>
>> > +template<typename _Scalar, class StorageType = Matrix<_Scalar, 4, 1>>
>> > +class Quaternion : public RotationBase<Quaternion<_Scalar,
>> > StorageType>,3>
>>
>> Why make it templated in both the Scalar and StorageType? The Scalar
>> could be deduced from the storage type:
>>
>>  typedef typename StorageType::Scalar Scalar;
>>
>> fewer template argument passing, is better.
>
> well, I guess we still want the user to be able to just write:
>
> typedef Quaternion<MyScalar> MyQuat;
>
> and not:
>
> typedef Quaternion<Matrix<MyScalar,4,1> > MyQuat;
>
> right ?
>
> then the first template argument of Quaternion must be the scalar type.

This is what Mathieu's patch does.

> But
> if we prefer we can also introduce a QuaternionBase and a QuaternionWrapper
> classes like we did for DiagonalMatrix. Of course this is more work, but
> perhaps that's a good idea to harmonize that over Eigen.

The reason why I introduced DiagonalBase was that DiagonalMatrix and
DiagonalWrapper can't be completely unified, they need e.g. different
constructors and have different ei_traits. Now looking at the case of
quaternions, there too we need different constructors: the
QuaternionWrapper needs a ctor taking a pointer and doesn't want the
ctor taking 4 numbers, for example.

So, you're right.

Benoit