Skip to content

[3.x] Use approximate test when comparing properties, thoroughly #114210

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

Open
RandomShaper wants to merge 1 commit into
base: 3.x
Choose a base branch
from
+70 −40
Open

[3.x] Use approximate test when comparing properties, thoroughly #114210

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions core/array.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ void Array::clear() {
_p->array.clear();
}

bool Array::deep_equal(const Array &p_array, int p_recursion_count) const {
bool Array::deep_equal(const Array &p_array, int p_recursion_count, bool p_approximate) const {
// Cheap checks
ERR_FAIL_COND_V_MSG(p_recursion_count > MAX_RECURSION, true, "Max recursion reached");
if (_p == p_array._p) {
Expand All @@ -105,7 +105,7 @@ bool Array::deep_equal(const Array &p_array, int p_recursion_count) const {
// Heavy O(n) check
p_recursion_count++;
for (int i = 0; i < size; i++) {
if (!a1[i].deep_equal(a2[i], p_recursion_count)) {
if (!a1[i].deep_equal(a2[i], p_recursion_count, p_approximate)) {
return false;
}
}
Expand Down
2 changes: 1 addition & 1 deletion core/array.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ class Array {
bool empty() const;
void clear();

bool deep_equal(const Array &p_array, int p_recursion_count = 0) const;
bool deep_equal(const Array &p_array, int p_recursion_count = 0, bool p_approximate = false) const;
bool operator==(const Array &p_array) const;

uint32_t hash() const;
Expand Down
6 changes: 3 additions & 3 deletions core/dictionary.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,7 @@ bool Dictionary::erase(const Variant &p_key) {
return _p->variant_map.erase(p_key);
}

bool Dictionary::deep_equal(const Dictionary &p_dictionary, int p_recursion_count) const {
bool Dictionary::deep_equal(const Dictionary &p_dictionary, int p_recursion_count, bool p_approximate) const {
// Cheap checks
ERR_FAIL_COND_V_MSG(p_recursion_count > MAX_RECURSION, 0, "Max recursion reached");
if (_p == p_dictionary._p) {
Expand All @@ -178,8 +178,8 @@ bool Dictionary::deep_equal(const Dictionary &p_dictionary, int p_recursion_coun
p_recursion_count++;
while (this_E && other_E) {
if (
!this_E.key().deep_equal(other_E.key(), p_recursion_count) ||
!this_E.value().deep_equal(other_E.value(), p_recursion_count)) {
!this_E.key().deep_equal(other_E.key(), p_recursion_count, p_approximate) ||
!this_E.value().deep_equal(other_E.value(), p_recursion_count, p_approximate)) {
return false;
}

Expand Down
2 changes: 1 addition & 1 deletion core/dictionary.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ class Dictionary {

bool erase(const Variant &p_key);

bool deep_equal(const Dictionary &p_dictionary, int p_recursion_count = 0) const;
bool deep_equal(const Dictionary &p_dictionary, int p_recursion_count = 0, bool p_approximate = false) const;
bool operator==(const Dictionary &p_dictionary) const;
bool operator!=(const Dictionary &p_dictionary) const;

Expand Down
80 changes: 57 additions & 23 deletions core/variant.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -601,35 +601,69 @@ bool Variant::can_convert_strict(Variant::Type p_type_from, Variant::Type p_type
return false;
}

bool Variant::deep_equal(const Variant &p_variant, int p_recursion_count) const {
ERR_FAIL_COND_V_MSG(p_recursion_count > MAX_RECURSION, true, "Max recursion reached");
template <typename T>
static bool _equal_approx_recursive(const Variant &p_a, const Variant &p_b, int p_recursion_count, bool p_approximate) {
if (p_a.get_type() != p_b.get_type()) {
return false;
}

// Containers must be handled with recursivity checks
switch (type) {
case Variant::Type::DICTIONARY: {
if (p_variant.type != Variant::Type::DICTIONARY) {
return false;
}
const T &a_as_t = p_a.operator T();
const T &b_as_t = p_b.operator T();

const Dictionary v1_as_d = Dictionary(*this);
const Dictionary v2_as_d = Dictionary(p_variant);
return a_as_t.deep_equal(b_as_t, p_recursion_count + 1, p_approximate);
}

return v1_as_d.deep_equal(v2_as_d, p_recursion_count + 1);
} break;
case Variant::Type::ARRAY: {
if (p_variant.type != Variant::Type::ARRAY) {
return false;
}
template <typename T>
static bool _equal_approx_primitive(const Variant &p_a, const Variant &p_b) {
if (p_a.get_type() != p_b.get_type()) {
return false;
}

const Array v1_as_a = Array(*this);
const Array v2_as_a = Array(p_variant);
const T &a_as_t = p_a.operator T();
const T &b_as_t = p_b.operator T();

return v1_as_a.deep_equal(v2_as_a, p_recursion_count + 1);
} break;
default: {
return *this == p_variant;
} break;
return a_as_t.is_equal_approx(b_as_t);
}

bool Variant::deep_equal(const Variant &p_variant, int p_recursion_count, bool p_approximate) const {
ERR_FAIL_COND_V_MSG(p_recursion_count > MAX_RECURSION, true, "Max recursion reached");

// Containers must be handled with recursivity checks
if (type == ARRAY) {
return _equal_approx_recursive<Array>(*this, p_variant, p_recursion_count, p_approximate);
} else if (type == DICTIONARY) {
return _equal_approx_recursive<Dictionary>(*this, p_variant, p_recursion_count, p_approximate);
} else if (p_approximate) {
switch (type) {
case REAL:
return Math::is_equal_approx((double)*this, (double)p_variant);
case VECTOR2:
return _equal_approx_primitive<Vector2>(*this, p_variant);
case RECT2:
return _equal_approx_primitive<Rect2>(*this, p_variant);
case VECTOR3:
return _equal_approx_primitive<Vector3>(*this, p_variant);
case TRANSFORM2D:
return _equal_approx_primitive<Transform2D>(*this, p_variant);
case PLANE:
return _equal_approx_primitive<Plane>(*this, p_variant);
case QUAT:
return _equal_approx_primitive<Quat>(*this, p_variant);
case AABB:
return _equal_approx_primitive<::AABB>(*this, p_variant);
case BASIS:
return _equal_approx_primitive<Basis>(*this, p_variant);
case TRANSFORM:
return _equal_approx_primitive<Transform>(*this, p_variant);
case DICTIONARY:
return _equal_approx_recursive<Dictionary>(*this, p_variant, p_recursion_count, p_approximate);
case ARRAY:
return _equal_approx_recursive<Array>(*this, p_variant, p_recursion_count, p_approximate);
default: {
}
}
}
return *this == p_variant;
}

bool Variant::operator==(const Variant &p_variant) const {
Expand Down
2 changes: 1 addition & 1 deletion core/variant.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -408,7 +408,7 @@ class Variant {

//argsVariant call()

bool deep_equal(const Variant &p_variant, int p_recursion_count = 0) const;
bool deep_equal(const Variant &p_variant, int p_recursion_count = 0, bool p_approximate = false) const;
bool operator==(const Variant &p_variant) const;
bool operator!=(const Variant &p_variant) const;
bool operator<(const Variant &p_variant) const;
Expand Down
14 changes: 5 additions & 9 deletions scene/property_utils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,15 +37,11 @@
#include "scene/resources/packed_scene.h"

bool PropertyUtils::is_property_value_different(const Variant &p_a, const Variant &p_b) {
if (p_a.get_type() == Variant::REAL && p_b.get_type() == Variant::REAL) {
//this must be done because, as some scenes save as text, there might be a tiny difference in floats due to numerical error
return !Math::is_equal_approx((float)p_a, (float)p_b);
} else {
// For our purposes, treating null object as NIL is the right thing to do
const Variant &a = p_a.get_type() == Variant::OBJECT && (Object *)p_a == nullptr ? Variant() : p_a;
const Variant &b = p_b.get_type() == Variant::OBJECT && (Object *)p_b == nullptr ? Variant() : p_b;
return !a.deep_equal(b);
}
// For our purposes, treating null object as NIL is the right thing to do
const Variant &a = p_a.get_type() == Variant::OBJECT && (Object *)p_a == nullptr ? Variant() : p_a;
const Variant &b = p_b.get_type() == Variant::OBJECT && (Object *)p_b == nullptr ? Variant() : p_b;
// Approximmation must be used because, as some scenes save as text, there might be a tiny difference in floats due to numerical error
return !a.deep_equal(b, 0, true);
}

Variant PropertyUtils::get_property_default_value(const Object *p_object, const StringName &p_property, bool *r_is_valid, const Vector<SceneState::PackState> *p_states_stack_cache, bool p_update_exports, const Node *p_owner, bool *r_is_class_default) {
Expand Down