Re: [eigen] Rigid Transformations in eigen: discussion thread

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

2009/9/17 Rohit Garg <rpg.314@xxxxxxxxx>:
> What do others here think about this. Is the API, the unit tests etc.
> good enough for inclusion in eigen?

OK, indeed RigidTransform.h looks good, but:

1) please indent only with spaces, not tabs

2) absolutely need the standard comment at the top with your copyright
and the licensing blabla

3) about the name of the class, why not DualQuaternion? We already
have a Quaternion class that behaves like a transform, so it seems
that you're doign the analogue here. Of course, my hidden motivation
for avoiding the name RigidTransform is that i still think that it
should be called Isometry ;) basically there's a debate between
RigidTransform and Isometry while at least there is no debate about
DualQuaternion.

4) You implement operator+ returning by value. That raises 3 questions.
a) The sum of two rigid transformations isn't rigid anymore, so if you
keep the name RigidTransform, you can't have an operator+.
b) is there any use case, in 3D graphics, to add two dualquaternions?
c) we need to adopt a uniform policy, either we have such operator+
returning by value in both DualQuaternion and Quaternion, or in
neither.
For Quaternion, Gael showed us that the operator+ wasn't actually
slow. The limit is between size 6 and 7. So DualQuaternion::operator+
will be slow.

5) You have some room for optimization, for example in
  inline RigidTransform operator*(const AngleAxisType& aa) const
	  {
	  RigidTransform temp(aa);
	  return (*this)*temp;	
	  }
  inline RigidTransform operator*(const TranslationType& delta) const
	  {
	  RigidTransform temp(delta);
	  return (*this)*temp;	
	  }
It's not optimal to construct the temporary and then multiply, as a
simplification occurs if you do both at once.

6) You also have operator*(Scalar). Same remarks as for operator+, you
said that a scaling wasn't a rigid transformation....? etc.

***

Here's  a little idea to make it more easy for the user to do his own
operations like sum.
Currently you have two member quaternions so it's not easy for the
user, e.g. if he wants an optimized sum he must do:
dualquat.quat[0].coeffs() + dualquat.quat[0].coeffs()
dualquat.quat[1].coeffs() + dualquat.quat[1].coeffs()
etc...

Instead you could have only one data member, that would be a vector
m_coeffs of size 8. You would have a method coeffs() returning a
reference to it. So the user could do, just like with quaternions,
dualquat.coeffs() + dualquat.coeffs()

Now how do we address the individual quaternions ? You would have
member methods quat0() and quat1() returning quaternion references
obtained with the reinterpret_cast trick.

QuatType &quat0()
{
  return *reinterpret_cast<QuatType*>(&m_coeffs);
}

QuatType &quat1()
{
  return *(reinterpret_cast<QuatType*>(&m_coeffs) + 1);
}

By the way, are there more human friendly names for quat0 and quat1 ?
Or perhaps they really don't have any simple geometric interpretation?

Benoit