Page MenuHomeWildfire Games

Fix shiftlag and implement HotkeyDown event, change HotkeyPress event to be non-repeating
Needs ReviewPublic

Authored by bb on Mar 17 2018, 6:59 PM.

Details

Reviewers
elexis
vladislavbelov
Trac Tickets
#5055
Summary

Currently the onPress hotkey action for gui elements are called repeatedly when holding a hotkey (except for special keys on linux due to #4915). However in most cases only the first press is relevant and we can save perfomance with only executing on the first such message. massbarter and batchtrain are examples of such. Some other hotkey (like tab scrolling and idle unit) still need the repeated events and get so.

Test Plan

Give warnings on the different events for different hotkeys
Make sure all hotkeys still work correctly and have the correct behaviour
This should fix the performance drain on windows when holding shift.
yell at linux for not repeating special keys

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
elexis added inline comments.Mar 20 2018, 1:10 PM
binaries/data/mods/public/gui/common/tab_buttons.xml
8

I doubt anyone will set the keyboard frequency to adjust it to the tabs in 0ad.
A page with 100 tabs seems unlikely, more like 10 max. In every of these cases it's easier for the user to press 5 times as the distance is known in advance rather than relying on luck to hit it. (There is the delay after the first press, still don't see the reason why it would change to Keydown)

binaries/data/mods/public/gui/session/hotkeys/misc.xml
55–67

Can you give an example of that stop-units-in-battle use case? To me it seems like needlessly spamming simulation commands.

bb added inline comments.Mar 20 2018, 5:29 PM
binaries/data/mods/public/gui/common/tab_buttons.xml
8

unlikely != impossible, so why not support the hypothetical case? (also see the revision to merge horizontal tabs in this file, and there could be many of those next too eachother)
With 10 button even some ppl will still go the long way pressing 9 times (not everyone sees that the back route is shorter at first sight)

binaries/data/mods/public/gui/session/hotkeys/misc.xml
55–67

Have units with a cc just in vision range, so the cc doesn't shoot at the units, but the units will run at the cc when idle, probably a player doesn't want to run the units in (but wait for an ally to join attack) and he can't move back for whatever reason. The player could hold its units now in place by holding "h".

source/ps/Hotkey.cpp
312

hmm yes, the phantom one above is correct then however as tht is a custom KEYDOWN

bb updated this revision to Diff 6236.Mar 20 2018, 5:29 PM

Is this casting correct?

Build failure - The Moirai have given mortals hearts that can endure.

Linter detected issues:
Executing section Default...
Executing section Source...
Executing section JS...

binaries/data/mods/public/gui/session/unit_actions.js
| 557| »   »   »   switch·(tradingDetails.type)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.

Link to build: https://jenkins.wildfiregames.com/job/differential/287/display/redirect

Perhaps @Imarok or anyone else has an opinion on when the GUI should use Keydown and when Press.

binaries/data/mods/public/gui/session/unit_actions.js
1173 ↗(On Diff #6199)

(If half of the hotkeys use Keydown, then we don't need the additional advertizement here, idc though. Feature implementation can be described in tickets or be avoided by actual implementation)

source/gui/CGUI.cpp
86

since the linux keyboard event manager sends the KEYDOWN only once.

This statement can't be verified or corrected by only considering our code.
It's a complex issue, how do we know it's not some SDL bug?
Keyboard event manager is the "Linux Input Subsystem userspace API"?
So should the bugreport or interpretation be removed from the code?
(On the other hand, idc)

86–88

(\n?)

87

(The int conversion is needed because data2 is a void pointer and those should have a type before they are compared to an int?)
(Also repeat is an Uint8.)

source/ps/Hotkey.cpp
219

The consequent statement reads from a property that isn't defined in all cases where the condition is true.

220

Imarok and we two have already wondered about the number. The specs https://wiki.libsdl.org/SDL_KeyboardEvent say:

non-zero if this is a key repeat

So I see a possible way to write this line a bit closer to the specs (s/int/bool).

The next bullet point to be examined is the completeness (cases in which the property is set).

Third To Consider (TOCO?) is this line:

// Just send them to this handler; don't let the imaginary event codes leak back to real SDL.

312

The new line of code is good.

"custom KEYDOWN" means SDL_HOTKEYDOWN? (Can you reprhrase or elaborate, I didn't get the sentence)

Either way, when reading from or assigning a property we have to lookup if that property exists at all. Compiler complains in most cases, but not this one by the looks of things.

(As long as SDL_HOTKEYDOWN is a https://wiki.libsdl.org/SDL_UserEvent it doesn't have a data field called repeat.)

Depending on the existence of other input devices that trigger a different SDL events that also have a repeat flag, this line below here might have a duplicate condition the above new line.

bb marked an inline comment as done.Mar 21 2018, 10:25 PM
bb added inline comments.
source/gui/CGUI.cpp
86
87

not needed indeed

source/ps/Hotkey.cpp
312

don't like that compiler warning however.

(KEYDOWN stuff, nvm )

bb updated this revision to Diff 6251.Mar 21 2018, 10:26 PM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/differential/298/display/redirect

bb updated this revision to Diff 6252.Mar 21 2018, 10:56 PM

Make a bool in SDL_HOTKEYDOWN

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/differential/299/display/redirect

elexis accepted this revision.Mar 22 2018, 3:44 PM

Checking the completeness test quickly:

typedef union SDL_Event
{
    Uint32 type;                    /**< Event type, shared with all events */
    SDL_CommonEvent common;         /**< Common event data */
    SDL_WindowEvent window;         /**< Window event data */
    SDL_KeyboardEvent key;          /**< Keyboard event data */
    SDL_TextEditingEvent edit;      /**< Text editing event data */
    SDL_TextInputEvent text;        /**< Text input event data */
    SDL_MouseMotionEvent motion;    /**< Mouse motion event data */
    SDL_MouseButtonEvent button;    /**< Mouse button event data */
    SDL_MouseWheelEvent wheel;      /**< Mouse wheel event data */
    SDL_JoyAxisEvent jaxis;         /**< Joystick axis event data */
    SDL_JoyBallEvent jball;         /**< Joystick ball event data */
    SDL_JoyHatEvent jhat;           /**< Joystick hat event data */
    SDL_JoyButtonEvent jbutton;     /**< Joystick button event data */
    SDL_JoyDeviceEvent jdevice;     /**< Joystick device change event data */
    SDL_ControllerAxisEvent caxis;      /**< Game Controller axis event data */
    SDL_ControllerButtonEvent cbutton;  /**< Game Controller button event data */
    SDL_ControllerDeviceEvent cdevice;  /**< Game Controller device event data */
    SDL_AudioDeviceEvent adevice;   /**< Audio device event data */
    SDL_QuitEvent quit;             /**< Quit request event data */
    SDL_UserEvent user;             /**< Custom event data */
    SDL_SysWMEvent syswm;           /**< System dependent window event data */
    SDL_TouchFingerEvent tfinger;   /**< Touch finger event data */
    SDL_MultiGestureEvent mgesture; /**< Gesture event data */
    SDL_DollarGestureEvent dgesture; /**< Gesture event data */
    SDL_DropEvent drop;             /**< Drag and drop event data */

    /* This is necessary for ABI compatibility between Visual C++ and GCC
       Visual C++ will respect the push pack pragma and use 52 bytes for
       this structure, and GCC will use the alignment of the largest datatype
       within the union, which is 8 bytes.

       So... we'll add padding to force the size to be 56 bytes for both.
    */
    Uint8 padding[56];
} SDL_Event;

This doesn't have repeat https://wiki.libsdl.org/SDL_JoyButtonEvent
This neither https://wiki.libsdl.org/SDL_ControllerButtonEvent
The other ones neither or aren't input events at all, so it's complete.

All occurrences of SDL_KEYDOWN in the context of hotkeys are handled correctly.

The JS / XML changes are not so relevant, only the shift-lag ones.
The ones in other GUI pages can be assumed to use Press by default which they do in the current svn.

I didn't test.

source/gui/CGUI.cpp
87

I actually don't know. Maybe clang or some other compiler complains. Should do what similar code does.

source/ps/Hotkey.cpp
219

This is better. thanks.

220

SDL likely passes an u8 range here because it can only pass entire bytes, not a single bit, likely due to the platforms it can work on.
But value ranges should be restricted as far as possible. Propagating it by itself isn't good if the propagated value is nonideal.
Readers of the line below might assume that repeat contains something more than a boolean, like you me and Imarok already did before the specs were looked up.
(So use byte or bool, whatever, but I declare myself unconvinced for now)
(Compiler warning should be addressable.)

source/ps/Hotkey.h
30

Ok (We could also have a HOTKEYDOWN and HOTKEYPRESS event, thoug no benefit right now)

This revision is now accepted and ready to land.Mar 22 2018, 3:44 PM
vladislavbelov requested changes to this revision.Mar 22 2018, 7:40 PM
vladislavbelov added a subscriber: vladislavbelov.
vladislavbelov added inline comments.
source/ps/Hotkey.cpp
312

static_cast. Also write here a comment, why and how do use this hack.

This revision now requires changes to proceed.Mar 22 2018, 7:40 PM
bb added inline comments.Mar 23 2018, 12:04 AM
source/ps/Hotkey.cpp
220

phantom.ev.key is a SDL_KeyEvent thus the repeat flag is a u8, and as the phantom will be rerun through this function the code below expects that, so seeing no other solution than keeping the u8.

source/ps/Hotkey.h
30

Well one benefit: we don't need that casting hell below

bb added inline comments.Mar 23 2018, 12:23 AM
source/ps/Hotkey.h
30

Actually found another benefit (and more arguable): main.cpp has some hotkeys too (screenshots, exit etc.) which could use the press event.

bb updated this revision to Diff 6261.EditedMar 23 2018, 12:42 AM

Make SDL_HOTKEYPRESS event, so also hotkeys defined in cpp can benefit

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/differential/305/display/redirect

bb updated this revision to Diff 7755.Apr 15 2019, 10:06 PM

years

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/gui/CGUI.cpp
|1678| »   m_ScrollBarStyles[name]·=·scrollbar;
|    | [MAJOR] CPPCheckBear (uninitStructMember):
|    | Uninitialized struct member: scrollbar.m_ScrollWheel

source/gui/CGUI.cpp
|1678| »   m_ScrollBarStyles[name]·=·scrollbar;
|    | [MAJOR] CPPCheckBear (uninitStructMember):
|    | Uninitialized struct member: scrollbar.m_ScrollSpeed

source/gui/CGUI.cpp
|1678| »   m_ScrollBarStyles[name]·=·scrollbar;
|    | [MAJOR] CPPCheckBear (uninitStructMember):
|    | Uninitialized struct member: scrollbar.m_ScrollButtons
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/differential/1218/display/redirect

bb updated this revision to Diff 8945.Jul 17 2019, 1:14 AM

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/ps/Hotkey.h
|  64| The line belonging to the following result cannot be printed because it refers to a line that doesn't seem to exist in the given file.
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCStrW:' is invalid C code. Use --std or --language to configure the language.

source/ps/Hotkey.h
|  64| The line belonging to the following result cannot be printed because it refers to a line that doesn't seem to exist in the given file.
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCStr:' is invalid C code. Use --std or --language to configure the language.

source/main.cpp
|   0| }
|    | [NORMAL] CPPCheckBear (toomanyconfigs):
|    | Too many #ifdef configurations - cppcheck only checks 12 configurations. Use --force to check all configurations. For more details, use --enable=information.

source/gui/CGUI.cpp
|1680| »   m_ScrollBarStyles[name]·=·scrollbar;
|    | [MAJOR] CPPCheckBear (uninitStructMember):
|    | Uninitialized struct member: scrollbar.m_ScrollWheel

source/gui/CGUI.cpp
|1680| »   m_ScrollBarStyles[name]·=·scrollbar;
|    | [MAJOR] CPPCheckBear (uninitStructMember):
|    | Uninitialized struct member: scrollbar.m_ScrollSpeed

source/gui/CGUI.cpp
|1680| »   m_ScrollBarStyles[name]·=·scrollbar;
|    | [MAJOR] CPPCheckBear (uninitStructMember):
|    | Uninitialized struct member: scrollbar.m_ScrollButtons
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/109/display/redirect

bb updated this revision to Diff 9539.Aug 30 2019, 7:18 PM
Owners added a subscriber: Restricted Owners Package.Aug 30 2019, 7:18 PM

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/ps/Hotkey.h
|  64| The line belonging to the following result cannot be printed because it refers to a line that doesn't seem to exist in the given file.
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCStrW:' is invalid C code. Use --std or --language to configure the language.

source/ps/Hotkey.h
|  64| The line belonging to the following result cannot be printed because it refers to a line that doesn't seem to exist in the given file.
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCStr:' is invalid C code. Use --std or --language to configure the language.

source/main.cpp
|   0| }
|    | [NORMAL] CPPCheckBear (toomanyconfigs):
|    | Too many #ifdef configurations - cppcheck only checks 12 configurations. Use --force to check all configurations. For more details, use --enable=information.
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/516/display/redirect

Stan added inline comments.Aug 30 2019, 8:30 PM
source/gui/CGUI.cpp
87

Do we use SFML as well I thought we only used SDL ?

source/ps/CConsole.cpp
641

static_cast

bb added inline comments.Aug 30 2019, 8:32 PM
source/gui/CGUI.cpp
87

we use SDL indeed, but this is not an SDL issue, it most likely is a linux kernel issue

bb updated this revision to Diff 9628.Sep 4 2019, 8:05 PM
Vulcan added a comment.Sep 4 2019, 8:06 PM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/72/display/redirect

Vulcan added a comment.Sep 4 2019, 8:13 PM

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/ps/Hotkey.h
|  64| The line belonging to the following result cannot be printed because it refers to a line that doesn't seem to exist in the given file.
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCStrW:' is invalid C code. Use --std or --language to configure the language.

source/ps/Hotkey.h
|  64| The line belonging to the following result cannot be printed because it refers to a line that doesn't seem to exist in the given file.
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCStr:' is invalid C code. Use --std or --language to configure the language.

source/main.cpp
|   0| }
|    | [NORMAL] CPPCheckBear (toomanyconfigs):
|    | Too many #ifdef configurations - cppcheck only checks 12 configurations. Use --force to check all configurations. For more details, use --enable=information.
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/581/display/redirect

elexis retitled this revision from Implement a press action to be called upon the first keyDown message of a hotkey and use keydown for current press to Fix shiftlag and implement HotkeyDown event, change HotkeyPress event to be non-repeating.Sep 4 2019, 10:03 PM
Stan added inline comments.Sep 4 2019, 11:17 PM
source/ps/Hotkey.cpp
317

Helper function ?

What I wrote about a year ago but didn't submit:

So first we the evaluation of the design:

We have the choice between:
1. Implementing the Keydown event
2. Implementing the Keypress event
3. Implementing both

Currently 1. is implemented, and it's problematic because on some platforms the event can repeat often, triggering more JS code than the user intended to and that can even trigger a brief rendering freeze.
3. is proposed now, perhaps that has the disadvantage over 2. (keypress only) that editors still have the freedom to repeat the mistake of 1.
So 3. should only be implemented if there is one use case imagineable where OnHotkeyPress is definitely insufficient.
Or both could be implemented, while all of 0 A.D. XML files and examples use Press, so that the copy-paste hotkey editors will not chose hotkeydown due to Wildfire Games examples.

For the currently proposed hotkey changes:
Using hotkeydown for entity actions seems like a 50% solution. The keyrepeat rate is platform dependent, so if the use case is to change the entity selection while keeping the key pressed, the better solution would probably be to test the hotkey state during every tick. But perhaps it would be more consistent to just use OnPress and have the player press the button N times to perform N actions? When I think about situations where the specific hotkeys are to be pressed very urgently, the user typically buttonsmashes, since there is this one second delay (or however long it is on the different platforms) until the repeatrate becomes active. And typically the specific hotkey action smashing isn't endured for longer than one second (until either no hotkey is pressed or until a different hotkey is pressed).

Cycling through entities (currently only implemented for idle units) however is a solid use case for the HotkeyDown event.

One could also not change the C++ code at (1.) and change the JS/XML code so that the slow barter hotkey code is not performed at all unless the barter dialog is visible.
Perhaps one can reassign other Shift hotkeys to GUI objects that aren't in focus when the barter dialog is active, but that sounds likely to run into conflicts.

Regardless, `session.massbarter` should probably

then I apparently lost motivation.

See IRClogs from August 30:
http://irclogs.wildfiregames.com/2019-08/2019-08-30-QuakeNet-%230ad-dev.log

For the question when to use HotkeyDown and when to use HotkeyPress, the following rule of thumb seemed most sensible:

  • hotkey events that cause a Toggle or PushPage / ClosePage command inherently are not meant to be repeated, so these must use a HotkeyPress event. These are most events.
  • unit actions that depends on the selection should whatever choice they do consistently. So either all of them use HotkeyDown or all of them use HotkeyPress, so that the user isnt left clueless why there are behavior differences. We argued that the selection can change during the course of holding the key combination, and that thus we chose HotkeyDown for these few events. Perhaps we can add an <!-- XML Comment --> manifesting that, so that new hotkey are added in consistency with this decision?

While testing I found the following:

  • Scrolling with the mousewheel ingame to zoom in doesnt work anymore
binaries/data/mods/public/gui/common/tab_buttons.xml
9

An argument for using Press woould be that keeping it pressed means it switches tabs uncontrollably (unpredictable tab selection) as long as it doesn't stop at the first or last tab. And we can assume < 10 tabs, so there is no use case where one wants to scroll many tabs forwards quickly.

binaries/data/mods/public/gui/session/hotkeys/misc.xml
56

A reason for Press is to avoid accidental deletes. But I guess who presses shift-delete and changes selection afterwards is to be served what he ordered.
A reason for Keydown being able to delete many units in different places quickly (ragequit use case?), more importantly session unit action consistency.

binaries/data/mods/public/gui/session/hotkeys/training.xml
2

Suspicious (because the user should not queue more units than he intended to), but possible (because stupid deathmatch spam games).

binaries/data/mods/public/gui/session/minimap_panel.xml
30 ↗(On Diff #6261)

yes

binaries/data/mods/public/gui/summary/summary.xml
25

<include tab_buttons.xml>

source/ps/Hotkey.cpp
328

Maybe this is a bit easier? Something like this?
(The linux comment should be on a separate line since its distinct information)

		// Send a KeyPress event when the key, mouse button or mousewheel was pressed initially
		if (ev->ev.type != SDL_KEYDOWN || ev->ev.key.repeat == 0)

		// Send a HotkeyDown event while the key combination is held.
		// The event is repeated depending on hardware and OS configured interval.
		// On linux, modifier keys (shift, alt, ctrl) are not repeated, see https://github.com/SFML/SFML/issues/122.
		if (ev->ev.type == SDL_KEYDOWN)
328

There are three functions that run these 4 lines of code.
So I wonder if a in_push_priority_hotkey_event with the enum and closestMapNames[i] argument wouldn't hurt. (not so important, can be omitted)

source/ps/ProfileViewer.cpp
338

yes

bb marked an inline comment as done.Sep 5 2019, 1:41 PM
bb added inline comments.
binaries/data/mods/public/gui/common/tab_buttons.xml
9

One can't assume <10. A mod could very well have a use case for many more tabs, hence the scrolling can be useful

binaries/data/mods/public/gui/summary/summary.xml
25

see D1206

bb updated this revision to Diff 9690.Sep 9 2019, 8:28 PM

Notice that mouseWheel and mouseButton events are never repeated (at least that is what testing says here)

Vulcan added a comment.Sep 9 2019, 8:29 PM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/117/display/redirect

Vulcan added a comment.Sep 9 2019, 8:36 PM

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/ps/Hotkey.h
|  64| The line belonging to the following result cannot be printed because it refers to a line that doesn't seem to exist in the given file.
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCStrW:' is invalid C code. Use --std or --language to configure the language.

source/ps/Hotkey.h
|  64| The line belonging to the following result cannot be printed because it refers to a line that doesn't seem to exist in the given file.
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCStr:' is invalid C code. Use --std or --language to configure the language.

source/main.cpp
|   0| }
|    | [NORMAL] CPPCheckBear (toomanyconfigs):
|    | Too many #ifdef configurations - cppcheck only checks 12 configurations. Use --force to check all configurations. For more details, use --enable=information.
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/626/display/redirect

bb updated this revision to Diff 10140.Oct 12 2019, 11:06 PM

Rebase

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/960/display/redirect

bb added inline comments.Oct 12 2019, 11:08 PM
source/ps/Hotkey.cpp
157

One could notice that this change actually doesn't do anything since a HOTKEYDOWN is always followed after a HOTKEYPRESS and stops at HOTKEUYP

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/445/display/redirect

bb updated this revision to Diff 10141.Oct 12 2019, 11:11 PM

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/961/display/redirect

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/446/display/redirect

Stan added inline comments.Oct 12 2019, 11:25 PM
source/graphics/CameraController.cpp
644

Could invert that to make it early return :)

source/ps/CConsole.cpp
641

Still missing :)

source/ps/Hotkey.h
43

Do they need to be signed ints ?