Many math_funcs.h float-related functions actually promotes floats to doubles

[Arech]

Godot version: 3.2.1.stable (seems that occurs in current master branch also)

Issue description: Hi all. I might be wrong with that (then just close the issue), but I thought generally the whole point in switching to float related calculations instead of using double is a need for speed. For some occasion I've peered into core/math/math_funcs.h and found that many float-related functions actually promotes an argument to double when interacting with some hand-coded constants. For example, here's a Math::round() function implementation:

static _ALWAYS_INLINE_ float round(float p_val) {
    return (p_val >= 0) ? Math::floor(p_val + 0.5) : -Math::floor(-p_val + 0.5);
}

With this implementation, positive float p_val is converted to double first, added to double(0.5) and then resulting double is used to call double floor(double p_x), not the (probably) intended proper float floor(float p_x).

Not a very big deal for a result in most use-cases probably, but... well, why bother with all the floats then at all, if not to shave some more performance for free?

Almost every function that uses hand-coded constants are suffering from the described issue of unnecessary (I think) float-to-double promotion...

---

I could have provided a fix for the issue, but unfortunately, I'm low on funding now and have no free time for side projects, so just trying to draw some attention... Cheers!

[charliewhitfield]

See #288. It's currently Milestone 4.0 to fully integrate double precision. My astronomy project @ivoyager would die an ugly death if existing double support were removed.

[bojidar-bg]

Checked with GodBolt: https://godbolt.org/z/sA-5bu -- seems like even with -O3 -ffast-math, both GCC and Clang use Math::floor(double) when calling Math::round(float), indeed.

[Arech]

hey, @charliewhitfield, eerm... I guess, I was actually talking about entirely different thing, - if one is using floats, he's likely doing it because floats work much faster than double's. Implementing float Math::round(float) in a way it is today seems to have no point - it just wastes computing cycles and nothing more. In most cases additional precision of double won't make any difference in rounded result. So why waste cycles / i.e. introduce artificial slowdown then?

[charliewhitfield]

OK, I'm a dummy. Never mind.

[Arech]

@bojidar-bg thanks for confirmation.

It's a language core rules. Floating point constants by default are doubles and in computations arguments are promoted to a highest precision type (double in that case). To make constants floats they must have f suffix attached, i.e. not a 0.5, but 0.5f...

BTW - it is actually a very good idea to raise a compiler warning level to a highest possible value and audit the code (dropping only the most annoying and useless warnings manually if there are any). I guess, every major compiler would warn in our case that the result of double Math::floor() are going to be truncated to suit return type of float Math::round()... I understand that it (raising compilers warning level) would be a nightmare for such a huge project as Godot (because it seems it was never done before), but (I think) it is definitely should be done, it helps very much to pin-point a tremendous set of issues with the code...

[Arech]

If I'm allowed to place suggestions, I would make the following single template instead of multiple function definitions and forget about the issue entirely.

template<typename T>
static T round(T p_val) {
    static_assert(::std::is_floating_point<T>::value, "Hey, only floating point T is allowed here!");
    return (p_val >= 0) ? Math::floor(p_val + T(0.5)) : -Math::floor(-p_val + T(0.5));
}

or

template<typename T>
static ::std::enable_if_t<::std::is_floating_point<T>::value, T> round(T p_val) {
    return (p_val >= 0) ? Math::floor(p_val + T(0.5)) : -Math::floor(-p_val + T(0.5));
}

to make the function defined only for floating point types.

[akien-mga]

I understand that it (raising compilers warning level) would be a nightmare for such a huge project as Godot (because it seems it was never done before), but (I think) it is definitely should be done, it helps very much to pin-point a tremendous set of issues with the code...

As a matter of fact, we do use the highest level of warning (-Wall -Wextra and some more enabled manually in SConstruct) with GCC and Clang on our CI builds (and most devs do locally): https://github.com/godotengine/godot/blob/master/.travis.yml#L34

For Windows, we use all, which is our alias for /W3.

We do however disable truncation warnings on MSVC as the vast majority of them is not relevant in our experience: https://github.com/godotengine/godot/blob/master/SConstruct#L406 But any that can be fixed without adding unnecessary complexity would of course be welcome.