Fix SIGN(NAN) returning 1

[AcatXIo]

Fixes #79036. sign(NAN) now returns 0.0.
This should not impact performance much in any way.
Adds a test for the NAN case. Updates the documentation to clarify the new behavior.

[AThousandShips]

Have you confirmed that no code is affected by SIGN returning nan?

[Sauermann]

supersedes #81451

[AcatXIo]

This is different from #81451 by not having a check for zero, if a value is neither positive nor negative, it is returned.
This means that sign now returns NAN for NAN, -0.0 for -0.0, and possibly -NAN for -NAN.

After a few tries to check performance in Windows editor with this script:

func _ready():
	var _a
	var start_time
	var end_time
	await get_tree().create_timer(1).timeout
	start_time = Time.get_ticks_usec()
	for y in range(100000000):
		for x in range(40):
			_a = sign(32.0)
		for x in range(39):
			_a = sign(-15.0)
		for x in range(20):
			_a = sign(0.0)
		for x in range(1):
			_a = sign(NAN)
	end_time = Time.get_ticks_usec()
	print("Running time: " + str(end_time - start_time) + "μs.")

I did not manage to find a performance change higher than ~3% between this and current master.
It would be great if somebody else tried to test the performance of this on their machine.

[AcatXIo]

Have you confirmed that no code is affected by SIGN returning nan?

I did not find anything that explicitly does a NaN check before calling sign(), move_toward() now returns NAN if any of the parameters is NAN (before it was only from and delta parameters).

I thing somebody who knows Godot's code well should recheck that nothing breaks.

[aaronfranke]

Makes sense to me. The old behavior was a bit weird to not be symmetric around 0. I think it's good to check >0 and <0 instead of ==0 and <0.

This does mean that there are are more than 3 possible return values, but I think this is necessary. NaN does not have a sign, and in most floating-point operations, is expected to propagate as NaN rather than be converted into a valid value.

As for -0.0, I think it's good to preserve that information. It should be rare that it breaks existing code because multiplying by the sign of a zero value will still give you zero and if you check if it's equal to zero then -0.0 will be true. But technically, some may consider this behavior change a compatibility breakage.

[AThousandShips]

Note that signbit does not work this way, nor does copysign

[AcatXIo]

NaN does not have a sign

The reason I thought that there might be -NAN passed is this note for log function in the docs:

Note: The logarithm of 0 returns -inf, while negative values return -nan.

I think this should be corrected if there is no -nan, right?

[aaronfranke]

@AcatXIo To be clear, negative NaN does exist as a bit pattern, but since it is expected to interpret a NaN value as not a number, then conceptually speaking NaN does not have a sign, even though you can have -NAN as a value. So, it would be expected that a sign function on NaN won't return +1, -1, or 0 as a sign, instead it should return some kind of NaN. It's the same reason why 0.0 doesn't return +1 from the sign function, sure it has a sign bit, but conceptually speaking it is not a positive number or a negative number.

[AThousandShips]

Gonna reiterate my point and argue that this might be better to restrict to the GDScript side, but if we can ensure that it won't break existing behavior I guess it makes a kind of sense, I'd say it doesn't make much sense to take the sign of NaN and that if you're expecting NaN to maybe occur to do checking for it, I'd view SIGN as expecting the input to be a number

This change doesn't change the performance however so I'd say it's safe from that perspective, but I am still not convinced that this doesn't risk having unforseen side-effects from having a method that didn't ever return NaN now return NaN

If we are to change SIGN I'd suggest instead returning 0 for NaN, we have several functions from the standard library that does not return NaN when the standard library equivalents do, like asin/acos

[AcatXIo]

I'm not sure what did you mean; while the @GlobalScope function asin(999) does not return NAN in GDScript (like it does in JavaScript for example), asin(NAN) still returns NAN, and sign(NAN) would not be different with this PR, unless I'm confusing something here.

[timothyqiu]

I share the same concern as @AThousandShips. While sign(NAN) returning NAN is better, expanding the value range means breaking compatibility.

[AThousandShips]

What I meant was that we already do clamping to results where the standard versions would return NaN. The change to return -0.0 for -0.0 is also possibly risking some confusion.

Also because of the nature of NaN, NaN values being propagated and causing errors are extremely difficult to pin down, trust me I've done some of that, so I feel concern over the risk that this will spawn such errors that can pop up now or in the future and be hard to pin down.

For example: if a audio player has an invalid transform, or has an origin that's at infinity, all sound will be muted, due to NaN poisoning, which took some time to pin down and hasn't yet been resolved as the exact way to fix it is complicated

[AcatXIo]

OK, thank you. I've changed all 3 files, feel free to check them out again.

[AThousandShips]

LGTM and makes sense while being safe

[akien-mga]

Thanks!