From 8ff6e538336135837630e313027826e9bd8d036e Mon Sep 17 00:00:00 2001 From: Hein-Pieter van Braam Date: Fri, 14 Apr 2017 11:28:51 +0200 Subject: [PATCH] Correct Variant::hash_compare() There was a logic error in #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 #8081 and probably many more random weirdness. --- core/hashfuncs.h | 18 ------------------ core/variant.cpp | 2 +- 2 files changed, 1 insertion(+), 19 deletions(-) diff --git a/core/hashfuncs.h b/core/hashfuncs.h index fbd2e161b3..8392984565 100644 --- a/core/hashfuncs.h +++ b/core/hashfuncs.h @@ -81,24 +81,6 @@ static inline uint32_t hash_one_uint64(const uint64_t p_int) { return (int)v; } -static inline uint32_t hash_djb2_one_float(float p_in, uint32_t p_prev = 5381) { - union { - float f; - uint32_t i; - } u; - - // Normalize +/- 0.0 and NaN values so they hash the same. - if (p_in == 0.0f) - u.f = 0.0; - else if (Math::is_nan(p_in)) - u.f = Math_NAN; - else - u.f = p_in; - - return ((p_prev << 5) + p_prev) + u.i; -} - -// Overload for real_t size changes static inline uint32_t hash_djb2_one_float(double p_in, uint32_t p_prev = 5381) { union { double d; diff --git a/core/variant.cpp b/core/variant.cpp index 6e675d07de..67ce8af483 100644 --- a/core/variant.cpp +++ b/core/variant.cpp @@ -2839,7 +2839,7 @@ uint32_t Variant::hash() const { } #define hash_compare_scalar(p_lhs, p_rhs) \ - ((p_lhs) == (p_rhs)) || (Math::is_nan(p_lhs) == Math::is_nan(p_rhs)) + ((p_lhs) == (p_rhs)) || (Math::is_nan(p_lhs) && Math::is_nan(p_rhs)) #define hash_compare_vector2(p_lhs, p_rhs) \ (hash_compare_scalar((p_lhs).x, (p_rhs).x)) && \