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

Add _on_Input_action(action) signal #1853

Open
golddotasksquestions opened this issue Nov 17, 2020 · 15 comments · May be fixed by godotengine/godot#75971
Open

Add _on_Input_action(action) signal #1853

golddotasksquestions opened this issue Nov 17, 2020 · 15 comments · May be fixed by godotengine/godot#75971

Comments

@golddotasksquestions
Copy link

golddotasksquestions commented Nov 17, 2020

Describe the project you are working on:
Applies to literally any project with Inputs.

Describe the problem or limitation you are having in your project:
Here, I admit it: I really hate working with _input(event). It is very cumbersome to filter out the event you actually care about and I really have a hard time remembering the syntax and scancode constants for each and every input. I have to look up the API references nearly every time and always end up testing for actions. Imput Map actions in _process work fine, but the actual problem is that just like in _input, every Input has to be tested against every action:

if event.is_action_pressed("action"):
if Input.is_action_just_pressed("action"):

This results in either having to test against an endless list of every possible action defined in the Input Map

if Input.is_action_just_pressed("action1"):
	do("action1")
if Input.is_action_just_pressed("action2"):
	do("action2")
if Input.is_action_just_pressed("action3"):
	do("action3")
...

or having to loop through all possible actions whenever there is an action press:

var possible_actions = ["action1", "action2", "action3", ...]

func _input(event):
	if event is SomeInputEventType and event.pressed:
		for i in possible_actions.size():
			if event.is_action_pressed(possible_actions[i]):
				do(i)

When I first started out with Godot, I expected there something like a collision signal to be emitted when pressing an Input. Now, more than two years later working with Godot Inputs every day, I still have not figured out how to do that and I stopped believing it's just my limited expertise.

Describe the feature / enhancement and how it helps to overcome the problem or limitation:
Whenever the user triggers one of the events in the Input Map the corresponding input_action(action) signal is emitted:

Besides being a lot more intuitive to work with compared to _input(event), this would allow us to write action agnostic code, just like it is already possible with collision events (_on_body_entered(body) for example)

func _on_input_action(action):
	$Animationplayer.play(action)
	if input_buffer.find(action):
		do(action)

Describe how your proposal will work, with code, pseudocode, mockups, and/or diagrams:
see above

If this enhancement will not be used often, can it be worked around with a few lines of script?:
It can't be worked around without using inefficient loops and user unfriendly _input(event) or mountains of repetitious code.

Is there a reason why this should be core and not an add-on in the asset library?:
Input is an already existing core class.

@Calinou Calinou changed the title Input: Add _on_Input_action(action) signal Add _on_Input_action(action) signal Nov 17, 2020
@Xrayez
Copy link
Contributor

Xrayez commented Nov 17, 2020

Technically this is possible to do upon input event parsing in https://github.com/godotengine/godot/blob/fbe0386c653615a373e45af1ebe027c83b924d33/core/input/input.cpp#L643-L658.

What makes sense to me is to handle is_action_just_pressed/released at least, else the signal would emit continuously, not sure what performance cost would that add or how it would affect the message queue. And it would probably be non-deterministic anyway due to echo.

At least, the Input singleton can have a signal which allows you to tell whether action's pressed state was changed (pressed down or released). Somewhat reminds me of godotengine/godot#36460 where duration == 0 means "action press changed", so might also help this proposal to some degree.

I've managed to do this kind of thing in GDScript:

extends Node2D

class InputHandler:
	signal action_pressed(action)

	var actions = InputMap.get_actions()

	func poll():
		for action in actions:
			if Input.is_action_just_pressed(action):
				emit_signal("action_pressed", action)


var input_handler = InputHandler.new()

func _ready():
	input_handler.connect("action_pressed", self, "_on_action_pressed")


func _process(delta):
	input_handler.poll()


func _on_action_pressed(action):
	print("Pressed action: " + action)

I guess this is already what you do to some degree for InputBuffer logic (#100).

The downsides are:

  1. Have to process each and every action in a loop, which may impact performance negatively, especially if you have many of them.
  2. Have to poll in _process() manually.

See also #639 (comment).

@golddotasksquestions
Copy link
Author

golddotasksquestions commented Nov 17, 2020

What makes sense to me is to handle is_action_just_pressed/released at least, else the signal would emit continuously, not sure what performance cost would that add or how it would affect the message queue.

Yeah, that's the idea.

Implement input action duration #36460

Oh that's super neato! This will help a lot writing input buffers and all kinds of Inputs, without having to create timers for everything.

The downsides are: Have to process each and every action in a loop, which may impact performance negatively, especially if you have many of them.

I'm not sure if you would need to use poll, and process, you could use my _input(event) example above for the same purpose, I would believe. Just emit a signal at the end. It's just a lot of extra steps to make something that could be so incredibly simple, beginner friendly and intuitive from a user perspective as

func _on_input_action(action):
	$Animationplayer.play(action)

Every other solution feels like a massive clunky workaround you can only figure out after you gathered a lot of experience.

See also #639 (comment).

I'm afraid this proposal is above my paygrade, I don't really understand it.

@Xrayez
Copy link
Contributor

Xrayez commented Nov 17, 2020

See also #639 (comment).

I'm afraid this proposal is above my paygrade, I don't really understand it.

I was referring to reduz concern that the input no longer goes through the MainLoop (where it was polled I presume), I don't know implementation details much either. So yeah there might be ways to bypass polling.

@katoneko
Copy link

I really have a hard time remembering the syntax

May I ask, what syntax are you referring to? Input handling doesn't introduce any new syntax. It's all functions and if statements that are hardly anything special both in GDScript and programming languages in general.

It is very cumbersome to filter out the event you actually care

It only takes one if statement to check for the event, I don't think it's that cumbersome:

func _input(event):
    if event.is_action_pressed("enter"):
        # It's enter!
    elif event.is_action_pressed("space"):
        # It's space!

@golddotasksquestions
Copy link
Author

golddotasksquestions commented Nov 18, 2020

May I ask, what syntax are you referring to? Input handling doesn't introduce any new syntax.

I was not talking about any new syntax, but the grammatical structure how you test for something if you don't use Input Map actions.
You can't just use the Input class like when you do it in _process(), instead you have to use the InputEvent class. But then there is also InputEventKey, InputEventMouseButton, InputEventMouseMotion, InputEventAction, InputEventJoypadButton, InputEventJoypadMotion, InputEventMIDI, InputEventScreenDrag, InputEventScreenTouch, InputEventWithModifiers. If that's not confusing to a beginner then I don't know what is. Plus countless scancodes for Key presses with a different syntax yet again. It took me forever to figure out how to reliably use this system.

It only takes one if statement to check for the event, I don't think it's that cumbersome: func _input(event): if event.is_action_pressed("enter"):

Hm, have you read my proposal completely? The point is not having to write an endless list of if statements or loop through all actions, but instead just get the action as an argument:

func _on_input_action(action):
	$Animationplayer.play(action)

Besides doing what you suggested imho would be ill advised. Think about it. _input(event) fires every time you move your mouse. More often than not get multiple events per frame. Check against ALL your actions in every input event seems very unefficient. Instead what you most likely instead want to do is to check for a specific InputEvent type first, and only if you have an event of that type loop through your actions (see _input(event) example in the OP). Doing it this way requires you to have an understanding of all those types first, and it's a unique approach compared to what you would do in _process() with the Input class or when you get a collision event from an Area.

When you get the collision event from an Area you can use the "body" or "area" argument to do whatever you like with the colliding object. You can test against it's type, name or group -whatever-, but in most cases it's not necessary at all. Resulting in very simple, short and easy to remember code. It's a syntax structure the Godot user is already familiar with, since build in signals work the same everywhere in Godot.

@Xrayez
Copy link
Contributor

Xrayez commented Nov 18, 2020

It took me 5 minutes to come up with initial implementation to solve this proposal, see godotengine/godot#43646. 🙂

@dalexeev
Copy link
Member

dalexeev commented Nov 30, 2020

See also #497. There I asked if it is possible to make all InputEventActions go to _input(), but this breaks compatibility and, as @groud explained, can greatly reduce performance. Below I suggested adding a virtual method Node._input_action(action: String, pressed: bool), and this is something @Xrayez implemented in his PR (funny, hehe).

But now I am leaning towards the idea that Node._input_action(event: InputEventAction) is better. I don't know how much the InputEventAction is more expensive or cheaper than String + bool, but InputEventAction also has strength property and everything that it inherits from InputEvent.

@Xrayez's example from PR

func _on_action_toggled(action, pressed):
    if pressed:
        print("Action pressed: " + str(action))
    else:
        print("Action released: " + str(action))

does not change much with this replacement:

func _input_action(event):
    if event.is_echo():
        return
    if event.pressed:
        print("Action pressed: " + event.action)
    else:
        print("Action released: " + event.action)

In conclusion, I want to say that this would be a good feature of 4.0, and in general use _input_action() instead of _input() (the first for named actions, the second for hardcoded keycodes or some kind of low-level input).

@me2beats
Copy link

@dalexeev
Does func _input_action(event): save only one code line:
if event.is_action_just_pressed(): ?

@me2beats
Copy link

@golddotasksquestions

  1. If you need your character to run if Shift is pressed, otherwise walk, do you still use _input() for that?
    Or maybe having is_pressed param would solve it?
    _on_input_action(action:String, is_pressed:bool)

  2. should _on_input_action be triggered on every action (for example ui_cancel) or only on user defined actions (say player_left, player_right etc)?
    Because if first, then I think you still need to check action name before doing something (playing animation by name etc)

  3. If you have different animated objects, say player and the door that opens on action open_door it seems we still need to check action name.

@golddotasksquestions
Copy link
Author

@me2beats
1: there are many ways to do this. You can do it in code of solve it with an action. Are you aware you can make input map actions with modifier keys? Like action "run_right" = Shift + "D"
2,3 I see nothing wrong with checking action name. I only miss the option to do agnostic things using the action as an argument, like with buttons, that's all.

@qaptoR
Copy link

qaptoR commented Apr 9, 2023

Hello, I'm just arriving to this party about InputEventAction Signals, but it has been something I've considered 'would be nice to have' for some time.

I'm currently looking into this closed PR: godotengine/godot#43646

I can see that it was shuttered due to some controversy for which I'm sorry to find out, but I'd like to open to possibility of salvaging the work done in the PR however that may be legally possible.

I'm currently attempting to merge the code into my personal Godot Build, and if I'm successful I'd like to open a new PR, but I'm not certain what the rules over copyright are regarding the code. If anyone can shed light on this I'd be very grateful.

EDIT:
After the most superficial changes were applied I can confirm that the code still works effectively as it did before it was closed.
I have a build working for the master branch as it stood at the time of this writing.

@qaptoR
Copy link

qaptoR commented Apr 9, 2023

as an argument for why 'toggled' vs 'pressed' and 'released' are all necessary, I'd refer to this output:
image

which is based on this line of code
Input.action_toggled.connect(func(action, pressed): print('%s: %s' %[action, pressed]))
so that whenever a key is pressed, every action that uses the key as a trigger will emit the signal

Where something like
Input.connect('test_action_pressed', func(): print('test action is pressed'))
will trigger only when the specific action triggers (from possibly multiple different keys)

Thus, I think as an implementation both use cases have a valid reason to continue to exist.

@qaptoR
Copy link

qaptoR commented Apr 9, 2023

also, currently the custom InputEventAction signals such as *_pressed and *_released are added when the Notification POSTINITIALIZE fires.

Therefore, any new InputEventActions that are added via script at runtime don't have signals that can be connected to.

Are there any thoughts on whether this should be possible / encouraged?

Personally, since there is only an add_user_signal function, and no remove_user_signal I think it is in keeping with the current state of Godot that dynamically adding signals for InputEventActions should be discouraged.

Dynamically handling such actions is probably a very niche use-case anyway, and the input can be handled in the traditional ways available without this PR, such as with _input(event).

@qaptoR
Copy link

qaptoR commented Apr 12, 2023

A little bit of discussion went on in the dev chat to help me decide if I should create a PR.
I'm adding it here for posterity so that we can move past or speed up the discussion about copyright:
https://chat.godotengine.org/channel/devel?msg=n2RaX48Nre7x3CpZC

The consensus seemed to be that since the original closed PR constitutes a public distribution in good faith at the time it was made under the MIT license then the code itself is available to use fairly.

The further questions to be answered are
a) should the code be made to be significantly different due to the removal of the author from the community
b) if so, what constitutes a significant difference, especially when there is particularly very little to the work itself to begin with.

The following PR:
should be used as a basis for the discussion as it constitutes the original work, reauthored to be brought in line with the master branch (so minor changes have been made, but not significantly to change the underlying algorithm at play)

@Bloodyaugust
Copy link

I'm very interested in this functionality, I frequently find myself wishing I could Input.action_signal("action").connect(my_func). Looks like the linked PR was left in a state where it needed review, then was never reviewed.

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