eLEcTRiCZiTy / libfixmath

libfixmath
0 stars 0 forks source link

Incorrect conversions from int to float and more #18

Closed GoogleCodeExporter closed 8 years ago

GoogleCodeExporter commented 8 years ago
Your library seems to assume that if I convert a fix16 to a float then I 
originally assigned a float to it. However this is not the case. For example:

Fix16 fixedValue = 32767;
32767.0f != (float)fixedValue

(float)fixed is actually 0.499985

Here are my tests and results (ignore the ms; I had a break point set):

[----------] 9 tests from Fix16Conversion
[ RUN      ] Fix16Conversion.Integer
[       OK ] Fix16Conversion.Integer (0 ms)
[ RUN      ] Fix16Conversion.Float
[       OK ] Fix16Conversion.Float (0 ms)
[ RUN      ] Fix16Conversion.Double
[       OK ] Fix16Conversion.Double (0 ms)
[ RUN      ] Fix16Conversion.IntegerToFloat
Failure
Value of: (float)fixedValue
  Actual: 0.499985
Expected: 32767.0f
Which is: 32767
[  FAILED  ] Fix16Conversion.IntegerToFloat (84874 ms)
[ RUN      ] Fix16Conversion.IntegerToDouble
Failure
Value of: (double)fixedValue
  Actual: 0.499985
Expected: 32767.0
Which is: 32767
[  FAILED  ] Fix16Conversion.IntegerToDouble (0 ms)
[ RUN      ] Fix16Conversion.FloatToInteger
[       OK ] Fix16Conversion.FloatToInteger (0 ms)
[ RUN      ] Fix16Conversion.FloatToDouble
[       OK ] Fix16Conversion.FloatToDouble (0 ms)
[ RUN      ] Fix16Conversion.DoubleToInteger
Failure
Value of: (int)fixedValue
  Actual: 2147418112
Expected: 32767
[  FAILED  ] Fix16Conversion.DoubleToInteger (0 ms)
[ RUN      ] Fix16Conversion.DoubleToFloat
[       OK ] Fix16Conversion.DoubleToFloat (0 ms)
[----------] 9 tests from Fix16Conversion (84875 ms total)

[==========] 10 tests from 2 test cases ran. (84875 ms total)
[  PASSED  ] 7 tests.
[  FAILED  ] 3 tests, listed below:
[  FAILED  ] Fix16Conversion.IntegerToFloat
[  FAILED  ] Fix16Conversion.IntegerToDouble
[  FAILED  ] Fix16Conversion.DoubleToInteger

 3 FAILED TESTS

Code for the tests:
TEST(Fix16Conversion, Integer) {
    Fix16 fixedValue = 2400982;
    EXPECT_EQ(2400982, (int)fixedValue);
}

TEST(Fix16Conversion, Float) {
    Fix16 fixedValue = 10.0f;
    EXPECT_EQ(10.0f, (float)fixedValue);
}

TEST(Fix16Conversion, Double) {
    Fix16 fixedValue = 32767.0;
    EXPECT_EQ(32767.0, (double)fixedValue);
}

TEST(Fix16Conversion, IntegerToFloat) {
    Fix16 fixedValue = 32767;
    EXPECT_EQ(32767.0f, (float)fixedValue);
}

TEST(Fix16Conversion, IntegerToDouble) {
    Fix16 fixedValue = 32767;
    EXPECT_EQ(32767.0, (double)fixedValue);
}

TEST(Fix16Conversion, FloatToInteger) {
    Fix16 fixedValue = 10.0f;
    EXPECT_EQ(10.0f, (float)fixedValue);
}

TEST(Fix16Conversion, FloatToDouble) {
    Fix16 fixedValue = 10.0f;
    EXPECT_EQ(10.0, (double)fixedValue);
}

TEST(Fix16Conversion, DoubleToInteger) {
    Fix16 fixedValue = 32767.0;
    EXPECT_EQ(32767, (int)fixedValue);
}

TEST(Fix16Conversion, DoubleToFloat) {
    Fix16 fixedValue = 32767.0;
    EXPECT_EQ(32767.0f, (float)fixedValue);
}

Suspected bugged code:
static inline fix16_t fix16_from_int(int a) { return a * fix16_one; }
static inline float fix16_to_float(fix16_t a) { return (float)a / fix16_one; }
static inline double fix16_to_dbl(fix16_t a) { return (double)a / fix16_one; }

What version of the product are you using? On what operating system?
Latest, mac book pro june 2007, Mountain Lion

Original issue reported on code.google.com by iza...@gmail.com on 8 Jan 2013 at 3:14

GoogleCodeExporter commented 8 years ago
Hey, this isn't a bug. When you assign integers to a fixed, it's giving it the 
literal binary representation. It's not particularly pretty but C/C++ are 
pretty limited in this regard, there's no way to add in our own number parser 
to correctly parse a fixed point number and the only way to accurately 
initialize is an int.

I think there's an issue ticket somewhere to make this more clear. If you want 
to initialize a number as a float then you'll have to use the conversion 
functions.

The integer value 32767 is actually 0.5 in Q16.

Original comment by Flatmush@googlemail.com on 8 Jan 2013 at 3:23

GoogleCodeExporter commented 8 years ago
Thanks for the quick reply!

Original comment by iza...@gmail.com on 8 Jan 2013 at 3:29

GoogleCodeExporter commented 8 years ago
Yeah, to clear things up for anyone else who might read this bug report later, 
you need to do it like this:

fix16_t value1 = fix16_from_int(123);
float value2 = fix16_to_float(value1);

For declaring constant values, you can also use shorthand macro F16(), like 
this:

fix16_t value3 = F16(1234.53); // Works for int, float, double arguments

Original comment by Petteri.Aimonen on 8 Jan 2013 at 3:33

GoogleCodeExporter commented 8 years ago
I would also like to point out that the F16 macro is not part of the code thats 
in the downloads section as of 1/8/13 but it is in the latest commit of the 
repository.

Original comment by iza...@gmail.com on 8 Jan 2013 at 3:48