Input modifier

[ThompsonTyler]

Contains

This PR works towards solving #2551.

What was added:

• Ability to bind keys with modifiers(Alt, Shift, or Control) with @DefaultBinding annotation and ingame input binding changer
• Json deserialization for modified inputs
• A new extension of Input that allows for a modifier to be attached to it
• Alteration of some current existing input functions to allow for use of new modified input as well
• A bunch of logic to the InputSystem that accounts for these modifier keys.

Currently you cannot bind a key to just control, alt, or shift unless it is from a defaultBinding, but everything still works as expected if something is bound to those keys due to defaultBinding.

New variation on Default binding:

@DefaultBinding(type = InputType.KEY, id = Keyboard.KeyId.R)

Can now also be done if a default binding with a modifier is wanted:

@DefaultBinding(type = InputType.KEY, id = Keyboard.KeyId.R, mod = InputModified.Modifier.ALT)

Currently designed that if multiple modifier keys are pressed, all variations are checked, but no multiple modifiers are pressed:

• If Alt, Shift, W, and E are being pressed then it is same as the keys (Alt), (Shift), (Alt + W), (Alt + E), (Shift + W), and (Shift + E) being pressed
• When binding with inputs if multiple modifiers are pressed they take priority of alt > control > shift

How to test

This PR changes a bit of the logic behind the inputSystem and therefore is very expansive for testing, I would not be surprised if I missed a certain test or what not, but I will list below everything that I tested as I developed.

• Having a default binding as any of the modifier keys or the modifier "none"

• Reset to the above binding

• Set a binding based on alt, control, or shift and test all possible holding combinations(solo, alt +control, alt + shift, all three, etc)

• Test for if there is overlap on setting an input that matches (alt + w vs alt + w)

• Test for overlap on inputs that match only on modifier (alt + w vs alt + r)

• Test for overlap on inputs that match only on key (alt + w vs w)

• Test for overlap on inputs that match with same key, different modifier (alt +w vs shift + w)

• Test all of the above in game on actual binding inputs

• Test for proper interaction between pressing and unpressing modifier or normal keys

• Tests for simulated key presses (Auto run for example)

  Having something bound to W and Alt + W. Pressing alt by self -> nothing, then w -> (Alt + W) binding, release Alt -> stop (Alt + W) and trigger (W). Hold just W -> (W), press alt -> Stop (W) and start (Alt + W), release W -> Stop (Alt + W), but don't start (W)

• Test inputs on different types of input (Inputs that only check press portion, and inputs that check repeats, etc)

• Good ol button spamming and seeing a "correct" response across output with no crashing

Outstanding before merging

Currently only with Keys. Could be extended to controller/mouse inputs as well, but didn't really seem to fit for the current design. But, this does allow for a very good idea on how to implement say shift clicking items in an inventory and what not.

Still new to programming in larger projects and so will probably have some slight coding oddities that would need to be double checked before fully merging, but just let me know and I will be glad to make fixes/work towards different methods as needed.

[GooeyHub]

Can one of the admins please verify this patch?

[Cervator]

@GooeyHub: ok to test

@ThompsonTyler: great to see this make it to a PR, very nicely done! Yeah it might take a little while to fully test and merge, but that's OK :-)

[GooeyHub]

Hooray Jenkins reported success with all tests good!

[ThompsonTyler]

@Cervator Yeah, it feels pretty good to finally not run into a dead-end while working on this I gotta say.

[oniatus]

Took some time to get an overview :)

I really like the feature but i think we should take the time to get the InputSystem in a better state. I found the actual implementation being pretty hard to test and this is a feature i would like to see being backed up by tests and same for the original bindings. Also I hope to cover the required changes of this feature to the InputSystem in less lines and more readable than the actual PR. The current 200+ lines processKeyboardInput-method is beyond spaghetti and I am unable to understand what it does or even give a decent review there 😥

@ThompsonTyler Are you interested in continuing on this? I would prefer to clean up the original system first, maybe even prepare it for FlexibleConfig and implement this feature on top of that.
Using the KeyboardDevice it is pretty easy to fake keyboard input for tests, everything that's missing is probably possible with refactoring.
You can also ping me on slack for a shorter discussion on the topic (same nickname).

Also ping to @emanuele3d, the final Config/BindsConfig was one of my main problem when i tried to write tests for the InputSystem myself. Maybe we can work towards the FlexibleConfig change, e.g by wrapping the bindsConfig (inputsystem takes the config only to poll the bindsConfig) to a simple system where we can replace it later on, when the FlexibleConfig is ready, without breaking everything. There is also a lot of logic in the BindsConfig class which will be in way when we want to replace it with a key-value format.

[emanuele3d]

@oniatus: I'm sorry the FlexibleConfig discussions are taking a long time. If you have a look at the architectural discussion in #2893 and the UI discussion in #2894 you will get a sense of the complexity of the issue.

One thing I'm on the verge of suggesting in those discussions is to focus, to begin with, on the global configs (RenderingConfigs, InputBindings, etc) alone. This way we get some experience with the new FlexibleConfig-related classes before we plunge into more complicated, overlapping things such as Module configs and Per-saved-game configs.

Meanwhile, @oniatus, feel free to join the FC discussion & development and definitely proceed in the way you outlined in your previous post, so that migration to FCs will eventually be easier.

[oniatus]

Closing this PR for now as it became outdated and a bit inactive. I think it is worth to keep the ideas so I added a reference to #2551. Would be nice to see if someone or @ThompsonTyler himself will catch up with the feature again 👍