fix quattomatrix33 row/column-major mismatch#347
Conversation
quattomatrix33 outputs column-major (comment says "opengl major") but quatfrommatrix33 reads row-major, and the 4x4 quattomatrix also outputs row-major. This means a quat->matrix33->quat roundtrip produces the conjugate (inverse rotation) instead of the original quaternion. Fix by swapping the three off-diagonal pairs so quattomatrix33 outputs row-major, consistent with the rest of the codebase. The only runtime caller is barycentric_svd.c which passes the result to cn_copy_in_row_major() — previously loading the transpose, now correct. This doesn't affect tracking results because it only needs an arbitrary valid rotation for control point initialization. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Nice find, I'm surprised this has gone unnoticed for such a long time. I double checked and you're right, there is indeed a mismatch. Thanks for the patch. |
|
I wonder if this delayed build is from an upstream bug we saw while running our tests. The bug: In survive_kalman_tracker.c:1488, integration_variance[16] is stack-allocated but variance_tracker_calc() doesn't write all elements when the tracker has zero observations (which happens in the test replay data). The uninitialized array is then read at line 1490, triggering the MSan error. How it showed up: The test_replays ctest (an existing upstream integration test that replays recorded tracking data) ran for 3+ hours under MSan before hitting it, and timed out under ASan+UBSan at the 300s ctest limit. The fix: It's a one-line fix (= {0} to zero-initialize the array). It's a stats-printing bug, not a tracking bug. In our CI we worked around it by excluding upstream replay tests (-E '.(rec|pcap).') and only running our property test suites under sanitizers. I'll submit a PR for that one shortly. |
|
Thanks |
Summary
quattomatrix33 outputs column-major (comment says "opengl major") but quatfrommatrix33 reads row-major, and the 4x4 quattomatrix also outputs row-major. The 3x3 version is the odd one out.
A quat → matrix33 → quat roundtrip produces the conjugate (inverse rotation) instead of the original quaternion. Fix: swap the three off-diagonal pairs so quattomatrix33 outputs row-major, consistent with the rest of the codebase.
Demonstration
For a 45° rotation around Z (q = [0.923880, 0, 0, 0.382683]):
quattomatrix (4x4) -- row-major:
[+0.7071 -0.7071 +0.0000]
[+0.7071 +0.7071 +0.0000]
[+0.0000 +0.0000 +1.0000]
quattomatrix33 BEFORE fix -- column-major (BUG):
[+0.7071 +0.7071 +0.0000]
[-0.7071 +0.7071 +0.0000]
[+0.0000 +0.0000 +1.0000]
^ off-diagonals are TRANSPOSED vs the 4x4 version
Roundtrip with buggy quattomatrix33:
recovered = [0.923880, 0.000000, 0.000000, -0.382683]
expected = [0.923880, 0.000000, 0.000000, 0.382683]
dot(q, recovered) = 0.707107 (should be ~1.0)
dot(conj(q), recovered) = 1.000000 <- matches conjugate, not original!
quattomatrix33 AFTER fix -- row-major:
[+0.7071 -0.7071 +0.0000]
[+0.7071 +0.7071 +0.0000]
[+0.0000 +0.0000 +1.0000]
^ now matches the 4x4 version
Roundtrip with fixed quattomatrix33:
recovered = [0.923880, 0.000000, 0.000000, 0.382683]
expected = [0.923880, 0.000000, 0.000000, 0.382683]
dot(q, recovered) = 1.000000 <- correct roundtrip!
Impact
The only runtime caller is barycentric_svd.c which passes the result to cn_copy_in_row_major(). It was loading the transpose, but this didn't affect tracking because it only needs an arbitrary valid rotation for control point initialization. After this fix it gets the intended rotation.
Change
One file, three off-diagonal swaps in redist/linmath.c:
m[1]/m[3]: xy+zw ↔ xy-zw
m[2]/m[6]: xz-yw ↔ xz+yw
m[5]/m[7]: yz+xw ↔ yz-xw
Found via property-based testing (randomized quat→matrix→quat roundtrip, 10,000 trials)