godotengine / godot-proposals

Godot Improvement Proposals (GIPs)
MIT License
1.1k stars 69 forks source link

Define C++ StringName class constants to use as signal/theme item names instead of using string literals #2152

Open Calinou opened 3 years ago

Calinou commented 3 years ago

Describe the project you are working on

The Godot editor :slightly_smiling_face:

Describe the problem or limitation you are having in your project

In Godot 4.0, GDScript will have first-class signals, but this change doesn't apply to the C++ code. Therefore, string literals are still used to reference signal names (among other things such as theme items).

As in any programming language, using string literals for "fixed" strings poses several problems:

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

Switch to string constants that are considered actual symbols in the codebase.

Another upside of using string constants (which are class members) is that we can add comments above each constant. In most IDEs, these comments will appear when hovering the signal's name. Since we already document signals in the XML class reference, I would recommend doing this only for signals that aren't exposed to the scripting API.

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

In the header file, add C++11 class constants to provide typo-resistant, autocompletable names for things like signals and GUI theme items:

class NetworkedMultiplayerPeer {
public:
    // This is the only way I've found to have a statically reference-able constant string
    // as we can't use StringName here.
    static constexpr const char* SIGNAL_PEER_CONNECTED = "peer_connected";

    // ...

Then in the implementation file, turn something like:

network_peer->connect("peer_connected", callable_mp(this, &MultiplayerAPI::_add_peer));

Into:

network_peer->connect(NetworkedMultiplayerPeer::SIGNAL_PEER_CONNECTED, callable_mp(this, &MultiplayerAPI::_add_peer));

I've tested this change locally and it works fine. The class qualifier can be omitted when referencing the constant in the original class' code.

This change will lengthen the connect() calls and will require a lot of work to adapt the whole codebase, but I think it's worth the effort.

Why not enums? We can't use enums with StringName or string values in general (not even C++11 scoped enums).

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

No, as this change only pertains to the core C++ code.

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

This change only pertains to the core C++ code.

hilfazer commented 3 years ago

I fully support reduction of magic numbers/strings. They caused me some troubles when i was working on a code that was producing and reading .tscn files.

I don't remember what exact string i was searching for, let's say it was "path". Results i got didn't contain the code i was looking for. Why? Because the string in source code was something like this: "[path" which won't be found if i search for "path". And if i search for path i'll also get any variable with such name. And all comments containing that word on top of that.

Constants would spare me those troubles.

AaronRecord commented 3 years ago

Found https://github.com/godotengine/godot/blob/master/scene/scene_string_names.cpp which I think does a similar thing, but only for certain signals, and I like your solution better

AaronRecord commented 3 years ago

Would it be possible to make signals actual objects like they are in gdscript? It'd be nice to be able to do something like: button->pressed.connect(callable_mp(...));

KoBeWi commented 9 months ago

https://github.com/godotengine/godot/pull/81303 adds a macro for using singleton StringNames, the syntax is SceneStringName(peer_connected) etc.