Godot 4.0: Open a 3.2 scene works, but after saving, the scene gets corrupted

[Blackiris]

Godot version: 4.0 (a9b151c664d00d353640e6b2c52d026347230269)

OS/device including version: Linux, Nvidia 1050ti

Issue description: When opening some scenes from 3.2, Godot 4.0 opens it successfully. However, after saving, the file get corrupted: it shows Scene file 'Light.tscn' appears to be invalid/corrupt After opening the .tscn file, I notice the "= null" and when removing it, it works again.

[node name="BallLight" type="Node3D"]
script = ExtResource( 2 )
 = null

It looks like the code migrating scene has a bug.

Steps to reproduce:

1. Open the reproduction project
2. open res://Scenery/Light/TogglableOmnilight.tscn
3. Save it (Ctrl + S)
4. Close the scene, and open it again

Minimal reproduction project: CorruptAfterSaveWithSetter.zip

[Blackiris]

Just add the reproduction project, sorry for missing it at the beginning

[Blackiris]

I am trying to look at the code but it is quite hard for a noob like myself... However I notice two things:

1. Once "= null" is removed manually, Godot can open again the scene, but after saving, "= null" reappears => the cause can't be the migration code (if it ever exists)
2. The bug only appears when there is some light in my scenes

[Blackiris]

I tried to remove the lights from the scene but the bug is still reproduced. It looks more like a parsing issue, creating an extra 'empty' property to the null, hence the "= null" when saving

EDIT: well, again, parsing the 3.2 scene (or even the 4.0 scene) seems not to be the issue From what I understand, in packed_scene.cpp (l. 472):

    for (List<PropertyInfo>::Element *E = plist.front(); E; E = E->next()) {
        if (!(E->get().usage & PROPERTY_USAGE_STORAGE)) {
            continue;
        }

        String name = E->get().name;
        Variant value = p_node->get(E->get().name);

        ... (containing "continue" to stop the loop step if conditions are not met)

        NodeData::Property prop;
        prop.name = _nm_get_string(name, name_map);
        prop.value = _vm_get_variant(value, variant_map);
        nd.properties.push_back(prop);
    }

During a loop step, we get name = empty, which create the value = null . However, for some reasons, none of the "continue" is triggered and this fake property is added to the NodeData.

Adding the following code in the middle (with the others continue), fix my issue:

        if (name == String()) {
            continue;
        }

But, the real issue is, how does it gets an empty property name.

[Blackiris]

Looks like this is due to the setter in (the export type tested was bool and String). For example, the following code failed:

@export var isOn: bool = false:
    set(val):
        pass

But this one succeed: @export var isOn: bool = false

[Blackiris]

CorruptAfterSave.zip

With the previous information, here is a simpler reproduction project, with just 5 lines of gdscript (also updated in first post)

[Blackiris]

I have found that the root cause is from gdscript_parser.cpp, GDScriptParser::VariableNode *GDScriptParser::parse_variable.

The name should be set in l.827: variable->export_info.name = variable->identifier->name;

However, when the variable has a setter, it is caught before by l.800, thus never reaching l.827:

if (current.get_identifier() == "get" || current.get_identifier() == "set") {
    return parse_property(variable, false);
}

Thus, moving l.827 to l.781 fixes the issue for me:

[NykolaR]

This change also fixed a similar bug I was experiencing on current master branch, but in my case I was getting the scene corruption from having an inherited scene/script with exported vars with setters.

EDIT: I spoke too soon, my scenes don't corrupt, but the setter funcs get called recursively now and stack overflow. I'll probably look into it a bit more this weekend, for now just going to keep manually calling setters/getters

[Blackiris]

Indeed, the setter is currently broken. It is not due to this PR because when I revert this PR and launch the following code, I can still reproduced the same bug:

class_name OnOffObject
extends Node3D

var objname : String:
    set(val):
        objname = val
        pass

func _ready():
    objname = "aa"
    print("Name:"+ objname)

After launch: Stack Overflow (Stack Size: 1024)

[Blackiris]

Btw, the current bug ( = null in .tscn file) is also mentionned in #42463

[Blackiris]

I have created another issue for the Stack Overflow: #47662

[Blackiris]

The stack overflow issue in #47662 is now fixed in master.

However, I can still reproduce this bug with the current master, and the proposed PR seems to fix it.

I have updated the reproduction project to have this time a working setter. Please find the updated reproduction project in the first post

@export var objname : String:
    set(val):
        objname = val
        print("Setter: " + objname)