Bug 458

Clarification: That boils down to the question what the invariant of the class Quaternion is.

Especially this comment in the docs made me wonder: "Warning: Operations interpreting the quaternion as rotation have undefined behavior if the quaternion is not normalized".
In other words, different member functions assume different class invariants, e.g.  ``toRotationMatrix`` requires a unit quaternion, while other methods don't guarantee that unity will be preserved.

A good long-term solution might be to introduce a class UnitQuaternion with a stronger invariant than Quaternion, so it can compute members such as ``toRotationMatrix``slightly more efficient (without violating the programming by contract principle).

Yes. I'd suggest concentrating the meta-discussion about Unit and Non-Unit quaternions at Bug 560 (which blocks other bugs related to that).
For extracting the rotational part of non-unit quaternions your patch is the right solution, it would however cost an unnecessary and (comparatively) expensive division for people who already guarantee unity of their quaternions.

I prefer additional normalization by "n2".

Here is my "pros"
 - It is big performance issue to keep quaternion normalized. Any quaternion multiplication introduce scale drift, and in real usage we mast call renormalization after every operation. Especially when we model some entities updating its state (sceletal, physics, camera etc). The hardest case include conversion to matrix and back assuming the matrix is orthonormal.  So normalization anyway is required, but during conversion to matrix it is much cheaper(4mul 1div).

- Even after normalization we can have small scale drift (several epsilons), and additional normalization can remove few bits of error.

- It is very hard to recover, such kind of accuracy loss. Even mature 3d packages introduce scale drift without control. From my experience , it is one of the common error when using quaternions.

"cons"
- Lower performance
- It can hide from user that quaternions are not unit, and introduce more bugs in other math

I've benchmarked both versions with both clang and gcc, and with both compilers I observe a x2 slow-down. The impact is more significant than expected.

(In reply to Gael Guennebaud from comment #4)
> I've benchmarked both versions with both clang and gcc, and with both
> compilers I observe a x2 slow-down. The impact is more significant than
> expected.

Hmm... this is very surprising to me. With such a slow-down, im not sure it is way to do. 

Then it is better to wait next versions with unit and nonunit policies.

If the division is the reason for the x2 slowdown, a compromise might be to do a taylor-based fast normalization if the norm is close to 1, i.e., (pseudo code):

  if(abs(n2-1)<sqrt(epsilon))
    two_by_squarednorm = 2*(2-x);
  else
    two_by_squarednorm = 2/x;

and hope that branch prediction always chooses the first.

We could also consider providing a normalizeFast() method (also for arbitrary vectors), which multiplies by 0.5*(3.0-squaredNorm()), if squaredNorm() is close enough to 1.0

(In reply to Christoph Hertzberg from comment #6)
> We could also consider providing a normalizeFast() method (also for
> arbitrary vectors), which multiplies by 0.5*(3.0-squaredNorm()), if
> squaredNorm() is close enough to 1.0

Method can be very usable on practice, but i propose to use other name. for example "stabilizeUnity" etc. Cause normalizeFast() usualy mean some tricks with inverse sqrt.

Nope, most of the slowdown comes from squaredNorm even though it is perfectly vectorized.

-- GitLab Migration Automatic Message --

This bug has been migrated to gitlab.com's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.com/libeigen/eigen/issues/458.