PackedScene API outside the editor overrides all child node properties

[jasonwinterpixel]

Godot version

3.5

System information

Linux Mint

Issue description

Packing a scene via the PackedScene API in a running game produces an incorrect serialization of the scene which differs from saving a scene in the editor.

The incorrect serialisation does not properly compute what is overriden by instances and incorrectly writes every property into the .tscn. This makes it so changes to the scenes that are instanced in this incorrectly saved scene will not apply to this incorrect scene.

If you view this incorrect scene in the editor and then save it in the editor, the editor will repair this incorrect serialization.

Properties in a Child Scene which are not changed in a Parent Scene should not be present in Parent.tscn, or else it makes it so Parent is overriding Child. The editor serializes correctly but the PackedScene api at runtime does not.

Note: This work is being done to create a level editor where players can make levels in game, using nodes that we provide. We need this code to work to allow us to update those nodes without having to reserialize every player created level.

Steps to reproduce

Create a Scene, and set a property (like sprite.texture) call it Child in the editor and save it to the project

Write a script which creates a node called Parent, instances a Child and saves Parent via the PackedScene API

View Parent.tscn, note that all properties (eg: sprite.texture) are being saved into the .tscn. This is incorrect as some of these properties are defined in Child and are not overriden in Parent.

Change some of these properties in Child.

Load the parent via script, (as to be sure not to have the Editor reserialize Parent.tscn). Note that the instances of Child.tscn have not been updated to reflect the changed property.

Make a copy of Parent.tscn, open it in the editor and hit save in the editor and you will see how it should be serialized. Specifically properties that are in Child.tscn but not overriden in the Parent scene are not present in Parent.tscn file and now properly reflect future changes made to Child.tscn

Minimal reproduction project

PackedSceneTest.zip

Normal, correct scene created in the editor:

[gd_scene load_steps=3 format=2]

[ext_resource path="res://BasicScript.gd" type="Script" id=1]
[ext_resource path="res://NestedScene.tscn" type="PackedScene" id=3]

[node name="Node2D" type="Node2D"]

[node name="Node" type="Node" parent="."]
script = ExtResource( 1 )

[node name="NestedScene" parent="." instance=ExtResource( 3 )]

That same scene loaded and saved at runtime, with incorrectly computed overriden properties:

[gd_scene load_steps=4 format=2]

[ext_resource path="res://NestedScene.tscn" type="PackedScene" id=1]
[ext_resource path="res://icon.png" type="Texture" id=2]
[ext_resource path="res://BasicScript.gd" type="Script" id=3]

[node name="Node2D" type="Node2D"]

[node name="Node" type="Node" parent="."]
script = ExtResource( 3 )
some_property = 1

[node name="NestedScene" type="Sprite" parent="." instance=ExtResource( 1 )]
self_modulate = Color( 1, 0, 0, 1 )
texture = ExtResource( 2 )

The code to load and save the scene at runtime to produce the incorrect serialization:

var instanced_node = scene.instance()
get_tree().root.add_child(instanced_node)
var packed_scene := PackedScene.new()
packed_scene.pack(instanced_node)
ResourceSaver.save("res://scene_saved_at_runtime.tscn, packed_scene)

[jasonwinterpixel]

property_utils.cpp line 98:

        // Should be called in the editor only and not at runtime,
        // otherwise it can cause problems because of missing instance state support.

Looks like relevant information. This comment was written by Pedro Estebanez. Looks like all this code was written by Pedro.

[RandomShaper]

I'll take a look. However, that comment plus the condition date back to older than my changes. I just brought those lines to a partial rework of that function which involved moving it to a new file.

[jasonwinterpixel]

At runtime, this code returns an empty Vector, while the editor finds a non empty states_stack.

// packed_scene.cpp
// in SceneState::_parse_node()
Vector<SceneState::PackState> states_stack = PropertyUtils::get_node_states_stack(p_node, p_owner, &instanced_by_owner);

[jasonwinterpixel]

I have discovered that at least part of this is due to

// PropertyUtils::get_node_states_stack()
//...
} else if (n->get_filename() != String()) {
// this is not empty in editor, is in exported game
const Ref<SceneState> &state = n->get_scene_inherited_state();

Now scene inherited state is empty due to the way packed scenes are instanced.

Node *PackedScene::instance(GenEditState p_edit_state) const {
#ifndef TOOLS_ENABLED
    ERR_FAIL_COND_V_MSG(p_edit_state != GEN_EDIT_STATE_DISABLED, NULL, "Edit state is only for editors, does not work without tools compiled.");
#endif

    Node *s = state->instance((SceneState::GenEditState)p_edit_state);
    if (!s) {
        return nullptr;
    }

    if (p_edit_state != GEN_EDIT_STATE_DISABLED) {
        s->set_scene_instance_state(state);
    }

I can fix some amount of the incorrect serialization by instantiating scenes with GEN_EDIT_STATE_INSTANCE as the argument:

var instanced_node = scene.instance(PackedScene.GEN_EDIT_STATE_INSTANCE)

namely this is the difference: Editor serialization:

[gd_scene load_steps=3 format=2]

[ext_resource path="res://packed_scene_test/BasicScript.gd" type="Script" id=1]
[ext_resource path="res://packed_scene_test/NestedScene.tscn" type="PackedScene" id=3]

[node name="BasicSceneRoot" type="Node2D"]

[node name="BasicScriptNode" type="Node" parent="."]
script = ExtResource( 1 )

[node name="NestedNode2D" parent="." instance=ExtResource( 3 )]

Saving something that was created using default PackedScene.instance() call:

[gd_scene load_steps=3 format=2]

[ext_resource path="res://packed_scene_test/BasicScript.gd" type="Script" id=1]
[ext_resource path="res://packed_scene_test/NestedScene.tscn" type="PackedScene" id=2]

[node name="BasicSceneRoot" type="Node2D"]

[node name="BasicScriptNode" type="Node" parent="."]
script = ExtResource( 1 )
some_property = 1

[node name="NestedNode2D" type="Node2D" parent="." instance=ExtResource( 2 )]
modulate = Color( 0.968627, 0, 0, 1 )

Saving something that was instanced using PackedScene.instance(1):

[gd_scene load_steps=3 format=2]

[ext_resource path="res://packed_scene_test/BasicScript.gd" type="Script" id=1]
[ext_resource path="res://packed_scene_test/NestedScene.tscn" type="PackedScene" id=2]

[node name="BasicSceneRoot" type="Node2D"]

[node name="BasicScriptNode" type="Node" parent="."]
script = ExtResource( 1 )
some_property = 1

[node name="NestedNode2D" parent="." instance=ExtResource( 2 )]

Note that the NestedNode2D no longer overrides the modulate property.

However godot is attemping to restrict this behaviour to tools builds. I am going to be disabling that force in godot source code and attempting to instance and save nodes in exported games using GEN_EDIT_STATE_INSTANCE.

I think we need better documentation on PackedScene.instance()'s parameters.

I stil don't know how to use the PackedScene API to produce correctly serialized scenes in exported games.

Either a new GEN_EDIT_STATE needs to be created for this behaviour to work, or perhaps this condition can be relaxed:

// PackedScene::instance()
if (p_edit_state != GEN_EDIT_STATE_DISABLED) {
    s->set_scene_instance_state(state);
}

Or what "does not work" and why should be elaborated upon in this comment so we can fix it to enable exported games to do what seems to be in their power already, namely to piece together existing nodes into new scenes and save them without overwriting their settings and preventing them from being updated.

#ifndef TOOLS_ENABLED
    ERR_FAIL_COND_V_MSG(p_edit_state != GEN_EDIT_STATE_DISABLED, NULL, "Edit state is only for editors, does not work without tools compiled.");
#endif

This work is being done to create a level editor where players can make levels in game, using nodes that we provide. We need this code to work to allow us to update those nodes without having to reserialize every player created level.

[jasonwinterpixel]

GDScript default values are not recorded into exported builds. Why not? Is there a performance cost to this? Is there a subset of these values that can be enabled on exported builds such that the PackedScene api can produce correctly serialized scene files?

gdscript.h:
#ifdef TOOLS_ENABLED

    Map<StringName, int> member_lines;

    Map<StringName, Variant> member_default_values;

    List<PropertyInfo> members_cache;
    Map<StringName, Variant> member_default_values_cache;
    Ref<GDScript> base_cache;
    Set<ObjectID> inheriters_cache;
    bool source_changed_cache;
    bool placeholder_fallback_enabled;
    void _update_exports_values(Map<StringName, Variant> &values, List<PropertyInfo> &propnames);

#endif

[jasonwinterpixel]

Seems like this an age old deliberate decision by Godot core developers. I don't imagine it will ever be changed unless I do it.

For now, I have worked around these problems by storing all data that shouldnt be overridable in a subnode inside the scene:

The PackedScene API outside the editor still 'incorrectly' serializes the resulting Levels. It specifies a 'script' for every LevelBlock, which is a property defined inside the LevelBlock scene. If we ever want to change a script to an existing LevelNode, we will have to reserialize every single level. This is still a pretty big problem for us.

[Calinou]

GDScript default values are not recorded into exported builds. Why not? Is there a performance cost to this?

Serializing default values increases file sizes, especially with complex objects that have lots of properties (and few of them are changed). This used to be done back in the Godot 2.x days, but it was changed in 3.0.

There's been a few requests to have a property hint that allows serializing a specific property even if its value is the default, but it hasn't been implemented yet.

[RandomShaper]

The engine needs to know the default values of every property of the builtin classes. That information is stored in ClassDB and is filled on every startup of the editor. I have not checked, but I guess those values GDScript only keeps in tool builds come from there. That means that, in order to allow running projects to benefit from the expected serialization either the engine should do that heavy collection at the startup of a project (not feasible), or the engine could have a snapshot of that information in an easy to load format, suitable for non-tools builds as well as tool builds running projects.

[jasonwinterpixel]

Serializing default values increases file sizes,

I was unclear in my earlier comment when I said "exported builds dont record default values" - I dont think the engine should serialize default values. I think an exported game should be able to compute what is a default or inherited value. Currently, only the editor can do that, and I dont think it needs to be that way.

the engine should do that heavy collection at the startup of a project (not feasible)

Just to clarify, we're referring to an exported, non tools, game starting up right? Why is it not feasible? Seems like it should be feasible. Perhaps we just need to take some stuff out of TOOLS_ENABLED? Is there some massive cost to computing what is and what is not a default value? Perhaps it could be lazily evaluated only when a PackedScene is used? Maybe the computation of default values was disabled in non tools builds without considering the implications on the PackedScene api and it is simply an oversight.

[RandomShaper]

Why is it not feasible?

It takes a long time, in the order of seconds, and likely a non negligible amount of memory.

[jasonwinterpixel]

It takes a long time, in the order of seconds,

When and how frequently is this performance cost paid? On script load per script? Or once on game start?

If the cost is only paid on usage of the PackedScene API, then that would perfectly acceptable.

Maybe there is a subset of the system in question that can let the PackedScene API work consistently between editor and non editor but not have this performance cost.

[RandomShaper]

Quoting myself:

That information is stored in ClassDB and is filled on every startup of the editor.

Maybe there could be some function you can call to trigger that process in non-tools builds.

[nezvers]

I have also created an in-game level editor that is saving rooms as Godot scenes. Room created from room_node = template.(PackedScene.GEN_EDIT_STATE_MAIN) Spawned objects instantiated with new_inst = instantiate(PackedScene.GEN_EDIT_STATE_INSTANCE), and on room node call room_node.set_editable_instance(new_inst, true). That enables editing object sub node exported variables and save into PackedScene. But level editor is dysfunctional in exported build. It errors at a room instantiation. Such functionality shouldn't be locked behind in-editor builds.

I tried building with target=template_release tools=yes but that didn't allow to use PackedScenes functionality. Workaround that worked was building (maybe that's not needed) target=editor and putting game's *.pck with same name as exe. That opened game with needed functionality. That functionality is needed and expected to be available in exports.