fixing ambiguous btQuaternion negation operator

[GoogleCodeExporter]

unary btQuaternion::operator- was defined as both class member and external 
function, which leads to ambiguous selection if users simply specify -q.

This patch removes the function version, updates documentation for the class 
member version, and simplifies some usage where operator-(btQuaternion) was 
being explicitly called.

Regarding simplified usage cases: acos() only returns positive values (no need 
for negative check that doesn't even do anything when detected), and also can 
preemptively check w() to predict angle instead of recalculating angle 
afterward.  (Only get angles greater than π if w is negative, so just negate 
to keep w positive... in other words, flip axis and angle to keep angle between 
0-π.

Original issue reported on code.google.com by ejtt...@gmail.com on 9 Sep 2011 at 4:33

Attachments:

• fix-btQuaternion-operator-negate.diff

[GoogleCodeExporter]

I'm currently trying to focus on Bullet 3.x, and rather not break the Bullet 
2.x API whenever possible.

I'll keep it in mind for Bullet 3.x though.

Original comment by erwin.coumans on 12 Sep 2011 at 11:00

[GoogleCodeExporter]

Actually, the problem is 2.x API is currently broken... if you try to do a 
simple '-q', the compiler replies:

error: ISO C++ says that these are ambiguous, even though the worst conversion 
for the first is better than the worst conversion for the second:
/Library/Frameworks/LinearMath.framework/Headers/btQuaternion.h:320: note: 
candidate 1: btQuaternion operator-(const btQuaternion&)
/Library/Frameworks/LinearMath.framework/Headers/btQuaternion.h:248: note: 
candidate 2: btQuaternion btQuaternion::operator-() const

If you prefer to keep the global operator-(const btQuaternion&) and drop the 
btQuaternion::operator-() instead, that would also fix the problem and still 
retain the explicit 'operator-(q)' usage which appeared in two places.  I've 
attached a new version of the patch that does this (and still includes the 
simplifications where operator-(q) happened to be used, can always revert those 
files if you want)

Also I should note I made couple a tweaks to btQuaternion::getAxis(): one does 
x*x instead of btPow(x,2) (should be much faster), and using 1/s in order to 
multiply by the normalizing factor (probably would be optimized as such anyway, 
but that'll make it run faster in debug builds).

Original comment by ejtt...@gmail.com on 13 Sep 2011 at 12:39

Attachments:

• fix-btQuaternion-operator-negate2.diff

[GoogleCodeExporter]

Good point, it has been fixed in the latest trunk:

http://code.google.com/p/bullet/source/detail?r=2422

Original comment by erwin.coumans on 13 Sep 2011 at 11:37

• Changed state: Fixed

[GoogleCodeExporter]

fyi, I'm not sure why you're keeping these wtf checks in there... I guess so 
you can set a breakpoint on the wtf assignment?  Seems like an assert is more 
appropriate so you'd be alerted if it started happening when you weren't 
already looking for it? *shrug*

Index: src/BulletDynamics/ConstraintSolver/btConeTwistConstraint.cpp
===================================================================
--- src/BulletDynamics/ConstraintSolver/btConeTwistConstraint.cpp   (revision 
2424)
+++ src/BulletDynamics/ConstraintSolver/btConeTwistConstraint.cpp   (working copy)
@@ -921,11 +921,6 @@
        qMinTwist = -(qTwist);
        twistAngle = qMinTwist.getAngle();
    }
-   if (twistAngle < 0)
-   {
-       // this should never happen
-       int wtf = 0; wtf = wtf;         
-   }

    vTwistAxis = btVector3(qMinTwist.x(), qMinTwist.y(), qMinTwist.z());
    if (twistAngle > SIMD_EPSILON)

Original comment by ejtt...@gmail.com on 14 Sep 2011 at 2:33