Skip to content
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.

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

Fix default NodePaths saved in scene #92095

Merged
merged 1 commit into from
Jun 3, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion editor/editor_inspector.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
2 changes: 1 addition & 1 deletion editor/editor_node.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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 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<Node>(current_value);
Expand Down
2 changes: 1 addition & 1 deletion editor/scene_tree_dock.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4171,7 +4171,7 @@ void SceneTreeDock::_create_remap_for_node(Node *p_node, HashMap<Ref<Resource>,

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;
}

Expand Down
20 changes: 13 additions & 7 deletions scene/property_utils.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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<Node>(p_object);
const Node *target_node = Object::cast_to<Node>(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<SceneState::PackState> *p_states_stack_cache, bool p_update_exports, const Node *p_owner, bool *r_is_class_default) {
Expand Down
2 changes: 1 addition & 1 deletion scene/property_utils.h
Original file line number Diff line number Diff line change
Expand Up @@ -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<SceneState::PackState> *p_states_stack_cache = nullptr, bool p_update_exports = false, const Node *p_owner = nullptr, bool *r_is_class_default = nullptr);

Expand Down
2 changes: 1 addition & 1 deletion scene/resources/packed_scene.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
73 changes: 73 additions & 0 deletions tests/scene/test_node.h
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand Down Expand Up @@ -62,6 +64,12 @@ class TestNode : public Node {
}
}

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) {
Expand All @@ -75,7 +83,12 @@ class TestNode : public Node {
int process_counter = 0;
int physics_process_counter = 0;

Node *exported_node = 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") {
Expand Down Expand Up @@ -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<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") {
Node *node = memnew(Node);

Expand Down
Loading