New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Euler to Quaternion conversion is incorrect #164

Closed
MusicMonkey5555 opened this Issue Dec 7, 2012 · 6 comments

Comments

Projects
None yet
4 participants
@MusicMonkey5555
Contributor

MusicMonkey5555 commented Dec 7, 2012

It seems that using the set method for the QuatF object and passing it an EulerF it incorrectly converts. Somewhere an axis ends up being negative when it was positive. This doesn't happen when passing it an angle axis.

I didn't really change the existing code to fix it, because I didn't have time to look at it real close, but instead commented it out and just wrote my own based on(http://www.euclideanspace.com/maths/geometry/rotations/conversions/eulerToQuaternion/index.htm):
// Assuming the angles are in radians.
F32 c1 = mCos(e.y/2.0f);
F32 s1 = mSin(e.y/2.0f);
F32 c2 = mCos(e.z/2.0f);
F32 s2 = mSin(e.z/2.0f);
F32 c3 = mCos(e.x/2.0f);
F32 s3 = mSin(e.x/2.0f);
F32 c1c2 = c1_c2;
F32 s1s2 = s1_s2;
w =c1c2_c3 - s1s2_s3;
x =c1c2_s3 + s1s2_c3;
y =s1_c2_c3 + c1_s2_s3;
z =c1_s2_c3 - s1_c2_s3;

That should fix it. Also I don't don't if there was originally a QuatD class, but I made one so if there is change it respectively there as well.

Once I figure out a workflow for submitting fixes on here I will start making these changes myself, but don't really have the time at work yet.

Also if you want something to test with just use this:
EulerF eRot(0.0f, -0.0f, 1.5707963267948966f);
QuatD rot(eRot);

You will notice rot has a negative 0.7071067811... which is incorrect, then if you use this:
EulerF eRot(0.0f, -0.0f, 1.5707963267948966f);
MatrixF mat(eRot);
AngAxisF aaRot(mat);
QuatD rot(aaRot);

you will get something like this: "0.00000000000000000 0.00000000000000000 0.70710678118654746 0.70710678118654757"

I was using doubles (F64) the entire time so that is why my values are huge.

@crabmusket crabmusket added this to the 3.6 milestone Mar 31, 2014

@crabmusket

This comment has been minimized.

Show comment
Hide comment
@crabmusket

crabmusket Mar 31, 2014

Contributor

We'd welcome anybody making a pull-request with these changes! Ideally we'd have some unit tests as well ;).

Contributor

crabmusket commented Mar 31, 2014

We'd welcome anybody making a pull-request with these changes! Ideally we'd have some unit tests as well ;).

@jamesu

This comment has been minimized.

Show comment
Hide comment
@jamesu

jamesu May 18, 2014

Contributor

Just a note, the original code makes use of mSinCos which at least on x86 uses the fsincos instruction which in theory is faster than calling mSin & mCos separately.

Contributor

jamesu commented May 18, 2014

Just a note, the original code makes use of mSinCos which at least on x86 uses the fsincos instruction which in theory is faster than calling mSin & mCos separately.

@Azaezel

This comment has been minimized.

Show comment
Hide comment
@Azaezel

Azaezel May 18, 2014

Contributor

If I'm reading it right, translating it to stock would give:

QuatF& QuatF::set( const EulerF & e )
{
F32 cx, sx;
F32 cy, sy;
F32 cz, sz;
mSinCos( e.x * 0.5f, sx, cx ); //flip
mSinCos( e.y * 0.5f, sy, cy ); //flip
mSinCos( e.z * 0.5f, sz, cz ); //flip

// Qyaw(z) = [ (0, 0, sin z/2), cos z/2 ]
// Qpitch(x) = [ (sin x/2, 0, 0), cos x/2 ]
// Qroll(y) = [ (0, sin y/2, 0), cos y/2 ]
// this = Qresult = Qyaw_Qpitch_Qroll ZXY
//
// The code that folows is a simplification of:
// roll_=pitch;
// roll_=yaw;
// _this = roll;
F32 cycz, sysz, sycz, cysz;
cycz = cy_cz;
sysz = sy_sz;
sycz = sy_cz;
cysz = cy_sz;
w = cycz_cx - sysz_sx; //flip
x = cycz_sx + sysz_cx;
y = sycz_cx + cysz_sx; //flip
z = cysz_cx - sycz*sx;

return *this;
}

Bit beyond my personal comfort level though for throwing that out as a PR, since it's not math I'm heavily familiar with.

Contributor

Azaezel commented May 18, 2014

If I'm reading it right, translating it to stock would give:

QuatF& QuatF::set( const EulerF & e )
{
F32 cx, sx;
F32 cy, sy;
F32 cz, sz;
mSinCos( e.x * 0.5f, sx, cx ); //flip
mSinCos( e.y * 0.5f, sy, cy ); //flip
mSinCos( e.z * 0.5f, sz, cz ); //flip

// Qyaw(z) = [ (0, 0, sin z/2), cos z/2 ]
// Qpitch(x) = [ (sin x/2, 0, 0), cos x/2 ]
// Qroll(y) = [ (0, sin y/2, 0), cos y/2 ]
// this = Qresult = Qyaw_Qpitch_Qroll ZXY
//
// The code that folows is a simplification of:
// roll_=pitch;
// roll_=yaw;
// _this = roll;
F32 cycz, sysz, sycz, cysz;
cycz = cy_cz;
sysz = sy_sz;
sycz = sy_cz;
cysz = cy_sz;
w = cycz_cx - sysz_sx; //flip
x = cycz_sx + sysz_cx;
y = sycz_cx + cysz_sx; //flip
z = cysz_cx - sycz*sx;

return *this;
}

Bit beyond my personal comfort level though for throwing that out as a PR, since it's not math I'm heavily familiar with.

@crabmusket

This comment has been minimized.

Show comment
Hide comment
@crabmusket

crabmusket Jul 8, 2014

Contributor

I'm gonna work on this, and add some unit tests in my gtest-tests branch.

Contributor

crabmusket commented Jul 8, 2014

I'm gonna work on this, and add some unit tests in my gtest-tests branch.

@crabmusket

This comment has been minimized.

Show comment
Hide comment
@crabmusket

crabmusket Jul 8, 2014

Contributor

Okay, I'm writing unit tests for this using the following approach. I create an EulerF(x, 0, 0) and an AngAxisF(Point3F(1, 0, 0), x), which should represent identical rotations, right? Then I set two QuatFs from each representation. Then I check angleBetween the two quaternions.

Currently it works if I expect the angle to be less than 1e-3. Is that too large a tolerance? I mean, that's 1/1000th of a radian...

Contributor

crabmusket commented Jul 8, 2014

Okay, I'm writing unit tests for this using the following approach. I create an EulerF(x, 0, 0) and an AngAxisF(Point3F(1, 0, 0), x), which should represent identical rotations, right? Then I set two QuatFs from each representation. Then I check angleBetween the two quaternions.

Currently it works if I expect the angle to be less than 1e-3. Is that too large a tolerance? I mean, that's 1/1000th of a radian...

@crabmusket

This comment has been minimized.

Show comment
Hide comment
@crabmusket

crabmusket Oct 30, 2014

Contributor

This fix is apparently good. Will need to fix existing logic that relies on this conversion (looking at you, TurretShape).

Contributor

crabmusket commented Oct 30, 2014

This fix is apparently good. Will need to fix existing logic that relies on this conversion (looking at you, TurretShape).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment