Merge pull request #92095 from KoBeWi/TDD,_never_again

Fix default NodePaths saved in scene

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 {

editor/editor_node.cpp

@@ -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);

editor/scene_tree_dock.cpp

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

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<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) {

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<SceneState::PackState> *p_states_stack_cache = nullptr, bool p_update_exports = false, const Node *p_owner = nullptr, bool *r_is_class_default = nullptr);
 

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) {

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<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") {
@@ -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);