bitshifter / glam-rs

A simple and fast linear algebra library for games and graphics
Apache License 2.0
1.46k stars 145 forks source link

Quat / Euler tests fail on Apple Silicon #506

Closed waywardmonkeys closed 2 weeks ago

waywardmonkeys commented 3 months ago

When running the tests on main on an M3 laptop:

     Running tests/euler.rs (target/debug/deps/euler-9c54ea15900fa0aa)

running 12 tests
test euler::quat::test_euler_xyz ... FAILED
test euler::quat::test_euler_xzy ... FAILED
test euler::quat::test_euler_yxz ... FAILED
test euler::quat::test_euler_yzx ... FAILED
test euler::quat::test_euler_zyx ... FAILED
test euler::quat::test_euler_zxy ... FAILED
test euler::dquat::test_euler_xyz ... ok
test euler::dquat::test_euler_xzy ... ok
test euler::dquat::test_euler_yxz ... ok
test euler::dquat::test_euler_yzx ... ok
test euler::dquat::test_euler_zxy ... ok
test euler::dquat::test_euler_zyx ... ok

failures:

---- euler::quat::test_euler_xyz stdout ----
thread 'euler::quat::test_euler_xyz' panicked at tests/euler.rs:113:9:
assertion failed: `(left !== right)` (left: `Quat(-0.65328145, -0.27059802, 0.6532815, 0.27059805)`, right: `Quat(-0.6533943, -0.27055135, 0.65316874, 0.27064478)`, expect diff: `1e-5`, real diff: `Quat(0.00011283159, 4.6670437e-5, 0.00011277199, 4.673004e-5)`), additional context: angles (-180, -90, -45) -> (-135.0, -89.98022, 0.0)

---- euler::quat::test_euler_xzy stdout ----
thread 'euler::quat::test_euler_xzy' panicked at tests/euler.rs:113:9:
assertion failed: `(left !== right)` (left: `Quat(0.1830127, 0.18301271, -0.68301266, 0.6830127)`, right: `Quat(0.18304437, 0.18298118, -0.6828948, 0.6831306)`, expect diff: `1e-5`, real diff: `Quat(3.167987e-5, 3.1530857e-5, 0.00011783838, 0.00011789799)`), additional context: angles (-180, -90, -150) -> (-330.0, -89.98022, 0.0)

---- euler::quat::test_euler_yxz stdout ----
thread 'euler::quat::test_euler_yxz' panicked at tests/euler.rs:113:9:
assertion failed: `(left !== right)` (left: `Quat(-0.68301266, 0.1830127, 0.18301271, 0.6830127)`, right: `Quat(-0.6828948, 0.18304437, 0.18298118, 0.6831306)`, expect diff: `1e-5`, real diff: `Quat(0.00011783838, 3.167987e-5, 3.1530857e-5, 0.00011789799)`), additional context: angles (-180, -90, -150) -> (-330.0, -89.98022, 0.0)
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

---- euler::quat::test_euler_yzx stdout ----
thread 'euler::quat::test_euler_yzx' panicked at tests/euler.rs:113:9:
assertion failed: `(left !== right)` (left: `Quat(0.6532815, -0.65328145, -0.27059802, 0.27059805)`, right: `Quat(0.65316874, -0.6533943, -0.27055135, 0.27064478)`, expect diff: `1e-5`, real diff: `Quat(0.00011277199, 0.00011283159, 4.6670437e-5, 4.673004e-5)`), additional context: angles (-180, -90, -45) -> (-135.0, -89.98022, 0.0)

---- euler::quat::test_euler_zyx stdout ----
thread 'euler::quat::test_euler_zyx' panicked at tests/euler.rs:113:9:
assertion failed: `(left !== right)` (left: `Quat(0.18301271, -0.68301266, 0.1830127, 0.6830127)`, right: `Quat(0.18298118, -0.6828948, 0.18304437, 0.6831306)`, expect diff: `1e-5`, real diff: `Quat(3.1530857e-5, 0.00011783838, 3.167987e-5, 0.00011789799)`), additional context: angles (-180, -90, -150) -> (-330.0, -89.98022, 0.0)

---- euler::quat::test_euler_zxy stdout ----
thread 'euler::quat::test_euler_zxy' panicked at tests/euler.rs:113:9:
assertion failed: `(left !== right)` (left: `Quat(-0.27059802, 0.6532815, -0.65328145, 0.27059805)`, right: `Quat(-0.27055135, 0.65316874, -0.6533943, 0.27064478)`, expect diff: `1e-5`, real diff: `Quat(4.6670437e-5, 0.00011277199, 0.00011283159, 4.673004e-5)`), additional context: angles (-180, -90, -45) -> (-135.0, -89.98022, 0.0)

failures:
    euler::quat::test_euler_xyz
    euler::quat::test_euler_xzy
    euler::quat::test_euler_yxz
    euler::quat::test_euler_yzx
    euler::quat::test_euler_zxy
    euler::quat::test_euler_zyx

test result: FAILED. 6 passed; 6 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.02s
waywardmonkeys commented 3 months ago

Since the macOS CI is using macos-latest, it can be either macos-12 (Intel) or macos-14 (Apple Silicon) as they are rolling out the update over some weeks.

bitshifter commented 3 months ago

I think I encountered this on the neon branch (on a Raspberry Pi 4). If you have a Mac, could you possibly try the neon branch or just this change and see if it fixes it? https://github.com/bitshifter/glam-rs/pull/379/files#diff-bfee85a55d20e3f570066acdc78e0bc38a8ec04a57198d681bdac80cad3c06f5L39-L47

waywardmonkeys commented 3 months ago

Both that one change to reduce the precision required for the tests to pass as well as using the neon branch work and pass all tests on my M3 Mac.

That's a pretty big variation in precision though ...

waywardmonkeys commented 3 months ago

Worth noting then that this also fails (on my Mac):

cargo test --no-default-features --features libm
bitshifter commented 3 months ago

Yes I get the same thing with libm on my raspberrypi, but not on x86_64. It is a bit surprising, I would have thought that libm would give the same results on different architectures.

waywardmonkeys commented 3 months ago

I don't have an x86_64 machine handy to compare on (!) at the moment, but I wonder if it is always using the CPU instructions while we have software approximations elsewhere.

The libm sources are derived from musl libc, which is what is used in Emscripten as well. The comments in the files indicate that at least parts of them come from FreeBSD, so they may well be similar to what's in the macOS standard library, but I didn't check further.

There are some very old bugs in the libm repo about needing to do tests for precision and other things. Seems like this might be a corner of the Rust world that needs some love, but well outside the scope of this.

bitshifter commented 3 months ago

On my Raspberrypi cargo test --no-default-features --features libm,scalar-math passed. This is pretty odd, on the main branch there is no neon implementation so I think the only real difference would be Quat and Vec4 are 16 byte aligned, the scalar-math feature turns that off. I will try look at the generated assembly some time.

bitshifter commented 3 months ago

Ah scalar-math lowers the precision of the test :) (I didn't write this, it was contributed, not that I remember everything I write either).

bitshifter commented 3 months ago

Given this precision is lowered in the test with scalar-math or wasm, I think probably the sse2 implementation of quat mul quat must have slightly better precision than the scalar implementation. I think that is the only part of the Euler conversion code that would be any different between x86_64 and arm. On arm it will be using the scalar code because there is no neon implementation on main. So it's probably not a libm issue either.

bitshifter commented 3 months ago

I've submitted the tolerance change so this test passes. I'm still investigating if there is anything in particular that makes scalar-math less precise than the SSE2 implementation.

bitshifter commented 2 weeks ago

Changed the implementation which improves precision #537