From 153b5266de221d4affc283b0c44d0ede80212029 Mon Sep 17 00:00:00 2001
From: Pablo Andres Fuente <pfuente@heroku.com>
Date: Mon, 16 Sep 2024 11:48:04 -0300
Subject: [PATCH] Making `inst_to_dict` and `dict_to_inst` recursive

Fixes #6533

Making GDScript inst_to_dict/dict_to_inst utility functions recursive.
Adding also a new macro to validate the number of the required arguments
and another to validate that an argument is boolean.
---
 modules/gdscript/doc_classes/@GDScript.xml    |  4 +
 .../gdscript/gdscript_utility_functions.cpp   | 93 ++++++++++++++++---
 .../scripts/utility_functions/bar.notest.gd   |  9 ++
 .../tests/scripts/utility_functions/bar.tres  |  9 ++
 .../errors/inst_to_dict_not_resource.gd       | 10 ++
 .../errors/inst_to_dict_not_resource.out      |  6 ++
 .../scripts/utility_functions/foo.notest.gd   |  9 ++
 .../tests/scripts/utility_functions/foo.tres  |  9 ++
 .../scripts/utility_functions/inst_to_dict.gd | 54 +++++++++++
 .../utility_functions/inst_to_dict.out        |  6 ++
 .../scripts/utility_functions/qux.notest.gd   |  7 ++
 .../tests/scripts/utility_functions/qux.tres  |  7 ++
 12 files changed, 208 insertions(+), 15 deletions(-)
 create mode 100644 modules/gdscript/tests/scripts/utility_functions/bar.notest.gd
 create mode 100644 modules/gdscript/tests/scripts/utility_functions/bar.tres
 create mode 100644 modules/gdscript/tests/scripts/utility_functions/errors/inst_to_dict_not_resource.gd
 create mode 100644 modules/gdscript/tests/scripts/utility_functions/errors/inst_to_dict_not_resource.out
 create mode 100644 modules/gdscript/tests/scripts/utility_functions/foo.notest.gd
 create mode 100644 modules/gdscript/tests/scripts/utility_functions/foo.tres
 create mode 100644 modules/gdscript/tests/scripts/utility_functions/inst_to_dict.gd
 create mode 100644 modules/gdscript/tests/scripts/utility_functions/inst_to_dict.out
 create mode 100644 modules/gdscript/tests/scripts/utility_functions/qux.notest.gd
 create mode 100644 modules/gdscript/tests/scripts/utility_functions/qux.tres

diff --git a/modules/gdscript/doc_classes/@GDScript.xml b/modules/gdscript/doc_classes/@GDScript.xml
index ede4ce6617aa..17dc5d6b4fce 100644
--- a/modules/gdscript/doc_classes/@GDScript.xml
+++ b/modules/gdscript/doc_classes/@GDScript.xml
@@ -77,8 +77,10 @@
 		<method name="dict_to_inst">
 			<return type="Object" />
 			<param index="0" name="dictionary" type="Dictionary" />
+			<param index="1" name="deep" type="bool" default="false" />
 			<description>
 				Converts a [param dictionary] (created with [method inst_to_dict]) back to an Object instance. Can be useful for deserializing.
+				If [param deep] is [code]true[/code], the method converts any inner instances recursively. Use this option only if you're sure that instances have no circular references to each other as it can lead to an endless conversion loop.
 			</description>
 		</method>
 		<method name="get_stack">
@@ -106,8 +108,10 @@
 		<method name="inst_to_dict">
 			<return type="Dictionary" />
 			<param index="0" name="instance" type="Object" />
+			<param index="1" name="deep" type="bool" default="false" />
 			<description>
 				Returns the passed [param instance] converted to a Dictionary. Can be useful for serializing.
+				If [param deep] is [code]true[/code], the method converts any inner instances recursively. Use this option only if you're sure that instances have no circular references to each other as it can lead to an endless conversion loop.
 				[b]Note:[/b] Cannot be used to serialize objects with built-in scripts attached or objects allocated within built-in scripts.
 				[codeblock]
 				var foo = "bar"
diff --git a/modules/gdscript/gdscript_utility_functions.cpp b/modules/gdscript/gdscript_utility_functions.cpp
index 82460696964d..bcbd80301bce 100644
--- a/modules/gdscript/gdscript_utility_functions.cpp
+++ b/modules/gdscript/gdscript_utility_functions.cpp
@@ -224,16 +224,21 @@ struct GDScriptUtilityFunctionsDefinitions {
 		*r_ret = ResourceLoader::load(*p_args[0]);
 	}
 
-	static inline void inst_to_dict(Variant *r_ret, const Variant **p_args, int p_arg_count, Callable::CallError &r_error) {
-		DEBUG_VALIDATE_ARG_COUNT(1, 1);
-		DEBUG_VALIDATE_ARG_TYPE(0, Variant::OBJECT);
+	static inline void _inst_to_dict(Variant *r_ret, const Variant *p_var, bool p_deep, int p_recursion_count, Callable::CallError &r_error) {
+		if (p_var->get_type() == Variant::NIL) {
+			*r_ret = Variant();
+			return;
+		}
 
-		if (p_args[0]->get_type() == Variant::NIL) {
+		if (unlikely(!Variant::can_convert_strict(p_var->get_type(), Variant::OBJECT))) {
 			*r_ret = Variant();
+			r_error.error = Callable::CallError::CALL_ERROR_INVALID_ARGUMENT;
+			r_error.argument = 0;
+			r_error.expected = Variant::OBJECT;
 			return;
 		}
 
-		Object *obj = *p_args[0];
+		Object *obj = *p_var;
 		if (!obj) {
 			*r_ret = Variant();
 			return;
@@ -268,19 +273,52 @@ struct GDScriptUtilityFunctionsDefinitions {
 
 		for (const KeyValue<StringName, GDScript::MemberInfo> &E : base->member_indices) {
 			if (!d.has(E.key)) {
-				d[E.key] = inst->members[E.value.index];
+				Variant member = inst->members[E.value.index];
+				if (p_deep && member.get_type() == Variant::OBJECT) {
+					Variant inner_dict;
+					_inst_to_dict(&inner_dict, &member, p_deep, ++p_recursion_count, r_error);
+					if (r_error.error != Callable::CallError::CALL_OK) {
+						return;
+					} else {
+						member = inner_dict;
+					}
+				}
+				d[E.key] = member;
 			}
 		}
 
 		*r_ret = d;
+		return;
 	}
 
-	static inline void dict_to_inst(Variant *r_ret, const Variant **p_args, int p_arg_count, Callable::CallError &r_error) {
-		DEBUG_VALIDATE_ARG_COUNT(1, 1);
-		DEBUG_VALIDATE_ARG_TYPE(0, Variant::DICTIONARY);
+	static inline void inst_to_dict(Variant *r_ret, const Variant **p_args, int p_arg_count, Callable::CallError &r_error) {
+		DEBUG_VALIDATE_ARG_COUNT(1, 2);
+
+		bool deep = false;
+		if (p_arg_count > 1) {
+			DEBUG_VALIDATE_ARG_TYPE(1, Variant::BOOL);
+			deep = *p_args[1];
+		}
 
-		Dictionary d = *p_args[0];
+		_inst_to_dict(r_ret, p_args[0], deep, 1, r_error);
+	}
 
+	static inline void _dict_to_inst(Variant *r_ret, const Variant *p_var, bool p_deep, int p_recursion_count, Callable::CallError &r_error) {
+		if (p_recursion_count > MAX_RECURSION) {
+			ERR_PRINT("Max recursion reached");
+			*r_ret = Variant();
+			return;
+		}
+
+		if (unlikely(!Variant::can_convert_strict(p_var->get_type(), Variant::DICTIONARY))) {
+			*r_ret = Variant();
+			r_error.error = Callable::CallError::CALL_ERROR_INVALID_ARGUMENT;
+			r_error.argument = 0;
+			r_error.expected = Variant::DICTIONARY;
+			return;
+		}
+
+		Dictionary d = *p_var;
 		VALIDATE_ARG_CUSTOM(0, Variant::DICTIONARY, !d.has("@path"), RTR("Invalid instance dictionary format (missing @path)."));
 
 		Ref<Script> scr = ResourceLoader::load(d["@path"]);
@@ -299,20 +337,45 @@ struct GDScriptUtilityFunctionsDefinitions {
 			VALIDATE_ARG_CUSTOM(0, Variant::DICTIONARY, !gdscr.is_valid(), RTR("Invalid instance dictionary (invalid subclasses)."));
 		}
 
-		*r_ret = gdscr->_new(nullptr, -1 /* skip initializer */, r_error);
+		Variant ret = gdscr->_new(nullptr, -1 /* skip initializer */, r_error);
 		if (r_error.error != Callable::CallError::CALL_OK) {
 			*r_ret = RTR("Cannot instantiate GDScript class.");
 			return;
 		}
 
-		GDScriptInstance *inst = static_cast<GDScriptInstance *>(static_cast<Object *>(*r_ret)->get_script_instance());
+		GDScriptInstance *inst = static_cast<GDScriptInstance *>(static_cast<Object *>(ret)->get_script_instance());
 		Ref<GDScript> gd_ref = inst->get_script();
 
 		for (KeyValue<StringName, GDScript::MemberInfo> &E : gd_ref->member_indices) {
 			if (d.has(E.key)) {
-				inst->members.write[E.value.index] = d[E.key];
+				Variant member = d[E.key];
+				if (p_deep && member.get_type() == Variant::DICTIONARY) {
+					Variant inner_instance;
+					_dict_to_inst(&inner_instance, &member, p_deep, ++p_recursion_count, r_error);
+					if (r_error.error != Callable::CallError::CALL_OK) {
+						return;
+					} else {
+						member = inner_instance;
+					}
+				}
+				inst->members.write[E.value.index] = member;
 			}
 		}
+
+		*r_ret = ret;
+		return;
+	}
+
+	static inline void dict_to_inst(Variant *r_ret, const Variant **p_args, int p_arg_count, Callable::CallError &r_error) {
+		DEBUG_VALIDATE_ARG_COUNT(1, 2);
+
+		bool deep = false;
+		if (p_arg_count > 1) {
+			DEBUG_VALIDATE_ARG_TYPE(1, Variant::BOOL);
+			deep = *p_args[1];
+		}
+
+		_dict_to_inst(r_ret, p_args[0], deep, 1, r_error);
 	}
 
 	static inline void Color8(Variant *r_ret, const Variant **p_args, int p_arg_count, Callable::CallError &r_error) {
@@ -575,8 +638,8 @@ void GDScriptUtilityFunctions::register_functions() {
 	REGISTER_FUNC( _char,          true,  RET(STRING),        ARGS( ARG("char", INT)                ), false, varray(     ));
 	REGISTER_FUNC( range,          false, RET(ARRAY),         NOARGS,                                  true,  varray(     ));
 	REGISTER_FUNC( load,           false, RETCLS("Resource"), ARGS( ARG("path", STRING)             ), false, varray(     ));
-	REGISTER_FUNC( inst_to_dict,   false, RET(DICTIONARY),    ARGS( ARG("instance", OBJECT)         ), false, varray(     ));
-	REGISTER_FUNC( dict_to_inst,   false, RET(OBJECT),        ARGS( ARG("dictionary", DICTIONARY)   ), false, varray(     ));
+	REGISTER_FUNC( inst_to_dict,   false, RET(DICTIONARY),    ARGS( ARG("instance", OBJECT), ARG("deep", BOOL)       ), false, varray( false ));
+	REGISTER_FUNC( dict_to_inst,   false, RET(OBJECT),        ARGS( ARG("dictionary", DICTIONARY), ARG("deep", BOOL) ), false, varray( false ));
 	REGISTER_FUNC( Color8,         true,  RET(COLOR),         ARGS( ARG("r8", INT), ARG("g8", INT),
 																	ARG("b8", INT), ARG("a8", INT)  ), false, varray( 255 ));
 	REGISTER_FUNC( print_debug,    false, RET(NIL),           NOARGS,                                  true,  varray(     ));
diff --git a/modules/gdscript/tests/scripts/utility_functions/bar.notest.gd b/modules/gdscript/tests/scripts/utility_functions/bar.notest.gd
new file mode 100644
index 000000000000..a5fcba8185f7
--- /dev/null
+++ b/modules/gdscript/tests/scripts/utility_functions/bar.notest.gd
@@ -0,0 +1,9 @@
+extends Resource
+
+@export var text: String
+@export var qux: Resource
+
+
+func _init(p_text = "", p_qux = null):
+	text = p_text
+	qux = p_qux
diff --git a/modules/gdscript/tests/scripts/utility_functions/bar.tres b/modules/gdscript/tests/scripts/utility_functions/bar.tres
new file mode 100644
index 000000000000..4d3202ddc8ec
--- /dev/null
+++ b/modules/gdscript/tests/scripts/utility_functions/bar.tres
@@ -0,0 +1,9 @@
+[gd_resource type="Resource" load_steps=2 format=3 uid="uid://cqd4xsl30rr7b"]
+
+[ext_resource type="Script" path="./bar.notest.gd" id="1_bar"]
+[ext_resource type="Script" path="./qux.tres" id="1_qux"]
+
+[resource]
+script = ExtResource("1_bar")
+text = "lorem ipsum"
+qux = ExtResource("1_qux")
diff --git a/modules/gdscript/tests/scripts/utility_functions/errors/inst_to_dict_not_resource.gd b/modules/gdscript/tests/scripts/utility_functions/errors/inst_to_dict_not_resource.gd
new file mode 100644
index 000000000000..fe2cc820c101
--- /dev/null
+++ b/modules/gdscript/tests/scripts/utility_functions/errors/inst_to_dict_not_resource.gd
@@ -0,0 +1,10 @@
+extends Resource
+
+
+class NotResource:
+	@export var number: int
+
+func test():
+	var obj:NotResource = NotResource.new()
+	inst_to_dict(obj)
+	print('not ok')
diff --git a/modules/gdscript/tests/scripts/utility_functions/errors/inst_to_dict_not_resource.out b/modules/gdscript/tests/scripts/utility_functions/errors/inst_to_dict_not_resource.out
new file mode 100644
index 000000000000..9f8aae02b393
--- /dev/null
+++ b/modules/gdscript/tests/scripts/utility_functions/errors/inst_to_dict_not_resource.out
@@ -0,0 +1,6 @@
+GDTEST_RUNTIME_ERROR
+>> SCRIPT ERROR
+>> on function: test()
+>> utility_functions/errors/inst_to_dict_not_resource.gd
+>> 9
+>> Error calling GDScript utility function "inst_to_dict()": Not based on a resource file.
diff --git a/modules/gdscript/tests/scripts/utility_functions/foo.notest.gd b/modules/gdscript/tests/scripts/utility_functions/foo.notest.gd
new file mode 100644
index 000000000000..ffd35aa7bc03
--- /dev/null
+++ b/modules/gdscript/tests/scripts/utility_functions/foo.notest.gd
@@ -0,0 +1,9 @@
+extends Resource
+
+@export var number: int
+@export var bar: Resource
+
+
+func _init(p_number = 0, p_bar = null):
+	number = p_number
+	bar = p_bar
diff --git a/modules/gdscript/tests/scripts/utility_functions/foo.tres b/modules/gdscript/tests/scripts/utility_functions/foo.tres
new file mode 100644
index 000000000000..758745253cfd
--- /dev/null
+++ b/modules/gdscript/tests/scripts/utility_functions/foo.tres
@@ -0,0 +1,9 @@
+[gd_resource type="Resource" load_steps=2 format=3 uid="uid://cqd4xsl30rr7a"]
+
+[ext_resource type="Script" path="./foo.notest.gd" id="1_foo"]
+[ext_resource type="Script" path="./bar.tres" id="1_bar"]
+
+[resource]
+script = ExtResource("1_foo")
+number = 42
+bar = ExtResource("1_bar")
diff --git a/modules/gdscript/tests/scripts/utility_functions/inst_to_dict.gd b/modules/gdscript/tests/scripts/utility_functions/inst_to_dict.gd
new file mode 100644
index 000000000000..28657fdf52a8
--- /dev/null
+++ b/modules/gdscript/tests/scripts/utility_functions/inst_to_dict.gd
@@ -0,0 +1,54 @@
+extends Resource
+
+var foo = preload("./foo.tres")
+var foo_bar_null = preload("./foo.tres")
+
+
+func test():
+	# Sunny day
+	var obj:Object = foo
+	var dict:Dictionary = inst_to_dict(obj)
+	var dict_ok = true
+	dict_ok = dict_ok && dict.get("number") == 42
+	dict_ok = dict_ok && dict.get("bar") is Resource
+	if not dict_ok:
+		printerr("Can't convert foo instance to dictionary properly")
+
+	dict = inst_to_dict(obj, true)
+	print(dict.keys())
+	print(dict.values())
+
+	var inst = dict_to_inst(dict, true)
+	var equals = true
+	equals = equals && foo.number == inst.number
+	equals = equals && foo.bar.text == inst.bar.text
+	equals = equals && foo.bar.qux.decimal == inst.bar.qux.decimal
+	if not equals:
+		printerr("Can't revert from foo instance to dictionary properly")
+
+	# null in inner object
+	foo_bar_null.bar = null
+	obj = foo_bar_null
+	dict = inst_to_dict(obj)
+	dict_ok = true
+	dict_ok = dict_ok && dict.get("number") == 42
+	dict_ok = dict_ok && dict.get("bar") == null
+	if not dict_ok:
+		printerr("Can't convert foo_bar_null instance to dictionary properly")
+
+	dict = inst_to_dict(obj, true)
+	print(dict.keys())
+	print(dict.values())
+
+	inst = dict_to_inst(dict, true)
+	equals = true
+	equals = equals && foo.number == inst.number
+	equals = equals && foo.bar == null
+	if not equals:
+		printerr("Can't revert from foo_bar_null instance to dictionary properly")
+
+	var should_be_null = inst_to_dict(null)
+	if should_be_null != null:
+		printerr("It should return null")
+
+	print('ok')
diff --git a/modules/gdscript/tests/scripts/utility_functions/inst_to_dict.out b/modules/gdscript/tests/scripts/utility_functions/inst_to_dict.out
new file mode 100644
index 000000000000..9fbf595141ec
--- /dev/null
+++ b/modules/gdscript/tests/scripts/utility_functions/inst_to_dict.out
@@ -0,0 +1,6 @@
+GDTEST_OK
+["@subpath", "@path", &"number", &"bar"]
+[^"", "res://utility_functions/foo.notest.gd", 42, { "@subpath": ^"", "@path": "res://utility_functions/bar.notest.gd", &"text": "lorem ipsum", &"qux": { "@subpath": ^"", "@path": "res://utility_functions/qux.notest.gd", &"decimal": 0.5 } }]
+["@subpath", "@path", &"number", &"bar"]
+[^"", "res://utility_functions/foo.notest.gd", 42, <null>]
+ok
diff --git a/modules/gdscript/tests/scripts/utility_functions/qux.notest.gd b/modules/gdscript/tests/scripts/utility_functions/qux.notest.gd
new file mode 100644
index 000000000000..f85d7c2daa39
--- /dev/null
+++ b/modules/gdscript/tests/scripts/utility_functions/qux.notest.gd
@@ -0,0 +1,7 @@
+extends Resource
+
+@export var decimal: float
+
+
+func _init(p_decimal = ""):
+	decimal = p_decimal
diff --git a/modules/gdscript/tests/scripts/utility_functions/qux.tres b/modules/gdscript/tests/scripts/utility_functions/qux.tres
new file mode 100644
index 000000000000..3acc93b25c2d
--- /dev/null
+++ b/modules/gdscript/tests/scripts/utility_functions/qux.tres
@@ -0,0 +1,7 @@
+[gd_resource type="Resource" load_steps=2 format=3 uid="uid://cqd4xsl30rr7c"]
+
+[ext_resource type="Script" path="./qux.notest.gd" id="1_qux"]
+
+[resource]
+script = ExtResource("1_qux")
+decimal = 0.5