Fix controller keyboard simulation

[RattleSN4K3]

Fixes controller keyboard simulation (like button 3 and 4 for [M] and [R]), resolves issue reported by #242. Reverts changes from 2180f0b done to controller keyboard simulation, by removing the translation of ScanCode inputs (from KeyDown event) to a key code (UCS4CharKey).

Additionally, to prevent duplicate errors (what the orignal fix was about) there's a double check for a text input event; added additional comment about that specific if-check.

This change needs testing if a duplicate character input still occurs.

@daniel-j, since you worked on that original fix to avoid the duplicate characters, can you verify it doesn't happen with this change?

[daniel-j]

I can verify this later, thanks!

[daniel-j]

With this PR, using a keyboard and searching text recreates the old bug. Pressing a key results in two letters appearing in text boxes (both keydown and textinput gets fired). Here I pressed the keys ABC on my keyboard.

I will test with a steam controller soon.

[basisbit]

this happens most likely because key pressed and text entered events both are thrown. Can we please agree on changing the gamepad key simulation code instead of changing this currently nice code?

[daniel-j]

I think rewriting how the game handles input would be a good idea. For example the menu looks for the M key to be pressed. Would be better to trigger BUTTON_MENU or something, and add key/button remapping to settings.
A quick-fix would be to send text.text from the joystick code to event queue, but that would put M and R etc. in a text input if one is active.
I tested with steam controller and it works with the PR (pressing X and Y keys for R and M), but since it breaks text input it's not a solution. I agree with @basisbit.

[RattleSN4K3]

I think rewriting how the game handles input would be a good idea. For example the menu looks for the M key to be pressed. Would be better to trigger BUTTON_MENU or something, and add key/button remapping to settings.

Yep. Somewhere here (or on the old gitter) I've mentioned something like that. But it would have taken too many changes on the existing code therefore I left the existing code intact and used the workaround. Due to the duplicate character doesn't happen on my systems (Windows) and didn't on the Linux I used I didn't know the conflict (altough I thought about that).

this happens most likely because key pressed and text entered events both are thrown. Can we please agree on changing the gamepad key simulation code instead of changing this currently nice code?

Any suggestion, @basisbit?

A quick-fix would be to send text.text from the joystick code to event queue, but that would put M and R etc. in a text input if one is active.

It already is added to a text input (that's how input is parsed in various screens). I thought about a fix on this issue by not checking unicode for 0 but using either timestamp or using unicode := SizeOf (Uint32). In order to prevent the KeyDown event resulting into a valid char code (KeyCharUnicode). It seems like Event.key.keysym.unicode is also 0 on systems causing the duplicate letters.

Sending text event should be reserved for controllers with a keyboard (in case these are not triggering SDL's text input event).

[RattleSN4K3]

Updated the pull request. Changed the "unset" value of the unicode field to properly check if it is a desired Joystick-simulated keyboard event and thus translating the input to the character. Hopefully this doesn't result into the duplicate character bug again (when both events trigger and forwarding a valid unicode char).

Since I am not able to reproduce the original bug of duplicate characters, can anyone verify/test this change?

[daniel-j]

It's not the prettiest fix, but it solves the problem. I don't have a controller here to test with at the moment, but I get no duplicate text input :)

[basisbit]

Event.key.keysym.unicode is deprecated. Please do not use it...

[RattleSN4K3]

Event.key.keysym.unicode is deprecated. Please do not use it!

I know. But as long as the lib is providing the field, I see no problem with that (and once it is removed, it is known that it should be rewritten). It is not intended to be kept. The input handling could be rewritten to support proper text handling (including delete, cursor, and what not). With such change a keyboard simulation will likely be able to push custom input events on the stack/queue without conflicting text input.

I wanted to use timestamp instead but it looks like it is populated/set by the PUSH/POLL method. The other fields (like padding2, padding3 altough not mentioned in the docs) might be conflicting with other intended behavior, same as windowID or state. Unicode was the easiest-working-on-top-of-existing-code solution, due to possible conflicts (with text input), and now being deprecated, it was/is declared as hacky solution.

I could think of guard (semaphore like, or stack) which is set on pushing such event, and is cleared on successfully polling it. But things like that could block input. Or timing problems could result into input being unrecognized.

There are better solutions, there are cleaner solution, it all up to us to provide it. The existing code is meant to provide the requested and back then bugged Joystick support. The text input handling was a problem for a long time, and without touching any of that code without proper planning, I just decided to use that workaround.

Could you suggest any change I should do?

[basisbit]

Sending text event should be reserved for controllers with a keyboard (in case these are not triggering SDL's text input event).

Do these even exist? I'd vote for having the controller Menu / ... buttons actually add text input events and thus simulate the normal keyboard key pressed events. That would at least not break whenever the unicode field gets removed / changed.

[RattleSN4K3]

Do these even exist?

I know at least one, a prominent one; the old Chatpad for Xbox360 controllers. I haven't tested the Chatpad on Windows systems myself. There are ways to use the chatpad with a driver and it works like a keyboard HID. Not sure how a possible generic device would work and if it's even compatible with SDL.

Nowadays most of such devices do work over Bluetooth and likely add a keyboard HID.

I'd vote for having the controller Menu / ... buttons actually add text input events [...]. That would at least not break whenever the unicode field gets removed / changed.

Well, it would work for [M] and [R] but not for control keys like [Enter] or non-printable keys etc. And if things like that are customizable, the code would need to determine what kind of event it needs to send. Makes it a bit more complicated.

And TextInput doesn't work with repeated key presses (KeyUp following KeyDown, contrary to consecutive polled KeyDown-TextInput events as fast as the delay allows it). But that is another problem and not fully related to the Joystick implementation.

thus simulate the normal keyboard key pressed events.

Yep, that was the intent of the keyboard simulation pushing KeyDown events (since it is used in all the existing code). TextInput is the additional event triggered only on text input (by SDL). IIRC the current input handling is already tricky. For instance, the editor requires various key inputs from keys initially paired to each other on one specific layout but the same keys are not at the same place or do require a key modifier on other layouts/keyboards. There are issues about that already.

So the question would be is the low-level KeyDown/Up event the right one to choose or the special TextInput event? Both do have downsides and both are used the same way in USDX (where only TextInput is translated to a proper key char, keysym.sym is still passed). I simply chose KeyDown/Up, being more consistent and extendable.

---

So far from my tests and the tests by @daniel-j regarding duplicate letters, I would say the current code is polished enough as long as other problems/missing-features regarding text input is still present and not considered to change anytime soon (and how).

[basisbit]

any new progress on this one? the initial problem seems to not exist any more afaik? Will close this issue end of the month if there is nothing new regarding this.