Skip to content
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

Fix handling of small magnitude quaternions #1022

Merged

Conversation

qsantos
Copy link

@qsantos qsantos commented Jul 13, 2020

Hello again! I contributed to the project last year in #946. I have finally updated the version of glm in my project, and I noticed that there is still actually something afoul with small-angle quaternions.

The special case handling in glm::qua::pow is specifically intended for the case when the magnitude is null. Even for magnitudes smaller than epsilon, the common formula should be used. Because comparing a floating-point value by equality triggers a warning through the -Weveverything setting of clang, it must be selectively disabled for the condition.

I initially did so in a921b51 but, as you can see, the pipeline failed because of -Wfloat-equal, and I fixed it too quickly by using a comparison to epsilon like in other parts of the project.

In passing, glm::epsilon is defined in ext/scalar_constants.inl as std::numeric_limits<T>::epsilon. According to cppreference.com, it “returns the difference between 1.0 and the next representable value of the given floating-point type”. But, for small values of x, there are many floating-point values between x and x + epsilon. Dawson's series on floating-point numbers seems to agree with me. Also, for values of x larger than 2.0, comparing to epsilon is the same as simple equality. So it is possible that other floating-point discrepancies have crept up somewhere else due to comparing to epsilon.

Tell me if this fix is acceptable (for information, I am actually using the version with if (VectorMagnitude == 0) { if (< std::numeric_limits<T>::min()) { in my project, and it works perfectly).

@qsantos qsantos force-pushed the fix-small-angle-quaternion-zero-test branch from b559576 to 15790fa Compare July 28, 2020 17:15
The special case handling in glm::qua::pow is specifically intended for
the case when the magnitude is null. Even for magnitudes smaller than
epsilon, the common formula should be used. Because comparing a
floating-point value by equality triggers a warning through the
-Weveverything setting of clang, it must be selectively disabled for the
condition.
@qsantos qsantos force-pushed the fix-small-angle-quaternion-zero-test branch from 15790fa to d6d272e Compare July 28, 2020 17:22
@qsantos
Copy link
Author

qsantos commented Jul 28, 2020

I finally took the time of fixing the pipeline for this PR.

@Groovounet Groovounet merged commit b3f8772 into g-truc:master Aug 30, 2020
@Groovounet
Copy link
Member

Thanks for contributing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants