From 364f2e8082b01010628294d7ab7418687874993e Mon Sep 17 00:00:00 2001 From: Hein-Pieter van Braam Date: Wed, 15 Feb 2017 14:41:16 +0100 Subject: [PATCH] Correct hash behavior for floating point numbers This backports the work in #7815 and the subsequent fixes in #8393 The following program now works as expected in this branch in both release_debug and debug mode: ```gdscript print(sqrt(-1)) print(sqrt(-1)) var simple1=asin(10.0) var simple2=acos(10.0) print(simple1) print(simple2) ``` And successfully prints -nan 4 times This fixes #9580 and fixes #8925 --- core/hash_map.h | 64 +++++---- core/hashfuncs.h | 43 ++++-- core/object.h | 2 +- core/variant.cpp | 188 ++++++++++++++++++++++++++- core/variant.h | 7 +- drivers/gles2/shader_gles2.h | 2 +- editor/plugins/baked_light_baker.cpp | 4 +- modules/gdscript/gd_compiler.h | 4 +- modules/gdscript/gd_tokenizer.cpp | 6 +- scene/resources/packed_scene.cpp | 14 +- scene/resources/packed_scene.h | 4 +- 11 files changed, 272 insertions(+), 66 deletions(-) diff --git a/core/hash_map.h b/core/hash_map.h index 93006ddb09..ea81f9eec4 100644 --- a/core/hash_map.h +++ b/core/hash_map.h @@ -30,36 +30,43 @@ #ifndef HASH_MAP_H #define HASH_MAP_H -#include "error_macros.h" -#include "hashfuncs.h" #include "list.h" +#include "hashfuncs.h" +#include "math_funcs.h" #include "os/memory.h" #include "ustring.h" -class HashMapHahserDefault { -public: - static _FORCE_INLINE_ uint32_t hash(const String &p_string) { return p_string.hash(); } - static _FORCE_INLINE_ uint32_t hash(const char *p_cstr) { return hash_djb2(p_cstr); } - static _FORCE_INLINE_ uint32_t hash(const uint64_t p_int) { - uint64_t v = p_int; - v = (~v) + (v << 18); // v = (v << 18) - v - 1; - v = v ^ (v >> 31); - v = v * 21; // v = (v + (v << 2)) + (v << 4); - v = v ^ (v >> 11); - v = v + (v << 6); - v = v ^ (v >> 22); - return (int)v; - } - static _FORCE_INLINE_ uint32_t hash(const int64_t p_int) { return hash(uint64_t(p_int)); } +struct HashMapHasherDefault { + static _FORCE_INLINE_ uint32_t hash(const String &p_string) { return p_string.hash(); } + static _FORCE_INLINE_ uint32_t hash(const char *p_cstr) { return hash_djb2(p_cstr); } + static _FORCE_INLINE_ uint32_t hash(const uint64_t p_int) { return hash_one_uint64(p_int); } + static _FORCE_INLINE_ uint32_t hash(const int64_t p_int) { return hash(uint64_t(p_int)); } + static _FORCE_INLINE_ uint32_t hash(const float p_float) { return hash_djb2_one_float(p_float); } + static _FORCE_INLINE_ uint32_t hash(const double p_double){ return hash_djb2_one_float(p_double); } static _FORCE_INLINE_ uint32_t hash(const uint32_t p_int) { return p_int; } - static _FORCE_INLINE_ uint32_t hash(const int32_t p_int) { return (uint32_t)p_int; } + static _FORCE_INLINE_ uint32_t hash(const int32_t p_int) { return (uint32_t)p_int; } static _FORCE_INLINE_ uint32_t hash(const uint16_t p_int) { return p_int; } - static _FORCE_INLINE_ uint32_t hash(const int16_t p_int) { return (uint32_t)p_int; } - static _FORCE_INLINE_ uint32_t hash(const uint8_t p_int) { return p_int; } - static _FORCE_INLINE_ uint32_t hash(const int8_t p_int) { return (uint32_t)p_int; } - static _FORCE_INLINE_ uint32_t hash(const wchar_t p_wchar) { return (uint32_t)p_wchar; } - // static _FORCE_INLINE_ uint32_t hash(const void* p_ptr) { return uint32_t(uint64_t(p_ptr))*(0x9e3779b1L); } + static _FORCE_INLINE_ uint32_t hash(const int16_t p_int) { return (uint32_t)p_int; } + static _FORCE_INLINE_ uint32_t hash(const uint8_t p_int) { return p_int; } + static _FORCE_INLINE_ uint32_t hash(const int8_t p_int) { return (uint32_t)p_int; } + static _FORCE_INLINE_ uint32_t hash(const wchar_t p_wchar){ return (uint32_t)p_wchar; } + //static _FORCE_INLINE_ uint32_t hash(const void* p_ptr) { return uint32_t(uint64_t(p_ptr))*(0x9e3779b1L); } +}; + +template +struct HashMapComparatorDefault { + static bool compare(const T& p_lhs, const T& p_rhs) { + return p_lhs == p_rhs; + } + + bool compare(const float& p_lhs, const float& p_rhs) { + return (p_lhs == p_rhs) || (Math::is_nan(p_lhs) && Math::is_nan(p_rhs)); + } + + bool compare(const double& p_lhs, const double& p_rhs) { + return (p_lhs == p_rhs) || (Math::is_nan(p_lhs) && Math::is_nan(p_rhs)); + } }; /** @@ -72,13 +79,14 @@ public: * @param TKey Key, search is based on it, needs to be hasheable. It is unique in this container. * @param TData Data, data associated with the key * @param Hasher Hasher object, needs to provide a valid static hash function for TKey + * @param Comparator comparator object, needs to be able to safely compare two TKey values. It needs to ensure that x == x for any items inserted in the map. Bear in mind that nan != nan when implementing an equality check. * @param MIN_HASH_TABLE_POWER Miminum size of the hash table, as a power of two. You rarely need to change this parameter. * @param RELATIONSHIP Relationship at which the hash table is resized. if amount of elements is RELATIONSHIP * times bigger than the hash table, table is resized to solve this condition. if RELATIONSHIP is zero, table is always MIN_HASH_TABLE_POWER. * */ -template +template , uint8_t MIN_HASH_TABLE_POWER=3,uint8_t RELATIONSHIP=8> class HashMap { public: struct Pair { @@ -200,7 +208,7 @@ private: while (e) { /* checking hash first avoids comparing key, which may take longer */ - if (e->hash == hash && e->pair.key == p_key) { + if (e->hash == hash && Comparator::compare(e->pair.key,p_key) ) { /* the pair exists in this hashtable, so just update data */ return e; @@ -367,7 +375,7 @@ public: while (e) { /* checking hash first avoids comparing key, which may take longer */ - if (e->hash == hash && e->pair.key == p_custom_key) { + if (e->hash == hash && Comparator::compare(e->pair.key,p_custom_key) ) { /* the pair exists in this hashtable, so just update data */ return &e->pair.data; @@ -393,7 +401,7 @@ public: while (e) { /* checking hash first avoids comparing key, which may take longer */ - if (e->hash == hash && e->pair.key == p_custom_key) { + if (e->hash == hash && Comparator::compare(e->pair.key,p_custom_key) ) { /* the pair exists in this hashtable, so just update data */ return &e->pair.data; @@ -422,7 +430,7 @@ public: while (e) { /* checking hash first avoids comparing key, which may take longer */ - if (e->hash == hash && e->pair.key == p_key) { + if (e->hash == hash && Comparator::compare(e->pair.key,p_key) ) { if (p) { diff --git a/core/hashfuncs.h b/core/hashfuncs.h index 03219db3ea..2159563144 100644 --- a/core/hashfuncs.h +++ b/core/hashfuncs.h @@ -30,6 +30,8 @@ #ifndef HASHFUNCS_H #define HASHFUNCS_H +#include "math_funcs.h" +#include "math_defs.h" #include "typedefs.h" /** @@ -68,22 +70,35 @@ static inline uint32_t hash_djb2_one_32(uint32_t p_in, uint32_t p_prev = 5381) { return ((p_prev << 5) + p_prev) + p_in; } -static inline uint32_t hash_djb2_one_float(float p_in, uint32_t p_prev = 5381) { - union { - float f; - uint32_t i; - } u; - - // handle -0 case - if (p_in == 0.0f) - u.f = 0.0f; - else - u.f = p_in; - - return ((p_prev << 5) + p_prev) + u.i; +static inline uint32_t hash_one_uint64(const uint64_t p_int) { + uint64_t v=p_int; + v = (~v) + (v << 18); // v = (v << 18) - v - 1; + v = v ^ (v >> 31); + v = v * 21; // v = (v + (v << 2)) + (v << 4); + v = v ^ (v >> 11); + v = v + (v << 6); + v = v ^ (v >> 22); + return (int) v; } -template +static inline uint32_t hash_djb2_one_float(double p_in, uint32_t p_prev = 5381) { + union { + double d; + uint64_t i; + } u; + + // Normalize +/- 0.0 and NaN values so they hash the same. + if (p_in==0.0f) + u.d=0.0; + else if (Math::is_nan(p_in)) + u.d=NAN; + else + u.d=p_in; + + return ((p_prev<<5)+p_prev) + hash_one_uint64(u.i); +} + +template static inline uint32_t make_uint32_t(T p_in) { union { diff --git a/core/object.h b/core/object.h index 8630c713b3..8225b78db3 100644 --- a/core/object.h +++ b/core/object.h @@ -676,7 +676,7 @@ class ObjectDB { unsigned long i; } u; u.p = p_obj; - return HashMapHahserDefault::hash((uint64_t)u.i); + return HashMapHasherDefault::hash((uint64_t)u.i); } }; diff --git a/core/variant.cpp b/core/variant.cpp index 89bd25bd9a..b4b83397a4 100644 --- a/core/variant.cpp +++ b/core/variant.cpp @@ -30,6 +30,7 @@ #include "variant.h" #include "core_string_names.h" #include "io/marshalls.h" +#include "math_funcs.h" #include "print_string.h" #include "resource.h" #include "scene/gui/control.h" @@ -2598,14 +2599,10 @@ uint32_t Variant::hash() const { case INT: { return _data._int; - } break; case REAL: { - MarshallFloat mf; - mf.f = _data._real; - return mf.i; - + return hash_djb2_one_float(_data._real); } break; case STRING: { @@ -2840,6 +2837,187 @@ uint32_t Variant::hash() const { return 0; } +#define hash_compare_scalar(p_lhs, 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)) && \ + (hash_compare_scalar((p_lhs).y, (p_rhs).y)) + +#define hash_compare_vector3(p_lhs, p_rhs)\ + (hash_compare_scalar((p_lhs).x, (p_rhs).x)) && \ + (hash_compare_scalar((p_lhs).y, (p_rhs).y)) && \ + (hash_compare_scalar((p_lhs).z, (p_rhs).z)) + +#define hash_compare_quat(p_lhs, p_rhs)\ + (hash_compare_scalar((p_lhs).x, (p_rhs).x)) && \ + (hash_compare_scalar((p_lhs).y, (p_rhs).y)) && \ + (hash_compare_scalar((p_lhs).z, (p_rhs).z)) && \ + (hash_compare_scalar((p_lhs).w, (p_rhs).w)) + +#define hash_compare_color(p_lhs, p_rhs)\ + (hash_compare_scalar((p_lhs).r, (p_rhs).r)) && \ + (hash_compare_scalar((p_lhs).g, (p_rhs).g)) && \ + (hash_compare_scalar((p_lhs).b, (p_rhs).b)) && \ + (hash_compare_scalar((p_lhs).a, (p_rhs).a)) + +#define hash_compare_pool_array(p_lhs, p_rhs, p_type, p_compare_func)\ + const DVector& l = *reinterpret_cast*>(p_lhs);\ + const DVector& r = *reinterpret_cast*>(p_rhs);\ + \ + if(l.size() != r.size()) \ + return false; \ + \ + DVector::Read lr = l.read(); \ + DVector::Read rr = r.read(); \ + \ + for(int i = 0; i < l.size(); ++i) { \ + if(! p_compare_func((lr[0]), (rr[0]))) \ + return false; \ + }\ + \ + return true + +bool Variant::hash_compare(const Variant& p_variant) const { + if (type != p_variant.type) + return false; + + switch( type ) { + case REAL: { + return hash_compare_scalar(_data._real, p_variant._data._real); + } break; + + case VECTOR2: { + const Vector2* l = reinterpret_cast(_data._mem); + const Vector2* r = reinterpret_cast(p_variant._data._mem); + + return hash_compare_vector2(*l, *r); + } break; + + case RECT2: { + const Rect2* l = reinterpret_cast(_data._mem); + const Rect2* r = reinterpret_cast(p_variant._data._mem); + + return (hash_compare_vector2(l->pos, r->pos)) && + (hash_compare_vector2(l->size, r->size)); + } break; + + case MATRIX32: { + Matrix32* l = _data._matrix32; + Matrix32* r = p_variant._data._matrix32; + + for(int i=0;i<3;i++) { + if (! (hash_compare_vector2(l->elements[i], r->elements[i]))) + return false; + } + + return true; + } break; + + case VECTOR3: { + const Vector3* l = reinterpret_cast(_data._mem); + const Vector3* r = reinterpret_cast(p_variant._data._mem); + + return hash_compare_vector3(*l, *r); + } break; + + case PLANE: { + const Plane* l = reinterpret_cast(_data._mem); + const Plane* r = reinterpret_cast(p_variant._data._mem); + + return (hash_compare_vector3(l->normal, r->normal)) && + (hash_compare_scalar(l->d, r->d)); + } break; + + case _AABB: { + const AABB* l = _data._aabb; + const AABB* r = p_variant._data._aabb; + + return (hash_compare_vector3(l->pos, r->pos) && + (hash_compare_vector3(l->size, r->size))); + + } break; + + case QUAT: { + const Quat* l = reinterpret_cast(_data._mem); + const Quat* r = reinterpret_cast(p_variant._data._mem); + + return hash_compare_quat(*l, *r); + } break; + + case MATRIX3: { + const Matrix3* l = _data._matrix3; + const Matrix3* r = p_variant._data._matrix3; + + for(int i=0;i<3;i++) { + if (! (hash_compare_vector3(l->elements[i], r->elements[i]))) + return false; + } + + return true; + } break; + + case TRANSFORM: { + const Transform* l = _data._transform; + const Transform* r = p_variant._data._transform; + + for(int i=0;i<3;i++) { + if (! (hash_compare_vector3(l->basis.elements[i], r->basis.elements[i]))) + return false; + } + + return hash_compare_vector3(l->origin, r->origin); + } break; + + case COLOR: { + const Color* l = reinterpret_cast(_data._mem); + const Color* r = reinterpret_cast(p_variant._data._mem); + + return hash_compare_color(*l, *r); + } break; + + case ARRAY: { + const Array& l = *(reinterpret_cast(_data._mem)); + const Array& r = *(reinterpret_cast(p_variant._data._mem)); + + if(l.size() != r.size()) + return false; + + for(int i = 0; i < l.size(); ++i) { + if(! l[0].hash_compare(r[0])) + return false; + } + + return true; + } break; + + case REAL_ARRAY: { + hash_compare_pool_array(_data._mem, p_variant._data._mem, real_t, hash_compare_scalar); + } break; + + case VECTOR2_ARRAY: { + hash_compare_pool_array(_data._mem, p_variant._data._mem, Vector2, hash_compare_vector2); + } break; + + case VECTOR3_ARRAY: { + hash_compare_pool_array(_data._mem, p_variant._data._mem, Vector3, hash_compare_vector3); + } break; + + case COLOR_ARRAY: { + hash_compare_pool_array(_data._mem, p_variant._data._mem, Color, hash_compare_color); + } break; + + default: + bool v; + Variant r; + evaluate(OP_EQUAL,*this,p_variant,r,v); + return r; + } + + return false; +} + + bool Variant::is_ref() const { return type == OBJECT && !_get_obj().ref.is_null(); diff --git a/core/variant.h b/core/variant.h index a6be5bb71d..7663d755f1 100644 --- a/core/variant.h +++ b/core/variant.h @@ -401,6 +401,7 @@ public: bool operator<(const Variant &p_variant) const; uint32_t hash() const; + bool hash_compare(const Variant& p_variant) const; bool booleanize(bool &valid) const; void static_assign(const Variant &p_variant); @@ -438,8 +439,12 @@ struct VariantHasher { static _FORCE_INLINE_ uint32_t hash(const Variant &p_variant) { return p_variant.hash(); } }; -Variant::ObjData &Variant::_get_obj() { +struct VariantComparator { + static _FORCE_INLINE_ bool compare(const Variant &p_lhs, const Variant &p_rhs) { return p_lhs.hash_compare(p_rhs); } +}; + +Variant::ObjData &Variant::_get_obj() { return *reinterpret_cast(&_data._mem[0]); } diff --git a/drivers/gles2/shader_gles2.h b/drivers/gles2/shader_gles2.h index 3e2b7dfc20..25a90e3f4a 100644 --- a/drivers/gles2/shader_gles2.h +++ b/drivers/gles2/shader_gles2.h @@ -134,7 +134,7 @@ private: struct VersionKeyHash { - static _FORCE_INLINE_ uint32_t hash(const VersionKey &p_key) { return HashMapHahserDefault::hash(p_key.key); }; + static _FORCE_INLINE_ uint32_t hash( const VersionKey& p_key) { return HashMapHasherDefault::hash(p_key.key); }; }; //this should use a way more cachefriendly version.. diff --git a/editor/plugins/baked_light_baker.cpp b/editor/plugins/baked_light_baker.cpp index cee3c98b6d..80261029b7 100644 --- a/editor/plugins/baked_light_baker.cpp +++ b/editor/plugins/baked_light_baker.cpp @@ -1217,7 +1217,7 @@ void BakedLightBaker::_make_octree_texture() { base <<= 16; base |= int((pos.z + cell_size * 0.5) / cell_size); - uint32_t hash = HashMapHahserDefault::hash(base); + uint32_t hash = HashMapHasherDefault::hash(base); uint32_t idx = hash % hash_table_size; octhashptr[oct_idx].next = hashptr[idx]; octhashptr[oct_idx].hash = hash; @@ -1243,7 +1243,7 @@ void BakedLightBaker::_make_octree_texture() { base <<= 16; base |= int((pos.z + cell_size * 0.5) / cell_size); - uint32_t hash = HashMapHahserDefault::hash(base); + uint32_t hash = HashMapHasherDefault::hash(base); uint32_t idx = hash % hash_table_size; uint32_t bucket = hashptr[idx]; diff --git a/modules/gdscript/gd_compiler.h b/modules/gdscript/gd_compiler.h index 9793b630af..507b5d61e9 100644 --- a/modules/gdscript/gd_compiler.h +++ b/modules/gdscript/gd_compiler.h @@ -91,8 +91,8 @@ class GDCompiler { } //int get_identifier_pos(const StringName& p_dentifier) const; - HashMap constant_map; - Map name_map; + HashMap constant_map; + Map name_map; int get_name_map_pos(const StringName &p_identifier) { int ret; diff --git a/modules/gdscript/gd_tokenizer.cpp b/modules/gdscript/gd_tokenizer.cpp index 15983ec2ee..09e45a951f 100644 --- a/modules/gdscript/gd_tokenizer.cpp +++ b/modules/gdscript/gd_tokenizer.cpp @@ -1124,9 +1124,9 @@ Vector GDTokenizerBuffer::parse_code_string(const String &p_code) { Vector buf; - Map identifier_map; - HashMap constant_map; - Map line_map; + Map identifier_map; + HashMap constant_map; + Map line_map; Vector token_array; GDTokenizerText tt; diff --git a/scene/resources/packed_scene.cpp b/scene/resources/packed_scene.cpp index edc0103886..087af48d89 100644 --- a/scene/resources/packed_scene.cpp +++ b/scene/resources/packed_scene.cpp @@ -306,7 +306,7 @@ static int _nm_get_string(const String &p_string, Map &name_map return idx; } -static int _vm_get_variant(const Variant &p_variant, HashMap &variant_map) { +static int _vm_get_variant(const Variant& p_variant, HashMap &variant_map) { if (variant_map.has(p_variant)) return variant_map[p_variant]; @@ -316,7 +316,7 @@ static int _vm_get_variant(const Variant &p_variant, HashMap &name_map, HashMap &variant_map, Map &node_map, Map &nodepath_map) { +Error SceneState::_parse_node(Node *p_owner,Node *p_node,int p_parent_idx, Map &name_map,HashMap &variant_map,Map &node_map,Map &nodepath_map) { // this function handles all the work related to properly packing scenes, be it // instanced or inherited. @@ -673,7 +673,7 @@ Error SceneState::_parse_node(Node *p_owner, Node *p_node, int p_parent_idx, Map return OK; } -Error SceneState::_parse_connections(Node *p_owner, Node *p_node, Map &name_map, HashMap &variant_map, Map &node_map, Map &nodepath_map) { +Error SceneState::_parse_connections(Node *p_owner,Node *p_node, Map &name_map,HashMap &variant_map,Map &node_map,Map &nodepath_map) { if (p_node != p_owner && p_node->get_owner() && p_node->get_owner() != p_owner && !p_owner->is_editable_instance(p_node->get_owner())) return OK; @@ -860,10 +860,10 @@ Error SceneState::pack(Node *p_scene) { Node *scene = p_scene; - Map name_map; - HashMap variant_map; - Map node_map; - Map nodepath_map; + Map name_map; + HashMap variant_map; + Map node_map; + Map nodepath_map; //if using scene inheritance, pack the scene it inherits from if (scene->get_scene_inherited_state().is_valid()) { diff --git a/scene/resources/packed_scene.h b/scene/resources/packed_scene.h index 35d63bbfdb..7fb407ed98 100644 --- a/scene/resources/packed_scene.h +++ b/scene/resources/packed_scene.h @@ -88,8 +88,8 @@ class SceneState : public Reference { Vector connections; - Error _parse_node(Node *p_owner, Node *p_node, int p_parent_idx, Map &name_map, HashMap &variant_map, Map &node_map, Map &nodepath_map); - Error _parse_connections(Node *p_owner, Node *p_node, Map &name_map, HashMap &variant_map, Map &node_map, Map &nodepath_map); + Error _parse_node(Node *p_owner,Node *p_node,int p_parent_idx, Map &name_map,HashMap &variant_map,Map &node_map,Map &nodepath_map); + Error _parse_connections(Node *p_owner,Node *p_node, Map &name_map,HashMap &variant_map,Map &node_map,Map &nodepath_map); String path;