Mtasa-blue: getKeyState('lshift') returns true for 'rshift'

[ArranTuna]

Describe the bug
getKeyState('lshift') returns true if rshift is pushed down.

This happens for "lshift", "lalt", "lctrl", "rshift", "ralt", "rctrl" as well.

Expected behavior
It should work properly.

Additional context
From https://bugs.mtasa.com/view.php?id=5876

[patrikjuvonen]

I know this sounds like a bad hacky idea but one alternative would be to introduce some argument that checks it more strictly or have a prefix such as real_keynamehere for each of the keys.

I do agree changing Wiki might be the best solution, but an ultimate solution, I'm not so sure.

[qaisjp]

I think we should just fix this

[4O4]

@patrikjuvonen Are you working on this or can I give it a try?

[patrikjuvonen]

@patrikjuvonen Are you working on this or can I give it a try?

Feel free to.

[4O4]

https://github.com/multitheftauto/mtasa-blue/blob/master/Client/core/CKeyBinds.cpp#L26

Table of bindable keys doesn't distinguish between left or right keys, i.e. VK_SHIFT keycode (0x10) is used for lshift and rshift instead of VK_LSHIFT and VK_RSHIFT (0xA0, 0xA1). It's the same for alt and ctrl too.

Do you know if this is intentional? It's kind of suspicious that this was like this for many years already. I believe there must have been a reason behind it.

Also, wouldn't it be better if actual winapi VK_* defines were used here instead of magic numbers?

[patrikjuvonen]

@4O4
I asked around. @sbx320 said it would be better to use the VK_* codes. According to our current knowledge there is no good reason why it would have been intentionally left like that; maybe the developer didn't realize there are more than one of those.

Also @qaisjp noted that lshift and rshift in Lua code means it is already explicitly defined by the scripter, which means it should be okay to _change_ MTA behaviour as it is considered more like a bug than a breaking change in the end.

To add to that, we could have new shift, ctrl and alt keys which would trigger on either corresponding key. For now however I think adding those is optional and not required although it would be arguably faster to write getKeyState('shift') in code than both separately to get the same end result... maybe we even have these already, didn't check...

If someone has accurate info on why these keys behave as they do right now feel free to bring us up to speed but for now we believe it was a mistake originally.

[qaisjp]

To add to that, we could have new shift, ctrl and alt keys which would trigger on either corresponding key. For now however I think adding those is optional and not required although it would be arguably faster to write getKeyState('shift') in code than both separately to get the same end result... maybe we even have these already, didn't check...

sounds like a good idea (for a separate pr) :+1:

esp. for those that want a quick fix.

what does onClientKey report? does it just report shift or does it report lshift/rshift?

[4O4]

Ok, thanks for the clarification. Expect two PRs then soon

[4O4]

One more thing - there are no VK_s for ascii characters a-z, 0-9. Quote from winapi:

/*
 * VK_0 - VK_9 are the same as ASCII '0' - '9' (0x30 - 0x39)
 * 0x40 : unassigned
 * VK_A - VK_Z are the same as ASCII 'A' - 'Z' (0x41 - 0x5A)
 */

So the question is do you prefer to leave hex notation for them, or to replace it with literal char?
For example:

{"0", '0', GTA_KEY_0, DATA_NONE, 0},

vs

{"0", 0x30, GTA_KEY_0, DATA_NONE, 0},

Edit: Third option - add custom #defines somewhere (if so, where?)

[qaisjp]

lit char looks good to me. leave a comment somewhere with // VK_A - VK_Z are the same as ASCII 'A' - 'Z' (0x41 - 0x5A) so the reader doesn't have to search for this info

[4O4]

what does onClientKey report? does it just report shift or does it report lshift/rshift?

Sorry, I've missed this question before somewhow... onClientKey currently does distinguish between lefties and righties. And after the changes I'm experiencing regression here, because alt and ctrl are not reported at all.