Rework floating point compression #23
Conversation
| @@ -90,8 +90,6 @@ void BitArray::store_bits(int p_bit_offset, uint64_t p_value, int p_bits) { | |||
|
|
|||
| val >>= bits_to_write; | |||
| } | |||
|
|
|||
| CRASH_COND_MSG(val != 0, "This is a bug, please open an issue"); | |||
There was a problem hiding this comment.
Was this been removed for mistake?
There was a problem hiding this comment.
Oh, no, I forgot that this causes issues when I sending signed values. I would have to manually zero out the unnecessary bits. I thought that the check could be removed, since we have already covered everything with tests. Let me know If I should put it back.
There was a problem hiding this comment.
Yup, that's fine, you right. The test are much better than this check.
| if (p_compression_level == DataBuffer::COMPRESSION_LEVEL_0) { | ||
| p_compression_level = DataBuffer::COMPRESSION_LEVEL_1; | ||
| } | ||
| #endif |
There was a problem hiding this comment.
Why don't put this fallback directly inside the float compressor function? I think it would make more sense anyway.
Also, I think we need to add a warning here, so the user knows that doesn't make sense use compression 0 when it's using a single precision binary.
There was a problem hiding this comment.
Because add_real does not depend on this definition. It accepts doubles. But Vector2 and Vector3 use real_t as underlying type.
You will have an incredible amount of warnings because this function gets called very often.
There was a problem hiding this comment.
Yes but it doesn't make senses anyway compress a single precision to double to convert it back to single precision.
The warning would tell the user how to properly use the api, and it would spam only when the api is not properly used. Which make sense. What do you think?
There was a problem hiding this comment.
Yes but it doesn't make senses anyway compress a single precision to double to convert it back to single precision.
It depends on what type is used. If double is used, and not real_t, then there will be no narrowing conversion.
GDScript documentation says that 64-bit floats are used and only some types have (such as Vector2) uses real_t.
The warning would tell the user how to properly use the api, and it would spam only when the api is not properly used. Which make sense. What do you think?
Hm... Okay, I will add a warning!
| @@ -90,8 +90,6 @@ void BitArray::store_bits(int p_bit_offset, uint64_t p_value, int p_bits) { | |||
|
|
|||
| val >>= bits_to_write; | |||
| } | |||
|
|
|||
| CRASH_COND_MSG(val != 0, "This is a bug, please open an issue"); | |||
There was a problem hiding this comment.
Yup, that's fine, you right. The test are much better than this check.
| if (p_compression_level == DataBuffer::COMPRESSION_LEVEL_0) { | ||
| p_compression_level = DataBuffer::COMPRESSION_LEVEL_1; | ||
| } | ||
| #endif |
This PR implements real numbers compression according to IEEE 754 floating point representation. Some corner cases not covered here (like
INF,NAN) and will be added soon.Note: I changed the type from
real_ttodouble, because the code will not work correctly with theREAL_T_IS_DOUBLEdefine.