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

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

Open
limbonaut opened this issue Mar 14, 2023 · 36 comments

Comments

@limbonaut
Copy link

limbonaut commented Mar 14, 2023


Bugsquad note: This issue has been confirmed several times already. No need to confirm it further.


Godot version

4.0.stable

System information

Manjaro Linux

Issue description

This behavior is poorly documented, and it breaks expectations. It is mentioned in the docs only under Array.duplicate(), but not Dictionary class, or Resource.duplicate().

This affects also "Make Unique" and "Make Unique (Recursive)" for custom resources.

Here's the problematic code:

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);
}
}
*/
return *this;

The "breaks stuff :(" comment in the code was left in 2018 (d280b14).

Steps to reproduce

For simple code see reproduction project.

To reproduce on your own:

  1. Have a resource with a property of type Array[SomeOtherResource]
  2. One of the following:
    2a) "Make Unique" on parent resource in inspector.
    2b) Or duplicate(true) from code.
  3. Check if SomeOtherResource items inside array are unique.

Minimal reproduction project

res_bug_reproduction.zip

@dandeliondino
Copy link

Thank you! I believe I experienced this with 4.0-stable trying to duplicate a custom resource with a dictionary that contained additional dictionaries. I ended up writing a function to create a new resource and copy over the data piece-by-piece, but never figured out why duplicate(true) hadn’t worked. Some additional documentation would save the next person a lot of time.

limbonaut added a commit to limbonaut/SmartShape2D that referenced this issue Mar 16, 2023
- Single points setter, single property, no setter overridding
- Copy-pasted shapes are sharing geometry by default now (in line with
the editor conventions)
- Implement Make Unique toggle with undo support (workaround for godotengine/godot#74918)
- SS2D_Point_Array.clone()
- A confirmation dialog for "Make Unique" action to avoid accidental
misclicks
- Fix: Unable to clear SS2D_Point_Array resource in inspector
- Fix: Opening a scene with a shape generates change in the scene file
even if shape wasn't touched
@popcar2
Copy link

popcar2 commented Jul 12, 2023

It's troublesome that this isn't fixed yet, my game relies a lot on duplicating resources and arrays and I was really miffed when I just found out deep copying isn't functioning properly. This should be tracked and added to a milestone.

@stryker313
Copy link

This also occurs on all 3.x variants.

@luislodosm
Copy link

I confim that my_resource = my_resource.duplicate(true) doesn't make unique sub-resources as the documentation says. Godot 4.1.

@zhuyeaini9
Copy link

I also met this situation in c#,I set the resourcelocaltoscene = true, and the resource members are not shared except the array and dictionary.
this really affect,it can provide one override copy function for resource.

@A-Sandwich
Copy link

Does anyone know if this is being looked at or at least planned to be included in a future release?

@benfrankel
Copy link

I hit this bug during a game jam (v4.1.2).

@DDarby-Lewis
Copy link

DDarby-Lewis commented Nov 29, 2023

I'm guessing one of the issues with this (though perhaps not the only reason it was removed) is because resources can self-reference which would break this. Here is a pseudo-code thought on it:

Variant Variant::recursive_duplicate(bool p_deep, int recursion_count, std::unordered_map<Ref<Resource>, Ref<Resource>> refs) const {
	switch (type) {
		case OBJECT: {
			Ref<Resource> resource = _get_obj().ref;
			if (p_deep && !resource.is_null() && resource.is_valid()) {
				if (refs.contains(resource))
					return refs[resource];
				Ref<Resource> resource_new = resource->duplicate(true, refs);
				refs[resource] = resource_new;
				return resource_new;
			}
			return *this;
		} break;

This isn't in a working state but I am trying to look at what direction it might be able to go and help the conversation around this as I need this fixed myself.

@frozenwood
Copy link

Oh my god, I was confused why all my enemy mobs die at the same time like they have shared HP. Had to make multiple loops to manually duplicate subresources in my ready function to work around this. This is rough for stat heavy games out there.

@Yinimi
Copy link

Yinimi commented Jan 4, 2024

I came across this issues when duplicating CollisionShape2D in the Scene... but i guess this is using the same function beneath

@henrypickler
Copy link

henrypickler commented Feb 6, 2024

This seems to have been fixed. Using Resource.duplicate(true) duplicates the subresources as well in v4.2.1

Edit: This is not true for Array[Resource], though which is the intent of the issue. My bad!

@dfego
Copy link

dfego commented Feb 6, 2024

This seems to have been fixed. Using Resource.duplicate(true) duplicates the subresources as well in v4.2.1

I've tested this recently in 4.2.1 and no, it wasn't fixed when using Array at least. If you think it's fixed, please post a simple example of it working, as it might help solve the issue.

@GalaxasaurusGames

This comment was marked as off-topic.

@T4n4t0s

This comment was marked as off-topic.

@AThousandShips
Copy link
Member

Please don't bump without contributing significant new information. Use the 👍 reaction button on the first post instead.

@devonveldhuis
Copy link

Can this at least be updated in the documentation ASAP even if it won't be fixed any time soon? The Resource.duplicate docs confidently state:

If subresources is true, a deep copy is returned; nested subresources will be duplicated and are not shared.

Which is clearly not true and causing headaches for many users in this thread. It's now been over 1 year with no comments from the team.

@595145489
Copy link

595145489 commented Apr 9, 2024

it's not fix now. has any way to fix this problem?

I made a gdscript class, it's extend Resource to implement a function call _duplicate to fix this problem.

But please fix this in the engine as soon as possible.

the class code is :

class_name BaseResource

extends Resource


func _duplicate(flag):
	var newRes = self.duplicate(flag)
	if flag:
		newRes._CheckDuplicateRes()
	return newRes

func _ArrayCheck(type,pvalue):
	var needcpobj = {}
	for i in range(pvalue.size()):
		var value = pvalue[i]
		var vtype = typeof(value)
		#handle type obj copy
		if vtype == TYPE_OBJECT:
			if not value.has_method("_duplicate"): continue
			var dpvalue = value._duplicate(true)
			needcpobj[i] = dpvalue
		else:
			self._CheckType(vtype,value)

	for i in needcpobj.keys():
		pvalue[i] = needcpobj[i]
	return

func _DictionaryCheck(type,pvalue):
	var needcpobj = {}
	for key in pvalue:
		var value = pvalue[key]
		var vtype = typeof(value)
		#handle type obj copy
		if vtype == TYPE_OBJECT:
			if not value.has_method("_duplicate"): continue
			var dpvalue = value._duplicate(true)
			needcpobj[key] = dpvalue
		else:
			self._CheckType(vtype,value)

	for i in needcpobj.keys():
		pvalue[i] = needcpobj[i]
	return

func _CheckType(type,value):
	match type:
		TYPE_ARRAY:
			_ArrayCheck(type,value)
			return
		TYPE_DICTIONARY:
			_DictionaryCheck(type,value)
			return
		TYPE_OBJECT:
			if value.has_method("_CheckDuplicateRes"):
				value._CheckDuplicateRes()
			return

func _CheckDuplicateRes():
	for property in self.get_property_list():
		if property.usage & PROPERTY_USAGE_SCRIPT_VARIABLE:
			var value = self.get(property.name)
			if value == null: continue
			var type = property.type
			self._CheckType(type,value)
	return

@Saadies
Copy link

Saadies commented Apr 14, 2024

it's not fix now. has any way to fix this problem?

I made a gdscript class, it's extend Resource to implement a function call _duplicate to fix this problem.

But please fix this in the engine as soon as possible.

the class code is :

class_name BaseResource

Edit:
So, I kinda figured it out where my (and many others) issues come from regarding duplicating resources and saving and loading them.

The initial problem is that saving and loading subresources just does not work if you simply save and load using ResourceSaver and RexourceLoader IF any of your subresources are actually instanced. ResourceSaver will just save a reference to the project file instance.

Now, the "FLAG_BUNDLE_RESOURCES " should fix it, but it is actually not working right now (#65393)

If you want to save your instanced subresources you need to deep duplicate them using "BaseResource".

It's also important du make deep duplications when you have a resource "template" saved that you later want to add to an array List multiple times.


I am trying to use this, together with: #65393 (comment)
to save and load resources that contain an Array with other Resources.

It does not work unfortunately:

E 0:00:09:0688   SaveManager.gd:17 @ loadAll(): Attempted to assign an object into a TypedArray, that does not inherit from 'GDScript'.
  <C++ Error>    Condition "!other_script->inherits_script(script)" is true. Returning: false
  <C++ Source>   core/variant/container_type_validate.h:140 @ validate_object()
  <Stack Trace>  SaveManager.gd:17 @ loadAll()
                 SaveManager.gd:9 @ _init()
		var loadfile = ResourceLoader.load(SAVEDIR,"GameStats", 0)

E 0:00:09:0688   SaveManager.gd:17 @ loadAll(): unsupported format character
  <C++ Error>    Condition "error" is true. Returning: String()
  <C++ Source>   ./core/variant/variant.h:834 @ vformat()
  <Stack Trace>  SaveManager.gd:17 @ loadAll()
                 SaveManager.gd:9 @ _init()

E 0:00:09:0688   SaveManager.gd:17 @ loadAll(): Method/function failed.
  <C++ Source>   core/variant/array.cpp:222 @ assign()
  <Stack Trace>  SaveManager.gd:17 @ loadAll()
                 SaveManager.gd:9 @ _init()

E 0:00:09:0695   SaveManager.gd:17 @ loadAll(): Attempted to assign an object into a TypedArray, that does not inherit from 'GDScript'.
  <C++ Error>    Condition "!other_script->inherits_script(script)" is true. Returning: false
  <C++ Source>   core/variant/container_type_validate.h:140 @ validate_object()
  <Stack Trace>  SaveManager.gd:17 @ loadAll()
                 SaveManager.gd:9 @ _init()

E 0:00:09:0695   SaveManager.gd:17 @ loadAll(): unsupported format character
  <C++ Error>    Condition "error" is true. Returning: String()
  <C++ Source>   ./core/variant/variant.h:834 @ vformat()
  <Stack Trace>  SaveManager.gd:17 @ loadAll()
                 SaveManager.gd:9 @ _init()

E 0:00:09:0695   SaveManager.gd:17 @ loadAll(): Method/function failed.
  <C++ Source>   core/variant/array.cpp:222 @ assign()
  <Stack Trace>  SaveManager.gd:17 @ loadAll()
                 SaveManager.gd:9 @ _init()

Has anyone managed to save and load resources that contain an Array of another resources?

@filipinyo
Copy link

Any news on this? It's pretty crucial for a resource heavy game :/

@JonRoberts44
Copy link

Yeah this seems like a pretty major bug to me. It's workaroundable but I don't particuarly like the elegancy of any of the workarounds.

@LeifintheWind
Copy link

LeifintheWind commented Jun 11, 2024

I'm not sure if this is related or a new bug, but I'm finding nothing is duplicated in my custom resource. I've got a custom resource with a bunch of different variables including dictionaries and standalone variable ints and such. And when attempting to use the duplicate(true) function none of it is copied over. The dictionaries are blank and the integers are also their default value. It seems like integers based on enums are not copied, but regular integers are.... strange.

But essentially I have to manually copy over every value from one resource to another, which is far from ideal.

@AdriaandeJongh
Copy link
Contributor

AdriaandeJongh commented Jul 9, 2024

Also stumbled upon this issue after a day of frustration. The docs should really be updated as "If subresources is true, a deep copy is returned; nested subresources will be duplicated and are not shared." is factually not correct.

As nobody explicitly mentioned the simple (but dumb) workaround: after creating a duplicate using duplicate(true), you can iterate over the arrays and dictionaries in those resources (and the ones in those, if you have them nested) and replace the elements with duplicates of themselves. Like so:

func duplicate_deep_workaround() -> SomeCustomResource:
	var dup: SomeCustomResource = duplicate(true) as SomeCustomResource
	for i: int in dup.some_array.size():
		dup.some_array[i] = dup.some_array[i].duplicate(true)
	return dup

@Leggy7
Copy link

Leggy7 commented Jul 9, 2024

@AdriaandeJongh
that was actually already said though.
It's a workaround but it doesn't actually ease the pain of nested sub resources 💔

@Calinou Calinou changed the title Resource.duplicate(true) doesn't duplicate subresources stored in Array or Dictionary properties Resource.duplicate(true) doesn't duplicate subresources stored in Array or Dictionary properties Jul 12, 2024
@briankellydev
Copy link

It seems to me to be a larger bug with duplication functionality. If I (on 4.2.2, OSX) duplicate an item, then change any of its properties, the properties of the original are also changed. Doesn't matter what kind of node it is or what property it is. Is there a memory reference issue going on maybe?

@Calinou
Copy link
Member

Calinou commented Jul 12, 2024

If I (on 4.2.2, OSX) duplicate an item

Are you duplicating a node or a resource? How are you duplicating it?

Duplicating a node does not automatically make its Resource-based properties (or their subresources) unique, with a handful of exceptions (properties marked with PROPERTY_USAGE_ALWAYS_DUPLICATE).

@briankellydev
Copy link

If I (on 4.2.2, OSX) duplicate an item

Are you duplicating a node or a resource? How are you duplicating it?

Duplicating a node does not automatically make its Resource-based properties (or their subresources) unique, with a handful of exceptions (properties marked with PROPERTY_USAGE_ALWAYS_DUPLICATE).

I would expect the behavior would be that creating a duplicate of a node would make a new copy that is a unique node unconnected to the original. E.g. let's say I have to make two identical decks in a blackjack game, one for the dealer and one for the player, but I want their positions to be different (or, maybe their sizes, etc). The current behavior I'm seeing is that changing such parameters on the duplicate also changes them on the original.

@Calinou
Copy link
Member

Calinou commented Jul 12, 2024

I would expect the behavior would be that creating a duplicate of a node would make a new copy that is a unique node unconnected to the original.

This is being tracked in godotengine/godot-proposals#317 and godotengine/godot-proposals#4672.

@shanayyy
Copy link

After thinking for a while why copying resources nested in arrays didn't work, I opened github and searched for this issue as expected.
The document should be adjusted quickly to avoid more victims. 🌝

@AdriaandeJongh
Copy link
Contributor

Here's the PR I made to add this exception to the docs: #94142

@moshujsg
Copy link

For anyone wondering how to circumvent this while it gets fixed:

You can create a method in your resource that duplicates itself, then also duplicate each of the elements of the array in a for loop and reutrn the new resource. This should work in the mean time

@AThousandShips
Copy link
Member

You can create a method in your resource that duplicates itself, then also duplicate each of the elements of the array in a for loop and reutrn the new resource. This should work in the mean time

This has been mentioned several times with example code above, no need to repeat this suggestion again

@mihaphd
Copy link

mihaphd commented Jul 22, 2024

The method in resource that duplicates itself is kind of a pain in the ass if you have a million resources inside of resources isn't it? You would have to make the same method for a billion different resources. This really needs to be fixed

@jitspoe
Copy link
Contributor

jitspoe commented Aug 14, 2024

This doesn't seem to be limited to arrays. I have a resource that contains a node, set on ready, and no matter what I do, all resources on my enemies seem to share that same node setting, even if it's set AFTER the resource is duplicated.

func enemy_ready(enemy : Enemy):
	var actions := enemy.actions
	for i in actions.size():
		var action_template : NPCActionResource = actions[i]
		var action := action_template.duplicate(true) # Resources aren't unique by default.
		if (action.type == NPCActionResource.ActionType.FIRE_PROJECTILE):
			var projectile_action : FireProjectileActionResource = action
			projectile_action.projectile_position_node = enemy.get_node(projectile_action.projectile_position_node_path)
		if (action is MeleeActionResource): # Covers jump as well, since jump inherits from this.
			action.melee_origin_node = enemy.get_node(action.melee_origin_node_path)
		enemy.actions[i] = action # Overwrite the template resource with the unique one.

The problem is specifically on the line projectile_action.projectile_position_node = enemy.get_node(projectile_action.projectile_position_node_path)

All enemies fire projectiles from the same node (so enemy A may be shooting at me, but the projectile fires from enemy F).

For reference, here's the fire projectile resource:

@tool
extends NPCActionResource

class_name FireProjectileActionResource

@export var projectile_scene : PackedScene
@export var projectile_radius := 0.0
@export var projectile_speed := 20.0
@export var projectile_gravity := 9.8
@export var projectile_impact_scene : PackedScene
@export var projectile_impact_decal : PackedScene
@export var projectile_max_distance := 1000.0
@export var projectile_impulse := 10.0
@export var projectile_damage := 20.0
@export var projectile_splash_damage := 0.0
@export var projectile_splash_radius := 0.0
@export var projectile_type := ProjectileSystem.ProjectileType.BULLET
@export var projectile_delay := 0.3
@export var projectile_position_node_path : NodePath

var projectile_position_node : Node3D


func _init() -> void:
	super._init()
	type = ActionType.FIRE_PROJECTILE
	state = NPCSystem.State.PROJECTILE_WINDUP
	next_state = NPCSystem.State.PROJECTILE_FIRE

Even if I set the resource as local to scene, it still has the issue. Am I doing a dumb or are non-export values within a resource shared?

Edit: Ok, this does actually loop around to being relevant to the topic at hand. Seems the problem for me wasn't the duplicate() of the resource but, in fact, the array of resources on the enemy scene being shared.

@export var actions : Array[NPCActionResource]

So, after duplicating the array, it fixed the issue:

func enemy_ready(enemy : Enemy):
	var actions := enemy.actions.duplicate() # Duplicate the array -- seems this is shared for each enemy instance?
	for i in actions.size():
		var action_template : NPCActionResource = actions[i]
		var action := action_template.duplicate() # Resources aren't unique by default.
		if (action.type == NPCActionResource.ActionType.FIRE_PROJECTILE):
			var projectile_action : FireProjectileActionResource = action
			projectile_action.projectile_position_node = enemy.get_node(projectile_action.projectile_position_node_path)
		if (action is MeleeActionResource): # Covers jump as well, since jump inherits from this.
			action.melee_origin_node = enemy.get_node(action.melee_origin_node_path)
		actions[i] = action # Overwrite the template resource with the unique one.
	enemy.actions = actions

Juules32 added a commit to Juules32/fusion-forge that referenced this issue Aug 28, 2024
Also added hitpoints, visuals, playing cards with effects.
@elektito
Copy link

I'm not using the duplicate method, or the duplication functionality in the editor, but this still seems to cause me pain. I have a scene with another sub-scene which has an array of custom resources. I create two separate instances of the first scene using the editor, and they still seem to share the custom resource array. I've marked the resources in the array as "local to scene" but that doesn't seem to do anything. I also tried using the editor's "make unique" feature on it, again to no avail.

I'm totally stumped as to what to do. Should I forgo using the editor and create the nodes in code, with the above-mentioned custom duplicate method? No other way around this?

Just in case, I've attached a minimal sample project.
test-uniqueness-bug.tar.gz

@caiomcoliveira
Copy link

I had the same problem

class_name ExampleA
var example: Array[ExampleB] = []

Both were resources, the only thing that worked was @AdriaandeJongh solutions
had to iterate over each item of the array and call duplicate separately

Otherwise the reference for the array was being kept the same, even with duplicate on the array itself, or duplicate(true) on main resource:
(These were supposed to be two different nodes)
image

@limbonaut
Copy link
Author

We were discussing this issue on a discord with @monxa and Yuuto, and here are some key takes:

  1. As @DDarby-Lewis suggested, the cyclic reference issue can be solved by keeping a map of original resources to duplicates. If an original resource is already mapped (already duplicated), we use the reference from the map instead of duplication.
  2. This issue exists for years now, and it's unclear what issues that code was causing.
  3. The "breaks stuff :(" comment in the code was left in 2018 (d280b14) - maybe @reduz can shed some light on what kind of problems they had with this at the time, so we can take some steps to getting it fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests