Fix default NodePaths saved in scene

This commit is contained in:
kobewi 2024-05-19 00:17:34 +02:00
parent 0d11108a01
commit be111004dd
7 changed files with 91 additions and 12 deletions

View File

@ -480,7 +480,7 @@ bool EditorPropertyRevert::can_property_revert(Object *p_object, const StringNam
return false; return false;
} }
Variant current_value = p_custom_current_value ? *p_custom_current_value : p_object->get(p_property); 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 { StringName EditorProperty::_get_revert_property() const {

View File

@ -4118,7 +4118,7 @@ HashMap<StringName, Variant> EditorNode::get_modified_properties_for_node(Node *
Variant revert_value = EditorPropertyRevert::get_property_revert_value(p_node, E.name, &is_valid_revert); Variant revert_value = EditorPropertyRevert::get_property_revert_value(p_node, E.name, &is_valid_revert);
Variant current_value = p_node->get(E.name); Variant current_value = p_node->get(E.name);
if (is_valid_revert) { 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 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) { if (E.type == Variant::OBJECT && E.hint == PROPERTY_HINT_NODE_TYPE) {
Node *target_node = Object::cast_to<Node>(current_value); Node *target_node = Object::cast_to<Node>(current_value);

View File

@ -4171,7 +4171,7 @@ void SceneTreeDock::_create_remap_for_node(Node *p_node, HashMap<Ref<Resource>,
bool is_valid_default = false; bool is_valid_default = false;
Variant orig = PropertyUtils::get_property_default_value(p_node, E.name, &is_valid_default, &states_stack); 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; continue;
} }

View File

@ -39,16 +39,22 @@
#include "editor/editor_node.h" #include "editor/editor_node.h"
#endif // TOOLS_ENABLED #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) { 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); return !Math::is_equal_approx((float)p_a, (float)p_b);
} else { } else if (p_a.get_type() == Variant::NODE_PATH && p_b.get_type() == Variant::OBJECT) {
// For our purposes, treating null object as NIL is the right thing to do const Node *base_node = Object::cast_to<Node>(p_object);
const Variant &a = p_a.get_type() == Variant::OBJECT && (Object *)p_a == nullptr ? Variant() : p_a; const Node *target_node = Object::cast_to<Node>(p_b);
const Variant &b = p_b.get_type() == Variant::OBJECT && (Object *)p_b == nullptr ? Variant() : p_b; if (base_node && target_node) {
return a != b; 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<SceneState::PackState> *p_states_stack_cache, bool p_update_exports, const Node *p_owner, bool *r_is_class_default) { 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) {

View File

@ -36,7 +36,7 @@
class PropertyUtils { class PropertyUtils {
public: 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 // 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<SceneState::PackState> *p_states_stack_cache = nullptr, bool p_update_exports = false, const Node *p_owner = nullptr, bool *r_is_class_default = nullptr); static Variant get_property_default_value(const Object *p_object, const StringName &p_property, bool *r_is_valid = nullptr, const Vector<SceneState::PackState> *p_states_stack_cache = nullptr, bool p_update_exports = false, const Node *p_owner = nullptr, bool *r_is_class_default = nullptr);

View File

@ -816,7 +816,7 @@ Error SceneState::_parse_node(Node *p_owner, Node *p_node, int p_parent_idx, Has
bool is_valid_default = false; bool is_valid_default = false;
Variant default_value = PropertyUtils::get_property_default_value(p_node, name, &is_valid_default, &states_stack, true); 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)) { if (value.get_type() == Variant::ARRAY && has_local_resource(value)) {
// Save anyway // Save anyway
} else if (value.get_type() == Variant::DICTIONARY) { } else if (value.get_type() == Variant::DICTIONARY) {

View File

@ -31,7 +31,9 @@
#ifndef TEST_NODE_H #ifndef TEST_NODE_H
#define TEST_NODE_H #define TEST_NODE_H
#include "core/object/class_db.h"
#include "scene/main/node.h" #include "scene/main/node.h"
#include "scene/resources/packed_scene.h"
#include "tests/test_macros.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: private:
void push_self() { void push_self() {
if (callback_list) { if (callback_list) {
@ -75,7 +83,12 @@ public:
int process_counter = 0; int process_counter = 0;
int physics_process_counter = 0; int physics_process_counter = 0;
Node *exported_node = nullptr;
List<Node *> *callback_list = nullptr; List<Node *> *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") { 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); 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<TestNode>(node->duplicate());
Node *new_exported = Object::cast_to<Node>(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<PackedScene> 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<PackedScene> 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<FileAccess> 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") { TEST_CASE("[Node] Processing checks") {
Node *node = memnew(Node); Node *node = memnew(Node);