[Marble-devel] Eigen

[Benoit Jacob]

Hi again,

Here's a quick code review of your Quaternion class. Secret hope:
convince you that maintaining your own Quaternion is a bad idea.

******
#define quatNorm (v[Q_W] * v[Q_W] + v[Q_X] * v[Q_X] + v[Q_Y] * v[Q_Y]
+ v[Q_Z] * v[Q_Z])

if you used eigen you could do v.squaredNorm() and enjoy SSE or
AltiVec vectorization!

******
void Quaternion::normalize()
{
    scalar( 1.0 / sqrt(quatNorm) );
}

same remark (vectorization).

******
void Quaternion::scalar(qreal mult)
{
    v[Q_W] *= mult;
    v[Q_X] *= mult;
    v[Q_Y] *= mult;
    v[Q_Z] *= mult;
}

same remark (vectorization) plus, the naming of this method feels
strange, did you mean scale() ? We don't have this as a method
currently in Eigen's Quaternion but we could overload operator*= if
you like it; or the current Eigen lets you do: quaternion.coeffs() *=
mult;

*****
bool Quaternion::operator==(const Quaternion &q) const
{

    return ( v[Q_W] == q.v[Q_W]
         && v[Q_X] == q.v[Q_X]
         && v[Q_Y] == q.v[Q_Y]
         && v[Q_Z] == q.v[Q_Z] );
}

Your operator== doesn't work, basically it will very often return
false when it should return true, for floating point you really need
fuzzy compares. Eigen offers a isApprox() method also in Quaternion.

******
Quaternion Quaternion::operator*(const Quaternion &q) const
{
    qreal  w, x, y, z;

    w = v[Q_W] * q.v[Q_W] - v[Q_X] * q.v[Q_X] - v[Q_Y] * q.v[Q_Y] -
v[Q_Z] * q.v[Q_Z];
    x = v[Q_W] * q.v[Q_X] + v[Q_X] * q.v[Q_W] + v[Q_Y] * q.v[Q_Z] -
v[Q_Z] * q.v[Q_Y];
    y = v[Q_W] * q.v[Q_Y] - v[Q_X] * q.v[Q_Z] + v[Q_Y] * q.v[Q_W] +
v[Q_Z] * q.v[Q_X];
    z = v[Q_W] * q.v[Q_Z] + v[Q_X] * q.v[Q_Y] - v[Q_Y] * q.v[Q_X] +
v[Q_Z] * q.v[Q_W];
    return Quaternion( w, x, y, z );
}

same for us: we also haven't vectorized that as that would be a bit of
work. Still, if one does this work, it's worth sharing "upsteam" in a
lib like eigen, no? Plus, Eigen has the infrastructure to allow doing
the work once and running on both SSE + AltiVec (+ any future
platform).

(same for other functions)

Cheers,
Benoit
2009/1/14 Benoit Jacob <jacob.benoit.1 at gmail.com>:
> Hi Marble,
>
> (please keep me in To: field as i'm not subscribed)
>
> These days the kde 4.3 development starts. Meanwhile, we're going to
> release Eigen 2.0 very soon, and from this point on we'll stick to API
> stability except for parts that we call "experimental". So if any part
> of our API needs to be reconsidered, it's extremely useful for us to
> learn that now. After that we can add new API and modify
> "experimental" API, and we'll make a 2.1 release for KDE 4.3.
>
> So my questions are:
> - do you have plans to use Eigen? I ask because you have e.g. a
> Quaternion implementation... if our Quaternion is missing stuff you
> need we sure can get it in.
> - I don't know if Marble has more potential use cases for Eigen -- if
> this is all, then i could understand that you think it's not worth
> using eigen, although it's always good to have one class less to
> maintain.
> - if you have potential plans, any suggestions for the Eigen API?
>
> Cheers,
> Benoit
>