Skip to content

Conversation

@EZForever
Copy link

@EZForever EZForever commented Oct 28, 2020

While #946 fixes precision loss on small angles in various functions, the way patch is implemented for glm::angle() introduces a mathematical error.

https://github.com/g-truc/glm/pull/946/files#diff-e2a0bd93372fb1306bb3caaf8fb8ed34089fca0ebd5356d4e6306eac0244fa31R10

For angles in range (-1, 1) (in radians), glm::angle() use asin() on non-scale components to prevent precision loss. However, the way it extracts sin(theta/2) from the non-scale components - sqrt(x*x+y*y+z*z) - effectively takes the absloute value, thus the sign of result is lost.

This error affects angles in range (2*pi-1, 2*pi). Proof of concept (included in this PR):

glm::vec3 my_axis(0.0f, 1.0f, 0.0f);

glm::quat q1 = glm::angleAxis(1.0f, my_axis);
std::cout << q1.x << ", " << q1.y << ", " << q1.z << ", " << q1.w << std::endl; // 0, 0.479425, 0, -0.877583
std::cout << glm::angle(q1)  << std::endl; // 1 (correct)

glm::quat q2 = glm::angleAxis(glm::two_pi<float>() - 1.0f, my_axis);
std::cout << q2.x << ", " << q2.y << ", " << q2.z << ", " << q2.w << std::endl; // 0, 0.479426, 0, 0.877583
std::cout << glm::angle(q2)  << std::endl; // 1 (WRONG; should be 5.28319 or -1)

A simple fix will be take the sign of w into consideration, i.e. sign(w) * sqrt(x*x+y*y+z*z), but it will break continuity in angle calculations by introducing negative angles (glm::angleAxis(0.3f, my_axis) * glm::angleAxis(5.28f, my_axis) == glm::angleAxis(-0.973186f, my_axis), albeit equivalent in terms of rotations). Working on a better fix.

@EZForever EZForever changed the title [WIP] fix: glm::angle() discards the sign of result for angles in range (2*pi-1, 2*pi) fix: glm::angle() discards the sign of result for angles in range (2*pi-1, 2*pi) Oct 29, 2020
@EZForever EZForever marked this pull request as ready for review October 29, 2020 14:06
@Groovounet Groovounet self-assigned this Nov 9, 2020
@Groovounet Groovounet added the bug label Nov 9, 2020
@Groovounet Groovounet added this to the GLM 0.9.9 milestone Nov 9, 2020
@christophe-lunarg
Copy link

Thanks a lot for the detail explanation ! For what I understand, and I can't tell I dig further than your input, your change make thanks. Let's hope it doesn't break things !

Thanks for contributing,
Christophe

@qsantos
Copy link

qsantos commented Jan 14, 2021

Author of #946 here. I don't know how I did not encounter this problem, but this is a nice find! Thanks for the fix!

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants