[Shared] Key modifiers

[Daggolin]

Introduce modifiers for key binds:

• RSHIFT, LCTRL, RCTRL, LALT, RALT, LSUPER and RSUPER are usable
• CAPSLOCK and NUMLOCK are toggleable modifiers
• modifiers can be combined (for example: LCTRL+LALT+LSHIFT)
• the bind completion supports modifiers
• SP only: the menu supports modifiers
• cvars:
  • cl_keyModifiersOnlyIfUnbound (default: 1): if a modifier button is bound to another action, for example SHIFT as +speed, the corresponding modifier is not applied (for example if SHIFT is bound the LSHIFT and RSHIFT modifiers are not usable)
  • cl_keyModifiersFallbackToDefault (default: 1) if a button is not bound for the current modifiers the default bind for the button is used
  • cl_keyModifiersEnableLocks (default: 1) allow the use of the CAPSLOCK and NUMLOCK modifiers
  • cl_keyModifiersPassToUI (default: 0; SP only): pass modifiers along to the menu; when enabled combinations like LCTRL+1 can be bound in the menu, but the buttons can't be bound directly anymore (CTRL, SHIFT ALT, CAPSLOCK, KP_NUMLOCK)

The defaults for the cvars were chosen to work with existing configs:

• as modifiers are only applied when not bound (cl_keyModifiersOnlyIfUnbound 1) the default binds of the game should work
• as keys without a bind for the current modifier fallback to the default bind (cl_keyModifiersFallbackToDefault 1) any accidently activated modifiers (NUMLOCK, CAPSLOCK, holding a modifier button, ...) should not interfere with regular binds and cl_keyModifiersEnableLocks can default to 1
• as modifiers are not passed to the menu (cl_keyModifiersPassToUI 0) the regular CTRL, ALT, SHIFT, CAPSLOCK and KP_NUMLOCK keys can be bound via the menu

As menu support for the modifiers requires API changes and as the MP modules currently don't support an extensible API the modifiers are only supported via the console in MP. The menus can only access the default binds.

[taysta]

I think if this feature is being added to openjk, it should remain backwards compatible to the implementations that have existed for years already on forks of openjk, so that users aren't left wondering why their cfgs aren't working on openjk.

All it would require is aliasing +ALT, +SHIFT, +CTRL to +LALT, +LSHIFT, +LCTRL.

I know Daggo already suggested to just do this downstream on my fork, I think this is unacceptable, as the entire user base already used to using modifier keys will have configs incompatible with this PR.

If you are going to attempt to write a "better version" of my PR, it should at least be compatible with my PR.

It's a hostile design decision to require these users to have different configs for different clients, and certainly does not help openjk's reputation among these users.

[Daggolin]

I think if this feature is being added to openjk, it should remain backwards compatible to the implementations that have existed for years already on forks of openjk, so that users aren't left wondering why their cfgs aren't working on openjk.

There is no backwards compatibility to maintain on OpenJK, because it doesn't support any other variations of modifiers. The other clients also are not config compatible to OpenJK either way (different name, different meaning for the same cvars, etc.) you couldn't just carry the config over from those forks to OpenJK in the past. This implementation wouldn't change the situation either way.

All it would require is aliasing +ALT, +SHIFT, +CTRL to +LALT, +LSHIFT, +LCTRL.

Aliasing the non L/R variation of each button to the L variation prevents OpenJK from possibly setting binds for both sides when specifying the generic name in the future. The aliasing might match your fork's behavior and it's probably something you should do on TaystJK to load its old configs.

I know Daggo already suggested to just do this downstream on my fork, I think this is unacceptable, as the entire user base already used to using modifier keys will have configs incompatible with this PR.

I am sorry, but I do not understand what you consider unacceptable about adding those aliases downstream to your fork. By adding them to TaystJK your users can still use their old configs. Based on how you implement this your clients could read the aliases of your fork's config and write them back to the file in the new L/R format, making those binds compatible/portable between OpenJK and TaystJK going forward.

If you are going to attempt to write a "better version" of my PR, it should at least be compatible with my PR.

My pull request is intended to be a different approach. When we talked about it before this implementation, I said I would write one without reading yours first and only compare them afterwards, I personally thought you didn't seem to mind that during our talk and I had no intention to undermine your work. OpenJK doesn't have either of them merged into master, so there is no need for both pull requests to follow the same concepts or chosen values. I am sorry if this one is going to require a bit more effort downstream for you. I am going to attach a diff at the bottom that you can use to add those aliases to your fork to keep reading your old format but writing a format compatible with this pull request.

It's a hostile design decision to require these users to have different configs for different clients, and certainly does not help openjk's reputation among these users.

I am sorry if you feel this way, but this doesn't really add up in my opinion. In the past those forks have not maintained config compatibility with OpenJK, changing the meaning of cvars, adding new bind combinations and possibly more. The config compatibility never existed and a pull request to OpenJK could have corrected this. I would argue that any contributor to OpenJK should also not be tasked to chase after forks or their configs. Going forward, if you truly care about compatible binds, I request that you refer to the previously mentioned diff attached below. Again, I apologize for any misconceptions this may have caused.

The alias code diff:

diff --git a/code/client/cl_keys.cpp b/code/client/cl_keys.cpp
index 21423b89..c6ead1f7 100644
--- a/code/client/cl_keys.cpp
+++ b/code/client/cl_keys.cpp
@@ -946,6 +946,11 @@ static stringID_table_t keyModifiers[] {
        { "LSUPER",       KEYMOD_LSUPER       },
        { "RSUPER",       KEYMOD_RSUPER       },
 
+       // Aliases for old TaystJK clients
+       { "SHIFT",        KEYMOD_LSHIFT       },
+       { "CTRL",         KEYMOD_LCTRL        },
+       { "ALT",          KEYMOD_LALT         },
+
        // LOCK modifiers
        { "NUMLOCK",      KEYMOD_NUMLOCK      },
        { "CAPSLOCK",     KEYMOD_CAPSLOCK     },
@@ -1006,6 +1011,7 @@ char *Key_ModifiersStringFromModifiers( int modifiers ) {
                if ( modifiers & keyModifiers[i].id ) {
                        Q_strcat( modifiersStr, sizeof(modifiersStr), keyModifiers[i].name );
                        Q_strcat( modifiersStr, sizeof(modifiersStr), "+" );
+                       modifiers &= ~keyModifiers[i].id;
                }
        }
        return modifiersStr;
diff --git a/codemp/client/cl_keys.cpp b/codemp/client/cl_keys.cpp
index 0a8ee5df..a0b71ec6 100644
--- a/codemp/client/cl_keys.cpp
+++ b/codemp/client/cl_keys.cpp
@@ -1004,6 +1004,11 @@ static stringID_table_t keyModifiers[] {
        { "LSUPER",       KEYMOD_LSUPER       },
        { "RSUPER",       KEYMOD_RSUPER       },
 
+       // Aliases for old TaystJK clients
+       { "SHIFT",        KEYMOD_LSHIFT       },
+       { "CTRL",         KEYMOD_LCTRL        },
+       { "ALT",          KEYMOD_LALT         },
+
        // LOCK modifiers
        { "NUMLOCK",      KEYMOD_NUMLOCK      },
        { "CAPSLOCK",     KEYMOD_CAPSLOCK     },
@@ -1064,6 +1069,7 @@ char *Key_ModifiersStringFromModifiers( int modifiers ) {
                if ( modifiers & keyModifiers[i].id ) {
                        Q_strcat( modifiersStr, sizeof(modifiersStr), keyModifiers[i].name );
                        Q_strcat( modifiersStr, sizeof(modifiersStr), "+" );
+                       modifiers &= ~keyModifiers[i].id;
                }
        }
        return modifiersStr;

[taysta]

I don't think this pull request should be merged without the patch or some other approach addressing the issue, the entire ctf/seige community using newjk, and all taystjk users have been using different modifier keywords than what this pull request proposes for many years now, as was ported in my pull request preceding this one.

It doesn't make sense to require forks to make changes when they have already set the standard for how this feature looks in a config.

No one is asking you to double check every new feature against all existing forks, there is no need to check, as I have already made you aware multiple times, and you could have simply read my pull request to make yourself aware. It's just negligence by choice.

Someone wanting to use their cfg files between engines shouldn't have to rename every modifier in the config, it's a very questionable design choice to quite literally force this, especially when you already have the code fixing the problem written.