From bb1967fc2a66245582f152938115cad1cdbdbf1a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pedro=20J=2E=20Est=C3=A9banez?= Date: Tue, 17 Dec 2024 11:33:35 +0100 Subject: [PATCH] Overhaul `Resource::duplicate()` - Thanks to a refactor, `Resource::duplicate_for_local_scene()` and `Resource::duplicate()` are now both users of the same, parametrized, implementation - `Resource::duplicate()` now honors with/without subresources modes better. Among other changes, packed arrays, arrays and dictionaries are copied if, and only if, subresource duplication is requested. Given the lack of separate control for both, that gives a more predictable outcome. Separate control would be not possible anyway because duplicating deep resources may need to duplicate the arrays or dictionaries where it's involved regardless user will. The behavior afte this change is as follows: - With subresources: - Previously, only resources found as direct property values of the one to copy would be, recursively, duplicated. - Now, in addition, arrays and dictionaries are walked so the copy is truly deep. - The behavior with respect to packed arrays, arrays and dictionaries stays: recursive duplication. - Without subresources: - Previously, the first level of resources found as direct property values would be duplicated unconditionally. Packed arrays, arrays and dictionaries were non-recursively duplicated. - Now, no subresource found at any level in any form, will be duplicated, but the original reference kept instead. Packed arrays, arrays and dictionaries are referenced, not duplicated at all. --- core/io/resource.cpp | 97 ++++++++++++++++---------------------------- core/io/resource.h | 10 ++++- 2 files changed, 42 insertions(+), 65 deletions(-) diff --git a/core/io/resource.cpp b/core/io/resource.cpp index e3ba3b0dd415..7eff98ceba35 100644 --- a/core/io/resource.cpp +++ b/core/io/resource.cpp @@ -242,16 +242,16 @@ void Resource::reload_from_file() { copy_from(s); } -Variant Resource::_duplicate_recursive_for_local_scene(const Variant &p_variant, Node *p_for_scene, HashMap, Ref> &p_remap_cache) { +Variant Resource::_duplicate_recursive_impl(const Variant &p_variant, const DuplicateParams &p_params) const { switch (p_variant.get_type()) { case Variant::OBJECT: { const Ref &sr = p_variant; if (sr.is_valid() && sr->is_local_to_scene()) { - if (p_remap_cache.has(sr)) { - return p_remap_cache[sr]; + if (p_params.remap_cache->has(sr)) { + return p_params.remap_cache->get(sr); } else { - const Ref &dupe = sr->duplicate_for_local_scene(p_for_scene, p_remap_cache); - p_remap_cache[sr] = dupe; + const Ref &dupe = sr->_duplicate_impl(p_params); + p_params.remap_cache->insert(sr, dupe); return dupe; } } else { @@ -263,7 +263,7 @@ Variant Resource::_duplicate_recursive_for_local_scene(const Variant &p_variant, Array dst; dst.resize(src.size()); for (int i = 0; i < src.size(); i++) { - dst[i] = _duplicate_recursive_for_local_scene(src[i], p_for_scene, p_remap_cache); + dst[i] = _duplicate_recursive_impl(src[i], p_params); } return dst; } break; @@ -275,8 +275,8 @@ Variant Resource::_duplicate_recursive_for_local_scene(const Variant &p_variant, for (const Variant &k : keys) { const Variant &v = src[k]; dst.set( - _duplicate_recursive_for_local_scene(k, p_for_scene, p_remap_cache), - _duplicate_recursive_for_local_scene(v, p_for_scene, p_remap_cache)); + _duplicate_recursive_impl(k, p_params), + _duplicate_recursive_impl(v, p_params)); } return dst; } break; @@ -298,16 +298,18 @@ Variant Resource::_duplicate_recursive_for_local_scene(const Variant &p_variant, } } -Ref Resource::duplicate_for_local_scene(Node *p_for_scene, HashMap, Ref> &p_remap_cache) { +Ref Resource::_duplicate_impl(const DuplicateParams &p_params) const { List plist; get_property_list(&plist); Ref r = Object::cast_to(ClassDB::instantiate(get_class())); ERR_FAIL_COND_V(r.is_null(), Ref()); - p_remap_cache[this] = r; + p_params.remap_cache->insert(Ref(this), r); - r->local_scene = p_for_scene; + if (p_params.local_scene) { + r->local_scene = p_params.local_scene; + } for (const PropertyInfo &E : plist) { if (!(E.usage & PROPERTY_USAGE_STORAGE)) { @@ -315,13 +317,15 @@ Ref Resource::duplicate_for_local_scene(Node *p_for_scene, HashMapset(E.name, p); @@ -330,6 +334,14 @@ Ref Resource::duplicate_for_local_scene(Node *p_for_scene, HashMap Resource::duplicate_for_local_scene(Node *p_for_scene, HashMap, Ref> &p_remap_cache) const { + DuplicateParams params; + params.deep = true; + params.local_scene = p_for_scene; + params.remap_cache = &p_remap_cache; + return _duplicate_impl(params); +} + void Resource::_find_sub_resources(const Variant &p_variant, HashSet> &p_resources_found) { switch (p_variant.get_type()) { case Variant::ARRAY: { @@ -386,52 +398,11 @@ void Resource::configure_for_local_scene(Node *p_for_scene, HashMap Resource::duplicate(bool p_subresources) const { - List plist; - get_property_list(&plist); - - Ref r = static_cast(ClassDB::instantiate(get_class())); - ERR_FAIL_COND_V(r.is_null(), Ref()); - - for (const PropertyInfo &E : plist) { - if (!(E.usage & PROPERTY_USAGE_STORAGE)) { - continue; - } - Variant p = get(E.name); - - switch (p.get_type()) { - case Variant::Type::DICTIONARY: - case Variant::Type::ARRAY: - case Variant::Type::PACKED_BYTE_ARRAY: - case Variant::Type::PACKED_COLOR_ARRAY: - case Variant::Type::PACKED_INT32_ARRAY: - case Variant::Type::PACKED_INT64_ARRAY: - case Variant::Type::PACKED_FLOAT32_ARRAY: - case Variant::Type::PACKED_FLOAT64_ARRAY: - case Variant::Type::PACKED_STRING_ARRAY: - case Variant::Type::PACKED_VECTOR2_ARRAY: - case Variant::Type::PACKED_VECTOR3_ARRAY: - case Variant::Type::PACKED_VECTOR4_ARRAY: { - r->set(E.name, p.duplicate(p_subresources)); - } break; - - case Variant::Type::OBJECT: { - if (!(E.usage & PROPERTY_USAGE_NEVER_DUPLICATE) && (p_subresources || (E.usage & PROPERTY_USAGE_ALWAYS_DUPLICATE))) { - Ref sr = p; - if (sr.is_valid()) { - r->set(E.name, sr->duplicate(p_subresources)); - } - } else { - r->set(E.name, p); - } - } break; - - default: { - r->set(E.name, p); - } - } - } - - return r; + HashMap, Ref> remap_cache; + DuplicateParams params; + params.deep = p_subresources; + params.remap_cache = &remap_cache; + return _duplicate_impl(params); } void Resource::_set_path(const String &p_path) { diff --git a/core/io/resource.h b/core/io/resource.h index 15d6202568d6..92bd986cef82 100644 --- a/core/io/resource.h +++ b/core/io/resource.h @@ -74,7 +74,13 @@ class Resource : public RefCounted { SelfList remapped_list; - Variant _duplicate_recursive_for_local_scene(const Variant &p_variant, Node *p_for_scene, HashMap, Ref> &p_remap_cache); + struct DuplicateParams { + bool deep = false; // With subresources? + Node *local_scene = nullptr; + HashMap, Ref> *remap_cache = nullptr; + }; + Ref _duplicate_impl(const DuplicateParams &p_params) const; + Variant _duplicate_recursive_impl(const Variant &p_variant, const DuplicateParams &p_params) const; void _find_sub_resources(const Variant &p_variant, HashSet> &p_resources_found); protected: @@ -120,7 +126,7 @@ class Resource : public RefCounted { String get_scene_unique_id() const; virtual Ref duplicate(bool p_subresources = false) const; - Ref duplicate_for_local_scene(Node *p_for_scene, HashMap, Ref> &p_remap_cache); + Ref duplicate_for_local_scene(Node *p_for_scene, HashMap, Ref> &p_remap_cache) const; void configure_for_local_scene(Node *p_for_scene, HashMap, Ref> &p_remap_cache); void set_local_to_scene(bool p_enable);