Overhaul resource duplication by RandomShaper · Pull Request #100673 · godotengine/godot

RandomShaper · 2024-12-20T23:20:45Z

Fixes #74918.
2025-05-19: Fixes #96627.

TL;DR

This unifies the different paths in Godot for resource duplication so they all have the same base behavior and only differ in what is predictable. Also, an extremely comprehensive set of tests is added.

Long description

Each commit takes a self-contained step towards the overall goal. Quoting their messages for the details:

Overhaul `Resource::duplicate_for_local_scene()`

Serves as a first step for future refactors.
Code is simpler.
Algorithm is more efficient: instead of two passes (dumb copy + resolve copies), it's single-pass.
Now obeys PROPERTY_USAGE_NEVER_DUPLICATE.
Now handles deep self-references (the resource to be duplicated being referenced somewhere deep).

Overhaul `Resource::duplicate()` (Edited 2025-01-21.)

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 deepness in a more consistent and predictable fashion. Resource::duplicate_deep() is added (instead of just adding a parameter to the former, for compatibility needs).

The behavior after this change is as follows:

Deep (deep=true, formerly subresources=true):
- 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, and only local subresources found across are copied.
- Previously, subresources would be duplicated as many times as being referenced throughout the main resource.
- Now, each subresource is only duplicated once and from that point, a referenced to that single copy is used. That's the enhanced behavior that duplicate_for_local_scene() already featured.
- The behavior with respect to packed arrays is still duplication.
- Formerly, arrays and dictionaries were recursive duplicated, with resources ignored.
- Now, arrays and dictionaries are recursive duplicated, with resources duplicated.
- When doing it through duplicate_deep(), there's a deep_subresources_mode parameter, with various possibilites to control if no resources are duplicated (so arrays, etc. are, but keeping referencing the originals), if only the internal ones are (resources with no non-local path, the default), or if all of them are. The default is to copy every subresource, just like duplicate(true).
Not deep (deep=false, formerly subresources=false):
- 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.
- Now, resources found as values of always-duplicate properties are duplicated, recursively or not matching what was requested for the root call.

This commit also changes what's the virtual method to override to customize the duplication (now it's the protected _duplicate() instead of the public duplicate()).

Overhaul `Variant::duplicate()` for resources (Edited 2025-01-21.)

This in the scope of a duplication triggered via any type in the Variant realm. that is, the following: Variant itself, Array and Dictionary. That includes invoking duplicate() from scripts.

A duplicate_deep(deep_subresources_mode) method is added to Variant, Array and Dictionary (for compatibility reasons, simply adding an extra parameter was not possible). The default value for it is RESOURCE_DEEP_DUPLICATE_NONE, which is like calling duplicate(true).

Remarks:

The results of copying resources via those Variant types are exactly the same as if the copy were initiated from the Resource type at C++.
In order to keep some separation between Variant and the higher-level animal which is Resource, Variant still contains the original code for that, so it's self-sufficient unless there's a Resource involved. Once the deep copy finds a Resource that has to be copied according to the duplication parameters, the algorithm invokes the Resource duplication machinery. When the stack is unwind back to a nesting level Variant can handle, Variant duplication logic keeps functioning.

While that is good from a responsibility separation standpoint, that would have a caveat: Variant would not be aware of the mapping between original and duplicate subresources and so wouldn't be able to keep preventing multiple duplicates.

To avoid that, this commit also introduces a wormwhole, a sharing mechanism by which Variant and Resource can collaborate in managing the lifetime of the original-to-duplicates map. The user-visible benefit is that the overduplicate prevention works as broadly as the whole Variant entity being copied, including all nesting levels, regardless how disconnected the data members containing resources may be across al the nesting levels. In other words, despite the aforementioned division of duties between Variant and Resource duplication logic, the duplicates map is shared among them. It's created when first finding a Resource and, however how deep the copy was working at that point, the map kept alive unitl the stack is unwind to the root user call, until the first step of the recursion.

Thanks to that common map of duplicates, this commit is able to fix the issue that Resource::duplicate_for_local_scene() used to ignore overridden duplicate logic.

Caveats (Edited 2025-01-21.)

It may be the case that user projects are relying on the former behaviors. On the one hand, most of the time that means they were taking unreasonable or buggy behaviors as good, so I'd say it's not a big problem to take the all workings away.

Notes on other duplication entry points (Added 2025-01-03.)

In order for the scope of this PR to be better understood and also have a list of potential changes for subsequent PRs, let's be mindful of the following:

Node duplication (editor and runtime):
- Not directly modified by this PR. (*)
- Only duplicates top-level properties.
- Only duplicates resources in always-duplicate properties.
- Ignores never-duplicate.
- Only relevant to child nodes, it only recurses into Array properties that are typed as object, ignores Dictionary.
Resource make unique (editor):
- Single
  - Not directly modified by this PR. (*)
- Recursive:
  - Not directly modified by this PR. (*)
  - Recurses into nested resources found as values of properties that are not never-duplicate. Ignores Dictionary and Array.
  - Ignores always-duplicate.

*: All of the workflows above use shallow duplication. Therefore, even if not directly modified by this PR, each individual resource duplication is affected by the behavior changes to shallow resource duplication. (Added 2025-05-21): In order to keep the current behavior of resouces assigned to properties not being duplicated at all, a check is added to the Node duplication algorithm.

I've tested this locally and also the test suite is pretty comprehensive. Nonetheless, some extra testing is more than welcome.

RedMser · 2024-12-21T16:12:48Z

I tested both resource_local_to_scene and resource.duplicate(true) with sub-resources, arrays and dictionaries, using this MRP. I didn't notice any unexpected regressions compared to before the PR.

RandomShaper · 2025-01-21T11:21:43Z

Dear reviewers. I'm sorry, but I have needed to update this PR with a bunch of changes to support additional duplicate modes, for greater flexibility and compatibility preservation. I've updated all the commit messages as well as the description of the PR, which includes them.

I haven't rebased, so you can more easily see the changes recently made.

AdriaandeJongh · 2025-02-18T19:50:59Z

I so very appreciate this PR and really look forward to these changes making it into the engine! Thank you for addressing this. In the meantime I wanted to mention that I am very lost in the differences between duplicate(deep=false), duplicate(deep=true), and duplicate_deep(). You mention that duplicate_deep() is added for compatibility needs, but if duplicate() already takes a deep parameter, what is the specific compatibility need here? My intuition is that a single duplicate() function with all the necessary parameters is the least confusing for all users.

HampusHauffman · 2025-03-16T21:07:28Z

What is needed to get this merged?

Mickeon · 2025-03-16T21:56:44Z

A look from Core maintainers. It's a pretty extensive change so it may need a lot of scrutiny.

RandomShaper · 2025-03-18T06:32:59Z

In the meantime I wanted to mention that I am very lost in the differences between duplicate(deep=false), duplicate(deep=true), and duplicate_deep(). You mention that duplicate_deep() is added for compatibility needs, but if duplicate() already takes a deep parameter, what is the specific compatibility need here? My intuition is that a single duplicate() function with all the necessary parameters is the least confusing for all users.

The problem is Variant. It's not possible to extend the signature of duplicate() without breaking compatiblity with GDExtension. At the moment, Variant doesn't feature a system to provide compatibility methods, and such a thing would most likely be detrimental to performance across all Variant calls anyway. Therefore, I had no option but adding a brand new method and, for the sake of clarity, replicating the approach in Resource, where it would have been possible to have a compatibility method.

KoBeWi · 2025-03-18T14:58:20Z

I tested the MRP from the issue and the array is empty after duplication.

RandomShaper · 2025-03-20T10:37:33Z

@KoBeWi, thanks a lot for testing this. There was indeed a bug involving scripted properties. Given the new code deals directly with resources, it needed special treatment for those properties so the assignments are not rejected; namely:

script property has to be copied first, so the targe properties exist.
Duplication of typed arrays and dictionaries has to transfer the type to the copies.

I have fixed that in my latest push. A subsequent push will be made, rebased and with formatting fixed.

RandomShaper · 2025-05-21T12:18:21Z

Fixed. I've also added an update to the latest part of the PR description. Quoting it here:

In order to keep the current behavior of resouces assigned to properties not being duplicated at all, a check is added to the Node duplication algorithm.

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 deepness in a more consistent and predictable fashion. `Resource::duplicate_deep()` is added (instead of just adding a parameter to the former, for compatibility needs). The behavior after this change is as follows: - Deep (`deep=true`, formerly `subresources=true`): - 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, and only local subresources found across are copied. - Previously, subresources would be duplicated as many times as being referenced throughout the main resource. - Now, each subresource is only duplicated once and from that point, a referenced to that single copy is used. That's the enhanced behavior that `duplicate_for_local_scene()` already featured. - The behavior with respect to packed arrays is still duplication. - Formerly, arrays and dictionaries were recursive duplicated, with resources ignored. - Now, arrays and dictionaries are recursive duplicated, with resources duplicated. - When doing it through `duplicate_deep()`, there's a` deep_subresources_mode` parameter, with various possibilites to control if no resources are duplicated (so arrays, etc. are, but keeping referencing the originals), if only the internal ones are (resources with no non-local path, the default), or if all of them are. The default is to copy every subresource, just like `duplicate(true)`. - Not deep (`deep=false`, formerly `subresources=false`): <a name="resource-shallow"></a> - 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. - Now, resources found as values of always-duplicate properties are duplicated, recursively or not matching what was requested for the root call. This commit also changes what's the virtual method to override to customize the duplication (now it's the protected `_duplicate()` instead of the public `duplicate()`).

This in the scope of a duplication triggered via any type in the `Variant` realm. that is, the following: `Variant` itself, `Array` and `Dictionary`. That includes invoking `duplicate()` from scripts. A `duplicate_deep(deep_subresources_mode)` method is added to `Variant`, `Array` and `Dictionary` (for compatibility reasons, simply adding an extra parameter was not possible). The default value for it is `RESOURCE_DEEP_DUPLICATE_NONE`, which is like calling `duplicate(true)`. Remarks: - The results of copying resources via those `Variant` types are exactly the same as if the copy were initiated from the `Resource` type at C++. - In order to keep some separation between `Variant` and the higher-level animal which is `Resource`, `Variant` still contains the original code for that, so it's self-sufficient unless there's a `Resource` involved. Once the deep copy finds a `Resource` that has to be copied according to the duplication parameters, the algorithm invokes the `Resource` duplication machinery. When the stack is unwind back to a nesting level `Variant` can handle, `Variant` duplication logic keeps functioning. While that is good from a responsibility separation standpoint, that would have a caveat: `Variant` would not be aware of the mapping between original and duplicate subresources and so wouldn't be able to keep preventing multiple duplicates. To avoid that, this commit also introduces a wormwhole, a sharing mechanism by which `Variant` and `Resource` can collaborate in managing the lifetime of the original-to-duplicates map. The user-visible benefit is that the overduplicate prevention works as broadly as the whole `Variant` entity being copied, including all nesting levels, regardless how disconnected the data members containing resources may be across al the nesting levels. In other words, despite the aforementioned division of duties between `Variant` and `Resource` duplication logic, the duplicates map is shared among them. It's created when first finding a `Resource` and, however how deep the copy was working at that point, the map kept alive unitl the stack is unwind to the root user call, until the first step of the recursion. Thanks to that common map of duplicates, this commit is able to fix the issue that `Resource::duplicate_for_local_scene()` used to ignore overridden duplicate logic.

Calinou

Tested locally (by editing the 3D platformer demo project), it works as expected. Code looks good to me.

I like the addition of duplicate_deep() for more explicit code.

Calinou · 2025-05-26T16:01:41Z

PS: I'm curious if this PR would affect a rebase of #86779 in any way. Is it still technically feasible?

Repiteo · 2025-05-27T14:51:31Z

Thanks!

HampusHauffman · 2025-05-27T15:36:44Z

Thank you!

Yarwin · 2025-05-28T12:26:35Z

The enum name is wrongly formatted, which breaks our codegen. When it is used as an argument, in extension_api.json its type is specified as enum::ResourceDeepDuplicateMode, while it should be enum::Resource.ResourceDeepDuplicateMode.

It uses

Instead of, for example

which desugars to:

#define BIND_ENUM_CONSTANT(m_constant) \
    ::ClassDB::bind_integer_constant(get_class_static(), __constant_get_enum_name(m_constant, #m_constant), #m_constant, m_constant);

godot/core/variant/type_info.h

Lines 207 to 210 in 99f5a3d

    
           template <typename T> 
        
           inline StringName __constant_get_enum_name(T param, const String &p_constant) { 
        
           	return GetTypeInfo<T>::get_class_info().class_name; 
        
           }

godot/core/object/class_db.cpp

Line 1111 in 99f5a3d

    
           void ClassDB::bind_integer_constant(const StringName &p_class, const StringName &p_enum, const StringName &p_name, int64_t p_constant, bool p_is_bitfield) {

RandomShaper added bug enhancement topic:core labels Dec 20, 2024

RandomShaper added this to the 4.x milestone Dec 20, 2024

RandomShaper requested review from a team as code owners December 20, 2024 23:20

RandomShaper force-pushed the res_duplicate branch from fc28733 to 90ec9fc Compare December 20, 2024 23:25

RandomShaper requested a review from reduz December 20, 2024 23:27

RandomShaper force-pushed the res_duplicate branch from 90ec9fc to bbfe1db Compare December 20, 2024 23:35

Mickeon reviewed Dec 22, 2024

View reviewed changes

Comment thread doc/classes/Array.xml Outdated

Comment thread doc/classes/Dictionary.xml Outdated

Comment thread doc/classes/Resource.xml Outdated

Comment thread doc/classes/Resource.xml Outdated

RandomShaper force-pushed the res_duplicate branch 3 times, most recently from 415b488 to 76c16f9 Compare January 3, 2025 10:25

RandomShaper force-pushed the res_duplicate branch from 76c16f9 to fa193cb Compare January 21, 2025 11:18

RandomShaper force-pushed the res_duplicate branch from fa193cb to eda7476 Compare January 21, 2025 11:47

RandomShaper requested a review from akien-mga January 31, 2025 08:33

suddjian mentioned this pull request Feb 16, 2025

Typing an array causes it to always be empty after setting it with values. #102902

Open

akien-mga requested review from KoBeWi and bruvzg March 17, 2025 13:51

RandomShaper force-pushed the res_duplicate branch from eda7476 to be34608 Compare March 20, 2025 10:28

RandomShaper force-pushed the res_duplicate branch from be34608 to 5c0587f Compare March 20, 2025 10:43

AThousandShips reviewed May 21, 2025

View reviewed changes

Comment thread core/variant/variant_setget.cpp Outdated

Comment thread doc/classes/Resource.xml Outdated

Comment thread doc/classes/Array.xml Outdated

Comment thread doc/classes/Dictionary.xml Outdated

RandomShaper added 2 commits May 26, 2025 10:05

RandomShaper force-pushed the res_duplicate branch from 30fcfe5 to 23ac332 Compare May 26, 2025 08:07

Add tests for resource duplication

6841b45

RandomShaper force-pushed the res_duplicate branch from 23ac332 to 6841b45 Compare May 26, 2025 15:05

Calinou approved these changes May 26, 2025

View reviewed changes

Repiteo merged commit 63dff62 into godotengine:master May 27, 2025
20 checks passed

RandomShaper deleted the res_duplicate branch May 27, 2025 15:43

Yarwin mentioned this pull request May 29, 2025

Work around ResourceDeepDuplicateMode wrongly marked as an global enum in extension_api.json godot-rust/gdext#1180

Merged

Summersay415 mentioned this pull request Jun 4, 2025

Inherited scene loses reference to resource after instantiation #107104

Closed

Yarwin mentioned this pull request Jun 5, 2025

ResourceDeepDuplicateMode enum is wrongly formatted as a global enum in extension_api.json in method definitions. #107138

Closed

mew2057 mentioned this pull request Jun 5, 2025

Instantiation of dictionaries in resources with resource_local_to_scene set to true fail #107186

Closed

This was referenced Jun 6, 2025

Fix resources wrongly duplicated upon instantiating inherited scenes #107219

Merged

Fix Inherited Scenes Lost References #107173

Closed

kitbdev mentioned this pull request Jun 14, 2025

Fix Resource Duplication test errors #107516

Merged

AnidemDex mentioned this pull request Jun 26, 2025

Flow timing gets synchronized when multiple Command Processors execute the same Command Collection concurrently AnidemDex/Blockflow#183

Open

Ivorforce mentioned this pull request Jul 3, 2025

duplicate_deep(DEEP_DUPLICATE_ALL) throws an assert when duplicating a resource cointaining an exported PackedScene with a class_named script #108220

Closed

beicause mentioned this pull request Jul 22, 2025

Duplicated ShaderMaterial with a sampler2D[] uniform loses items when another object with the same duplicated material is removed #108815

Closed

mihe mentioned this pull request Aug 6, 2025

Resources with "local_to_scene" set to true do not duplicate Typed Dictionary(<?>, <CustomResource>) #109316

Closed

beicause mentioned this pull request Aug 8, 2025

Make shallow copy of arrays when duplicating #109427

Closed

WhalesState mentioned this pull request Sep 2, 2025

Resource.duplicate(true) no longer checks for valid resource values before setting the properties #110208

Closed

MadeScientist mentioned this pull request Oct 1, 2025

Copying AnimationLibrary resources(make unique), but no deep duplicate it's sub animation resources. #111130

Open

pewcworrell mentioned this pull request Nov 4, 2025

Deep duplication of a dictionary in _validate_property of a resource causes a crash #112391

Closed

bruvzg mentioned this pull request Nov 26, 2025

Do not attempt deleting local cache in Resource::_teardown_duplicate_from_variant. #113179

Merged

Uh oh!

Conversation

RandomShaper commented Dec 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

TL;DR

Long description

Overhaul Resource::duplicate_for_local_scene()

Overhaul Resource::duplicate() (Edited 2025-01-21.)

Overhaul Variant::duplicate() for resources (Edited 2025-01-21.)

Caveats (Edited 2025-01-21.)

Notes on other duplication entry points (Added 2025-01-03.)

Uh oh!

RedMser commented Dec 21, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

RandomShaper commented Jan 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AdriaandeJongh commented Feb 18, 2025

Uh oh!

HampusHauffman commented Mar 16, 2025

Uh oh!

Mickeon commented Mar 16, 2025

Uh oh!

RandomShaper commented Mar 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KoBeWi commented Mar 18, 2025

Uh oh!

RandomShaper commented Mar 20, 2025

Uh oh!

RandomShaper commented May 21, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Calinou left a comment

Choose a reason for hiding this comment

Uh oh!

Calinou commented May 26, 2025

Uh oh!

Uh oh!

Repiteo commented May 27, 2025

Uh oh!

HampusHauffman commented May 27, 2025

Uh oh!

Yarwin commented May 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

14 participants

RandomShaper commented Dec 20, 2024 •

edited

Loading

Overhaul `Resource::duplicate_for_local_scene()`

Overhaul `Resource::duplicate()` (Edited 2025-01-21.)

Overhaul `Variant::duplicate()` for resources (Edited 2025-01-21.)

RandomShaper commented Jan 21, 2025 •

edited

Loading

RandomShaper commented Mar 18, 2025 •

edited

Loading

Yarwin commented May 28, 2025 •

edited

Loading