JACoders / OpenJK

Community effort to maintain and improve Jedi Academy (SP & MP) + Jedi Outcast (SP only) released by Raven Software
GNU General Public License v2.0
2.01k stars 612 forks source link

Numpad keys don't work properly since SDL2 #626

Closed xycaleth closed 9 years ago

xycaleth commented 9 years ago

The numpad keys don't work as they should when using SDL2 (i.e. now on all platforms). For example, pressing numpad 2 in the console will cause it act as if the down arrow key was pressed and also types '2'. It seems there's something missing from the key translation in sdl_input.cpp.

xycaleth commented 9 years ago

@ensiform, you were looking at this. Are you okay to fix it up and then submit your fix?

ensiform commented 9 years ago

I can't even get it to work.

ensiform commented 9 years ago

Here's my patch. Tested on Windows.

http://slexy.org/view/s22GnaEObR

xycaleth commented 9 years ago

I would change IN_NumLockEnabled to look like this:

static bool IN_NumLockEnabled( void )
{
#if defined(_WIN32)
    return (GetKeyState( VK_NUMLOCK ) & 1) != 0;
#else
    return (SDL_GetModState() & KMOD_NUM) != 0;
#endif
}

And also add a comment to explain why Windows isn't using SDL_GetModState.

I don't think checking for keysym->sym >= SDLK_KP_1 && keysym->sym <= SDLK_KP_0 is necessary either. That will be handled correctly by the switch statement!

Other than that, looks good!

@smcv Would you mind testing Ensiform's changes on Linux? There's some issues regarding SDL_GetModState (at least on Windows) so we want to double check that it works as expected on Linux. Thanks

ensiform commented 9 years ago

https://bugzilla.libsdl.org/show_bug.cgi?id=2736 Suggests it doesn't work under X11.

smcv commented 9 years ago

Sure, I'll see if I can find a keyboard with a num pad...

What's the expected result, though? In the console, 8/Up should presumably either type 8 or go up depending on num lock state (right?); during gameplay, if I've bound different actions to 8, Up and KP_UPARROW, which one should I get with and without num lock?

ensiform commented 9 years ago

In the windows versions, (pre-SDL) the numpad NUMBER(0-9, not the period) keys are translated to number keys. Char events are unaffected. But the console supports some of the shortcuts to do the actions on the numpad keys thus the current/new behavior happens (non-patch above).

xycaleth commented 9 years ago

Here's a few test scenarios:

  1. Bring down the console
  2. Ensure num lock is enabled
  3. Press 1 on numpad
  4. Press 2 on numpad

Expected result is that "12" is typed. Problems we'd had in the past is that pressing 2 on numpad would be as if the user had pressed the down arrow key, and then pressed 2.

With num lock disabled, we wouldn't expect the numpad keys to type anything in the console.

Second test scenario:

  1. Go to controls menus
  2. Select one of the actions, e.g. walk forward
  3. Ensure num lock is enabled
  4. Press one of the numpad number keys.

Expected result is that the number key that was pressed is bound to the action.

With num lock disabled, the alternative key is bound to the action. For example, pressing numpad 1 would give KP_END, pressing numpad 3 would give KP_PGDN.

EDIT: Fixed expected results for the second test scenario.

smcv commented 9 years ago

This is all on Debian unstable (very similar to what is going to be Debian 8) with SDL 2.0.2+dfsg1-6.. Here are current results as of 366933d5f (without any extra patches) for comparison.

If the engine was started with numlock on, and it is still on, I get the same weird results you noted in the console, e.g.:

If the engine is started with numlock off, the console works fine.

In the Controls UI, pressing numpad keys always results in binding KP_UPARROW or whatever, regardless of numlock state at startup or numlock state now.

In-game, pressing numpad keys results in the action bound to KP_UPARROW or whatever, regardless of numlock state at startup or numlock state now. For instance if I

\bind KP_UPARROW +moveup
\bind UPARROW +forward
\bind 8 +attack

then pressing the 8/up key on the numpad always results in jumping.

smcv commented 9 years ago

ioquake3 (also using SDL2) works the same as OpenJK:

smcv commented 9 years ago

If numlock was on at startup, @ensiform's patch doesn't make any difference for me at the console: I still get the odd behaviour.

If numlock was off at startup, the console is still fine.

The numpad is now indistinguishable from the normal digit keys while numlock is on: Controls configuration and gameplay both treat 8/Up as 8 if numpad is on, or KP_UPARROW if numlock is off.

I feel as though the ideal behaviour might actually be more like this:

smcv commented 9 years ago

FYI the exact patch that I tested is https://github.com/smcv/OpenJK/tree/wip/numpad

smcv commented 9 years ago

I would suggest taking this "upstream" to ioquake3: if there's a "right thing" for how the numeric keypad should behave, then it should probably be the same in all SDL Quake III Arena derivatives.

ensiform commented 9 years ago

Well, JKA differs from Q3 behavior in this regard. They added it to JKA specifically, its not in the JK2 code either.

I don't think your suggestion is right either. As the console is explicitly interpreting both. I'm not entirely sure what happened in the original Q3/ioq3 before SDL2. And in order to detect it for controls menu, you would have to break API to ask for if waiting for a bind and not just KEYCATCH_UI / KEYCATCH_CGAME in general. We would rather have it be the same behavior as the way JKA windows produces if possible.

As such, there is definitely a bug in SDL2 because it does not seem to update the proper modstate with the LED state on application startup.

ensiform commented 9 years ago

https://bugzilla.libsdl.org/show_bug.cgi?id=2736

Yberion commented 9 years ago

Any idea when it will be fixed?

ensiform commented 9 years ago

Long time

xycaleth commented 9 years ago

@ensiform Can you push a commit with your fix please? We'll just have it working properly for Windows for now and we can say in our release notes that numpad doesn't work very well on Linux/OSX.