Parameters in Matrix3 euler methods

Minor issues with the Ogre API that can be trivial to fix
Post Reply
User avatar
Kojack
OGRE Moderator
OGRE Moderator
Posts: 7157
Joined: Sun Jan 25, 2004 7:35 am
Location: Brisbane, Australia
x 534

Parameters in Matrix3 euler methods

Post by Kojack »

Matrix3 has 6 methods for euler to matrix and 6 for matrix to euler.

Code: Select all

        bool ToEulerAnglesXYZ (Radian& rfYAngle, Radian& rfPAngle, Radian& rfRAngle) const;
        bool ToEulerAnglesXZY (Radian& rfYAngle, Radian& rfPAngle, Radian& rfRAngle) const;
        bool ToEulerAnglesYXZ (Radian& rfYAngle, Radian& rfPAngle, Radian& rfRAngle) const;
        bool ToEulerAnglesYZX (Radian& rfYAngle, Radian& rfPAngle, Radian& rfRAngle) const;
        bool ToEulerAnglesZXY (Radian& rfYAngle, Radian& rfPAngle, Radian& rfRAngle) const;
        bool ToEulerAnglesZYX (Radian& rfYAngle, Radian& rfPAngle, Radian& rfRAngle) const;
        void FromEulerAnglesXYZ (const Radian& fYAngle, const Radian& fPAngle, const Radian& fRAngle);
        void FromEulerAnglesXZY (const Radian& fYAngle, const Radian& fPAngle, const Radian& fRAngle);
        void FromEulerAnglesYXZ (const Radian& fYAngle, const Radian& fPAngle, const Radian& fRAngle);
        void FromEulerAnglesYZX (const Radian& fYAngle, const Radian& fPAngle, const Radian& fRAngle);
        void FromEulerAnglesZXY (const Radian& fYAngle, const Radian& fPAngle, const Radian& fRAngle);
        void FromEulerAnglesZYX (const Radian& fYAngle, const Radian& fPAngle, const Radian& fRAngle);
The problem is that all of them have the same parameter order of rfYAngle, rfPAngle, rfRAngle. It looks like that means you always give them yaw, pitch then roll and the method name decides the order to multiply them. But really the meaning of the parameters changes to match the method name. So for example FromEulerAnglesZYX treats the parameters as roll, yaw, pitch (even though they are named rfYAngle, rfPAngle, rfRAngle).

There's two ways to fix this. We can rename the parameters to represent their true meaning, such as:
void FromEulerAnglesZYX (const Radian& fRAngle, const Radian& fYAngle, const Radian& fPAngle);
(or even better change them to roll, yaw and pitch, easier to read)
Or we could change the code in the methods so you always give the parameters in the same order, then the method name chooses how to combine them (the way it looks like it should currently be working, but isn't).
Both require changes to the methods' code, but the former won't break any existing code (so is probably the best).
Post Reply