Probably incorrect implementation of Matrix3::ToEulerAnglesZYX Topic is solved

[bishopnator]

Hi, I think the function wrongly extracts (or rather exchanges) yaw/pitch angles. According to the comment in the function:

Code: Select all

        // rot =  cy*cz           cz*sx*sy-cx*sz  cx*cz*sy+sx*sz
        //        cy*sz           cx*cz+sx*sy*sz -cz*sx+cx*sy*sz
        //       -sy              cy*sx           cx*cy

    rfPAngle = Math::ASin( -m[2][0] );

The value stored in m[2][0] is a sinus of yaw angle and not pitch. Note that the comment seems to be written correctly according to this link.

I support the problem is also in ogre 1.x branch, but I have source code checkout only of 2.x branch, I am reporting it here.

[paroj]

this yields correct results:

Code: Select all

import Ogre

m = Ogre.Matrix3()
x, y, z = Ogre.Radian(0), Ogre.Radian(0), Ogre.Radian(0)

m.FromEulerAnglesXYZ(0, 0, Ogre.Math.HALF_PI)
m.ToEulerAnglesXYZ(x, y, z)
print(x, y, z)
m.ToEulerAnglesZYX(z, y, x)
print(x, y, z)

m.FromEulerAnglesXYZ(0, Ogre.Math.HALF_PI, 0)
m.ToEulerAnglesXYZ(x, y, z)
print(x, y, z)
m.ToEulerAnglesZYX(z, y, x)
print(x, y, z)

m.FromEulerAnglesXYZ(Ogre.Math.HALF_PI, 0, 0)
m.ToEulerAnglesXYZ(x, y, z)
print(x, y, z)
m.ToEulerAnglesZYX(z, y, x)
print(x, y, z)

[bishopnator]

All the signatures of the functions accepts the angles as rfYAngle, rfPAngle, rfRAngle. When I look on your usage of the functions, then you don't care how they are named. For me, it means that I have to pass the angles always in the same order. Try to rewrite your example using fYAngle, fPAngle, fRAngle as variable names instead of x, y, z and pass the references according to the current signatures of the functions.

Why all the functions accepts the angles named and ordered the same? Is there any reason for that? I would keep the names, but update the order. Then your usage in the code snippet can be justified.

Also in the header there is written a definition for yaw, pitch, roll rotations - the definition is correct, however it is up to the application, how the objects are oriented. In most of the applications, the right vector is the x-axis (and some apps use z-axis are direction axis or y-axis as direction axis). So we can assume, that pitch angle is the rotation around the x-axis. However the pitch angle is always as the 2nd parameter. It is really confusing. I would rather have there input as you used in your example code x, y, z and have the input in the correct order as stated by the function name. Current approach is very misleading.

[paroj]

likely those functions were just copy/ pasted without too much thought put into them.

I would just rename the function parameters to reflect the way the functions currently work, as we dont want to break any existing code.

This way the second argument becomes "y" which is yaw in ogre coordinates and thus matches your analysis.

[bishopnator]

Yes, exactly. Renaming should do the job - however there is a plenty of renaming (function signature + update the implementation).

[paroj]

https://github.com/OGRECave/ogre/pull/3441/files