GIM_INV_SQRT breaks non IEEE platforms

[David G.F.]

Hello there!

This is a bug I fixed on an old port that I did many (10+) years ago. The GIM_INV_SQRT function uses the quake3 sqrt-inv approximation, which works on IEEE floating point platforms. This can break other less-compatible architectures (which are rare these days anyway). Maybe a good idea would be to hide this optimization behind a flag? Also these days CPUs and compilers are better and have better performance at these operations, making the optimization less relevant.

Feel free to ping me if you have questions!

[Oleh Derevenko]

As far as I remember there were a few “optimizations” like this in the code. I once removed one or two of them that I randomly came upon and they looked too dangerous from precision loss point of view to me. I would not mind removing them all but I did not do it thinking that the code may lose its initial idea behind with such a change. Also, I did not use GIMPACT and would not be able to evaluate effects of these changes anyway.

So what flag do you suggest, a runtime flag in the objects or a compilation option?

[David G.F.]

Oh if you do not mind taking them at all I’d then recommend removing them altogether in favour of more reasonable floating point math :)

[Oleh Derevenko]

But how will you know there is no related “compensation” code that expects these approximations and corrects values or adjusts comparison for them? What if changing to standard implementations will break the results? I do not have any means to test effects of such changes.

[David G.F.]

I totally understand your concerns, it’s not nice to break things indeed.

From the looks of it, and also from my previous experience using GIMPACT, this function is mostly used to normalize vectors. In fact most of the uses are “dead”, like NORM_XFORM_2X2, VEC_INV_LENGTH_2 and VEC_INV_LENGTH_4 are not being used from the looks of it. Also it is also used for regular “sqrt” so this only adds error in the calculations.

I can send a PR to clean up some of this unused code. I can also build the examples and compare before and after if that helps us building trust on the changes.

Thanks!

[Oleh Derevenko]

Alright, I’ll change any not standard macros in GIMPACT to use default operations and you are to test effects of the changes.

[Oleh Derevenko]

Alright, done it.

[Oleh Derevenko]

One of evident changes, by the way, is that the original implementation used to return infinity for arguments less than 1e-6 and hence, the square root was precise zero for anything less than 1e-6. Now, with change to standard implementations, the square root will start returning small values and that can create new execution paths for small arguments.

[David G.F.]

Hey Sorry for the delay.

I build it with GIMPACT after and before the commit. I see no obvious difference in the demos. Precision-wise it seems both are similar (almost identical). However Ode would need regression tests so we could compare this better. I used some of the most useful demos to compare and I saw no visible differences, and the errors and error rates are comparable.

Thanks for this!

[Oleh Derevenko]

There was a change I don’t know consequences of and I don’t know how to test. Before the changes GIM_SQRT()/GIM_INV_SQRT() for arguments <= 1e-7 would evaluate to 0 and Infinity. With the changes, the results could be as high as 3.16e-4 (instead of the zero) and as low as 3162.3 (instead of the infinity). This might affect collisions in some corner cases, like nearly parallel planes.

[Oleh Derevenko]

On the other hand, we can modify the macros to have the same rounding behavior for small arguments as they had before. I just doubt this should be done globally like that. Rather each individual case should have its appropriate rounding, if necessary.

[David G.F.]

Hello there,

I believe this change actually makes it more precise right? For small arguments it will return non-infinite results, right? In the previous case any result being bigger than 1000 (invsqrt(1e-7)) was mapped to infinite, which is quite a big imprecision IMO, so I think the change can only bring more precision.

[Oleh Derevenko]

This also relates to small values as well. And small values rounded to zero could mean stability versus noise. Depending on particular place...