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

Map SDL_SCANCODE_GRAVE to the console key. #285

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mickael9
Copy link

@mickael9 mickael9 commented Apr 5, 2017

Currently, the only way to invoke the console which is both platform and
layout independent is to use Shift+Escape.

This change makes sure the QWERTY '`' key (at the left of the 1 key)
works as a console key and for all keyboard layout by matching against
its scan code (ie physical location).

Since this key can also be mapped to normal characters that could be
useful for text input (eg, '^' on German keyboards), we only interpret
it as a console key if one of those conditions is respected:

  • The resulting key is valid and outside ascii range (0x20-0x7f)
  • The resulting key is '~' or '`'
  • The console input field is empty and no chat message is being written

Additionaly, no modifiers (Shift, Ctrl, Alt, AltGr, Ctrl, Super/Win)
shall be pressed to allow usage of the keys in the Shift/AltGr group and
to allow bypassing the console (Alt works on X11, Windows key must be used
on Windows)

A new cl_consoleUseScanCode cvar (default 1) is added to enable the
feature.

@timangus
Copy link
Member

timangus commented Apr 5, 2017

This makes complete sense if it actually works. Sadly in the field testing of the scancode support in SDL did not go well. However, that was very soon after SDL 2 was released so hopefully it's better now.

@ensiform
Copy link

ensiform commented Apr 5, 2017

In the field it always maps to that key. But the issue is is that key is sometimes ^ or other possibly conflicting ones.

@ghost
Copy link

ghost commented Apr 5, 2017

mickael9, for a long time (since SDL2) opening console on german keyboard is a real disaster, I can only open console with Alt Gr + ~
With your proposed pull request, the good old behaviour is restored! You made my day, with your changes the console key works fine!

@ensiform
Copy link

ensiform commented Apr 5, 2017

JACoders/OpenJK@0a65fd6

This was my solution.

@DanielGibson
Copy link

one potential problem is that on the Brazilian keyboard layout, ' and " are on that key.. those chars are useful in the console, right?

Can't people just use Shift+Esc to open the console? Or bind one of the F keys or sth

@ensiform
Copy link

ensiform commented Apr 5, 2017

Alternatively you can also maybe add an alt key modifier when held to not process it as a console key, and proceed as text input for the above case too maybe ^ @DanielGibson ?

@DanielGibson
Copy link

so Alt+Key should give ' or " instead of opening/closing console? or the other way around?

@ghost
Copy link

ghost commented Apr 5, 2017

Does anyone of you understand what exactly the real problem behind all this is with SDL2?
Why was this not a problem with 1.2? Afaik other have serious problems as well (ET Legacy etc.)...

@DanielGibson
Copy link

it was a problem with 1.2, but as 1.2 didn't have (consistent) scancodes it wasn't solvable, so I think the "solution" was to just add support for Shift+Esc which works everywhere

@ghost
Copy link

ghost commented Apr 5, 2017

Ah thanks! In Spearmint (ioquake3 based) even Shift+Esc won't work (at least on a german keyboard). Didn't know about differences of ioquake3 forks,..., so Shift+Esc ,should' work on all layouts, but is not guaranteed?

@mickael9
Copy link
Author

mickael9 commented Apr 5, 2017

How about translating keys to their QWERTY-equivalent keycode and using that to compare against cl_consoleKeys ?

This would work by default, and Brazilian keyboard users can change it to avoid conflicts with the " key.

I'm not sure if matching against characters (rather than keys) in cl_consoleKeys would still be useful in that case, so perhaps remove that too?

@DanielGibson
Copy link

Shift+Esc should work if the game/port has code to support it.

And even ^ (console key on German keyboard) is useful in the quake3 console, for colored text/nicks IIRC?

OTOH I think I used the SDL_SCANCODE_GRAVE hack in some games (Daikatana and Yamagi Quake2, not sure about dhewm3 and RBD3BFG right now) and no one ever complained - but that doesn't mean it's not really a problem, it just means that so far there were no people from Brazil who use those games/ports and use the console and tried to input ' or " there and bothered to report the issue...

@ghost
Copy link

ghost commented Apr 5, 2017

Yeah, the ^ key is for colored text on german keyboard.
Hopefully you guys find a solution to satisfy all nations/players!

@kungfooman
Copy link

kungfooman commented Apr 5, 2017

This PR would cause that I cannot write any ^ chars in the console anymore, since it would close the console instantly. As solution I propose to simply check if the console text is empty.

We gotta add the special cases over time, atm I added for German/Brasilian keyboards.

@DanielGibson Could you try this code, if it works for you? Probably it only needs one case, ' or ". Based on the Brasilian keyboard screenshot I've seen, it's probably ' (the lower one).

In code:

if (keysym->scancode == SDL_SCANCODE_GRAVE || IN_IsConsoleKey( key, 0 ) )
{
	// The grave scancode toggles the console now, but this alone would prevent the use of the ^ key on some keyboard layouts.
	// So we need a way to ignore the grave scancode only on these keyboards, so the ^ key can be used to color text while in console.
	// A intuitive condition is to ignore the grave scancode only when there is already text entered into the console.
	// If there is no text, the grave scancode can toggle/close the console just as the non-problematic keyboard layouts.
	int keyconflict = 0;
	keyconflict += keysym->sym == '^' && keysym->scancode == SDL_SCANCODE_GRAVE; // German keyboards
	keyconflict += keysym->sym == '\'' && keysym->scancode == SDL_SCANCODE_GRAVE; // Brasilian keyboards (either this)
	keyconflict += keysym->sym == '"' && keysym->scancode == SDL_SCANCODE_GRAVE; // Brasilian keyboards (or this?)
	int console_has_text = strlen(g_consoleField.buffer) > 0;
	if (keyconflict && console_has_text)
		return key;

	// Console keys can't be bound or generate characters
	key = K_CONSOLE;
}

Thanks for the PR btw, this triggered me to check this console key issue again. Finally it feels "right".

@mickael9
Copy link
Author

mickael9 commented Apr 6, 2017

Thanks for the feedback. I think we probably don't want this if that key is useful on German and Brazilian layouts (and likely others).

We can add the Alt modifier to bypass console, but it's likely the user won't know about it, and won't be able to use the original keys. Users that know about console can always modify cl_consoleKeys to suit their needs.

@ensiform's solution doesn't work for Brazilian keyboards, and German layout users have to use Shift which makes it a bit useless (just use Shift-Escape)

The currently implemented solution matches both by characters and keys, which seems to be a good compromise:

  • It doesn't prevent usage of ^, " or ' as normal keys for German/Brazilian layouts
  • Console can still be opened using the ~ and ` characters (wherever the actual key is). This is more user friendly since if we tell an user the console key is ~, then it will work no matter the layout. Although this is still an issue on Windows if those are dead keys (need to press another key after it and double-pressing doesn't work like it would on X11).
  • For AZERTY, the ² character could be added in the default cl_consoleKeys. I'll make another PR for this.

@kungfooman Unfortunately there are many other layouts and conflicting keys, for instance BÉPO has # / $.

I have created a quick script that extracts all the possible symbols from all layouts for that key and this was the result (including only latin chars): https://git.io/vSgy0. There are probably bogus entries in there but it gives an idea.

Anyway, I think if we're going for your method (which IMHO is clearly an improvement over the current implementation), we might as well do the opposite and consider it a conflict if it matches any ASCII character in the 32-127 range except ~ and `. We might also only want to trigger the console if there is no keycatcher set in this specific case (eg, if we're typing chat or typing text in UI field).

Something like this (untested):

diff --git a/code/sdl/sdl_input.c b/code/sdl/sdl_input.c
index 4c8ff1fe..66b380f8 100644
--- a/code/sdl/sdl_input.c
+++ b/code/sdl/sdl_input.c
@@ -191,11 +191,13 @@ IN_TranslateSDLToQ3Key
 static keyNum_t IN_TranslateSDLToQ3Key( SDL_Keysym *keysym, qboolean down )
 {
        keyNum_t key = 0;
+       qboolean is_ascii = qfalse;
 
        if( keysym->sym >= SDLK_SPACE && keysym->sym < SDLK_DELETE )
        {
                // These happen to match the ASCII chars
                key = (int)keysym->sym;
+               is_ascii = qtrue;
        }
        else
        {
@@ -291,6 +293,15 @@ static keyNum_t IN_TranslateSDLToQ3Key( SDL_Keysym *keysym, qboolean down )
        {
                // Console keys can't be bound or generate characters
                key = K_CONSOLE;
+       } else if ( keysym->scancode == SDL_SCANCODE_GRAVE ) {
+               // We want the QWERTY grave key location to always match the console (whatever the layout)
+               // Since this can map to other useful keys, we only do this if:
+               // - Either the key is not in the ASCII range or is one of '~' and '`'
+               // - Or we have no key catchers set and the console field is empty
+               qboolean console_has_text = strlen(g_consoleField.buffer) > 0;
+
+               if ( !is_ascii || key == '~' || key == '`' || ( !Key_GetCatcher() && !console_has_text ) )
+                       key = K_CONSOLE;
        }
 
        return key;

@DanielGibson
Copy link

I don't have a Brazilian keyboard, just a German one - I only saw that layout somewhere (probably https://en.wikipedia.org/wiki/Portuguese_keyboard_layout#/media/File:KB_Portuguese_Brazil.svg)

Anyway, I like the idea of toggling the console with SDL_SCANCODE_GRAVE, but only if the console line empty - this should work even without checking all the potential key codes that key could have?

@kungfooman
Copy link

@DanielGibson: We could completly drop the conflict checking and just close console when it has no input. I just wanted to keep the old behaviour for people who are used to it (closing console even tho it has text - I don't even know if that's the default behaviour lol).

@mickael9: The ASCII idea seems ideal to detect the special cases. The keycatch idea is also great, so I can use ^ again in chat to color the chat text.

Using only if (Key_GetCatcher()) is too much, since then the console can't be toggled while being in console and neither can the console be opened in ESC menu. Makes most sense to check for chat only (unsure about UI field, is that KEYCATCH_MESSAGE aswell?).

I got this code so far, which works at least on German keyboard, so would be great if all kinds of keyboard layout users could test it aswell:

if (keysym->scancode == SDL_SCANCODE_GRAVE || IN_IsConsoleKey( key, 0 ) )
{
	int chatting = Key_GetCatcher() & KEYCATCH_MESSAGE;
	if (chatting)
		return key;

	// The grave scancode toggles the console now, but this alone would prevent the use of the ^ key on some keyboard layouts.
	// So we need a way to ignore the grave scancode only on these keyboards, so the ^ key can be used to color text while in console.
	// A intuitive condition is to ignore the grave scancode only when there is already text entered into the console.
	// If there is no text, the grave scancode can toggle/close the console just as the non-problematic keyboard layouts.

	// Consider all ASCII keys with grave scancode as conflict (might be too much tho?)
	// https://github.com/ioquake/ioq3/pull/285#issuecomment-292043905
	int keyconflict = keysym->scancode >= 32 && keysym->scancode <= 125; // ~ is 126, so it's already excluded
	if (keysym->scancode == '`') // This matches US keyboards, no conflict
		keyconflict = 0;
	int console_has_text = strlen(g_consoleField.buffer) > 0;
	if (keyconflict && console_has_text)
		return key;
	// Console keys can't be bound or generate characters
	key = K_CONSOLE;
}

@timangus
Copy link
Member

timangus commented Apr 6, 2017

Something something, I told you so.

@mickael9
Copy link
Author

mickael9 commented Apr 6, 2017

@timangus This has nothing to do with SDL though, it's about the inherent problems with the key itself which has other uses in some layouts

@timangus
Copy link
Member

timangus commented Apr 6, 2017

Fair enough. I remember it being problematic when I originally wrote it, but don't recall the specifics.

@mickael9
Copy link
Author

mickael9 commented Apr 6, 2017

Although I just found something that might be a SDL2 bug.
Apparently, dead keys (so ^ on a German keyboard) on X11 never give a valid SDL_Keycode (always equal to SDL_SCANCODE_MASK) but they do issue the correct scancode. I haven't checked if it's the case on other platforms.

@mickael9 mickael9 changed the title Always map SDL_SCANCODE_GRAVE to the console key. Map SDL_SCANCODE_GRAVE to the console key. Apr 6, 2017
Currently, the only way to invoke the console which is both platform and
layout independent is to use Shift+Escape.

This change makes sure the QWERTY '`' key (at the left of the 1 key)
works as a console key and for all keyboard layout by matching against
its scan code (ie physical location).

Since this key can also be mapped to normal characters that could be
useful for text input (eg, '^' on German keyboards), we only interpret
it as a console key if one of those conditions is respected:
 - The resulting key is valid and outside ascii range (0x20-0x7f)
 - The resulting key is '~' or '`'
 - The console input field is empty and no chat message is being written

Additionaly, no modifiers (Shift, Ctrl, Alt, AltGr, Ctrl, Super/Win)
shall be pressed to allow usage of the keys in the Shift/AltGr group and
to allow bypassing the console (Alt works on X11, Windows key must be used
on Windows)

A new `cl_consoleUseScanCode` cvar (default 1) is added to enable the
feature.
@mickael9
Copy link
Author

mickael9 commented Apr 6, 2017

I have updated this PR with changes to avoid conflicts, let me know if this is any better.

@DanielGibson
Copy link

Yeah, X11 doesn't have mapping for dead keys, so the standard mapping based on the scancode is used, see https://bugzilla.libsdl.org/show_bug.cgi?id=2071
But that was an improvement, before that patch dead keys didn't generate any key presses at all.

If you use a German layout without dead keys, you should get the expected ^ keycode.

@mickael9
Copy link
Author

mickael9 commented Apr 6, 2017

@DanielGibson I tested this, using setxkbmap de and in_keyboardDebug 1 and got this on pressing the ^ key:

+ Scancode: 0x35(`) Sym: 0x40000000() KMOD_NUM Q:0x00(0x00)
  Scancode: 0x35(`) Sym: 0x40000000() KMOD_NUM Q:0x00(0x00)

Is that what the "standard mapping for scancodes", is supposed to be? There is no valid keycode at all in there.
That is with SDL 2.0.5.

@DanielGibson
Copy link

DanielGibson commented Apr 7, 2017

hmm weird, I think that back when I tested, I'd get a backtick as sym in that case. Like I would in untranslated US layout.
But getting the same result as you now.
At the very least SDL should give SDL_SCANCODE_TO_KEYCODE(SDL_SCANCODE_GRAVE)

@DanielGibson
Copy link

DanielGibson commented Apr 7, 2017

Ok, I think I found the bug in SDL, will create an issue there later.
(If you feel like patching SDL, it's in SDL_x11keyboard.c X11_UpdateKeymap(): in that switch(keyScancode) { .. } add case SDL_SCANCODE_UNKNOWN: break;, it'll then just keep the default mapping from SDL_GetDefaultKeymap())

The behavior I get in ioquake3 then is (without your patch):

+ Scancode: 0x35(`) Sym: 0x60(`) KMOD_NUM Q:0x60(`)
  Scancode: 0x35(`) Sym: 0x60(`) KMOD_NUM Q:0x60(`)

Pressing that key will open/close the console, as the keycode is like on US keyboard.

I think that will break your patch, especially the if( ... || key == '`' || ...) part, because:
If someone uses a German (with dead keys) layout, they will get key == '`' , and the console will close - which again makes it impossible to type "^" (probably with ^ + Space; remember: even though the scancode->keycode mapping might be a bit broken, text input should still work and provide the ^ as expected)

Just never closing the console if there's any text in the input line should help. People should be able to get used to clear the line (e.g. with Ctrl-C or Arrow-Down) before closing the console with the "Console Key".
Making life for people using an US keyboard a bit harder while making it a lot better for all other layouts should be worth it ;-)

UPDATE: Pro-Tip: to write a backtick in inline code blocks, surround them with two backticks instead of just one, like

``key == '`'``

@DanielGibson
Copy link

Created a bugreport for SDL: https://bugzilla.libsdl.org/show_bug.cgi?id=3624

@mickael9
Copy link
Author

mickael9 commented Apr 7, 2017

I'm not entirely convinced that giving a wrong "default" is better than just giving an unknown keycode (SDL_KEYCODE_UNKNOWN, not SDL_SCANCODE_MASK obviously). While it's entirely possible for an application to generate the default themselves if the key is unknown (using the hacky SDL_GetKeyFromName(SDL_GetScancodeName(scancode)) since SDL doesn't expose the default keymap), there is no way to distinguish a wrong keycode from a good one.

Hopefully there is a way to actually get the right key though?
Running xev, you can see the dead key event just fine (I'm using the ^/¨ dead key on AZERTY here), so there must be some way for SDL to make use of that info, right?

KeyPress event, serial 34, synthetic NO, window 0x4000001,
    root 0xc0, subw 0x0, time 18148303, (160,127), root:(754,421),
    state 0x10, keycode 34 (keysym 0xfe52, dead_circumflex), same_screen YES,
    XLookupString gives 1 bytes: (5e) "^"
    XmbLookupString gives 0 bytes: 
    XFilterEvent returns: True

For comparison, this is what I get with the non-dead ^ key (AltGr-9 on AZERTY):

KeyPress event, serial 34, synthetic NO, window 0x4000001,
    root 0xc0, subw 0x0, time 19482081, (141,131), root:(735,425),
    state 0x90, keycode 18 (keysym 0x5e, asciicircum), same_screen YES,
    XLookupString gives 1 bytes: (5e) "^"
    XmbLookupString gives 1 bytes: (5e) "^"
    XFilterEvent returns: False

The keysym is dead_circumflex instead of asciicircum and XmbLookupString gives a result for both cases while XLookupString doesn't in the first case. Not sure why since I don't understand the mess that is X11.

EDIT: apparently, with a dead keys there are two consecutive KeyPress, where the two LookupString alternate being empty

With or without my patch, German keyboard users won't be able to type ^ by default because cl_consoleKeys will now match ` (perhaps that's what you meant, my patch needs to be fixed so it can work if the user removes ` from cl_consoleKeys)

Nice pro-tip, I used ``` ` ```, didn't known it worked with 2 backticks as well (and apparently you can also put three backticks in a two-backticks string like this, `` ``` ` ``` `` or 4 backticks, like I used to get this)

@DanielGibson
Copy link

It's almost 4 years ago I debugged X11/dead key stuff for SDL, so I don't remember all the details.. but it could be that xev has some information SDL2 doesn't, because SDL2 calls XFilterEvent() and xev probably doesn't.

This is the original patch to SDL2 to get events for dead keys: https://hg.libsdl.org/SDL/rev/713c6a333c33
it basically remembers the values of the event before filtering - which is a bit hacky, because the XFilterEvent() manpage says "If XFilterEvent returns True, then some input method has filtered the event, and the client should discard the event."

Related links discussing this issue for SDL2: https://bugzilla.libsdl.org/show_bug.cgi?id=2071 https://forums.libsdl.org/viewtopic.php?p=38900#38900

Anyway, if you can figure out a way for SDL2 to get the proper keysym (while still supporting dead keys for textinput) that'd be cool and you should mention it at https://bugzilla.libsdl.org/show_bug.cgi?id=3624 :)

Base automatically changed from master to main February 10, 2021 09:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants