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

[3.x] Input: add retire option to is_action_just_pressed() #97529

Closed
wants to merge 1 commit into from

Conversation

lawnjelly
Copy link
Member

@lawnjelly lawnjelly commented Sep 27, 2024

Retired actions will no longer return true to is_action_just_pressed() in the same frame / tick (unless re-pressed).

The retiring version would thus be:

if Input.is_action_just_pressed("wheel_up", false, true):

instead of:

if Input.is_action_just_pressed("wheel_up"):

Fixes #97526

Notes

  • Adds an optional argument to cancel actions after their first reading for "just pressed".
  • This is backward compatible, but considerably simplifies dealing with the problem in [3.x] _input reports is_action_just_pressed() multiple times per frame  #97526 of duplicate "just presses" during _input().
  • I also considered adding a new function retire_action_just_pressed() but this would have ended up duplicating the existing is_action_just_pressed() version in this PR (minus the retire argument).
  • The advantage of a separate function would be removing the somewhat confusing false required for p_exact, but neither is completely ideal.

@timothyqiu
Copy link
Member

I see a potential problem with this approach.

If there are multiple nodes doing this check, then the result will be correct for only one of them.

The same happens when there are multiple is_action_just_pressed() checks for the same action in the same function, and when doing the same check in both _input() and _process() for example.

@lawnjelly
Copy link
Member Author

lawnjelly commented Sep 27, 2024

Yup, that's true it will not work for people who expect to query the same action multiple times.

To a certain extent I'm trying to brainstorm ideas here.

At the moment is it works as expected, but users may need excessive boiler plate for the situation in the issue. They would need to e.g. record the current frame just pressed in their own code, and compare that when checking the second / third time, which is multiple lines of code.

I'm also slightly concerned that this report is highlighting an obvious case, but it may have been happening in people's projects to a lesser extent for years (maybe not affecting gameplay much), and it would be nice to have a way of closing the problem.

  • Originally I did think of adding an extra default bool to is_action_just_pressed() that allowed auto-cancelling, and default it to the old approach.

  • Another option is to have a bool state on the action that can be queried that is set when the action has already been read (that frame or tick).

  • Or an explicit function like cancel_action_just_pressed() and leave it to the user.

Retired actions will no longer return `true` to `is_action_just_pressed()` in the same frame / tick (unless re-pressed).
@lawnjelly lawnjelly changed the title [3.x] Input: auto-cancel actions after reading with is_action_just_pressed() [3.x] Input: add retire option to is_action_just_pressed() Sep 27, 2024
@lawnjelly
Copy link
Member Author

@timothyqiu I've updated this PR to go with adding an extra default argument so that users have to explicitly choose to retire actions.

I've also written a similar PR to add a retire_is_action_just_pressed() function, so could move to that if preferred. It's difficult to call which approach would be better so would welcome opinions.

@lawnjelly lawnjelly marked this pull request as ready for review September 27, 2024 12:22
@lawnjelly lawnjelly requested review from a team as code owners September 27, 2024 12:22
@timothyqiu
Copy link
Member

Tested locally and this works as expected. It does not create inconsistencies between different parts of the game unless explicitly asked to.

Issue-solving-wise, if the user knows/remembers that they should set retire to true, I think they'll mostly realize that event.is_action_pressed() should be used instead of Input.is_action_just_pressed() 😆 Isn't this more complex than turning on input_devices/compatibility/legacy_just_pressed_behavior in project settings?

@lawnjelly
Copy link
Member Author

Issue-solving-wise, if the user knows/remembers that they should set retire to true, I think they'll mostly realize that event.is_action_pressed() should be used instead of Input.is_action_just_pressed() 😆

is_action_pressed() doesn't tell you whether the action has just been pressed (e.g. a jump key) as opposed to held down (continuous thrust)? I'm not sure I understand.

Isn't this more complex than turning on input_devices/compatibility/legacy_just_pressed_behavior in project settings?

This would re-introduce the old bug of missing input #73339 that is fixed by #77040. The legacy mode was really just added in case of disaster.

<description>
Returns [code]true[/code] when the user has [i]started[/i] pressing the action event in the current frame or physics tick. It will only return [code]true[/code] on the frame or tick that the user pressed down the button.
This is useful for code that needs to run only once when an action is pressed, instead of every frame while it's pressed.
If [code]exact[/code] is [code]false[/code], it ignores additional input modifiers for [InputEventKey] and [InputEventMouseButton] events, and the direction for [InputEventJoypadMotion] events.
If [code]retire[/code] is [code]true[/code], [method is_action_just_pressed] will no longer return [code]true[/code] for a pressed action for subsequent calls on the same frame or tick, unless the action is re-pressed.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Simplifying wording slightly.

Suggested change
If [code]retire[/code] is [code]true[/code], [method is_action_just_pressed] will no longer return [code]true[/code] for a pressed action for subsequent calls on the same frame or tick, unless the action is re-pressed.
If [code]retire[/code] is [code]true[/code], [method is_action_just_pressed] will no longer return [code]true[/code] for the given action when called again during the same frame or tick, unless the action is pressed again.

"unless the action is pressed again" is a bit... vague? I'm not sure what that means in this context, and I'm not sure the reader will, either.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah it's kind of hard to get across in a small number of words, because to some extent it depends on an explanation of how just_pressed works.

We can do some more revisions of the text if we decide to commit to this approach (as there are a number of ways of addressing the issue), and I'll make this change tomorrow.

A separate retire_is_action_pressed() function would be just as good, it just depends what we think will be easier for the user to understand.

@timothyqiu
Copy link
Member

timothyqiu commented Sep 28, 2024

is_action_pressed() doesn't tell you whether the action has just been pressed (e.g. a jump key) as opposed to held down (continuous thrust)? I'm not sure I understand.

For _input(event) functions

  • event.is_action_pressed() is exactly "just" pressed (as it's checking the current event).
  • Input.is_action_just_pressed() is just pressed in current frame.
  • Input.is_action_pressed() is currently held down.

@lawnjelly
Copy link
Member Author

event.is_action_pressed() is exactly "just" pressed (as it's checking the current event).

Ah I see, I wasn't aware of that (I have limited experience of using _input(), aside from mouse look), so this is already possible, and is indeed more a documentation issue currently.

In that case there is likely for retiring actions, or a case for your proposal (godotengine/godot-proposals#10843). 👍

@lawnjelly lawnjelly closed this Sep 28, 2024
@AThousandShips AThousandShips removed this from the 3.7 milestone Sep 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants