Support to remove, edit and store user defined signals

[koodikulma-fi]

Feature (updated)

Adds support to:

1. Override a previously added user signal.
2. Support to add full PropertyInfo for user signal arguments - not only picking name and type.
3. Remove an existing user signal by remove_user_signal(name) method.
4. Also adds support to add the signal persistently (using _user_signals meta data) and auto-declare the signal when the scene is instatiated. This is done via optional 3rd argument in the add_user_signal(name, args=[], persistent=false) method.

GDScript examples

Feature 1: Allow to modify existing user signals.

add_user_signal("my_signal")
add_user_signal("my_signal", [ { name = "first_arg", type = TYPE_INT } ])

Feature 2: Full support for property infos.

 add_user_signal("my_signal", [ { name = "mode", type = TYPE_INT, hint = PROPERTY_HINT_ENUM, hint_string = "one,two" }])

Feature 3: Allow to remove an existing signal.

remove_user_signal("my_signal")

Feature 4: Store the signal persistently.

add_user_signal("my_signal", [], true)

Implementation

The implementation for 1-3 is very straight forward:

1. Just removed the check that prevented re-adding the signal into signal_map hashmap. If wants to prevent adding an already existing signal, should use has_user_signal(name) before.
2. Changed the manual picking of name and type to using PropertyInfo::from_dict() method.
3. Added a method to remove the user signal from the signal_map.

The feature for persistent user signals (4) is implemented by:

• Storing the signal using _user_signals meta data, which is an array of dictionaries: [ { name, args }, ... ].
• When a packed scene is instantiated, checks if has meta data and if so loops the array and declares all the signals. This part is done after adds to groups and before corrects the naming.

Also modified the docs (in Object.xml) accordingly.

Why

The purpose of this commit is to allow better setting up of dynamic (=user declared) signals when preparing scenes in the editor. The user may modify the signal arguments (= override) or rename the signal (remove + add) while developing the game scene (eg. using some tool using these methods), and then to use the newly generated signals to hook up to more things in the scene.

Note. In general terms, the user signals feature (= scriptless signals) is very useful, for example, for cases where several different game objects share the same root script (eg. using a common modular node structure), but want to provide custom signals that are emitted from somewhere within (eg. by features of one of those modules). For example, a helpless creature with a custom "panic" signal. (In this sense user signals are a bit like meta data - independent of the script.) So this commit aims to enhance / complete the existing feature set.

Further thoughts

(feature 5:) To totally complete this idea of "user signals" feature, the editor could provide a way to edit the user signals manually - just like it provides a way to edit persistent group names and meta data.

But at minimum, I'd at least like to change 1. add_user_signal to override the earlier signal, 2. add suppor for hint etc., and 3. to add remove_user_signal. If the 4. concept of persistent is "not worth it" (in larger context), or the implementation not good (eg. maybe meta data not the cleanest way(?)), then I'd like to reduce this PR to those three main aspects. (But I believe making signals as editor-editable as groups and meta data enhances flexibility of the Godot architecture.)

[Chaosus]

Thanks for your contribution. You need to squash the commits as required by our pipeline (see docs.godotengine.org/en/latest/community/contributing/pr_workflow.html).

[koodikulma-fi]

Okay, thanks for the help! I squashed the commits together now.

I'm still wondering about the "Further thoughts" above: If it'd actually be better to always override (without error) an existing user signal, and thus not consume the 3rd argument for it. This way, in the future, can potentially use the 3rd arg (eg. for persistent connections, like is done with groups). I would personally prefer that (= 2 args and always overriding), but it'll change the current behaviour slightly - though only very slightly and being that add_user_signal is the only(?) way to add user signals, the actual practical problems would be very rare (and easily worked around using has_user_signal anyway).

[Mickeon]

I noticed the lack of these methods to be an odd design flaw while I was making #67880, but at the same time, I'm struggling to come up with concrete examples for User Signals (the implementation of methods in the PR especially) that are not just generic use-cases, that may or may not be solvable with some code restructuring. Could this be worth of a proposal?

That is not to mention, how their existence may be a misnomer of sorts. Based on the way "User Signals" work, they should have probably been called "Instance Signals", or something along those lines.

[koodikulma-fi]

Here is a concrete example, although highly specific to a specific system:

• I'm building a general game tool set, which contains a "SignalEmitter" node class that allows you to define user created signals (for a node in the scene) and then trigger them conveniently using the SignalEmitter node.
• In more detail, the system works by a general background "bake" / "unbake" system that allows the user to easily swap between game-ready and editable forms (by pressing Ctrl+B). In this particular case, it only bakes the signals & their params.
  • For example, in the pic below there are two user signals: "Mangusti" and "value_changed" with arguments defined.
  • When unbaked:
  • When baked:
• Using this system, the user can easily define custom signals (with datatypes) and then save the scene whose root node now features custom user signals. Furthermore, the user can "hide" the triggering part inside the root node, while the signals are just exposed on the root (in the toolset above there's a separate "trigger" system to make triggering nodes with trigger params convenient).
• The feature of user / scriptless / instance signals becomes really powerful with modular node structure: in the above screenshot all nodes in the scene are modular / general (= not specific to the edited scene), but most importantly the root node where the signals are added knows nothing about the signals.

This is just to showcase possibilities of having user signals. By completing the features with remove & modify, the user signals are completely controllable from the GDScript side. In addition, this support could be enhanced with the "persistent" argument to automatically declare the signal (eg. when the scene is instantiated). I'm actually almost finished writing that part - with changing the override arg to persistent (and using meta _user_signals to store them).

[koodikulma-fi]

Okay, I've now made a refined version of this pull request, that features persistent as the 3rd argument for add_user_signal and by default allows to override existing user signals. I would like to replace this PR with the new one (and modify the commit and description above accordingly), is that okay?

Here are the principles:

1. When _add_user_signal(name, args, persistent) is called, the meta is updated accordingly after adding the signal.
   • The meta data key is _user_signals (META_USER_SIGNALS in Object.h) and the data is an array of dictionaries: { name: String, args: Array }.
   • If persistent is true, finds the dictionary in meta and modifies it, or appends new if didn't have.
   • If persistent is false, checks if had the signal already, then finds it in meta and removes it if had.
   • 
2. When _remove_user_signal(name) is called, makes sure the meta is removed as well.
   • 
3. The meta data is used in the instantiate method in scene/resources/packed_scene.cpp.
   • After handling groups (and before handling names), 1. gets the meta for _user_signals, 2.loops through the array, 3. creates method info from the dictionary, and 4. calls add_user_signal(mi) with it.
   • 
4. Unrelated to above, but related to enhancing add_user_signal method, I've also added the support to use hint and hint_string.
   • In that same ref. project, I'm currently hacking around it by storing the info in meta, just so that can read enum hints etc. I don't see why hint and hint_string would not be stored (they work just fine).
   • Eventually just removed the manual checks for type and hint and replaced with PropertyInfo::from_dict call:
   • 

In my view, this PR now "completes" the user signals as a feature in terms of core functionality. (Apart from core, the editor side could have a feature to help user create signals manually, in the same sense that can edit group names and meta data manually.)

Also, could anyone help to help me fix the xml failure in Linux builds..?

[Mickeon]

I feel like the override parameter is quite redundant and not something I've ever seen the Godot API do. It is typically up to the developer to control this. As for another example, for Node groups, add_group can override an existing group's persistence without any error or warning. I would suggest stripping it away outright, as the alternative PR you're aiming for kind of does already.

Persistence sounds quite useful but it requires some solid implementation across the board to avoid buggy behavior.

[koodikulma-fi]

@Mickeon I agree 100%. As this is my first PR, I don't know the protocol. Can I just make a new squashed commit, and update the description?

[Chaosus]

Documentation is broken, type ./godot.exe --doctool .. in command line to fix it.

[Mickeon]

You can change the PR's intent any time without needing to reopen a new PR. Doesn't matter too much besides losing some history and context.

[koodikulma-fi]

Okay, now I think the --doctool ran correctly - seems I didn't use it correctly earlier. Apologies.

[koodikulma-fi]

Sorry to bump, but could someone re-trigger the workflow tests? All should be fixed.

[Mickeon]

Support to add full PropertyInfo for user signal arguments - not only picking name and type.
Feature 2: Full support for property infos.

What would this do? They may share the same "Dictionary structure", but most of the entries in there would be entirely unused by the engine, as they are currently (unless I'm mistaken?).

Secondly, using metadata to serialise the signals is quite the workaround. It definitely should be serialised in some other, more personalised manner, but it is understandable that it would require more knowledge of the source code to implement such a thing.

Do note I'm a contributor like many, so this may be more up to the maintainers to decide.

[koodikulma-fi]

Support to add full PropertyInfo for user signal arguments - not only picking name and type.
Feature 2: Full support for property infos.

What would this do? They may share the same "Dictionary structure", but most of the entries in there would be entirely unused by the engine, as they are currently (unless I'm mistaken?).

Well, from my perspective, it seems odd that they are not included (= it's a surprising restriction) - or rather that it behaves as if they are overridden after a process frame (they are there if you ask immediately). Currently in one project I'm using a hackaround of storing the signal infos as meta data, just so that I can get full information out (it's related to that system with the screenshot above). In that context, it's very convenient to know the hints of an enum or bitflag, or perhaps the subtype of an array.

So, even though the engine might not currently make use of them, they might be useful to have there as information (eg. using get_signal_list()). ... And not sure, but if understood correctly, there's no less information stored (it anyway makes a PropertyInfo of the info) - so if that's true, there's no downside in this..? (Or to make sure, could perhaps refine that stores in the "picky way" and adds hint and hint_string only if they actually have some value in them.)

About the metadata, you're probably right - or, at least, I have way too little knowledge and experience with Godot's internals to argue one way or another. I consider the "persistent" feature as a secondary feature - maybe it should be another PR, and the core of this PR would be to just complete the support for 1. add and 3. remove signal (and 2. add full infos).

[Mickeon]

I think that including all Property Info is very "overkill" here, because most users will not ever make use of the property_hints, the hint_string, etc. It also makes the explanation on user signals much harder to follow, currently.

Judging by the code as I last saw it, the main reason it is theoretically possible to insert these attributes is because all signals share the same MethodInfo struct as... well, methods. This means it is possible to define a list of arguments that are formatted identically to the result of get_property_info(). This information is used to get the type, the enum, the class name, etc. of each individual argument. The hint, hint_string and flags of each argument is mostly superfluous.

However, and at least currently, that doesn't mean that user signals need to make use of all of the attributes... nor there may ever be a justifiable reason for them to in the base engine, any time soon...

... Unless, say for example, it was possible to define and save user signals from within the Scene Editor. In that case, exposing the same "power" to more advanced users would only be fair.

[koodikulma-fi]

I must say I disagree with that. I don't see any overkilling harm in there - especially if would implement it in the "picky" way (but maybe even with from_dict way?) It'll just be more consistent, and collect & provide the info feature for those signals that have hint and hint_string (eg. a full enum info, not just a half int). And it's not theory, it works in practice - and the info properties are there anyway in full PropertyInfo format (when using get_signal_list()) - feels very weird they get "overridden". About the doc text, it's very easy to write a shorter one. And about the advanced editing UI's, this is a step towards that. ... Personally, I still see no real reason to exclude hint and hint_string, but many reasons to include them.

[dalexeev]

I'm a little skeptical about this. Signals are more like methods than properties, right now you can't add/override/remove methods or implement _call/_get_method_list like _set/_get/_get_property_list.

I'm not saying it should never be implemented, but perhaps we should consider whether we really need it. Perhaps this should be implemented via _get_method_list/_get_signal_list, for consistency with properties. add_user_signal is somewhat "alien" in this regard, we don't have add_user_property or add_user_method.

Also note that you can use signal arguments instead of multiple homogeneous signals. Perhaps in some cases, individual signals are justified in terms of performance, but I'm not sure that empty calls can be so critical.

[koodikulma-fi]

I'm not 100% sure which points your skepticism (@dalexeev) extends to. But, here's my thoughts after a few months:

1. If there is a method to add_user_signal then it should also allow to override an existing signal.
2. Likewise just including the hint and hint_string should be implemented for consistency and symmetry (and for some practical cases!).
3. And finally the feature to remove_user_signal is an obvious counter part and such a tiny thing that it should be done - if there's a add_user_signal method. (It can be really useful / important in certain situations - related to preparing scenes in the editor with convenient game tools.)
   • In my arrogant view, the points 1, 2 and 3 are no-brainers: they should just be done (not even much discussion needed). If Godot later removes the user signal feature entirely (which would make me sad), then the counterparts would be removed too.
4. About the support to automatically add signals upon instantiating the scene, I agree that maybe it's not needed - and if it's needed, I think someone who is more familiar with C++ and Godot core functioning should review / implement them.
   • --> Actually, I've been thinking that I should refactor this PR into two parts. The first PR would just do the parts 1-3 (allow overriding + remove_user_signal + include hint and hint_string). And then another optional PR that (would require this first PR and) simply provides a way to automatically add user signals upon instantiating the scene. The discussion about that feature would then be separated as well.

[RadiantUwU]

Hello! It looks like this feature has already been implemented (see #90674)