godotengine / godot-proposals

Godot Improvement Proposals (GIPs)
MIT License
1.14k stars 93 forks source link

Add actions_changed() signal to InputMap #9402

Open Pennycook opened 6 months ago

Pennycook commented 6 months ago

Describe the project you are working on

An editor plugin providing input prompts.

Describe the problem or limitation you are having in your project

The plugin cannot detect when the InputMap changes (reported by a user in https://github.com/Pennycook/godot-input-prompts/issues/8).

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

Add a signal called actions_changed() to InputMap. The plugin would connect to the signal, and react accordingly whenever the actions in the InputMap change.

Adding separate signals for each function in InputMap (e.g. event_added for action_add_event) would work too, but a single signal would be enough for my use-case.

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

InputMap would emit the signal(s) whenever the user calls action_add_event, action_erase_event, action_erase_events, add_action, erase_action, or load_from_project_settings.

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

I can provide a function to manually inform my plugin that the InputMap has changed, but this is not ideal (see below).

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

If I provide a function to manually inform my plugin that the InputMap has changed, the user has to remember to call it every time they update the InputMap. If they use another plugin that also cares about the contents of the InputMap, that plugin will have to implement the same functionality, and the user will need to remember to call that function too. If they use yet another plugin that modifies the InputMap itself, they would need to modify that plugin to make it aware of all the other plugins.

If this is core functionality, any user code and/or plugin that cares about the contents of the InputMap can connect to a single signal.

KoBeWi commented 6 months ago

Well as already mentioned by the user, you can manually refresh the prompts when actions change. It's the typical case of signal that would be emitted after user action, which means you can manually cause the intended side-effect.

Pennycook commented 6 months ago

Well as already mentioned by the user, you can manually refresh the prompts when actions change. It's the typical case of signal that would be emitted after user action, which means you can manually cause the intended side-effect.

Right, and in simple cases that works. But it doesn't scale, because the user may have to manually cause multiple side effects, or they may not realize something they did should trigger the side effect (e.g. because they call a function provided by a plugin). I'd argue that signals (and the observer pattern in general) exist to solve this problem.

If what you said is part of Godot's design philosophy, I don't think it's applied consistently. ProjectSettings has a changed signal, so what makes InputMap different?

KoBeWi commented 6 months ago

or they may not realize something they did should trigger the side effect (e.g. because they call a function provided by a plugin)

If it's a plugin function, it can be handled by the plugin. If the user is supposed to do something, it should be documented by the plugin creator.

If what you said is part of Godot's design philosophy, I don't think it's applied consistently. ProjectSettings has a changed signal, so what makes InputMap different?

Nothing really, the class was just designed this way (probably because it's used almost everywhere); the current settings_changed signal was added relatively recently and it inherited the old change mechanism. There are other similar cases (Button, Range), and what they have in common is that they are among oldest classes. We can't change it now because of compatibility.

Pennycook commented 6 months ago

Nothing really, the class was just designed this way (probably because it's used almost everywhere); the current settings_changed signal was added relatively recently and it inherited the old change mechanism. There are other similar cases (Button, Range), and what they have in common is that they are among oldest classes. We can't change it now because of compatibility.

I see. Thanks for explaining that.

Is this a complete no-go from your perspective, then, or would you accept a PR with an implementation if enough people upvote this proposal?

If not a complete no-go, I can implement the manual workaround in my plugin, and direct people to weigh in here if they'd like to see Godot handle this case automatically.

KoBeWi commented 6 months ago

Well, it's not like I can reject this proposal alone. You can keep it open and wait for feedback from other people. And yeah, popularity can help a proposal get considered (but does not guarantee it).

One of the reasons I commented is that I have a very similar addon and solved this problem with a simple static method, so I don't think this needs any change.