Unnecessary precision loss in compressed animation tracks

greeble-dev commented 19 hours ago

Tested versions

Reproducible in Godot v4.4.dev [bbc54692c].

System information

Windows 10.0.19045. Compiled with MSVC.

Issue description

Compressed animation tracks are losing half of their precision due to the choice of rounding in unorm conversion. The visual impact is very small in most cases, but the fix is simple so I hope it's worth considering.

In Animation::_compress_key, the various conversions to unorm are done with int32_t(float_value * 65535.0). This rounds towards zero, so an input of 0.999999 will be encoded as 65534 and decompressed to 0.999985 (difference: \~0.000014).

Changing the conversions to int32_t((float_value * 65535.0) + 0.5f) would make them round to nearest, doubling the precision. So an input of 0.999999 would be encoded as 65535 and decompressed to 1.0 (difference: \~0.000001). In case it's relevant, this is the conversion used by most graphics APIs (e.g. DirectX).

I can make a PR if desired. Note that I've also filed a separate bug that involves the same unorm code (https://github.com/godotengine/godot/issues/99794). I will roll both fixes into the same PR unless advised otherwise.

And some bonus notes:

The unorm compress/uncompress triggers an unnecessary double <-> float conversion because the scaling constant is a double literal.
- Changing it to float is probably a negligible performance improvement, but seems like a safe change that can be folded into the other changes.
There's a few other cases of unorm conversions with rounding towards zero, including mesh vertex compression.
- I haven't tested these yet to confirm if they have the same precision loss.
- I can file these as separate issues if desired.

Steps to reproduce

I don't have a good repro as the visual difference is very subtle.

In case it's useful, https://github.com/greeble-dev/godot/commit/30a5e9afbe27a87e3a033b8053fa889765ed369d is a hacky code change I used to test a few values with the original conversion and proposed fix. If the change is applied, reimporting any compressed animation should trigger this debug output:

scene\resources\animation.cpp:4776 - Original: 1.000000 quantizes to 1.000000 (unorm 65535)
scene\resources\animation.cpp:4782 - Updated: 1.000000 quantizes to 1.000000 (unorm 65535)
scene\resources\animation.cpp:4776 - Original: 0.999999 quantizes to 0.999985 (unorm 65534)
scene\resources\animation.cpp:4782 - Updated: 0.999999 quantizes to 1.000000 (unorm 65535)
scene\resources\animation.cpp:4776 - Original: 0.000014 quantizes to 0.000000 (unorm 0)
scene\resources\animation.cpp:4782 - Updated: 0.000014 quantizes to 0.000015 (unorm 1)

Minimal reproduction project (MRP)

N/A

fire commented 18 hours ago

Are you able make a code contribution?

greeble-dev commented 18 hours ago

@fire, yes, I can make a PR.

godotengine / godot