Support numeric/binary hash comparison for floats derived from Variants (as well as existing semantic comparison)

[puchik]

Hash scalar compare for Variants which are floats no longer considers two NaN values as equal.

Hash comparison for Variant continues to perform semantic/logical comparison with NaN's considered equal by default (to prevent #16114, #7354, #6947, #8081, #16031), but now optionally allows for numeric comparison that does not consider NaN's equal to support proper value comparison (for #72222)

• Added a semantic_comparison argument to existing hash_compare implementation(s) that is set to true by default.
• Added a hash_comparison_base macro to support existing hash_compare values with default semantic comparison.
• Dictionary value and arrays use non-semantic/numeric/binary comparisons for floats.

Closes #72222

[akien-mga]

CC @hpvb who introduced this code back in 2017 with #7815 (+ #8393).

The behavior for Dictionary with NAN keys was then later changed with #16114 to fix #16031. Would this PR reintroduce that bug?

[AThousandShips]

Because of macros I would keep the parentheses around the values, just to be safe

For example: hash_compare_scalar(a&b,c) would break, not that it's likely to happen but macros are confusing

[AThousandShips]

Maybe we should add a hash compare that treats NaN as equal and use it for dictionary, where we technically care about binary equality

[vnen]

I believe this will reintroduce the bug as it is.

[puchik]

Looks like this was quite a controversial series of bugs and decisions 🙂

It does look like to support both the best way I can think of is two functions. I pushed a change that addresses this with I think the least code duplication as possible and doesn't break compatibility.

I tested with the original target issue (#72222) and previous issues (#16114, #7354, #6947, #8081, #16031) and all cases seem covered.

[Calinou]

Tested locally (rebased on top of master b0c3c00), it works as expected.

PR output

1
=======================
false
false
=======================
(-3, 2)
=======================

master output

1
=======================
false
true
=======================
(-3, 2)
=======================

Code

extends Node


func _ready():
	var d = {}
	d[Vector2(NAN, NAN)] = 0
	d[Vector2(NAN, NAN)] = 0
	print(d.size())

	print("=======================")

	var foo = { foo = NAN }
	var bar = { foo = NAN }
	print(foo.foo == bar.foo)  # false
	print(foo == bar)  # true

	print("=======================")

	Vector2(3, -2)
	print(Vector2(-3, 2))

	print("=======================")

	var x = 200 * cos(2 * PI / (0))
	var y = 200 * sin(2 * PI / (0))

[Calinou]

PS: In the future, when posting a screenshot of code, please include it in Markdown as well between triple backticks (and the gdscript syntax highlighting language) so that other people can test it easily. (This is also more accessible to screen readers.)

[reduz]

This makes sense to me, but hard to know if it will break something. Probably better to give it a go and be very attentive to see what happens.

[puchik]

I'm not sure if the memory error is a problem with the workflow. Especially because all I changed was the order of the if conditions and it seems to be connected to Vulkan, plus I'm not able to reproduce it.

[akien-mga]

That's a race condition unrelated to this PR, it happens every now and then. I restarted the build.

[puchik]

I see. Thanks!

[akien-mga]

Thanks!