From be111004dd012850f2050a2453b8f88c4ef5b10c Mon Sep 17 00:00:00 2001 From: kobewi Date: Sun, 19 May 2024 00:17:34 +0200 Subject: [PATCH] Fix default NodePaths saved in scene --- editor/editor_inspector.cpp | 2 +- editor/editor_node.cpp | 2 +- editor/scene_tree_dock.cpp | 2 +- scene/property_utils.cpp | 20 ++++++--- scene/property_utils.h | 2 +- scene/resources/packed_scene.cpp | 2 +- tests/scene/test_node.h | 73 ++++++++++++++++++++++++++++++++ 7 files changed, 91 insertions(+), 12 deletions(-) diff --git a/editor/editor_inspector.cpp b/editor/editor_inspector.cpp index c1ee2ef0e029..80e2302e91a4 100644 --- a/editor/editor_inspector.cpp +++ b/editor/editor_inspector.cpp @@ -480,7 +480,7 @@ bool EditorPropertyRevert::can_property_revert(Object *p_object, const StringNam return false; } Variant current_value = p_custom_current_value ? *p_custom_current_value : p_object->get(p_property); - return PropertyUtils::is_property_value_different(current_value, revert_value); + return PropertyUtils::is_property_value_different(p_object, current_value, revert_value); } StringName EditorProperty::_get_revert_property() const { diff --git a/editor/editor_node.cpp b/editor/editor_node.cpp index 2570e19b48f7..ebdad467ff61 100644 --- a/editor/editor_node.cpp +++ b/editor/editor_node.cpp @@ -4118,7 +4118,7 @@ HashMap EditorNode::get_modified_properties_for_node(Node * Variant revert_value = EditorPropertyRevert::get_property_revert_value(p_node, E.name, &is_valid_revert); Variant current_value = p_node->get(E.name); if (is_valid_revert) { - if (PropertyUtils::is_property_value_different(current_value, revert_value)) { + if (PropertyUtils::is_property_value_different(p_node, current_value, revert_value)) { // If this property is a direct node reference, save a NodePath instead to prevent corrupted references. if (E.type == Variant::OBJECT && E.hint == PROPERTY_HINT_NODE_TYPE) { Node *target_node = Object::cast_to(current_value); diff --git a/editor/scene_tree_dock.cpp b/editor/scene_tree_dock.cpp index 23588e0e6c8a..2f57dc56101d 100644 --- a/editor/scene_tree_dock.cpp +++ b/editor/scene_tree_dock.cpp @@ -4171,7 +4171,7 @@ void SceneTreeDock::_create_remap_for_node(Node *p_node, HashMap, bool is_valid_default = false; Variant orig = PropertyUtils::get_property_default_value(p_node, E.name, &is_valid_default, &states_stack); - if (is_valid_default && !PropertyUtils::is_property_value_different(v, orig)) { + if (is_valid_default && !PropertyUtils::is_property_value_different(p_node, v, orig)) { continue; } diff --git a/scene/property_utils.cpp b/scene/property_utils.cpp index be58f1c1e193..db17f9d64364 100644 --- a/scene/property_utils.cpp +++ b/scene/property_utils.cpp @@ -39,16 +39,22 @@ #include "editor/editor_node.h" #endif // TOOLS_ENABLED -bool PropertyUtils::is_property_value_different(const Variant &p_a, const Variant &p_b) { +bool PropertyUtils::is_property_value_different(const Object *p_object, const Variant &p_a, const Variant &p_b) { if (p_a.get_type() == Variant::FLOAT && p_b.get_type() == Variant::FLOAT) { - //this must be done because, as some scenes save as text, there might be a tiny difference in floats due to numerical error + // 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 != b; + } else if (p_a.get_type() == Variant::NODE_PATH && p_b.get_type() == Variant::OBJECT) { + const Node *base_node = Object::cast_to(p_object); + const Node *target_node = Object::cast_to(p_b); + if (base_node && target_node) { + return p_a != base_node->get_path_to(target_node); + } } + + // 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 != b; } Variant PropertyUtils::get_property_default_value(const Object *p_object, const StringName &p_property, bool *r_is_valid, const Vector *p_states_stack_cache, bool p_update_exports, const Node *p_owner, bool *r_is_class_default) { diff --git a/scene/property_utils.h b/scene/property_utils.h index e934792e34c1..0bf040ad267a 100644 --- a/scene/property_utils.h +++ b/scene/property_utils.h @@ -36,7 +36,7 @@ class PropertyUtils { public: - static bool is_property_value_different(const Variant &p_a, const Variant &p_b); + static bool is_property_value_different(const Object *p_object, const Variant &p_a, const Variant &p_b); // Gets the most pure default value, the one that would be set when the node has just been instantiated static Variant get_property_default_value(const Object *p_object, const StringName &p_property, bool *r_is_valid = nullptr, const Vector *p_states_stack_cache = nullptr, bool p_update_exports = false, const Node *p_owner = nullptr, bool *r_is_class_default = nullptr); diff --git a/scene/resources/packed_scene.cpp b/scene/resources/packed_scene.cpp index 0c57c6b7babc..b1742bd5a30b 100644 --- a/scene/resources/packed_scene.cpp +++ b/scene/resources/packed_scene.cpp @@ -816,7 +816,7 @@ Error SceneState::_parse_node(Node *p_owner, Node *p_node, int p_parent_idx, Has bool is_valid_default = false; Variant default_value = PropertyUtils::get_property_default_value(p_node, name, &is_valid_default, &states_stack, true); - if (is_valid_default && !PropertyUtils::is_property_value_different(value, default_value)) { + if (is_valid_default && !PropertyUtils::is_property_value_different(p_node, value, default_value)) { if (value.get_type() == Variant::ARRAY && has_local_resource(value)) { // Save anyway } else if (value.get_type() == Variant::DICTIONARY) { diff --git a/tests/scene/test_node.h b/tests/scene/test_node.h index b3362b02a86c..2b14be76e2f1 100644 --- a/tests/scene/test_node.h +++ b/tests/scene/test_node.h @@ -31,7 +31,9 @@ #ifndef TEST_NODE_H #define TEST_NODE_H +#include "core/object/class_db.h" #include "scene/main/node.h" +#include "scene/resources/packed_scene.h" #include "tests/test_macros.h" @@ -62,6 +64,12 @@ protected: } } + static void _bind_methods() { + ClassDB::bind_method(D_METHOD("set_exported_node", "node"), &TestNode::set_exported_node); + ClassDB::bind_method(D_METHOD("get_exported_node"), &TestNode::get_exported_node); + ADD_PROPERTY(PropertyInfo(Variant::OBJECT, "exported_node", PROPERTY_HINT_NODE_TYPE, "Node"), "set_exported_node", "get_exported_node"); + } + private: void push_self() { if (callback_list) { @@ -75,7 +83,12 @@ public: int process_counter = 0; int physics_process_counter = 0; + Node *exported_node = nullptr; + List *callback_list = nullptr; + + void set_exported_node(Node *p_node) { exported_node = p_node; } + Node *get_exported_node() const { return exported_node; } }; TEST_CASE("[SceneTree][Node] Testing node operations with a very simple scene tree") { @@ -478,6 +491,66 @@ TEST_CASE("[SceneTree][Node] Testing node operations with a more complex simple memdelete(node2); } +TEST_CASE("[SceneTree][Node]Exported node checks") { + TestNode *node = memnew(TestNode); + SceneTree::get_singleton()->get_root()->add_child(node); + + Node *child = memnew(Node); + child->set_name("Child"); + node->add_child(child); + child->set_owner(node); + + node->set("exported_node", child); + + SUBCASE("Property of duplicated node should point to duplicated child") { + GDREGISTER_CLASS(TestNode); + + TestNode *dup = Object::cast_to(node->duplicate()); + Node *new_exported = Object::cast_to(dup->get("exported_node")); + CHECK(new_exported == dup->get_child(0)); + + memdelete(dup); + } + + SUBCASE("Saving instance with exported node should not store the unchanged property") { + node->set_process_mode(Node::PROCESS_MODE_ALWAYS); + Ref ps; + ps.instantiate(); + ps->pack(node); + + String scene_path = OS::get_singleton()->get_cache_path().path_join("test_scene.tscn"); + ps->set_path(scene_path); + + Node *root = memnew(Node); + + Node *sub_child = ps->instantiate(PackedScene::GEN_EDIT_STATE_MAIN); + root->add_child(sub_child); + sub_child->set_owner(root); + + Ref ps2; + ps2.instantiate(); + ps2->pack(root); + + scene_path = OS::get_singleton()->get_cache_path().path_join("new_test_scene.tscn"); + ResourceSaver::save(ps2, scene_path); + memdelete(root); + + bool is_wrong = false; + Ref fa = FileAccess::open(scene_path, FileAccess::READ); + while (!fa->eof_reached()) { + const String line = fa->get_line(); + if (line.begins_with("exported_node")) { + // The property was saved, while it shouldn't. + is_wrong = true; + break; + } + } + CHECK_FALSE(is_wrong); + } + + memdelete(node); +} + TEST_CASE("[Node] Processing checks") { Node *node = memnew(Node);