search

czaloj / bullet

Automatically exported from code.google.com/p/bullet
0 stars 0 forks source link

btMatrix.setEulerZYX takes arguments in order X, Y, Z #157

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago 
I've had some push back from my users that this is wrong.  It is
implemented as documented however from the naming the expected order is Z Y X.

I had put in a comment when i documented that this was inconsistant with
other APIs but left it before. I've now patched it for myself.  I'd like it
to get merged in but it's not necessary.

Original issue reported on code.google.com by Tully.Foote on 2 Dec 2008 at 7:50

Attachments:

GoogleCodeExporter commented 9 years ago 

The patch is incompatible with the demos, they are all broken when applying.

Did you test Ragdoll Demo and GenericJointDemo and other demos?

Original comment by erwin.coumans on 2 Dec 2008 at 10:35

GoogleCodeExporter commented 9 years ago 
[deleted comment]
GoogleCodeExporter commented 9 years ago 
I just had a look at the setEulerZYX, and it is only the order of application 
that 
has Z,Y,X, not the argument order.

All Bullet APIs take arguments in order X,Y,Z, so we should be consistent with 
the 
argument order.

If we passed in just a single btVector3 (setEulerZYX(const btVector3& 
eulerAngles)), 
would that reduce confusion?

Can you point out a link to the discussion from your users, to learn what is 
wrong?

Original comment by erwin.coumans on 2 Dec 2008 at 10:58

GoogleCodeExporter commented 9 years ago 
Sorry for overlooking the Demos directory.  I didn't check outside of src for
compatibility.  Attached is the patch.  

I didn't update ContinuousConvexCollisionDemo since I couldn't build it and it
already used yaw, pitch, roll.  

I looked through the APIs and every other Euler angle call is in the order yaw,
pitch, roll.  Including btMatrix.setEulerYPR, btQuaternion constructor,
btMatrix.GetEulerYPR, btMatrix.getEulerZYX, 

I recieved verbal feedback from a number of users as well as this ticket
https://prdev.willowgarage.com/trac/personalrobots/ticket/623

Original comment by Tully.Foote on 4 Dec 2008 at 10:28

Attachments:

GoogleCodeExporter commented 9 years ago 

The patch still breaks the setEulerYPR/getEulerYPR roundrip, both assuming yaw 
around 
Y, pitch around X, roll around Z. See testcase TestBulletMath.zip in
http://code.google.com/p/bullet/issues/detail?id=104&can=1&q=euler

I propose a different fix: 

1) remove setEulerZYX from btQuaternion and btMatrix3x3

2) rename btQuaternion::setEuler into btQuaternion::setEulerYPR

3) only use setEulerYPR/getEulerYPR, with an optional 4rd argument, describing 
the 
order/mapping of YPR onto the XYZ axis. Default value is YPR_ORDER_YXZ.

3) This optional argument can also be applied to the quaternion constructor, so 
this BT_EULER_DEFAULT_ZYX define would become obsolete. Obviously wwe need to 
handle the 
problem that there is already a btQuaternion constructor with 4 floats.

So the input is always YPR, with a 4rd argument like:
YPR_ORDER_YXZ, YPR_ORDER_ZYZ etc.

How about that?

Original comment by erwin.coumans on 7 Dec 2008 at 4:19

GoogleCodeExporter commented 9 years ago 
I think that is a decent solution which will support both cases.  To avoid 
confusion
I think that the 4th argument shouldn't be optional though.  The define could 
be a
local static and be kept short enough to be not too much of a burden to be 
required,
such as YXZ, ZYX, etc.  

Resulting in 
setEulerYPR(y, p, r, YXZ); 
setEulerYPR(y, p, r, ZYX); 

I'm worried about not requiring the order definition for depending on which
background you come from the "default" order that no one would ever think about 
using
other order is different.  I've seen a number of people start using the wrong 
calls
as they stand now with two side by side implementations.  

Constructors
To resolve the constructor conflict an approach similar to Issue 139 could be 
used,
which would be to remove all but the default constructor and force using 
explicit
initializers to resolve the duplicity.  This will also would get rid of the 
#define
switch for the constructor.

Original comment by Tully.Foote on 10 Dec 2008 at 5:27

GoogleCodeExporter commented 9 years ago 

The tricky part of such changes is to improve the API without breaking it.

We will look into it, but removing btQuaternion constructors, and changing the 
default number of arguments to setEulerYRP sounds like a (too) big API change.

Thanks for the discussion,
Erwin

Original comment by erwin.coumans on 19 Jan 2009 at 7:57

GoogleCodeExporter commented 9 years ago 

There euler methods are a pain to support, and we likely just remove any euler 
methods from the API in a future 
revision.

Original comment by erwin.coumans on 3 Nov 2009 at 4:56