Saving as Packedscene: avoid duplicate data and infinite loops with "PackedNodeReference"

[LO-0116]

Describe the project you are working on

A game which saves and loads game states using PackedScene. Some nodes saved contain references to other nodes which ought to be saved. Some of these references are stored in arrays and dictionaries. Some nodes contain references to other nodes which contain a reference pointing back to the first node (mutual reference).

Describe the problem or limitation you are having in your project

When calling pack(...) on a PackedScene instance, several errors appear due to the way Godot currently stores node references:

1. mutual references create infinite loops
2. references stored in arrays and dictionaries created duplicate orphans when loaded

For the first problem, consider this class:

extends Node
class_name Person

@export var spouse : Person

If your scene contains two Person nodes, call them Adam and Eve, they may have mutual references. E.g.: Adam.spouse = Eve; Eve.spouse = Adam When attempting to save Adam, Godot will not only create a block of information containing everything about Adam, but it will also create a block of information containing everything about Eve, and because Eve contains a reference to Adam, the block of information about Eve will also contain a complete block of information about Adam, and so on, causing the game to stall.

For the second problem, while storing references to other nodes in the scene as a single @export var is okay, trying to save references inside exported dictionaries and arrays cause strange, dysfunctional behaviors when loading. See details here.

Describe the feature / enhancement and how it helps to overcome the problem or limitation

I believe the problems above are caused by PackedScene.pack creating a complete block of information when trying to save any node references stored in any @export variables. The solution to this would be saving only a reference of some kind instead of the complete information.

Here is what my suggestion for how PackedScene.pack should function:

1. when called with a Node as an argument, call it root, find all nodes whose owner is root.
2. created a list of "PackedNodeReference" for root and all nodes it owns. PackedNodeReference should be a unique string created based on the node it corresponds to.
3. continue to pack as usual, but when attempting to pack a variable that references another node, check to see if the node being packed has a PackedNodeReference in the list created above, if so, the variable should store that PackedNodeReference instead of a complete block of information.
4. when calling instantiate() on a PackedScene, use PackedNodeReference to reestablish saved references.

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

This is what a PackedScene currently looks like when saving both a node and a reference to a node:

[node name="Beta" type="Sprite2D" parent="." instance=ExtResource("1_dmsr8")]
position = Vector2(536.634, 115.838)
texture = ExtResource("2_pkg25")
script = ExtResource("3_pc6hu")
my_gamma_array = [Object(Sprite2D,"_import_path":NodePath(""),"unique_name_in_owner":false,"process_mode":0,"process_priority":0,"process_physics_priority":0,"process_thread_group":0,"editor_description":"","visible":true,"modulate":Color(1, 0, 1, 1),"self_modulate":Color(1, 1, 1, 1),"show_behind_parent":false,"top_level":false,"clip_children":0,"light_mask":1,"visibility_layer":1,"z_index":0,"z_as_relative":true,"y_sort_enabled":false,"texture_filter":0,"texture_repeat":0,"material":null,"use_parent_material":false,"position":Vector2(0, 0),"rotation":0.0,"scale":Vector2(0.5, 0.5),"skew":0.0,"texture":ExtResource("2_pkg25"),"centered":true,"offset":Vector2(0, 0),"flip_h":false,"flip_v":false,"hframes":1,"vframes":1,"frame":0,"region_enabled":false,"region_rect":Rect2(0, 0, 0, 0),"region_filter_clip_enabled":false,"script":null)
]

[node name="Gamma" type="Sprite2D" parent="Beta" instance=ExtResource("4_6w36h")]
modulate = Color(1, 0, 1, 1)
scale = Vector2(0.5, 0.5)
texture = ExtResource("2_pkg25")

this is what it should look like with with PackedNodeReference:

[packed_node_reference= "Beta0" node name="Beta" type="Sprite2D" parent="." instance=ExtResource("1_dmsr8")]
position = Vector2(536.634, 115.838)
texture = ExtResource("2_pkg25")
script = ExtResource("3_pc6hu")
my_gamma_array = [[packed_node_reference: "Gamma0"]
]

[packed_node_reference= "Gamma0" node name="Gamma" type="Sprite2D" parent="Beta" instance=ExtResource("4_6w36h")]
modulate = Color(1, 0, 1, 1)
scale = Vector2(0.5, 0.5)
texture = ExtResource("2_pkg25")

Note that the massive line in the my_gamma_array field used to store information functionally identical to the block about the node Gamma right below it.

If this enhancement will not be used often, can it be worked around with a few lines of script?

I believe it will be used often as the user community move away from manually saving with JSON or resources. Both JSON and resource-based saves require manual saving and loading functions in the objects affected and are thus error-prone. JSON lacks many of Godot's data types and resource-based saves have known security issues.

Is there a reason why this should be core and not an add-on in the asset library?

Because it addresses problems with an existing core feature (saving via PackedScene).

[ajreckof]

I think they should use id just as Resource do so something like

[node name="Beta" type="Sprite2D" parent="." instance=ExtResource("1_dmsr8") id="some_id"]
position = Vector2(536.634, 115.838)
texture = ExtResource("2_pkg25")
script = ExtResource("3_pc6hu")
my_gamma_array = Node("some_id2")
]

[node name="Gamma" type="Sprite2D" parent="Beta" instance=ExtResource("4_6w36h") id="some_id2"]
modulate = Color(1, 0, 1, 1)
scale = Vector2(0.5, 0.5)
texture = ExtResource("2_pkg25")

I feel like this would be more consistent. Moreover the parent could also use the id instead of a path but not sure if it would be usefull.

[LO-0116]

Yeah, I noticed recently that they already do something similar with subresources where the subresource is written only once and referred to by other things with id. I can see why it might be harder with nodes, but it shouldn't be impossible

[ajreckof]

I don't even see how it is harder (even though it is hard) and it might even make it easier in some places where we need to do some shenanigans to differentiate wether a stored nodePath is actually a Node or simply a NodePath.