libglui / glui

GLUI is a GLUT-based C++ user interface library which provides controls such as buttons, checkboxes, radio buttons, and spinners to OpenGL applications. It is window-system independent, using GLUT or FreeGLUT.
Other
196 stars 81 forks source link

Quaternion multiply operator is broken #22

Closed josch closed 5 years ago

josch commented 9 years ago

https://sourceforge.net/p/glui/bugs/29/

The quaternion multiply operator, implemented in quaternion.cpp, is wrongly implemented and will usually return the wrong result. It currently reads:

return quat( a.sb.s - a.vb.v, a.sb.v + b.sa.v + a.v^b.v )

This looks correct but unfortunately the precedence of the '^' operator, overloaded here to mean vector cross product, is lower than the '+' operator. Hence the above is actually parsed as

return quat( a.sb.s - a.vb.v, a.sb.v + (b.sa.v + a.v) ^ b.v )

which is completely different. Since the multiply operator is used to combine rotations, and rotations are the reason for using quaternion, any use of the quaternion class is pretty much doomed at present.

Since the arcball class uses the quaternion class, you might wonder how the arcball class can actually work in the presence of the above bug. The answer is that the arcball class only appears to use quaternions. In fact, the rotation calculations are done directly on rotation matrices, and the use of quaternions is largely spurious.

m-7761 commented 5 years ago

I just lost what felt like an hour hunting this down... WHAT WAS THE REASON IT WASN'T FIXED???? MADNESS.

The normalize method is also not implemented nor used, and I think this is mainly why the Arcball rolls over on the Z axis and cannot be returned upright. WTH is wrong GLUI. IT's the saddest little software in the world. Just fix this stuff. It gets a gold medal for being completely mismanaged.

nigels-com commented 5 years ago

gcc 7.4 picked up on the precendence problem for quaternion product.

The good news is a fix for that is in flight: https://github.com/libglui/glui/pull/118

As for normalize, perhaps just remove that, unless it is needed in future?

nigels-com commented 5 years ago

Fixed, closing.