godot-cpp icon indicating copy to clipboard operation
godot-cpp copied to clipboard

Node::_ready() and Node::_enter_tree() silently fail unless they are registered in register_methods

Open valentinAlkan opened this issue 4 years ago • 6 comments

This is something I knew about already but it got me again recently. It is not mentioned in the docs here, as far as I can tell, and I think that's very important information that should at least be in the example (the readme example does not show _ready or _enter_tree being used).

These are very important functions and I think there should be a note in the read me drawing users attention to this fact.

This is the existing example with my proposed changes in it already:

#include <Godot.hpp>
#include <Reference.hpp>

using namespace godot;

class SimpleClass : public Reference {
    GODOT_CLASS(SimpleClass, Reference);
public:
    SimpleClass() { }

    /** `_init` must exist as it is called by Godot. */
    void _init() { }

    void test_void_method() {
        Godot::print("This is test");
    }

    Variant method(Variant arg) {
        Variant ret;
        ret = arg;

        return ret;
    }

    static void _register_methods() {
        register_method("method", &SimpleClass::method);

        // All built in functions MUST be registered or they will be never called 
        // and there will be no warnings. Only the first parameter must match the 
        // function name that godot expects. If you wish to have them point to different
        // functions, you may do so they will be called at the normal times.
        register_method("_ready", &SimpleClass::_ready);
        register_method("_enter_tree", &SimpleClass::_enter_tree);
        register_method("_process", &SimpleClass::_process);

        /**
         * The line below is equivalent to the following GDScript export:
         *     export var _name = "SimpleClass"
         **/
        register_property<SimpleClass, String>("base/name", &SimpleClass::_name, String("SimpleClass"));

        /** Alternatively, with getter and setter methods: */
        register_property<SimpleClass, int>("base/value", &SimpleClass::set_value, &SimpleClass::get_value, 0);

        /** Registering a signal: **/
        // register_signal<SimpleClass>("signal_name");
        // register_signal<SimpleClass>("signal_name", "string_argument", GODOT_VARIANT_TYPE_STRING)
    }

    String _name;
    int _value;

    void set_value(int p_value) {
        _value = p_value;
    }

    int get_value() const {
        return _value;
    }

   void _ready() {
       // Here the function does not have to be named "_ready", so long as it is still 
       // registered as "_ready", it will still work.
       Godot:print("_ready");
   }

   void _enter_tree() {
       Godot:print("_enter_tree");
   }

   void _process(float delta) {
       Godot:print(delta);
   }
};

/** GDNative Initialize **/
extern "C" void GDN_EXPORT godot_gdnative_init(godot_gdnative_init_options *o) {
    godot::Godot::gdnative_init(o);
}

/** GDNative Terminate **/
extern "C" void GDN_EXPORT godot_gdnative_terminate(godot_gdnative_terminate_options *o) {
    godot::Godot::gdnative_terminate(o);
}

/** NativeScript Initialize **/
extern "C" void GDN_EXPORT godot_nativescript_init(void *handle) {
    godot::Godot::nativescript_init(handle);

    godot::register_class<SimpleClass>();
}

valentinAlkan avatar May 20 '21 20:05 valentinAlkan

@Zylann Is this something we can print a run-time error from in Godot itself?

Calinou avatar May 20 '21 22:05 Calinou

No, because they don't "fail": this is equivalent to litterally not writing a _ready function, or not writing a _process function in GDScript (which registers functions automatically). Not doing so is not an error. In C++, if you dont register them, they are not known to Godot, so they are not called. This is the same thing for any other function that Godot may call (by presence of it, or by connection to signal), so if that's not indicated in docs it probably should be.

Zylann avatar May 20 '21 22:05 Zylann

Sorry I didn't mean to imply it "failed" in the traditional sense, I meant to say they are never called in the first place. "fail silently" is just a more concise way to say that something doesn't work and no log is printed. It "fails" in the sense that fails to run in the first place.

Anyway my note in the example above should be pretty clear and (hopefully) accurate:

        // All built in functions MUST be registered or they will be never called 
        // and there will be no warnings. Only the first parameter must match the 
        // function name that godot expects. If you wish to have them point to different
        // functions, you may do so they will be called at the normal times.
        register_method("_ready", &SimpleClass::_ready);
        register_method("_enter_tree", &SimpleClass::_enter_tree);
        register_method("_process", &SimpleClass::_process);

valentinAlkan avatar May 20 '21 22:05 valentinAlkan

All built in functions MUST

"Should", if you want them called (but arguably if you name a function like a built-in one in that intent then it must) (newcomers should not think they absolutely have to make them)

registered first

I don't think they need to be registered in a particular order

_init must exist as it is called by Godot

It must exist, but not because it is called by Godot, it is actually called by the bindings https://github.com/godotengine/godot-cpp/issues/348#issuecomment-742084113

Zylann avatar May 20 '21 22:05 Zylann

I think its good practice to define them even if you don't use them. If they are not used, then the compiler simply sees that there is nothing in them and therefore ignores them and there isn't any performance penalty. I think its confusing to expect newcomers to know both that built-in functions must be registered or they will never be called, and also that registering them is optional. Seeing as there is no performance penalty for including an empty function, newcomers get the benefit of potentially avoiding a simple mistake, and as they become more advanced they will realize that they can omit them altogether.

This is also consistent with GDScript, which creates a default _ready(): pass when you create a new script, even though it is not necessary to have a _ready function in GDScript either.

All built in functions MUST be registered first or they will be never called

This simply means that they must be registered before they will be called, not that their order matters. However I can see how its confusing to non-native english speakers. I updated the comment.

valentinAlkan avatar May 21 '21 00:05 valentinAlkan

What is the status of this with GDExtension?

The behavior I am experiencing seems to be the opposite. If I register _ready in _bind_methods I get this error:

ERROR: Method already registered as non-virtual.
   at: bind_virtual_method (godot-cpp/src/core/class_db.cpp:292)

If I don't bind it and instead just override the built-in, it works perfectly.

This makes sense to me, I would only suggest improving the error message.

nathanfranke avatar Jun 17 '22 20:06 nathanfranke