godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
90.29k stars 21.05k forks source link

Add Input.get_action_pressed() #15681

Closed ElectricCoffee closed 4 years ago

ElectricCoffee commented 6 years ago

I've seen a fair bit of code that relies on the Input.is_action_pressed(action_string) method, which has sometimes lead to the creation of some pretty gnarly repeated code:

if Input.is_action_pressed("move_up"):
    # do stuff
elif Input.is_action_pressed("move_down"):
    # do stuff
elif Input.is_action_pressed("move_left"):
    # do stuff
elif Input.is_action_pressed("move_right"):
    # do stuff

With the advent of the new match keyword in Godot 3, this could easily be re-written as

match Input.get_action_pressed():
    "move_up": # do stuff
    "move_down": # do stuff
    "move_left": # do stuff
    "move_right": # do stuff

I can see a lot less boilerplate coming out of having a function that returns the action being pressed, rather than having to manually call the same method four times (or in the case of one tutorial, eight times, twice for each direction).

Alternatively, if the underlying code needs to be able to check for multiple simultaneous actions, there could be a sister function get_actions_pressed which returns an array of actions.

I can understand this could've been left out if the underlying code structure makes this sort of thing inefficient to implement. But if not, then I don't see a reason for it to not be there.

speakk commented 6 years ago

I can see this working well for movement keys, but what if the user is pressing the "shooting" key at the same time? get_action_pressed apparently would only return one key at a time, so how does it behave when there are multiple keys pressed?

ElectricCoffee commented 6 years ago

@speakk yeah I was wondering about that too, that's why I also proposed the use of get_actions_pressed (note the s in actions), which could work a-la this:

var actions = Input.get_actions_pressed()
if "move_left" in actions: 
    # do stuff
elif "move_right" in actions:
    # do stuff
# repeat for the other movement actions
if "shooting" in actions:
    # act on shooting

it's not as elegant, and the syntax is definitely more verbose now, but it's still just a single method call

reduz commented 6 years ago

get_action_pressed() does not make much sense. get_actions_pressed() on the other hand may work

reduz commented 6 years ago

also your example does not make much sense, you generally want to press more than one key at the same time for character movement

akien-mga commented 6 years ago

I'm not an expert of the match statement but it should be possible to use it with an array of actions returned by get_actions_pressed().

ElectricCoffee commented 6 years ago

@reduz yes, but you don't want to press both left and right, so while the match didn't make sense, I was just using it as a possible suggestion for contexts in which it might, such as grid-based movement that doesn't have diagonals

vnen commented 6 years ago

I'm not an expert of the match statement but it should be possible to use it with an array of actions returned by get_actions_pressed().

Yes, if you are expecting a single action per frame, you could do this:

match Input.get_actions_pressed():
    ["move_up"]: # do stuff
    ["move_down"]: # do stuff
    ["move_left"]: # do stuff
    ["move_right"]: # do stuff

Also: get_pressed_actions() sounds better IMO.

ElectricCoffee commented 6 years ago

Not all actions come from presses though, do they? So even calling functions is_action_pressed or get_actions_pressed would be kinda wrong, wouldn't it?

Or is this just a matter of "we couldn't think of a better name, so it stuck"?

vnen commented 6 years ago

Not all actions come from presses though, do they?

Some do not come from a user pressing a button, since joystick analog stick or mouse wheel can be actions too, but currently they work only with a binary state: they are either pressed or released.

organicpencil2 commented 6 years ago

As an alternative it'd be pretty dang cool if this worked:

var actions = Input.is_action_pressed
if actions("move_left"):
    # do stuff
elif actions("move_right"):
    do stuff
# repeat for the other movement actions
if actions("shooting"):
    # act on shooting

... which ends up being less keystrokes than the proposed get_actions_pressed example. But I'm guessing there's a reason funcref isn't already implemented this way.

akien-mga commented 6 years ago

... which ends up being less keystrokes than the proposed get_actions_pressed example.

I don't get the obsession for reducing the number of keystrokes in programming.. Do you know the famous "10 (good) lines of code per programmer per day" from the Mythical Man Month? Arguably it might be a bit more in a scripting language/for game logic, but I'm still pretty confident that with such sugar you'd save less keystrokes in one week than you had to use for this comment.

Programming is not speedtyping, it's about thinking a lot, debugging a lot, and writing sometimes.

What matters is not how fast you type, it's how readable, future-proof and failure-proof your code is.

ElectricCoffee commented 6 years ago

My argument for this is less to reduce the amount of typing, and more to reduce the amount of repeated method calls. Don't Repeat Yourself is a saying in programming after all, since copypasted code can lead to bugs, and calling the same method multiple times when once is enough can in some cases be a performance bottleneck.

The code in question that prompted me to even suggest this in the first place, looks exactly like this:

var is_moving = Input.is_action_pressed("move_up") or Input.is_action_pressed("move_right") or Input.is_action_pressed("move_down") or Input.is_action_pressed("move_left")

if is_moving:
    speed = MAX_SPEED

    if Input.is_action_pressed("move_up"):
        direction = TOP
    elif Input.is_action_pressed("move_right"):
        direction = RIGHT
    elif Input.is_action_pressed("move_down"):
        direction = DOWN
    elif Input.is_action_pressed("move_left"):
        direction = LEFT
else:
    speed = 0

Which can be re-written much simpler like this:

var actions = Input.get_actions_pressed()

if actions != []: 
    speed = MAX_SPEED

    if "move_up" in actions:
        direction = TOP
    elif "move_right" in actions:
        direction = RIGHT
    elif "move_down" in actions:
        direction = DOWN
    elif "move_left" in actions:
        direction = LEFT
else:
    speed = 0

I went from a maximum of 8 method calls, to just one, and got rid of a very very long boolean. I'd argue that the new version's intent is much clearer, too. But I guess that entirely depends on the individual reading the code.

akien-mga commented 6 years ago

You didn't need that boolean in the first place though :)

if Input.is_action_pressed("move_up"):
    direction = TOP
elif Input.is_action_pressed("move_right"):
    direction = RIGHT
elif Input.is_action_pressed("move_down"):
    direction = DOWN
elif Input.is_action_pressed("move_left"):
    direction = LEFT

speed = MAX_SPEED if direction != Vector2() else 0

That said, I already agreed with adding Input.get_actions_pressed() returns an array of actions pressed, it can be useful.

ElectricCoffee commented 6 years ago

@akien-mga Oh yeah, I know, I was just copying the code I found verbatim for context :)

speakk commented 6 years ago

@ElectricCoffee "I went from a maximum of 8 method calls, to just one, and got rid of a very very long boolean."

Yeah, but who knows how expensive the "if "move_up" in actions:" ? Because that might mean searching the array every time, which might not be any less expensive than the original method call version.

ElectricCoffee commented 6 years ago

@speakk and that is indeed a very valid concern, that I also have been wondering about.

However, if the performance is comparable, then the real gain is in the syntax, which is still an improvement.

reduz commented 6 years ago

sorry guys, I am in favor of writing more text and it being more clear than writing less text. Nowadays with autocompletion and copypasting, typing less is overrated.

remram44 commented 5 years ago

There is also a related problem with the InputEvent, where you have to do is_action() in a similar manner. There's an InputEventAction but it seems to only be script->engine, never sent engine->script.

My workaround is to have a global input singleton that processes input and calls methods on actors when there's an event for them, rather than have them process every input event... but it seems to me that it's exactly what InputMap is supposed to take care of.

Xrayez commented 4 years ago

I do support the idea of get_actions_pressed() being added to the Input API as it seems to be somewhat a common request I've seen people discussing over Discord, but we'll eventually stumble a user who wants:

Input.get_keys_pressed()
Input.get_joy_keys_pressed()

# It's quite possible that you want to make a combo move on keys released simultaneously.
Input.get_actions_released()

# The number of use cases multiplies for each type of query.
Input.get_actions_pressed_over_duration() # :)

These usages seem to be useful for fighting games and alike: godotengine/godot-proposals#100, so it's not only about "making less keyboard strokes".

Some ideas (and even snippets!) remind me of my own proposal regarding exposing the internal input states and how it's currently difficult to make bi-directional communication between input polling system and the actual input states which are currently hardcoded, see exhaustive explanations at godotengine/godot-proposals#104. With the dedicated InputState class the user can create his own layer and API similar to what Godot provides, having access to all the states.

Calinou commented 4 years ago

Feature and improvement proposals for the Godot Engine are now being discussed and reviewed in a dedicated Godot Improvement Proposals (GIP) (godotengine/godot-proposals) issue tracker. The GIP tracker has a detailed issue template designed so that proposals include all the relevant information to start a productive discussion and help the community assess the validity of the proposal for the engine.

The main (godotengine/godot) tracker is now solely dedicated to bug reports and Pull Requests, enabling contributors to have a better focus on bug fixing work. Therefore, we are now closing all older feature proposals on the main issue tracker.

If you are interested in this feature proposal, please open a new proposal on the GIP tracker following the given issue template (after checking that it doesn't exist already). Be sure to reference this closed issue if it includes any relevant discussion (which you are also encouraged to summarize in the new proposal). Thanks in advance!