Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
Calinou opened this issue Jan 19, 2021 · 4 comments

Comments

@Calinou
Copy link
Member

Calinou commented Jan 19, 2021

Describe the project you are working on

The Godot editor 🙂

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:

  • They are prone to typos, which can create bugs only noticeable at run-time instead of compile-time.
  • They can't be autocompleted using the IDE's features. This means you need to check the class reference more often while working on Godot's codebase.

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
Copy link

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
Copy link

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
Copy link

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
Copy link
Member

KoBeWi commented Nov 6, 2023

godotengine/godot#81303 adds a macro for using singleton StringNames, the syntax is SceneStringName(peer_connected) etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants