wip

core/io/resource.cpp

@@ -248,11 +248,11 @@ Variant Resource::_duplicate_recursive_impl(const Variant &p_variant, const Dupl
 			const Ref<Resource> &sr = p_variant;
 			// If duplicating for local-to-scene, only duplicate resources marked as scene-local.
 			if (sr.is_valid() && (!p_params.local_scene || sr->is_local_to_scene())) {
-				if (p_params.remap_cache->has(sr)) {
-					return p_params.remap_cache->get(sr);
+				if (thread_duplicate_remap_cache->has(sr)) {
+					return thread_duplicate_remap_cache->get(sr);
 				} else {
 					const Ref<Resource> &dupe = sr->_duplicate_impl(p_params);
-					p_params.remap_cache->insert(sr, dupe);
+					thread_duplicate_remap_cache->insert(sr, dupe);
 					return dupe;
 				}
 			} else {
@@ -300,13 +300,22 @@ Variant Resource::_duplicate_recursive_impl(const Variant &p_variant, const Dupl
 }
 
 Ref<Resource> Resource::_duplicate_impl(const DuplicateParams &p_params) const {
+	DuplicateRemapCacheT *remap_cache_backup = thread_duplicate_remap_cache;
+
+// These are for avoiding potential duplicates that can happen in custom code
+// from participating in the same duplication session (remap cache).
+#define BEFORE_USER_CODE thread_duplicate_remap_cache = nullptr;
+#define AFTER_USER_CODE thread_duplicate_remap_cache = remap_cache_backup;
+
 	List<PropertyInfo> plist;
 	get_property_list(&plist);
 
+	BEFORE_USER_CODE
 	Ref<Resource> r = Object::cast_to<Resource>(ClassDB::instantiate(get_class()));
+	AFTER_USER_CODE
 	ERR_FAIL_COND_V(r.is_null(), Ref<Resource>());
 
-	p_params.remap_cache->insert(Ref<Resource>(this), r);
+	thread_duplicate_remap_cache->insert(Ref<Resource>(this), r);
 
 	if (p_params.local_scene) {
 		r->local_scene = p_params.local_scene;
@@ -317,7 +326,9 @@ Ref<Resource> Resource::_duplicate_impl(const DuplicateParams &p_params) const {
 			continue;
 		}
 
+		BEFORE_USER_CODE
 		Variant p = get(E.name);
+		AFTER_USER_CODE
 
 		if (p_params.deep) {
 			bool should_recurse = true;
@@ -332,23 +343,48 @@ Ref<Resource> Resource::_duplicate_impl(const DuplicateParams &p_params) const {
 			if (p.get_type() == Variant::OBJECT && (E.usage & PROPERTY_USAGE_ALWAYS_DUPLICATE)) {
 				const Ref<Resource> &sr = p;
 				if (sr.is_valid()) {
+					// Subclasses may override duplicate(). They will have to be
+					// aware of the remap cache if they need to change their behavior somehow.
 					p = sr->duplicate(true);
 				}
 			}
 		}
 
+		BEFORE_USER_CODE
 		r->set(E.name, p);
+		AFTER_USER_CODE
 	}
 
 	return r;
+
+#undef BEFORE_USER_CODE
+#undef AFTER_USER_CODE
 }
 
-Ref<Resource> Resource::duplicate_for_local_scene(Node *p_for_scene, HashMap<Ref<Resource>, Ref<Resource>> &p_remap_cache) const {
+Ref<Resource> Resource::duplicate_for_local_scene(Node *p_for_scene, DuplicateRemapCacheT &p_remap_cache) const {
+#ifdef DEBUG_ENABLED
+	// Since this is an entry point, it should always be able to start a duplication session.
+	// This check failing means that this function is being called during an ongoing duplication,
+	// likely due to custom instantiation or setter code. It would be an engine bug because code
+	// starting or joining a duplicate session must ensure to exit it temporarily making calls
+	// that may in turn invoke such custom code.
+	if (thread_duplicate_remap_cache) {
+		ERR_PRINT("Resource::duplicate_for_local_scene() called during an ongoing duplication session. This is an engine bug.");
+	}
+#endif
+
+	// Since we can handle it gracefully anyway, we do.
+	DuplicateRemapCacheT *remap_cache_backup = thread_duplicate_remap_cache;
+	thread_duplicate_remap_cache = &p_remap_cache;
+
 	DuplicateParams params;
 	params.deep = true;
 	params.local_scene = p_for_scene;
-	params.remap_cache = &p_remap_cache;
-	return _duplicate_impl(params);
+	const Ref<Resource> &dupe = _duplicate_impl(params);
+
+	thread_duplicate_remap_cache = remap_cache_backup;
+
+	return dupe;
 }
 
 void Resource::_find_sub_resources(const Variant &p_variant, HashSet<Ref<Resource>> &p_resources_found) {
@@ -379,7 +415,7 @@ void Resource::_find_sub_resources(const Variant &p_variant, HashSet<Ref<Resourc
 	}
 }
 
-void Resource::configure_for_local_scene(Node *p_for_scene, HashMap<Ref<Resource>, Ref<Resource>> &p_remap_cache) {
+void Resource::configure_for_local_scene(Node *p_for_scene, DuplicateRemapCacheT &p_remap_cache) {
 	List<PropertyInfo> plist;
 	get_property_list(&plist);
 
@@ -407,11 +443,53 @@ void Resource::configure_for_local_scene(Node *p_for_scene, HashMap<Ref<Resource
 }
 
 Ref<Resource> Resource::duplicate(bool p_subresources) const {
-	HashMap<Ref<Resource>, Ref<Resource>> remap_cache;
+	DuplicateRemapCacheT remap_cache;
+	bool started_session = false;
+	if (!thread_duplicate_remap_cache) {
+		thread_duplicate_remap_cache = &remap_cache;
+		started_session = true;
+	}
+
 	DuplicateParams params;
 	params.deep = p_subresources;
-	params.remap_cache = &remap_cache;
-	return _duplicate_impl(params);
+	const Ref<Resource> dupe = _duplicate_impl(params);
+
+	if (started_session) {
+		thread_duplicate_remap_cache = nullptr;
+	}
+
+	return dupe;
+}
+
+Ref<Resource> Resource::_duplicate_from_variant(int p_recursion_count) const {
+	// When duplicating from Variant, this function may be called multiple times from
+	// different parts of the data structure being copied. Therefore, we need to create
+	// a remap cache instance in a way that can be shared among all of the calls.
+	// Whatever Variant, Array or Dictionary that initiated the call chain will eventually
+	// claim it, when the stack unwinds up to the root call.
+	// One exception is that this is the root call.
+
+	if (p_recursion_count == 0) {
+		return duplicate(true);
+	}
+
+	if (!thread_duplicate_remap_cache) {
+		thread_duplicate_remap_cache = memnew(DuplicateRemapCacheT);
+	}
+
+	DuplicateParams params;
+	params.deep = true;
+
+	const Ref<Resource> dupe = _duplicate_impl(params);
+
+	return dupe;
+}
+
+void Resource::_teardown_duplicate_from_variant() {
+	if (thread_duplicate_remap_cache) {
+		memdelete(thread_duplicate_remap_cache);
+		thread_duplicate_remap_cache = nullptr;
+	}
 }
 
 void Resource::_set_path(const String &p_path) {

core/io/resource.h

@@ -74,10 +74,13 @@ class Resource : public RefCounted {
 
 	SelfList<Resource> remapped_list;
 
+
+	using DuplicateRemapCacheT = HashMap<Ref<Resource>, Ref<Resource>>;
+	static thread_local inline DuplicateRemapCacheT *thread_duplicate_remap_cache = nullptr;
+
 	struct DuplicateParams {
 		bool deep = false; // With subresources?
 		Node *local_scene = nullptr;
-		HashMap<Ref<Resource>, Ref<Resource>> *remap_cache = nullptr;
 	};
 	Ref<Resource> _duplicate_impl(const DuplicateParams &p_params) const;
 	Variant _duplicate_recursive_impl(const Variant &p_variant, const DuplicateParams &p_params) const;
@@ -126,8 +129,10 @@ class Resource : public RefCounted {
 	String get_scene_unique_id() const;
 
 	virtual Ref<Resource> duplicate(bool p_subresources = false) const;
-	Ref<Resource> duplicate_for_local_scene(Node *p_for_scene, HashMap<Ref<Resource>, Ref<Resource>> &p_remap_cache) const;
-	void configure_for_local_scene(Node *p_for_scene, HashMap<Ref<Resource>, Ref<Resource>> &p_remap_cache);
+	Ref<Resource> _duplicate_from_variant(int p_recursion_count) const;
+	static void _teardown_duplicate_from_variant();
+	Ref<Resource> duplicate_for_local_scene(Node *p_for_scene, DuplicateRemapCacheT &p_remap_cache) const;
+	void configure_for_local_scene(Node *p_for_scene, DuplicateRemapCacheT &p_remap_cache);
 
 	void set_local_to_scene(bool p_enable);
 	bool is_local_to_scene() const;

core/variant/array.cpp

@@ -511,12 +511,19 @@ Array Array::recursive_duplicate(bool p_deep, int recursion_count) const {
 	}
 
 	if (p_deep) {
+		bool is_call_chain_end = recursion_count == 0;
+
 		recursion_count++;
 		int element_count = size();
 		new_arr.resize(element_count);
 		for (int i = 0; i < element_count; i++) {
 			new_arr[i] = get(i).recursive_duplicate(true, recursion_count);
 		}
+
+		// Variant::recursive_duplicate() may have created a remap cache by now.
+		if (is_call_chain_end) {
+			Resource::_teardown_duplicate_from_variant();
+		}
 	} else {
 		new_arr._p->array = _p->array;
 	}

core/variant/dictionary.cpp

@@ -582,10 +582,17 @@ Dictionary Dictionary::recursive_duplicate(bool p_deep, int recursion_count) con
 	}
 
 	if (p_deep) {
+		bool is_call_chain_end = recursion_count == 0;
+
 		recursion_count++;
 		for (const KeyValue<Variant, Variant> &E : _p->variant_map) {
 			n[E.key.recursive_duplicate(true, recursion_count)] = E.value.recursive_duplicate(true, recursion_count);
 		}
+
+		// Variant::recursive_duplicate() may have created a remap cache by now.
+		if (is_call_chain_end) {
+			Resource::_teardown_duplicate_from_variant();
+		}
 	} else {
 		for (const KeyValue<Variant, Variant> &E : _p->variant_map) {
 			n[E.key] = E.value;

core/variant/variant_setget.cpp

@@ -32,6 +32,8 @@
 
 #include "variant_callable.h"
 
+#include "core/io/resource.h"
+
 struct VariantSetterGetterInfo {
 	void (*setter)(Variant *base, const Variant *value, bool &valid);
 	void (*getter)(const Variant *base, Variant *value);
@@ -1940,15 +1942,12 @@ Variant Variant::duplicate(bool p_deep) const {
 Variant Variant::recursive_duplicate(bool p_deep, int recursion_count) const {
 	switch (type) {
 		case OBJECT: {
-			/*  breaks stuff :(
-			if (p_deep && !_get_obj().ref.is_null()) {
-				Ref<Resource> resource = _get_obj().ref;
-				if (resource.is_valid()) {
-					return resource->duplicate(true);
-				}
+			Resource *res = Object::cast_to<Resource>(_get_obj().obj);
+			if (res) {
+				return res->_duplicate_from_variant(recursion_count);
+			} else {
+				return *this;
 			}
-			*/
-			return *this;
 		} break;
 		case DICTIONARY:
 			return operator Dictionary().recursive_duplicate(p_deep, recursion_count);

doc/classes/Array.xml

@@ -331,8 +331,9 @@
 			<return type="Array" />
 			<param index="0" name="deep" type="bool" default="false" />
 			<description>
-				Returns a new copy of the array.
-				By default, a [b]shallow[/b] copy is returned: all nested [Array] and [Dictionary] elements are shared with the original array. Modifying them in one array will also affect them in the other.[br]If [param deep] is [code]true[/code], a [b]deep[/b] copy is returned: all nested arrays and dictionaries are also duplicated (recursively).
+				Creates and returns a new copy of the array.
+				By default, a [b]shallow[/b] copy is returned: all nested [Array], [Dictionary] and [Resource] elements are shared with the original array. Modifying them in one array will also affect them in the other.
+				If [param deep] is [code]true[/code], a [b]deep[/b] copy is returned: all nested arrays, dictionaries and resources are also duplicated (recursively). See [method Dictionary.duplicate] and [Resource.duplicate] for more information.
 			</description>
 		</method>
 		<method name="erase">

doc/classes/Dictionary.xml

@@ -187,7 +187,9 @@
 			<return type="Dictionary" />
 			<param index="0" name="deep" type="bool" default="false" />
 			<description>
-				Creates and returns a new copy of the dictionary. If [param deep] is [code]true[/code], inner [Dictionary] and [Array] keys and values are also copied, recursively.
+				Creates and returns a new copy of the dictionary.
+				By default, a [b]shallow[/b] copy is returned: all nested [Array], [Dictionary] and [Resource] elements are shared with the original dictionary. Modifying them in one array will also affect them in the other.
+				If [param deep] is [code]true[/code], a [b]deep[/b] copy is returned: all nested arrays, dictionaries and resources are also duplicated (recursively). See [method Array.duplicate] and [Resource.duplicate] for more information.
 			</description>
 		</method>
 		<method name="erase">

doc/classes/Resource.xml

@@ -54,7 +54,7 @@
 			<description>
 				Duplicates this resource, returning a new resource with its [code]export[/code]ed or [constant PROPERTY_USAGE_STORAGE] properties copied from the original.
 				If [param subresources] is [code]false[/code], a shallow copy is returned; nested resources within subresources are not duplicated and are shared with the original resource (with one exception; see below).
-				If [param subresources] is [code]true[/code], a deep copy is returned; nested subresources will be duplicated and are not shared (with two exceptions; see below). Nested arrays and dictionaries will also be recursively duplicated.
+				If [param subresources] is [code]true[/code], a deep copy is returned; nested subresources will be duplicated and are not shared (with two exceptions; see below). Nested arrays and dictionaries will also be recursively duplicated. See [method Array.duplicate] and [Dictionary.duplicate] for more information.
 				[param subresources] is usually respected, with the following exceptions:
 				- Subresource properties with the [constant PROPERTY_USAGE_ALWAYS_DUPLICATE] flag are always duplicated (recursively).
 				- Subresource properties with the [constant PROPERTY_USAGE_NEVER_DUPLICATE] flag are never duplicated.