Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Vector2 literals with swapped signs #8081

Closed
leonkrause opened this issue Mar 19, 2017 · 4 comments · Fixed by #8393
Closed

Vector2 literals with swapped signs #8081

leonkrause opened this issue Mar 19, 2017 · 4 comments · Fixed by #8393
Labels
bug confirmed topic:gdscript
Milestone
3.0

Comments

@leonkrause
Copy link
Contributor

Operating system or device - Godot version:
FreeBSD, 9061d6f

Issue description:

func _init():
	Vector2(3, -2)
	print(Vector2(-3, 2))

This code prints (3, -2). Expected is (-3, 2).
Doesn't occur in 2.1.2
@leonkrause leonkrause added this to the 3.0 milestone Mar 19, 2017
@bojidar-bg
Copy link
Contributor

Sounds related to #7815, can someone bisect?

@bojidar-bg bojidar-bg mentioned this issue Apr 13, 2017
Closed
@bojidar-bg
Copy link
Contributor

I tested this right before #7815 and right after #7815 (on 3698653 and 903a3aa), and I can confirm that the issue is caused by it.

Testing script:

func _ready():
  print(Vector3(-1, 0, 1))
  print(Vector3(1, 0, -1))

Pre-#7815 output:

(-1, 0, 1)
(1, 0, -1)

Post-#7815 output:

(-1, 0, 1)
(-1, 0, 1)

@hpvb Are you going to be able to look into this?

@hpvb
Copy link
Member

hpvb commented Apr 13, 2017

I found the issue, I'll make a PR asap. The problem is that I also switched to a different hashing algo for non-floats.

I don't even remember why I did that.

@hpvb
Copy link
Member

hpvb commented Apr 13, 2017

Seems that that function in the hash code wasn't used because it was broken. Whoops?

@hpvb hpvb mentioned this issue Apr 13, 2017
Merged
hpvb added a commit to hpvb/godot that referenced this issue Apr 14, 2017
@hpvb
Correct Variant::hash_compare()
8ff6e53 
There was a logic error in godotengine#7815 which made
Variant.hash_compare() == Variant.hash_compare() always true.
In an attempt to short-circuit the NaN check I made an (in hindsight) obvious
error: 10 == 12 || is_nan(10) == is_nan(12)

This will be true for all inputs, except for the NaN, not-NaN case. The macro
has been updated to now generate:

(10 == 12) || (is_nan(10) && is_nan(10))

so:

(10 == 12)   || (is_nan(10)  && is_nan(12))  = false
   False  or (False and False) is False
(10 == 10)   || (is_nan(10)  && is_nan(10))  = true
   True or (False and False) is True
(Nan == 10)  || (is_nan(NaN) && is_nan(10))  = false
   False or (True and False) is False
(Nan == Nan) || (is_nan(NaN) && is_nan(NaN)) = true
   False or (True and True) is True

Which is correct for all cases.

This bug was triggered because the hash function for floating point numbers
can very easily generate collisions for the tested Vector3(). I've also added
an extra hashing step to the float hash function to make this less likely to
occur.

This fixes godotengine#8081 and probably many more random weirdness.
groscalin pushed a commit to groscalin/godot that referenced this issue Jul 15, 2017
@hpvb
Correct Variant::hash_compare()
3b0a780 
There was a logic error in godotengine#7815 which made
Variant.hash_compare() == Variant.hash_compare() always true.
In an attempt to short-circuit the NaN check I made an (in hindsight) obvious
error: 10 == 12 || is_nan(10) == is_nan(12)

This will be true for all inputs, except for the NaN, not-NaN case. The macro
has been updated to now generate:

(10 == 12) || (is_nan(10) && is_nan(10))

so:

(10 == 12)   || (is_nan(10)  && is_nan(12))  = false
   False  or (False and False) is False
(10 == 10)   || (is_nan(10)  && is_nan(10))  = true
   True or (False and False) is True
(Nan == 10)  || (is_nan(NaN) && is_nan(10))  = false
   False or (True and False) is False
(Nan == Nan) || (is_nan(NaN) && is_nan(NaN)) = true
   False or (True and True) is True

Which is correct for all cases.

This bug was triggered because the hash function for floating point numbers
can very easily generate collisions for the tested Vector3(). I've also added
an extra hashing step to the float hash function to make this less likely to
occur.

This fixes godotengine#8081 and probably many more random weirdness.
@puchik puchik mentioned this issue Mar 11, 2023
Merged
puchik added a commit to puchik/godot that referenced this issue Aug 31, 2023
@puchik
Support both semantic (by default) and numeric Variant hash comparison
ee27254 
Hash comparison for Variant continues to perform semantic/logical comparison with NaN's considered equal by default (to prevent godotengine#16114, godotengine#7354, godotengine#6947, godotengine#8081), but now optionally allows for numeric comparison that does not consider NaN's equal to support proper value comparison (for godotengine#72222)
mandryskowski pushed a commit to mandryskowski/godot that referenced this issue Oct 11, 2023
@puchik @mandryskowski
Support both semantic (by default) and numeric Variant hash comparison
7f89f9f 
Hash comparison for Variant continues to perform semantic/logical comparison with NaN's considered equal by default (to prevent godotengine#16114, godotengine#7354, godotengine#6947, godotengine#8081), but now optionally allows for numeric comparison that does not consider NaN's equal to support proper value comparison (for godotengine#72222)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug confirmed topic:gdscript
Projects
None yet
Milestone
3.0
Development

Successfully merging a pull request may close this issue.

Correct Variant::hash_compare() hpvb/godot
3 participants
@hpvb @bojidar-bg @leonkrause