Refactor KeyCombination.ContainsKey() and .ContainsKeyPermissive() for better extensibility

[Susko3]

• Prerequisite for fixing Fix keyboard shortcuts not working as expected on non-QWERTY keyboard #5790
  • That PR is blocked because of faulty logic, this PR will make it easier to fix that logic

Terminology

• Physical key: physical key on a HID, such as a osuTK.Input.Key, MouseButton, JoystickButton and their direct mappings to InputKeys
• Virtual key: a logical key, an abstraction of some physical key(s). Example:
  • (Input)Key.LControl and (Input)Key.RControl map to the virtual key InputKey.Control
  • SDL_Keycode (the keyboard layout-dependant unicode character) that is used in Fix keyboard shortcuts not working as expected on non-QWERTY keyboard #5790

Problems

KeyCombination.ContainsKey() is used for two different purposes

These two purposes have very different semantics:

1. It's used to check that each InputKey from a candidate key combination is present in the pressedKey [sic] array.
   • In this case, the key param can be a virtual key.
2. For KeyCombinationMatchingMode.{Exact,Modifiers}, it's used to check that there are no excess InputKeys from pressedKeys present in the combination.
   • In this case, the key param can only be a physical key and the switch is unreachable code.

This is evident from the xmldoc which explains 1. in <summary> and 2. in <param name=.../>

osu-framework/osu.Framework/Input/Bindings/KeyCombination.cs

Lines 198 to 205 in 51d8215

	/// <summary>
	/// Check whether a single key from a candidate binding is relevant to the currently pressed keys.
	/// If the <paramref name="key"/> contains a left/right specific modifier, the <paramref name="candidate"/> must also for this to match.
	/// </summary>
	/// <param name="candidate">The candidate key binding to match against.</param>
	/// <param name="key">The key which has been pressed by a user.</param>
	/// <returns>Whether this is a match.</returns>
	internal static bool ContainsKey(ImmutableArray<InputKey> candidate, InputKey key)

This is fixed in e05083a and invalid tests are removed in 3e021dd (check commit message as to why).

Adding additional virtual keys is complicated

Since definitions of virtual keys are spread across ContainsKeyPermissive() and ContainsKey(), it's hard to add new ones, and impossible to do it programmatically.

This is fixed by 0ab0adc without changing semantics. The added getVirtualKey() static method is subject to change and will need to have the InputState propagated to it to make it work for #5790.

I'll be doing some follow-up cleanup after this PR is merged.

Breaking changes

KeyCombination.IsPressed now takes in an InputState parameter

-public bool IsPressed(KeyCombination pressedKeys, KeyCombinationMatchingMode matchingMode)
+public bool IsPressed(KeyCombination pressedKeys, InputState inputState, KeyCombinationMatchingMode matchingMode)

[bdach]

This is so difficult to make sense of that I am trying to approach this PR a second time and still not understanding any of it.

• Why are the methods called ContainsKey() and ContainsKeyPermissive() if the "permissive" one doesn't look all that more or less permissive than the other? They both check the "physical" and the "virtual" key. The actual difference seems to be in the directionality of the comparison? I think?

  The methods need renames to not be borderline obfuscatory.

• If I read in (2):

  In this case, the key param can only be a physical key and the switch is unreachable code"

  Where are the assertions for this? I want this to be asserted in code. So that determining that something breaks the invariant is so simple as seeing the game crash.

• Later on:

  Since definitions of virtual keys are spread across ContainsKeyPermissive() and ContainsKey(), it's hard to add new ones, and impossible to do it programmatically

  Why do we want this again?

[Susko3]

Since definitions of virtual keys are spread across ContainsKeyPermissive() and ContainsKey(), it's hard to add new ones, and impossible to do it programmatically

Why do we want this again?

This is needed to unblock #5790. That PR currently takes a naive approach to adding virtual keys: it just adds them in KeyBindingsContainer.pressedInputKeys. This same mistake (but with virtual modifier keys) was fixed in 39032c9, but there still exists a lot of code that expects both physical and virtual keys to be in the user pressed keys array. I believe this may add to the confusion.

[Susko3]

This is really bad for perf. Should be bubbled up to KeyBindingContainer.pressedInputKeys (currently a HashSet<InputKey>, but should be changed to a Dictionary<InputKey, InputKey?> that maps [physical key -> virtual key]).

Leaving for later, when the code makes it past the first round of review.

[bdach]

I don't get it, if that's a concern, why isn't there a static dictionary in this class then? Why involve KeyBindingContainer at all?

[Susko3]

Idk, it's really hard to explain and reason as the current code is broken in subtle ways and does things that don't make sense. But together it works out and the errors are not visible to the outside world.

[bdach]

seems better than before but i still think it could be better yet

[smoogipoo]

I suppose it's not a huge deal, but is there a reason this was structured to map physical/virtual keys in a separate array here rather than using getVirtualKey() at the points where it's required?

There are two minor concerns:

1. Obviously this will allocate an array every time. It's a small and infrequent allocation though, so I'm willing to let this pass.
2. The API of KeyBindingContains and IsPressed become a little bit weird now that the consumer is able to specify a separate physical and virtual key with what appears to be no validation. There's nothing stopping users from providing (Physical: A, Virtual: Shift) as an argument, for example, which would be an undefined operation.
   Maybe I'm misunderstanding something, but it sounds like given your commit message about the invalid test cases that the user should never be dealing with virtual keys outside of KeyCombination. The APIs I'd expect are:
   • KeyBindingContains(ImmutableArray<InputKey> candidateKeyBinding, InputKey physicalKey)
   • IsPressed(InputKey virtualOrPhysicalKey, InputKey candidateKey)

Again please let me know if I've missed something.

[smoogipoo]

Here's my attempt at refactoring this to better explain what I mean: https://gist.github.com/smoogipoo/af35edc6492de8d5ea47ca4d64b7fc42

I'm wondering if there's some part of the extensibility argument that I'm missing with this.

[Susko3]

2. The API of KeyBindingContains and IsPressed become a little bit weird now that the consumer is able to specify a separate physical and virtual key with what appears to be no validation. There's nothing stopping users from providing (Physical: A, Virtual: Shift) as an argument, for example, which would be an undefined operation.

The methods you're talking about are internal (and can even be made private) so that's not part of the public API.

Here's my attempt at refactoring this to better explain what I mean: https://gist.github.com/smoogipoo/af35edc6492de8d5ea47ca4d64b7fc42

Yes, your refactor is perfectly fine if the virtual keys can be statically determined. But in #5790 that will not the the case. Eg. (Physical: Y, Virtual: KeycodeY) on a QWERTY, and (Physical: Y, Virtual: KeycodeZ) on a QWERTZ keyboard. The virtual key can only be inferred from InputState.Keyboard.

So the public API of KeyCombination should ultimately be changed from

public bool IsPressed(KeyCombination pressedKeys, KeyCombinationMatchingMode matchingMode)

to one of these options:

// Pass in InputState, so the (Physical, Virtual) key array is created each time
// [or getVirtualKey(InputKey physical, InputState state) is called when needed].
// Hard to test directly. But we can always test an internal method like how
// `KeyCombinationTest` tests `KeyCombination.ContainsAll`.
public bool IsPressed(KeyCombination pressedKeys, InputState inputState, KeyCombinationMatchingMode matchingMode)

// KeyBindingContainer builds a dictionary of (Physical, Virtual)
// and reuses the same instance for each checked KeyCombination.
// This ensures that there are no duplicate physical keys.
// But the meaning of <InputKey, InputKey?> is not apparent.
public bool IsPressed(ImmutableDictionary<InputKey, InputKey?> pressedPhysicalAndVirtualMapping, KeyCombinationMatchingMode matchingMode)

// KeyBindingContainer builds an array of (Physical, Virtual)
// and reuses the same instance for each checked KeyCombination.
// Undefined if someone passes in duplicate physical keys.
public bool IsPressed(ImmutableArray<(InputKey Physical, InputKey? Virtual)> pressedKeys, KeyCombinationMatchingMode matchingMode)

I think the first one makes the most sense, as we can have the getVirtualKey() logic contained inside KeyCombination. Your refactor works well with that.

Do we want to make the breaking change to KeyCombination.IsPressed() now or in #5790?

[smoogipoo]

The methods you're talking about are internal

Oh, I didn't notice that.

I think the first one makes the most sense, as we can have the getVirtualKey() logic contained inside KeyCombination. Your refactor works well with that.

That one makes the most sense to me too for a public API. Maybe the breaking change should be in this PR because I don't know how it's going to be used - I'm only reviewing purely from a code-quality and isolation perspective because you definitely have a better vision for how this all works.

Unless the breaking change + implementation is significant :)

[bdach]

Why does this receive the full InputState? Is the goal here to eventually read the Characters thing from #5790, then I'd rather just pass that in specifically when it exists.

The reason why I have an issue with this is that the InputState is propagated through methods multiple levels and when I look at the method signature of ContainsAll() I get very uneasy because there's three key collections there now - there's candidateKeyBinding and pressedPhysicalKeys which you are supposed to be checking, but then there is also InputState which really just allows you to read everything from everywhere and that feels like terrible layering. KeyBindingContains() is similar but not as egregious imo.

I'm fine with the IsPressed() signature fwiw, it's everything below that that I have an issue with.

[smoogipoo]

Yeah that's what I was also hoping to figure out - how the InputState is actually going to be used here.

[Susko3]

The InputState will be used to read the Characters of the passed in InputKey. And if the character is [a-z], it'll return the corresponding InputKey.Keycode{A-Z}.

I'd rather just pass that in specifically when it exists.

Alright, I'll do that.

[bdach]

I think I can live with this diff now, but do note I brought some of the tests removed in 3e021dd in an altered manner. See 8162563 for reasoning.

[smoogipoo]

lgtm