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

q_math: do tracing in id Software/J.M.P. van Waveren's QuatFromMatrix() implementation like Martin John Baker's one did, fix model distortion with GCC 14 #1532

Merged
merged 4 commits into from
Jan 29, 2025

Conversation

illwieckz
Copy link
Member

@illwieckz illwieckz commented Jan 28, 2025

Do tracing in id Software/J.M.P. van Waveren's QuatFromMatrix() implementation like the one we have by Martin John Baker's did, fix model distortion with GCC 14.

Fix #1527:

It adds +1 to the first trace

Fix Unvanquished/Unvanquished#3140:

What's curious is that the Martin John Baker's page says they also removed the +1:

// I removed + 1.0f; see discussion with Ethan

But it also says (but we didn't use any epsilon):

// I changed M_EPSILON to 0

The said “discussion with Ethan Tira-Thompson” says:

Hi Martin,

I was looking at the matrixToQuaternion page, and I think the "Better code" and "C++ code" sections may have a problem with numerical stability as Tr + 1 may approach 0 in some cases. The test for greater-than-epsilon helps, but a better solution may be found in the implementation from the Wild Magic package by Dave Eberly:
http://www.geometrictools.com/LibFoundation/Mathematics/Wm4Quaternion.inl
(See "FromRotationMatrix" function)

There, they compare Tr > 0 (instead of Tr+1 > 0) for the initial conditional, thus avoiding getting close to 0 in the 0.5 / sqrt(Tr + 1) which follows it.

Do you have any thoughts, does this make sense to you? The implications of falling into the 'else' clause are the main concern here, but that seems safe, and appears to work in practice.

Thanks!

Then follows a lengthy discussion about “danger of division by zero or square root of a negative number” and “danger of division by zero or square root of a negative number”, maybe it was safe to rule out those concerns with no fast math but we need to take care of this with fast math?

Actually using that +1 means one compute is twice the same, so it may even be faster.

On a side note, the document by id Software/J.M.P. van Waveren mentions this:

The above routine also uses a fast reciprocal square root approximation, referencing works like the Chris Lomont one I quoted there when improving Q_rsqrt():

So I made the code also use Q_rsqrt().

@illwieckz illwieckz changed the title q_math: do tracing in id Software/J.M.P. van Waveren's QuatFromMatrix() implementtion like Martin John Baker's one does, fix model distortion with GCC 14 q_math: do tracing in id Software/J.M.P. van Waveren's QuatFromMatrix() implemention like Martin John Baker's one does, fix model distortion with GCC 14 Jan 28, 2025
@illwieckz illwieckz changed the title q_math: do tracing in id Software/J.M.P. van Waveren's QuatFromMatrix() implemention like Martin John Baker's one does, fix model distortion with GCC 14 q_math: do tracing in id Software/J.M.P. van Waveren's QuatFromMatrix() implementation like Martin John Baker's one does, fix model distortion with GCC 14 Jan 28, 2025
@illwieckz illwieckz force-pushed the illwieckz/quat-gcc-14 branch from 1f5889b to ad68e36 Compare January 28, 2025 10:03
@illwieckz
Copy link
Member Author

It's also worth noting that the id Software document also provides some SSE implementations, it may be interesting to look at them.

@illwieckz
Copy link
Member Author

So it looks like the same bug was affecting different binaries:

  • native cgame (model bug)
  • native engine (portal bug)

@illwieckz illwieckz changed the title q_math: do tracing in id Software/J.M.P. van Waveren's QuatFromMatrix() implementation like Martin John Baker's one does, fix model distortion with GCC 14 q_math: do tracing in id Software/J.M.P. van Waveren's QuatFromMatrix() implementation like Martin John Baker's one did, fix model distortion with GCC 14 Jan 28, 2025
@illwieckz
Copy link
Member Author

So, with grangermaze map on -176 204 203 -44 38 viewpos and a native GCC 14 engine and game build, I can reproduce both bugs:

Before:

unvanquished_2025-01-28_115655_000

After:

unvanquished_2025-01-28_115739_000

@illwieckz illwieckz force-pushed the illwieckz/quat-gcc-14 branch from 718e524 to 7bc7473 Compare January 28, 2025 11:02
@slipher
Copy link
Member

slipher commented Jan 28, 2025

LGTM

I won't dive into the math here since there is really no need to be using the QuatFromMatrix function in either case... For the cgame model rotation we should just use only matrices (without the pointless round trips matrix -> quat -> matrix). The other caller, QuatFromAngles, ought to be constructing the quat directly (there is a commented-out implementation for that) instead of constructing a matrix and converting from a matrix.

@illwieckz illwieckz merged commit f3acf80 into master Jan 29, 2025
9 checks passed
@illwieckz illwieckz deleted the illwieckz/quat-gcc-14 branch January 29, 2025 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Model distortion bug with GCC 14 Visual bug on Granger Maze map - Unvanquished 0.55
2 participants