Input - fix just pressed and released with short presses

[lawnjelly]

Previously if an action was both pressed and released on the same tick or frame, is_action_just_pressed() would return false, resulting in missed input.

This PR separately the timestamp for pressing and releasing so each can be tested independently.

Fixes #73339
Master version of #77040

Notes

• There are multiple ways of addressing this bug, so expecting some bikeshedding.
• It is possible to emulate the old (buggy) behaviour with a combination of is_action_just_pressed() and is_action_pressed().
• Alternative approaches include adding new functions specifically to cope with this (but that would mean changing game code). An alternative function could e.g. report the number of pressed / releases in the current tick / frame, but that probably would not be required.
• This bug causes lost input particular on Android (because of the separate input thread), and also particularly at low tick and frame rates.
• Now includes a project setting to revert to the old behaviour (I'm not totally sure this is needed, but am happy either way).

Example Project

See #77040 for demo project.

Before:

frame 1
frame 9
released tick 9
frame 17
frame 25
frame 23

After:

frame 1
frame 9
pressed tick 9
released tick 9
frame 17

[lawnjelly]

@RandomShaper : On reflection I went for the approach of fixing the bug without introducing the extra parameter to give the old behaviour. The old behaviour can be emulated by calling:

If Input.is_action_just_pressed() && Input.is_action_pressed():

But I'm struggling to think of many situations where this would be wanted. Users will likely want to simply know whether the action has been pressed recently.

[RandomShaper]

But I'm struggling to think of many situations where this would be wanted. Users will likely want to simply know whether the action has been pressed recently.

My only concern is how this change can break the assumption that just-pressed means currently pressed and the same goes for unpressed. If I'm not mistaken, with this PR it can be both true than an action has been just both pressed and unpressed and you then have to check the current state to know which happened later.

[lawnjelly]

My only concern is how this change can break the assumption that just-pressed means currently pressed and the same goes for unpressed. If I'm not mistaken, with this PR it can be both true than an action has been just both pressed and unpressed and you then have to check the current state to know which happened later.

This is why I originally had the extra parameter to try and emphasize this, but I suspect it would cause more confusion than it solves. I'm not sure there is an easy way to solve this - I would welcome ideas. It is more of a historical oversight I imagine, with the original implementer not considering this problem.

Should is_action_just_pressed() imply that it is currently pressed? Perhaps was_action_just_pressed() might have been better wording, because it hints that it might no longer be pressed. We could e.g. add an extra function was_action_just_pressed() and deprecate the old one, but that might be confusing to users, versus just fixing what effectively is a bug. But that is why bikeshedding is useful here to get a consensus.

One thing is certain - the current system of only returning true if currently pressed just plain fails to report keypresses in some situations. If you are using this to e.g. jump, or drop a bomb, this can be a massive problem, so we have a duty to fix this in some way, and many (perhaps all?) of these will involve some degree of compromise.

[adamscott]

Usually, users that check is_action_just_pressed() just care if the button was pressed, like to apply or not jump velocity. So the fact that the button is still pressed doesn't impact anything, I think.

@RandomShaper : On reflection I went for the approach of fixing the bug without introducing the extra parameter to give the old behaviour. The old behaviour can be emulated by calling:

If Input.is_action_just_pressed() && Input.is_action_pressed():

But I'm struggling to think of many situations where this would be wanted. Users will likely want to simply know whether the action has been pressed recently.

The fact that the old behavior can be emulated quite easily makes me think that it's the correct thing to do.

[RandomShaper]

var state = STATE_IDLE

func _process();
    match state:
        STATE_IDLE:
            if is_action_just_pressed("jump"):
                state = STATE_JUMP
        STATE_JUMP:
            y -= 5
            if is_action_just_released("jump"):
                state = STATE_IDLE

A script in a game could look like that. (Not correct GDScript syntax, but enough for the purposes.)

After the change, the is_action_just_released("jump") should be changed to is_action_released("jump"), right? I can acknowledge that should have been the function used in the first place, but I can image some scripts out there are written that way. So, either the change should be made very explicit in the release notes or a project setting should be added, defaulting to the old behavior, but explicitly enabled in project.godot files created from now on (as we did other times).

[lawnjelly]

A script in a game could look like that. (Not correct GDScript syntax, but enough for the purposes.)

Just to point out this is a buggy script - it neglects the possibility that a key can be both pressed and released on the same frame. It would probably produce bugs with the current version, given that input can currently be missed. But yes, it is possible there could be scripts that depend on the current behaviour.

We could have a backup to the old behaviour with a project setting and normally I go straight for this with a project setting. However in this case I'm not totally convinced it would be necessary and it could lead to the documentation becoming confusing. 🤔 But I'll put one in and we can debate either way.

The old behaviour was flat out wrong, and likely to be capable of causing errors for anyone depending on it (even though it may appear to work in most circumstances, it would fail at e.g. low frame rates). So we could argue this should be the "the straw that broke the camel's back" for them to fix their script. But I am happy to have a backup path if this is generally preferred, I do appreciate that people sometimes don't want to deal with fixing things properly (if they aren't aware that it is broken).

After the change, the is_action_just_released("jump") should be changed to is_action_released("jump"), right?

Well, I'm not convinced that would be a good idea, both in terms of backward compatibility, and also because users may incorrectly think is_action_released() is the opposite of is_action_pressed(), which is most definitely not what it does. It literally returns true if the action is just released, but not if the action is released but not just released. is_action_released() would be ambiguous. (Additionally you would have the paradox that is_action_released() could return true if the action was currently pressed, because releasing than pressing on the same frame still triggers a release 😃 ).

UPDATE: I've now included a project setting for the old behaviour. Although this is defaulted to the new behaviour as the new behaviour fixes the bug.

[RandomShaper]

My question about the change was about the sample script, to understand what changes are required for users relying on the old behavior.

Regarding the project setting, I don't see much the point if it defaults to the new behavior. What I suggested (default is legacy, but new projects override it to the fixed one) allows new projects to benefit from the new behavior while giving existing projects the choice to upgrade or stay, with the latter being automatic if they don't get to know about the change. Otherwise, their games will break and they won't understand.

[lawnjelly]

What I suggested (default is legacy, but new projects override it to the fixed one) allows new projects to benefit from the new behavior while giving existing projects the choice to upgrade or stay, with the latter being automatic if they don't get to know about the change. Otherwise, their games will break and they won't understand.

I do understand but am not sure this is a good idea, if we consider that this is a bug fix, rather than a feature. Existing games are broken, this fixes them. They will be missing input currently on some systems, even if it does not show on the development system.

I have a couple of games which I wrote a couple of years ago that were missing input, and I had no idea why. Now it is clear - the input logic is flawed in the engine. User code is usually not at fault. If we don't fix this by default, most existing games will continue to be broken. Remember the default action of users is no action.

Fixing bugs is one area where imo we have to be prepared to risk changes in behaviour (especially if we suspect it will adversely only impact very small percentage, and is easily solvable in those cases). Most users will not want to understand why there was a bug in the engine, they will just want it fixed.

Otherwise, their games will break and they won't understand.

This isn't an "either / or" though. The situation is very different if the change fixes 99% of games, and exposes a script error in 1% or games, versus exposing a script error in 99% of games and fixing 1%.

This kind of thing is exactly why we have beta testing. If we fix a bug by default, we can quickly identify regressions, and if at that point we discover a lot of regressions we should then consider defaulting to old behaviour. If we do it before, we will never know the correct course of action.

Project Setting

While I'm not 100% sure the legacy project setting is required, it could potentially be a failsafe, and we could for example, remove it after a beta if there were no reports of regressions.

The alternative script changes in order to get the old behaviour are:
if Input.is_action_just_pressed():
becomes
if Input.is_action_just_pressed() && Input.is_action_pressed():

and

if Input.is_action_just_released():
becomes
if Input.is_action_just_released() && !Input.is_action_pressed():

[RandomShaper]

This kind of thing is exactly why we have beta testing. If we fix a bug by default, we can quickly identify regressions, and if at that point we discover a lot of regressions we should then consider defaulting to old behaviour. If we do it before, we will never know the correct course of action.

That's a good point. Let users catch any problems during betas/RCs. With that perspective, I wouldn't even add a project setting. Just a prominent point in the release notes would do.

[RandomShaper]

But, well, now the project setting is already there, I don't think it's an issue to keep it there as a deprecated item, for extra safety. And we eventually remove it.

[lawnjelly]

These two do get overridden below (providing there are no side effects, which I don't think there are), but we could leave this in for historical / safety. It will be compiled out. But am equally happy to remove.

[lawnjelly]

I think I've caught all the 0.0fs. 😁 , and have changed the project setting.

I wasn't quite sure what the policy is on deprecated project settings in master, let me know if there's anything that needs to be added in that regard.

[akien-mga]

Thanks!

[vpellen]

Hey, very nice, thanks!