-
Notifications
You must be signed in to change notification settings - Fork 37
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
support for native keyboard input (from Chocolate Doom) #2030
base: master
Are you sure you want to change the base?
Conversation
src/i_input.c
Outdated
|
||
if (SDL_PeepEvents(&next_event, 1, SDL_PEEKEVENT, | ||
SDL_FIRSTEVENT, SDL_LASTEVENT) == 1 | ||
&& next_event.type == SDL_TEXTINPUT) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if there is a better way to do it these days. Maybe SDL3 has a better solution.
This also fixes #1792 |
I think we should allow users to enable this feature in the menu. The question is which menu section? Does this fall under "Quality of Life"? And how would this be called? I am quite dissatisfied with "Vanilla Keyboard Mapping". How about "Assume US-ASCII keyboard" or something similar? |
To match Chocolate Doom behavior, cheat codes should be read from Line 1313 in 0d5505b
|
src/i_video.c
Outdated
{ | ||
ProcessEvent(&sdlevents[i]); | ||
} | ||
while (SDL_PollEvent(&sdlevent)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is going to undo the performance gained by this PR: #1753
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should wait until SDL3 with this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know, but if I can make a request: Please save big changes to the input code until after the next release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I guess this issue is not that important. The SDL3 branch is the next thing I planned to do after the release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you guys please mind having a look at the solution I proposed? This increases the size of an array by one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you guys please mind having a look at the solution I proposed? This increases the size of an array by one.
event.data3.i = GetTypedChar(&sdlevent->key.keysym, sdlevent + 1);
I don't understand this line, we assume that there is always next event in queue?
While the diff is small, it's going to be hard to remember what's going on here in a couple weeks 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
event.data3.i = GetTypedChar(&sdlevent->key.keysym, sdlevent + 1);
I don't understand this line, we assume that there is always next event in queue?
Yes, indeed there always is, because the array that holds the event queue in I_GetEvent()
is allocated one element larger than the number of events retrieved by SDL_PeepEvents()
in that same function. So, at this point we just need to check if that "next" element is actually of SDL_TEXTINPUT
type and then read the typed char fro it. From this point on it's the same as the Chocolate code, but we peep into SDL's event queue at a different point.
While the diff is small, it's going to be hard to remember what's going on here in a couple weeks 😄
We have comments for this. 😉
Anyway, I think the solution that you proposed on Discord, i.e. use I_StartTextInput()
/I_StopTextInput()
to explicitly enable or disable processing of SDL_TEXTINPUT
type events and directly feed the typed char into the game's event queue, is preferable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anyway, I think the solution that you proposed on Discord, i.e. use
I_StartTextInput()
/I_StopTextInput()
to explicitly enable or disable processing ofSDL_TEXTINPUT
type events and directly feed the typed char into the game's event queue, is preferable.
Done here: 7172354
Fixed in 626dbc0 |
I thought it was supposed to be disabled for everyone. Who would want to use only the US layout? No problem for me with the old code either, though. |
Now keyboard navigation doesn't work anymore. |
And it still crashes when trying to type chars for which there is no glyph in the Doom font. |
What kind of navigation? Does it still not work with the latest commit? |
Nsvigating either the menu or the game with arrow keys.
Nope. |
Should be fixed in 26e1ab4 |
That fixed it. 👍 |
We can replace unknown symbols with |
Yes, please, one of those. We had |
Done in 8953e06. I think one day we should consolidate these functions. |
src/st_widgets.c
Outdated
// send a macro | ||
if (altdown) | ||
{ | ||
ch = ev->data1.i; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should macros be simplified or removed? Currently, users have to press “T” and then Alt-Number. I suggest just Alt-Number.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see no problem with macros being only available in chat mode.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine with me either way. I have never ever sent a single chat macro in my entire life. But I am afraid that it clashes with muscle memory. The ALT key was used for strafing in Vanilla Doom and the number keys were used to change weapons. So, chances are that you accidently send a chat macro when switching weapons while strafing in the heat of the combat. 😉
src/i_input.c
Outdated
{ | ||
// On desktop platforms, SDL_StartTextInput() is implicitly called on SDL | ||
// window creation | ||
if (vanilla_keyboard_mapping) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should always be disabled at startup, regardless of vanilla keyboard mapping.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 99a1c70
|
||
static int GetTypedChar(SDL_Keysym *sym) | ||
{ | ||
int result = TranslateKey(sym); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this rather be GetLocalizedKey()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MrAlaux Could you please test this PR? I think the save and chat lines should work as expected, but I'm not sure about the "Key Bindings" menu. Are the WASD keys in different places on your keyboard? Do we need to do something about this? |
Hmm, so we don't need to change "Key Bindings"? |
It might work for me (I'll test later), but other people with non-QWERTY keyboards may face issues. Couldn't we just change the OS's keyboard layout to test? |
Just switched my keyboard layout to DVORAK temporarily. WASD keyboard movement works as before, but savegame name entry follows the DVORAK switch as expected. 👍 |
This is good, but what if you change some key bindings in the menu with DVORAK layout? |
I'll have to check tomorrow. |
Changing key bindings with the DVORAK layout still shows the scancode names as found on an American ASCII keyboard. |
Should we do something about this? It could be problematic because we store ASCII key names in config instead of scancodes. |
Maybe we should move to |
I suggest we put that on hold for now. Should we merge this PR? |
Okay.
Not sure, this code is really invasive. |
Fix #1772
Fix #1773
Fix #1792
Fix #2032