godotengine / godot-proposals

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

Make Button/BaseButton control signals carry the responsible input event (if there is one) #3051

Open wareya opened 3 years ago

wareya commented 3 years ago

Describe the project you are working on

A menu-heavy 2D RPG

Describe the problem or limitation you are having in your project

In menu-heavy games, buttons are supposed to be triggered on press by keyboard/controller inputs, and on release by mouse inputs. Godot currently treats all these input sources the same for button signals, so it's not possible to differentiate between them.

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

button_down(), button_up(), and pressed() would carry the InputEvent responsible for the signal, if there is one; or null if the signal was triggered by code instead of by an input.

This would allow the signal handling code to differentiate between different input sources, so I could check that the input came from a keyboard/gamepad in button_down(), and that it didn't come from a keyboard/gamepad in pressed(), and have all manual button interactions behave the way they should.

An alternative possible solution would be to add a new signal that triggers differently for different hardware sources, but this would increase the complexity of button signals even further, make the documentation more confusing, and increase the chance of game logic developers accidentally using the wrong signals or accidentally responding to single button presses multiple times.

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

Functions attaching to the button_down() and pressed() signals of a Button control.

func _on_Defend_button_down(event : InputEvent):
    if !event_is_keyboard_or_gamepad(event):
        return
    next_action = [CombatTypes.Defend.new(), []]
    emit_signal("notify_combat_progress")

func _on_Defend_pressed(event : InputEvent):
    if event_is_keyboard_or_gamepad(event):
        return
    next_action = [CombatTypes.Defend.new(), []]
    emit_signal("notify_combat_progress")

event_is_keyboard_or_gamepad would check whether the event is either of InputEventJoypadButton or InputEventKey or not, or something like that.

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

This can currently be worked around by doing input code manually in _input() or _gui_input(), but doing so is very unwieldy and prone to keyboard focus bugs, and it's unlikely that everyone implementing this from scratch would avoid those keyboard focus bugs.

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

Mice acting differently from keyboard/gamepads when interacting with buttons is a basic and fundamental UI design principle, at least for menu-heavy games, and should not have to be hacked into existence on top of the engine when the engine could provide support or infrastructure for it itself.

wareya commented 3 years ago

This proposal overlaps with https://github.com/godotengine/godot-proposals/issues/1959 but the motivation of this proposal is different and agnostic of the particular input source; something that solves #1959's problems would not necessarily solve this proposal's problems (in particular, only passing mouse button events would break this proposal).

YuriSizov commented 3 years ago

Implementation-wise I wouldn't add it as an argument for a signal callback and would instead expose it via a method. This would achieve the same end goal, but would keep compatibility.

wareya commented 3 years ago

I would approve of that approach as well.

Calinou commented 3 years ago

In menu-heavy games, buttons are supposed to be triggered on press by keyboard/controller inputs, and on release by mouse inputs.

Shouldn't we do that in the button nodes' code themselves? This would make the feature available in every Godot project out of the box.

Likewise, this should also apply to Shortcuts assigned to buttons.

wareya commented 3 years ago

Shouldn't we do that in the button nodes' code themselves? This would make the feature available in every Godot project out of the box.

Likewise, this should also apply to Shortcuts assigned to buttons.

I wrote this proposal from the perspective of assuming that the current behavior should remain, even though it's "bad". If this proposal was the only behavior, it would probably break code that expects the button_down signal to get fired before the pressed signal.

If it was a flag or something that you could enable (or disable, if it was added to 4.0 and 4.0 made it the default), I could see it working, and it would probably be better for what I'm doing here than this proposal would be. But I'm not sure whether e.g. touchscreen presses should act like mouse inputs for this or not, and it might be something that differs from game to game (unlike how keyboard and gamepad inputs should always be on-press and mouse inputs should always be on-release).

There are other things devs try to do with buttons that would be made easier by this proposal (like those brought up in #1959).

Calinou commented 3 years ago

But I'm not sure whether e.g. touchscreen presses should act like mouse inputs for this or not,

Touch screen inputs on BaseButton nodes should act like mouse buttons in this regard, with button presses only registered when you release the button. If you need immediate feedback on press, use the TouchScreenButton node instead.

wareya commented 3 years ago

Makes sense, I forgot that node existed.

So you think that what I'm looking for here would be done better just by changing the built-in behavior of BaseButton?

Calinou commented 3 years ago

So you think that what I'm looking for here would be done better just by changing the built-in behavior of BaseButton?

One doesn't exclude the other, but I definitely think the default BaseButton behavior should match the common UI patterns expected in other software. I'm not sure if we need to add an option for this in master. If we backport this to 3.x, it should likely be disabled by default there and put behind a project setting that enables the new behavior.

wareya commented 3 years ago

That sounds good to me.

KoBeWi commented 3 years ago

Implementation-wise I wouldn't add it as an argument for a signal callback and would instead expose it via a method. This would achieve the same end goal, but would keep compatibility.

btw BaseButton already has a _pressed() method.

wareya commented 3 years ago

btw BaseButton already has a _pressed() method.

Yes. That is true. But it triggers the same way for different types of input devices, which isn't how buttons in games and applications normally work.

There's an action_mode property to buttons that controls when pressed() gets fired, but it only has two settings: on press and on release. There's no "on press for keyboards/gamepads but on release for mice/touch inputs" setting.

Come to think of it. just adding that "on press for keyboards/gamepads but on release for mice/touch inputs" would make more sense than having a project-wide setting like Calinou alluded to hypothetically happening for 3.x.