PackedScene won't save nodes added to non-root nodes

[LO-0116]

Tested versions

v4.2.stable.official [46dc27791]

System information

Godot v4.2.stable - Windows 10.0.22631 - GLES3 (Compatibility) - Intel(R) Iris(R) Plus Graphics (Intel Corporation; 27.20.100.9664) - Intel(R) Core(TM) i7-1065G7 CPU @ 1.30GHz (8 Threads)

Issue description

When saving with PackedScene.pack(), child nodes that are added at run time to nodes which are not the root of a scene will not be saved regardless of what owner they have.

Code in MRP:

func _on_button_add_feather_pressed():
	for c in get_children():
		if c.scene_file_path == "res://duck.tscn":
			var label = c.find_child("Sprite2D")
			
			var new1 = scn_feather.instantiate()
			var new2 = scn_feather.instantiate()
			
			c.add_child(new1) ##this adds the node to the root of duck.tscn
			new1.text = "this will be saved"
			
			label.add_child(new2) ##this adds the node to a non-root node inside duck.tscn
			new2.text = "this won't be saved"

Note that in the MRP, the owner of all saved nodes is set by a recursive call in main.gd, but the exact way the owner is set does not matter.

Steps to reproduce

with MRP:

1. open and run
2. click "make pond"
3. click "spawn ducks" on the pond
4. click "add feather" on the pond
5. click "save" then "load"
6. you will see that the feather (label) added to the root of duck.tscn will appear in the copy but the feather added to the sprite of the duck will not.

Minimal reproduction project (MRP)

PackedScene save test.zip

[aaronp64]

The feather added to the duck's sprite is not saved/loaded because the sprite's owner is the duck instance, and the owner value isn't being updated in _set_owner_recursive. _set_owner_recursive only sets the owner if owner is null, or if the node's owner is not a descendant of new_owner. Since the sprite's owner is the duck, which is a descendant of the new_owner pond value, it isn't being updated.

The Node.owner documentation says that "When saving a node (using PackedScene), all the nodes it owns will be saved with it" In this case, the feather instances you're trying to save do have the new_owner value applied, but one is being left out because it's parent's owner is not set. It looks like PackedScene.pack does not allow skipping over a node and including it's descendants. I don't know enough about why the node owner requirement is there, so I don't know if this is expected behavior.

Changing the _set_owner_recursive function to use var should_set := new_owner == null || new_owner.is_ancestor_of(node) should get everything saved/loaded into the packed scene.

[LO-0116]

@aaronp64 using var should_set := new_owner == null || new_owner.is_ancestor_of(node) in the MRP does work, but this code seems like it just naively sets all descendants of the new_owner as owned by new_owner, which I recall not working in my actual production.

I will be verifying this in the main project, but in the meantime, can you explain why new_owner == null is a part of the should_set condition? It seems like if that part of the statement is true the whole operation would be pointless since the owner is null, is this a typo?

[LO-0116]

Okay, I found a problem with changing _set_owner_recursive to use var should_set := new_owner == null || new_owner.is_ancestor_of(node): it creates orphaned nodes after a savefile is loaded

You can verify this using the "print orphan" button. With the MRP it seems to create an orphan of the sprite. With my main project it creates a huge cascade of orphans probably because the node tree is deeper with more inappropriate set owner operations. I remember someone warning me once that if a node already has an owner, setting it to something else will cause orphans to be created when the save file is loaded so naively setting owner for all descendants was probably a mistake.

simply adding

if node.owner != null:
		should_set = false

takes us back to square one where some of the labels are not saved

[aaronp64]

So it looks like the orphans are due to the PackedScene seeing that the Duck object came from Duck.tscn. When it loads back in, it uses that file, and loads its children in from there. But if we set the owner to the Pond before saving, the PackedScene also saves the Duck's child nodes separately, so there's two copies to load, and one gets orphaned.

It might be possible to have PackedScene handle the orphan case, or be able to skip a node and include its children, which would work with your original MRP, but I don't know enough about it to tell if either of those options makes sense - someone more familiar with the PackedScene logic would have to advise on that.

I did find a way to get everything saved/loaded without orphans (at least, it seems to work for the MRP), though it feels like there should be a simpler way to do this. Changing _set_owner_recursive to the below, and passing in an empty dictionary for old_paths:

func _set_owner_recursive(node : Node, new_owner : Node, old_paths : Dictionary):
	if new_owner == null:
		return
		
	var should_set := new_owner.is_ancestor_of(node)
	
	if should_set:
		node.owner = new_owner
		if node.scene_file_path:
			old_paths[new_owner.get_path_to(node)] = node.scene_file_path
			node.scene_file_path = ""
	for c in node.get_children():
		_set_owner_recursive(c, new_owner, old_paths)

If you don't need the original scene_file_path values, you can leave out the old_paths dictionary, and just clear out the old values. If you want to set them back after saving, you can loop through the dictionary keys and do something like pond.get_node(key).scene_file_path = value. To set them back after loading, you would need to also save/load the dictionary.

[LO-0116]

so it seems like a node having a non-empty scene_file_path is causing the duplication. I haven't had time to verify if the fix scales to bigger projects or whether setting scene_file_path to empty causes other problems elsewhere but it's definitely unintuitive that it has this side effect.

Not sure if I should close this issue and create a new one