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

Allow checking for exact matches with Action events. #44355

Conversation

EricEzaM
Copy link
Contributor

@EricEzaM EricEzaM commented Dec 14, 2020

Added additional parameter to action related methods to test for exactness.
If "p_exact_match" is true, then the action will only be "matched" if the provided input event exactly matches with the action event.

Before:

  • Action Event = KEY_S
  • Input Event = KEY_CONTROL + KEY_S
  • Is Action Pressed = True

Now:
You can still do the above, however you can optionally check that the input is exactly what the action event is:

  • Action Event = KEY_S
  • Input Event = KEY_CONTROL + KEY_S
  • p_exact_match = True
  • Is Action Pressed = False
  • If the Input Event was only KEY_S, then the result would be true.

Usage:

Input.is_action_pressed(action_name: String, exact_match: bool)
Input.is_action_pressed("my_action", true)

InputMap.event_is_action(p_event, "my_action", true)

func _input(event: InputEvent):
  event.is_action_pressed("my_action", false, true) # false = "allow_echo", true = "exact_match"
  event.is_action("my_action", true)

Closes #44180
Closes #18793

Note that as this is an additional default parameter, this does not break compat.

Comment on lines +109 to 112
if (p_exact_match && e->shortcut_match(p_event)) {
return E;
} else if (!p_exact_match && e->action_match(p_event, p_pressed, p_strength, p_raw_strength, p_action.deadzone)) {
return E;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is where the check actually happens.

"Action Match" has remained "fuzzy" as it was previously, and "Shortcut Match" is used to do the exact check.

This could potentially be changed to pass the "p_exact" parameter to action_match and just do the comparison inside that method.

@EricEzaM
Copy link
Contributor Author

@akien-mga @groud @Paulb23 This is a solution to the input issue discussed the other day on IRC.

@EricEzaM EricEzaM force-pushed the PR/fix-action-false-positives-and-allow-checking-exact-matches branch from 5b13205 to 19cfb31 Compare December 14, 2020 00:06
@Calinou Calinou added this to the 4.0 milestone Dec 14, 2020
Copy link
Member

@Paulb23 Paulb23 left a comment

Choose a reason for hiding this comment

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

Overall looks good, minor typo / copy paste noted inline.

Should make sure to document this here or in another PR after all the refactoring is finished.

core/input/input_map.h Outdated Show resolved Hide resolved
Added additional param to action related methods to test for exactness.
If "p_exact_match" is true, then the action will only be "matched" if the provided input event *exactly* matches with the action event.

Before:
* Action Event = KEY_S
* Input Event = KEY_CONTROL + KEY_S
* Is Action Pressed = True

Now:
You can still do the above, however you can optionally check that the input is exactly what the action event is:
* Action Event = KEY_S
* Input Event = KEY_CONTROL + KEY_S
* p_exact_match = True
* Is Action Pressed = False
* If the Input Event was only KEY_S, then the result would be true.

Usage:

```gdscript
Input.is_action_pressed(action_name: String, exact_match: bool)
Input.is_action_pressed("my_action", true)

InputMap.event_is_action(p_event, "my_action", true)

func _input(event: InputEvent):
  event.is_action_pressed("my_action", false, true) # false = "allow_echo", true = "exact_match"
  event.is_action("my_action", true)
```
@EricEzaM EricEzaM force-pushed the PR/fix-action-false-positives-and-allow-checking-exact-matches branch from 19cfb31 to b2f032e Compare December 14, 2020 23:14
@EricEzaM
Copy link
Contributor Author

@Paulb23 Yep, once the API is finalized with groud and akien I will write doco.

@EricEzaM
Copy link
Contributor Author

EricEzaM commented Dec 23, 2020

Had Discussion with @groud on IRC about a week ago - he does not agree with these changes. Says that the current way where actions can be triggered even when additional keys are pressed is the way it should be. He did recognise that this is still an issue.

For the record, from testing I do believe this would fix issues such as that described in #18793 (comment), however the user would need to put the 'exact match' action checks first, so it is on them to make sure they do that and return early if the action was handled. Probably too many catches/gotchas tbh.

@groud offered another idea - a system where actions can be grouped, and only actions in the same group can be triggered at the same time. An interesting idea but an implementation has not been explored.

So for example you would group movement actions (forward, left, right, back) and sprint, crouch, etc so they could all be activated at the same time.

At the end of the day the input action system just isnt designed with UI shortcuts as the intended use.

@Paulb23
Copy link
Member

Paulb23 commented Dec 23, 2020

At the end of the day the input action system just isnt designed with UI shortcuts as the intended use.

Yeah I think there's a bit of confusion around the problem this PR is solving. The main aim is a way to say "in this context we have a shortcut not an action". This cannot be done in an abstract way due to actions being global, as such we cannot know what context the action is triggered in, especially if they are resued for different things, e.g reusing "ui_up" as walk forward in game play. This is compounded if we also take into account that the user can rebind them, then as you've mentioned we have no idea what the keys will be.

As an example, we have the following three actions currently handled by TextEdit:

  • indent = Tab
  • de_indent = Shift+Tab
  • confirm_completion = Tab,Enter

Then the user presses Tab. No matter what system we arrive at using actions, it will ultimately come down to the order in which they are checked in the script. Mainly due to theInput system having no idea whether the completion menu is active or not.

The only "abstract" way I can see to solve this, as put in #18793 is a checkbox on the action that says "exact_matches_only" and make it the users problem if they want to re-use that action in a context that is not "action-as-a-shortcut". Otherwise we'd have to completely abstract it by reverting to our original discussion in godotengine/godot-proposals/issues/1532 and create an entirely separate system for them. This way "actions-as-shortcuts" and "true-actions" will not be mixed.

@groud
Copy link
Member

groud commented Dec 29, 2020

Here is my input on the problem. To me, users will never know when to use this "use exact match" property. This "use exact match" only makes sense when an action with modifiers might match as the same time as an action with modifiers. But theoretically, you don't know when this could happen, as you don't know the mapping the user is going to use beforehand.

Also, it does not cover all situations. Let's take an example with:

  • Crouching mapped to CTRL
  • Jumping mapped to SPACE
  • Saving mapped to CTRL+S

If you mark Crouching as use_exact_match, then pressing CTRL+SPACE wont work anymore to crouch while jumping. Then if you don't, saving will make you crouch, which is not really great. Both situations are not ideal, as we would simply want CTRL+S to take over Crouching , while not preventing Crouching and Jumping to work together.

In my opinion, the only clean way to solve this problem is to handle things beforehand. The idea here would be to find a way to choose whenever an action can be triggered at the same time as another. Here we would mark Crouching and Jumping to be allowed at the same time, but not Saving and Jumping or Saving and Crouching. The Saving action would take the priority here, making the "Input.is_action_pressed()" statement for other actions false.

The main question is how to present this to the user. Maybe we could allow for a hierarchy of actions? Or maybe a matrix with all existing actions, where you would choose for each pair of action if they are allowed together (likely defaulting to false I suppose). This likely require a proposal, but I guess you see the idea.

@akien-mga
Copy link
Member

akien-mga commented Dec 30, 2020

@groud In your example, isn't that a situation where you'd use "Use exact match" for the Saving action (CTRL+S), which would then achieve what you're describing in "The Saving action would take the priority here, making the "Input.is_action_pressed()" statement for other actions false" (at least if "Use exact match" does what I think, I admit I did not review the code in depth).

If you press CTRL, then S, you'd still start by crouching, and then crouching would be released when you press S and trigger the marked-as-exclusive CTRL+S action. But that's IMO expected, there's no way to prevent that unless you hold S first before pressing CTRL.

@Paulb23
Copy link
Member

Paulb23 commented Dec 31, 2020

Small discussion on IRC:

<Paulb23> Groud: The part I'm struggling to understand is how shortcut groups can handle different contexts, as the InputMap / actions are completely disconnected from scenes.
<Groud> Not sure I understand, but the question is about matching an event to an action
<Groud> If i am not mistaking
<Groud> so in any case, this problem cannot be solved locallyu. As my example showed, it has to be done globally, so that you know if another action is taking over another one
<Groud> Locally (when you match the action), this wont solve all issues
<Paulb23> The problem is not all actions are used in all scenes, eg. in your example, if saving is used in a different scene, the InputMap would not know to set crouching as true instead
<Groud> hmm I see what you mean.
<Groud> IN any case, I guess that, whatever we do, it will be up to the user to catch actions in their correct order of priority.
<Groud> But then, If we require the user to do so, I wonder why setting the input as handled wouldn't be enough
<Groud> IMHO, this is maybe the only clean solution then

@EricEzaM
Copy link
Contributor Author

EricEzaM commented Jan 4, 2021

@akien-mga Yes I think you have it right here. I think using the "exact match" argument to the is_action_pressed() method when checking for the "save" Ctrl+S functionality would have the correct effect. It would be the users responsibility to ensure that "save" is checked first, and that if it is indeed handled then the method returns or otherwise breaks out of the block which checks input.

func _input_check() -> void:
    if Input.is_action_pressed("save", true):
        save()
        return

    if Input.is_action_pressed("crouch"):
        set_crouched()

    if Input.is_action_pressed("jump"):
        jump()

Similarly I think this would fix this comment: #18793 (comment)

func _input_check() -> void:
    if Input.is_action_pressed("undo", true): # Ctrl Z
        undo()
        return

    if Input.is_action_pressed("c4"): # Z
        play_note(NOTE_C4)

    if Input.is_action_pressed("d4"):
        play_note(NOTE_D4)

    # etc...

In this case even if the inputs were swapped around so that Z was Undo and Ctrl Z was note C4, it should still work.

If both solutions require the user to check for input in the "correct" order (for their situation), then I think the above code is simpler than doing something more complex by changing the way actions are defined.

@akien-mga akien-mga requested review from groud and reduz January 8, 2021 14:55
@Mrjcowman
Copy link

@groud The situation you present relies on "exact match" to check if absolutely no other keys are being pressed. The only keys that actually matter, though, are the 4 modifiers. It's not possible for me to make a binding for "ctrl+s+space" so the "exact match" doesn't come into play if s and space are being pressed at the same time. To the same effect, an action bound to a single modifier key like "ctrl" wouldn't necessarily care whether a non-modifier is being pressed either.

This change would fix a lot of potential bugs in any case where the user is allowed to make a keymapping with modifiers. (I came across this issue while struggling with a minor keybinding bug in Pixelorama that stems from this lack of functionality). In any situation the user decides to have one keybind be a modified version of another keybind, the designer has to anticipate that choice and check for it in some not-so-intuitive ways.

@groud
Copy link
Member

groud commented Jan 16, 2021

@akien-mga Yes I think you have it right here. I think using the "exact match" argument to the is_action_pressed() method when checking for the "save" Ctrl+S functionality would have the correct effect.

How is that an exact match then? It would not change the previous behavior at all.
My idea of "exact match" made only sense when you have actions that have less keys than another, exactly what is depicted in the original post.

If we require the user to check for Ctrl + S beforehand, then there is absolutely no problem here. Because, if it is matched first, then you can simply mark the event as accepted and you're done. And if we are not using an event (using Input.is_action_pressed() instead), we might simply implement a way to mark an action as not pressed anymore (allowing modifying the Input singleton state manually). That would solve the problem in a cleaner way.

@EricEzaM
Copy link
Contributor Author

EricEzaM commented Jan 16, 2021

@groud It is an exact match because pressing Control + Shift + S, for example, would not return true if the action is Control + S, as it is not exact. So Ctrl Shift S could be used for another key bind without issue. In the current system, this is not possible.

@MrGreaterThan
Copy link

I'm a github beginner so I hope giving my opinion isn't considered poor etiquette. This scenario comes up when programming "control groups" for an RTS game. Typically Ctrl+(1-9) will create a collection of units called a control group which can then be selected by pressing the corresponding number key 1-9. Without an exact match, you'll end up selecting the control group and assigning to it at the same time and is unintended.

Adding the exact match parameter helps this, but I'd rather see a mode that still accepts inexact matches as long as they don't exactly match some other action. It's the conflicts that I care about.

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

We discussed this on Rocket.Chat with @EricEzaM, @groud and @Paulb23. See https://chat.godotengine.org/channel/devel/thread/m6d8i6PE3EACnETGw for details (needs to be logged in, thread permalinks don't seem to work for anonymous readers).

@akien-mga akien-mga merged commit 52964fd into godotengine:master Feb 15, 2021
@akien-mga
Copy link
Member

Thanks!

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.

Input Event action match returns false positives modifier+key action shows also key action as pressed
7 participants