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

support for native keyboard input (from Chocolate Doom) #2030

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

rfomin
Copy link
Collaborator

@rfomin rfomin commented Nov 18, 2024

Fix #1772
Fix #1773
Fix #1792
Fix #2032

src/i_input.c Outdated

if (SDL_PeepEvents(&next_event, 1, SDL_PEEKEVENT,
SDL_FIRSTEVENT, SDL_LASTEVENT) == 1
&& next_event.type == SDL_TEXTINPUT)
Copy link
Collaborator Author

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.

src/i_video.c Outdated Show resolved Hide resolved
src/i_input.c Outdated Show resolved Hide resolved
@fabiangreffrath
Copy link
Owner

This also fixes #1792

@fabiangreffrath
Copy link
Owner

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?

@fabiangreffrath
Copy link
Owner

fabiangreffrath commented Nov 18, 2024

To match Chocolate Doom behavior, cheat codes should be read from data2 here:

if (ev->type == ev_keydown && M_FindCheats(ev->data1.i))

src/i_video.c Outdated
{
ProcessEvent(&sdlevents[i]);
}
while (SDL_PollEvent(&sdlevent))
Copy link
Collaborator

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

Copy link
Collaborator Author

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?

Copy link
Collaborator

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.

Copy link
Collaborator Author

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.

Copy link
Owner

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.

Copy link
Collaborator Author

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 😄

Copy link
Owner

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.

Copy link
Collaborator Author

@rfomin rfomin Nov 19, 2024

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 of SDL_TEXTINPUT type events and directly feed the typed char into the game's event queue, is preferable.

Done here: 7172354

@rfomin
Copy link
Collaborator Author

rfomin commented Nov 18, 2024

To match Chocolate Doom behavior, cheat codes should be read from data2 here:

Fixed in 626dbc0

@rfomin
Copy link
Collaborator Author

rfomin commented Nov 18, 2024

I think we should allow users to enable this feature in the menu.

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.

@fabiangreffrath
Copy link
Owner

Now keyboard navigation doesn't work anymore.

@fabiangreffrath
Copy link
Owner

And it still crashes when trying to type chars for which there is no glyph in the Doom font.

@rfomin
Copy link
Collaborator Author

rfomin commented Nov 19, 2024

Now keyboard navigation doesn't work anymore.

What kind of navigation? Does it still not work with the latest commit?

@fabiangreffrath
Copy link
Owner

What kind of navigation?

Nsvigating either the menu or the game with arrow keys.

Does it still not work with the latest commit?

Nope.

@rfomin
Copy link
Collaborator Author

rfomin commented Nov 19, 2024

What kind of navigation?

Nsvigating either the menu or the game with arrow keys.

Should be fixed in 26e1ab4

@fabiangreffrath
Copy link
Owner

Should be fixed in 26e1ab4

That fixed it. 👍

@rfomin
Copy link
Collaborator Author

rfomin commented Nov 19, 2024

And it still crashes when trying to type chars for which there is no glyph in the Doom font.

We can replace unknown symbols with ? or check for NULL in text drawing functions.

@fabiangreffrath
Copy link
Owner

We can replace unknown symbols with ? or check for NULL in text drawing functions.

Yes, please, one of those. We had ! before, because it's the first glyph in both fonts.

@rfomin
Copy link
Collaborator Author

rfomin commented Nov 19, 2024

We can replace unknown symbols with ? or check for NULL in text drawing functions.

Yes, please, one of those. We had ! before, because it's the first glyph in both fonts.

Done in 8953e06. I think one day we should consolidate these functions.

@fabiangreffrath
Copy link
Owner

Done in 8953e06.

Now this also fixes #2032.

I think one day we should consolidate these functions.

Yes, definitely.

src/st_widgets.c Outdated
// send a macro
if (altdown)
{
ch = ev->data1.i;
Copy link
Collaborator Author

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.

Copy link
Owner

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.

Copy link
Owner

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)
Copy link
Owner

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.

Copy link
Collaborator Author

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);
Copy link
Owner

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()?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@rfomin
Copy link
Collaborator Author

rfomin commented Nov 20, 2024

@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?

@MrAlaux
Copy link
Collaborator

MrAlaux commented Nov 20, 2024

Are the WASD keys in different places on your keyboard?

Compared to US? I don't think so. It's a Spanish keyboard:

imagen

@rfomin
Copy link
Collaborator Author

rfomin commented Nov 20, 2024

Compared to US? I don't think so. It's a Spanish keyboard:

Hmm, so we don't need to change "Key Bindings"?

@MrAlaux
Copy link
Collaborator

MrAlaux commented Nov 20, 2024

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?

@fabiangreffrath
Copy link
Owner

Just switched my keyboard layout to DVORAK temporarily. WASD keyboard movement works as before, but savegame name entry follows the DVORAK switch as expected. 👍

@rfomin
Copy link
Collaborator Author

rfomin commented Nov 20, 2024

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?

@fabiangreffrath
Copy link
Owner

This is good, but what if you change some key bindings in the menu with DVORAK layout?

I'll have to check tomorrow.

src/i_input.c Outdated Show resolved Hide resolved
@fabiangreffrath
Copy link
Owner

This is good, but what if you change some key bindings in the menu with DVORAK layout?

Changing key bindings with the DVORAK layout still shows the scancode names as found on an American ASCII keyboard.

@rfomin
Copy link
Collaborator Author

rfomin commented Nov 22, 2024

This is good, but what if you change some key bindings in the menu with DVORAK layout?

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.

@fabiangreffrath
Copy link
Owner

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 data2, i.e. key symbol, but I'm not sure either.

@rfomin
Copy link
Collaborator Author

rfomin commented Nov 22, 2024

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 data2, i.e. key symbol, but I'm not sure either.

I suggest we put that on hold for now.

Should we merge this PR?

@fabiangreffrath
Copy link
Owner

I suggest we put that on hold for now.

Okay.

Should we merge this PR?

Not sure, this code is really invasive.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants