is_equal_approx works incorrectly with doubles

[lawnjelly]

Godot version:

3.x (3.3.1)
Probably also 4.x

OS/device including version:

N/A

Issue description:

is_equal_approx function works incorrectly for doubles.

Steps to reproduce:

	double d = (double) FLT_MAX + (double) FLT_MAX;
	double d2 = (double) FLT_MAX + (double) FLT_MAX + (double) FLT_MAX;
	
	// returns true
	bool equal = Math::is_equal_approx(d, d2);

Explanation
While looking at #48841 I noticed this bug.

It occurs because is_equal_approx is defined using real_t, which is currently float. When you call this function with doubles outside float range, I'm presuming they are truncated to INF, and comparison of INF to INF comes out as equal.

The solution is probably to define specific versions of these functions for float and for double, and probably let the compiler choose which to use when passing real_t. This problem may also be present in other math functions, I haven't looked through.

I'm not planning on doing a PR for this BTW, so anyone interested can take a look.

[aaronfranke]

We basically have two options here.

1. Simply change the type that these methods expect to be double. This would be the smallest amount of code but might be less performant as implicit casting from float to double would happen more often.
2. Make two versions, one for float and one for double, and keep the code identical except for the type change. This would probably be the "safest" option, but it does result in duplicating these methods, and also many calls to is_equal_approx will need to have casts added to ensure the arguments are all of the same type, otherwise we get "error: ISO C++ says that these are ambiguous, even though the worst conversion for the first is better than the worst conversion for the second".

A bit of history: this function used real_t before I first modified it, and when I did so I forgot to consider infinity values (the if statement that handles them was added later). Having it use double makes sense, although in the real world cases the only behavior difference would be in edge cases such as overflowing to infinity and possibly a few others.

[lawnjelly]

For (2), using a similar approach to abs might be an option, with explicit function for float and double, that way you could optionally call the explicit function directly and avoid casting, and not get the ambiguous warnings. And getting the warning in an ambiguous situation is probably not a bad thing to draw attention to, and easy enough to fix for existing code.

	static _ALWAYS_INLINE_ double abs(double g) { return absd(g); }
	static _ALWAYS_INLINE_ float abs(float g) { return absf(g); }
	static _ALWAYS_INLINE_ int abs(int g) { return g > 0 ? g : -g; }

Personally I would be tempted to go for this approach, rather than treat everything as doubles - at least from c++. On the flip side I don't know how often is_equal_approx is used in bottleneck code. So other opinions would be welcome.

Another thing is that apart from the conversions, the best epsilon to use might be different for float and doubles. The best epsilon also presumably varies according to where in the range you are comparing, as precision is higher close to zero than FLT_MAX... but I'm not sure there's a lot you can reasonably do about that - as the error also depends on prior operations etc. That's partly why real care has to be taken with blanket CMP_EPSILON type defines.

[akien-mga]

Make two versions, one for float and one for double, and keep the code identical except for the type change. This would probably be the "safest" option, but it does result in duplicating these methods, and also many calls to is_equal_approx will need to have casts added to ensure the arguments are all of the same type, otherwise we get "error: ISO C++ says that these are ambiguous, even though the worst conversion for the first is better than the worst conversion for the second".

I think this is fine. We have similar warnings for comparisons between different types (e.g int < unsigned int) and adding explicit casts, or fixing the types one is using, seems like an OK requirement.

I guess we'll see how many such warnings are raised by existing code if this gets implemented. If it's too many/too tedious we can consider another option. (I just hope that is_equal_approx((double)some_var, 0.0) wouldn't throw that warning, if so it's going to be tricky with code comparing real_t to literals.)

[lawnjelly]

Actually come to think of it, you can also deal with this using the following approach (with casts as needed). It should compile out:

bool is_equal_approx(float, float) {return is_equal_approxf();}
bool is_equal_approx(double, double) {return is_equal_approxd();}
bool is_equal_approx(float, double) {return is_equal_approxd();}
bool is_equal_approx(double, float) {return is_equal_approxd();}