Expose signals to GDExtension to be more similar to how they're exposed in GDScript

Describe the project you are working on

I'm working on a 3D game with heavy use of Signals to notify things about states being changed

Describe the problem or limitation you are having in your project

When connecting signals in C++, built-in nodes have to be connected using the StringName of the signal instead of the signal being part of the class itself. (I.E. childNode3D->connect("visiblity_changed",Callable(this,"do_something")) ) this is more so to prevent the ability to accidentally mispell the name of the signal

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

Where applicable, use the signal class to store the reference to the class' signal

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

either

class Node3D : public Object{

private:
    Signal visibility_changed{this,"visibility_changed"};

public:
       Signal& get_visibility_changed_signal() const
      {
          return visibility_changed;
      }
}

class Node3D : public Object{

public:
    Signal visibility_changed = Signal(this,"visibility_changed");
}

though based on what's already established, the first one may be better

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

this can be worked around with

class MyClass : public Node3D{
       GDCLASS(MyClass,Node3D)

public:
      Signal visibility_changed{this,"visibility_changed");
      Signal child_visibility_changed;
      void _ready() override
      {
          childNode = get_node<Node3D>(childNodePath);
         if(childNode != nullptr){
          child_visibility_changed = Signal(childNode,"visibility_changed");
       }
    }
private:
      Node3D* childNode = nullptr;
      NodePath childNodePath;       
};

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

according to what was said here

https://github.com/godotengine/godot-cpp/issues/1523

this would have to be part of the engine as it's due to how signals are exposed from the engine.

To clarify here: Signals are already exposed, but the limitation here is that when using godot-cpp the class generation doesn't create a public variable for the signal like described above

We could add this to the generation code in godot-cpp (though there could be some performance issues and other potential pitfalls related to pointer validity etc. that would need to be explored) but if we do this one of the core requirements for the design of godot-cpp would be broken, namely that: Code written so that it works correctly in godot-cpp should (with some minor considerations) be usable with the core engine itself in things like modules (see the text_server_adv/fb for a prime example, which can be built either as a module or an extension)

Without exposing these signal members in the engine classes themselves this would make this feature in godot-cpp only usable in code that's exclusively designed for extensions, which would cause confusion and reduce the usefulness of it

My view is that this is largely an unfortunate limitation of how c++ works, c++ lacks some of the features and generation support that, for example, C# has, or the way GDScript is written, it's not really built for dynamic variable generation and binding as this would require, that's why, for example, godot-cpp lacks properties like GDScript and C# has for example, forcing you to use the get/set methods exclusively. This means that any signal system would have to be hand crafted which makes it easy to make mistakes

Unlike method binds that reference a real method in c++ this would require you to add a signal bind to the header, and then to the method bind, and then there would have to be some system to automatically initialize these on class creation, that makes it more fragile than the current system of just one place in _bind_methods.

We already have cases where things can get a bit broken with virtual methods which are declared in the header, and then registered in _bind_methods, where errors have been made with registering names etc.

To summarize:

While I think this is an intriguing idea I see several issues with it unfortunately:

It makes the code more fragile and error prone in the engine, where we do no generation of source files and everything has to be doubled up
It meshes poorly with the general design and features of c++, which is unfortunately one of the weaknesses of this language, and we already use a lot of sometimes hacky macro use to get around this as is
The existing signal system is well established so I don't think it would make sense to replace the current way we do things in the engine with this, it would involve a lot of code being rewritten if we replace it entirely, and a lot of confusion if we gradually phase the new system in. This could be considered for Godot 5 though as a new system, but to make it align with godot-cpp we would at least have to register the signals this new way everywhere at once
It would also increase the size of every single object that has signals in it, at least 16 bytes per signal, further there are some performance issues involved, one issue is that the Signal class stores an ObjectID, not an object pointer, so it would have to fetch the object from ObjectDB each time which takes up performance, initializing each object would also be slower as you need to initialize the signal variable for each class in the inheritance hierarchy

tl;dr; Intriguing idea IMO, but unfortunately would require a lot of work and a lot of potentially fragile code, and it doesn't align with how we use c++, or (which is arguably more relevant) how c++ generally works (or works easily)

Unlike method binds that reference a real method in c++ this would require you to add a signal bind to the header, and then to the method bind, and then there would have to be some system to automatically initialize these on class creation, that makes it more fragile than the current system of just one place in _bind_methods.

wouldn't a solution to this be more or less default initializing a potential Signal class member to but be the pointer of this, and then the name of the signal? from what i've tested this seems to work as long as the signal was bound in the first place due to the order of which signals are bound and class members are initialized (though in order to prevent any mispelling from the signal used in the signal member and the signal bound to the classDB I used a static constexpr const char* variable to have the name be identical)

It meshes poorly with the general design and features of c++, which is unfortunately one of the weaknesses of this language, and we already use a lot of sometimes hacky macro use to get around this as is

(Correct me if i'm wrong) but to my knowledge, this point has been solved in later versions of C++ due to the introduction of reflection in C++20. However C++20 and beyond are still not general considered stable for some applications (and even if Godot were to move to C++20 there would most likely need to be a lot of work done in the first place). The only other thing I can think of is to use a macro generation system similar to what Unreal uses

IMO I think this would benefit from a wider discussion about how to theoretically do this with the current state of things

wouldn't a solution to this be more or less default initializing a potential Signal class member to but be the pointer of this, and then the name of the signal?

How? How would you get this? You can't automatically default it without explicitly doing so in the constructor, hence the two declarations (or do you mean in the header? That sounds pretty unorthodox, might be possible though I haven't seen it use this before myself)

But regardless of the future better support for this in c++ (I haven't seen anything in the C++20 that would improve this, it's mostly static reflection still) there are several issues as outlined above that won't be fixed, but it would be more feasible for a future version like Godot 5 and restructuring how we do signals entirely, as changing every signal bind in the entire engine is an enormous task, we also are not going to migrate to C++20 for quite a while yet, at least a few years AFAIK (especially as migrating to C++20 isn't just a matter of changing the build flag, this has been tested and it involves some non-minimal work across the engine to make it compile with c++20)

If this can be done with minimal overhead and fragility then it's more interesting, but note also that this would (unless we rewrite how Signal works) involve adding extra overhead performance wise to the engine when binding, and the handling of StringNames, and especially the non-insignificant memory addition (again, 16 bytes at least per signal per instance), this is because of the lack of dynamic interfaces with C++ where the signal would be generated on-demand

To me the better solution is to register the names of signals as static in the class that declares them, that makes connecting easy, you just reference the static signal name in the class, and there's none of the overhead (this is akin to how the C# bindings do it) and all the convenience, and it works far better with the general design of C++

Tbh that does seem like the best solution right now. Mainly I recommended having signals in some way shape or form able to be present as part of a class because of the fact that connecting a signal in GDExtension does have a large potential for misspellings, and in some instances confusion as to which class has what signal unless constantly referencing the documentation (Tbf that's what the documentation is there for, but for some people it's a hassle to keep having to look at it, mainly people who have 1 screen)

godotengine / godot-proposals