codecat / godot-tbloader

TrenchBroom Loader for Godot 4. (Alternative to Qodot)
MIT License
232 stars 29 forks source link

Entites should be added to the scene AFTER their properties have set. #86

Open greenfox1505 opened 10 months ago

greenfox1505 commented 10 months ago

When this line is called, the scenes func _ready() begins. When instantiating a scenes in Godot, the properties tend to be set before add_child(someNode) is called.

image

In this example, the value starts as defaultValue, then it gets set to CHANGED_BY_INSPECTOR, then _ready() runs, then lastly the value gets set to changedByTrenchBroom. Usually, I would add a await owner.ready, but even that is set after the object is added.

extends Node3D

@export var testString:String = "defaultValue":
    set(value):
        print("set from %s to %s" % [testString,value])
        testString = value

func _ready():
    print("ready! testString is %s" % testString)
#   await owner.ready #a common way to make sure properties are set correctly before proceding
    await get_parent().ready
    print("parent ready! testString is %s" % testString)
    pass
Building map res://levels/tutorial/tutorial.map
set from defaultValue to CHANGED_BY_INSPECTOR
ready! testString is CHANGED_BY_INSPECTOR
set from CHANGED_BY_INSPECTOR to changedByTrenchBroom
parent ready! testString is changedByTrenchBroom

I believe if you move line 170 m_loader->add_child(instance); to after line 220 right before the return, it should behave more like what I would expect a Godot object to work. But I haven't tested this. Owner should definitely be set before adding an a child. A common way to verify a scene is fully loaded is to use await owner.ready, and this method isn't useful if owner == null.

codecat commented 10 months ago

Good point! I'd have to test this, because I want to make sure it actually properly loads all of the properties correctly.

greenfox1505 commented 10 months ago

Yeah, obviously you wanna test it before merging it. I'd do it myself if I wasn't in the middle of a game jam.

Skaruts commented 8 months ago

Not sure it makes much of a difference, unless you need to do some post-processing on the node. But for that I think it would probably be more reliable if TBLoader called a specific function on the nodes during the creation stage:

Something like:

void Builder::build_entity(int idx, LMEntity& ent, LMEntityGeometry& geo, const String& classname) {
    for (int i = 0; i < arr.size(); i++) {
        // (...)

        if (resource_loader->exists(path, "PackedScene")) {
            // (...)

            if (instance->has_method("on_import_finished")) {    // <--- this
                instance->call("on_import_finished");
            }

            return;
        }
    }

    UtilityFunctions::printerr("Path to entity resource could not be resolved: ", classname);
}

That should be enough for some post-processing (some other things would require a full second pass). I'm using this in my own slightly modified TBLoader, on an experimental point_prop entity. In TB I give that entity a model index (a bit of advanced fgd magic there to show the 3d models in TB), and then in Godot I use that index to instance a PackedScene of the respective 3D model as child of the entity node, and also to check if the property solid is 0. If it's 0, remove the collision from the model. Very experimental, but it's working.

So yea, for that I need the node to be ready before I process the property, and I need another property to already be set as well (solid). In current state this is only possible if you await in _ready, and I think it makes no difference if its parented before or after the properties are set.

Persoanlly, I don't like using _ready. You have to not forget to keep track of whether you're running in the editor or not, and if you're building maps at runtime you'd want that editor-only-code to run... It's tricky.