Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Overhaul resource duplication #100673

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

RandomShaper
Copy link
Member

@RandomShaper RandomShaper commented Dec 20, 2024

Fixes #74918.

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.


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

@RedMser
Copy link
Contributor

RedMser commented Dec 21, 2024

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.

doc/classes/Array.xml Outdated Show resolved Hide resolved
doc/classes/Dictionary.xml Outdated Show resolved Hide resolved
doc/classes/Resource.xml Outdated Show resolved Hide resolved
doc/classes/Resource.xml Outdated Show resolved Hide resolved
@RandomShaper RandomShaper force-pushed the res_duplicate branch 2 times, most recently from db70574 to 415b488 Compare January 2, 2025 18:03
- 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).
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()`).
@RandomShaper
Copy link
Member Author

RandomShaper commented Jan 21, 2025

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.

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Resource.duplicate(true) doesn't duplicate subresources stored in Array or Dictionary properties
3 participants