[PATCH] Hotkey status outdated, Hotkey release event and display units to queue without waiting another turn

[elexis]

Sometimes when the game is laggy, it becomes apparent that you need to wait one turn in order to produce a batch of 5 units.

Reproduce in a multiplayer session where one player has paused. The other player won't be able to queue 5, despite pressing the shift-key.

(Or host a multiplayergame and type Engine.SetTurnLength(10000); to the JS console (F9) to produce 10 second long turns.)

[elexis]

Correction: 5 are queued, but its not displayed before the next turn!

[sanderd17]

This is part of the network model to guarantee deterministic execution. The commands are collected, and only executed on the next turn, which has a known time. There's no way you can know which command was first, when two players have a command at about the same time, but it took a while to reach the other players.

Only indirect solutions for this problem are possible, like blocking the gui when someone paused or reducing the lag.

So unless you prove me wrong with a deterministic patch, I'll close this as wontfix

[elexis]

The problem is if you pause the game with one client and then hold the shift key in the other client, it would schedule to queue 5 units on the same turn if you clicked the button, but not display that on that turn. However if you select the CC again while holding the shift key, it updates the GUI on the same turn and displays the correct number of units (five) that would be scheduled to be queued if you clicked the button.

Calling onSimulationUpdate() once in javascript is sufficient to update the GUI and fix the problem. onSimulationUpdate() is not only called when another turn is processed, but also OnTick. For example if one clicks somewhere, OnTick() will call onSimulationUpdate() and thus update the GUI.

The patch would technically fix the issue, but it is not optimized for performance (too many onSimulationUpdate() calls when not necessary). The solution with low performance impact would be to call onSimulationUpdate() once if a building that can produce units is selected and the shift-key state changes.

[elexis]

Same goes for bartering at the market.

[elexis]

This fixes the issue described above with less performance impact. It makes training units generally more responsive. Try pressing shift key many times, you will see that its only updated once per turn without the patch. One more patch to go before the ticket can be closed... (Display the units that will be queued on the next turn if the user started the production).

[elexis]

In 18416:

Improve bartering GUI responsiveness and immediately switch to the selected resource instead of waiting one turn. Refs #3194.

[elexis]

Should have mentioned that it was meant to be about showing the units to be trained, not the ones actually queued (The icon shows the number 5 if shift is pressed, otherwise no number).

The patch above just updates the GUI onTick instead of onSimulationUpdate, thus takes hotkey changes into account immediately.

The patch below updates the GUI only if one of the affected hotkeys is pressed or released, thus saves a relatively significant performance part.

[elexis]

With all debug warnings to investigate the bug when not calling HotkeyInputHandler.

[elexis]

release candidate

[Itms]

Move tickets from the review queue to the rfc one.

[elexis]

Feature freeze in 2 days.

[wraitii]

I'd say this can be committed.

[elexis]

Moving to the new WIP milestone.

[wraitii]

In 19028:

Display the updated batch size immediately when using the batch-train/barter hotkey, instead of waiting for the next turn. Patch by elexis. Fixes #3194

[elexis]

From ​http://irclogs.wildfiregames.com/2016-08-15-QuakeNet-%230ad-dev.log

13:17 < elexis> #3194 can be committed right away
13:17 < elexis> but noone will ever look at that

Thanks for proving me wrong. After this complaint Itms also took a look:

13:42 <@Itms> what does the HotkeyInputHandler part? This comment is not clear IMO
13:50 <@Itms> elexis: if I believe http://trac.wildfiregames.com/browser/ps/trunk/source/ps/GameSetup/GameSetup.cpp#L572 your patch will break chat boxes
14:20 <@Itms> I'm pretty sure one handler should not call the other in any case

It would have been more clear if the patch would have been split in three parts:

1) Weird bugfix 2) New Hotkey Release Event 2) XML use of that event

Explanation of 1: In the prior code, the GUI event was sent immediately from the GUI handler. Then the GUI manager processed that event _before_ the hotkey input handler code was executed that set the flag that this hotkey is pressed. So the pressed event was instantaneously called but when asking from inside the XML/JS event code whether the hotkey was pressed, the GUI manager returned that the key was not pressed (contradiction). Changing the order of the hotkey input handler and GUI input handler completely breaks everything (iirc), so it seems inevitable to either call the handler directly or call some helper function that does the same as the handler.

[wraitii]

To clarify: are you saying there's a problem with the commit or clarifying what it does?

[elexis]

Replying to wraitii:

To clarify: are you saying there's a problem with the commit or clarifying what it does?

It was a clarification responding to the previously unanswered question of Itms. But I mostly didn't commit the patch that passes all blackbox tests because I wasn't convinced myself that it is the best approach.

Itms: what does the HotkeyInputHandler part?

Reproduce of the fixed bug:

1. Remove the HotkeyInputHandler call and compile
2. Start a game and select the civic center
3. Press and release shift

We can observe that it doesn't take the shift key events into account.

The bug diff became noticeable when an event in the XML like <object hotkey="session.batchtrain"> called updateSelectionDetails(); which will call the code in selection_panels.js asking for Engine.HotkeyIsPressed("session.batchtrain") which return false when it should be true (as the button pressed triggered that event).

Anatomy of the bug: The user types a key, SDL creates the SDL_KEYDOWN event, then following the Gamesetup.cpp init order in InitInput(), the gui_handler is called with that event. If the focused GUI object is a textbox, it will add the text input, otherwise call the next event handler, which is the HotkeyInputHandler who creates an SDL_HOTKEYDOWN event from the SDL_KEYDOWN. This new event is then added to the priority queue, thus it will be the first event to be processed right after the SDL_KEYDOWN event. As determined by that handler order, the new SDL_HOTKEYDOWN event will be sent to the GUI handler first before the hotkey handler has a chance to update g_HotkeyStatus with the correct value.

Itms: if I believe ​http://trac.wildfiregames.com/browser/ps/trunk/source/ps/GameSetup/GameSetup.cpp#L572 your patch will break chat boxes

Doesn't break chat boxes because we don't change the init order nor send any hotkey notification to the GUI (using obj->SendEvent) when typing text (except #4400 independently from this ticket and commit).

See attached patches for alternative patches that may or may not be better. Either way it appears that we are forced to sometimes call the Hotkey handler before the GUI handler (see bug description) and sometimes call the GUI handler before the hotkey handler (see Gamesetup.cpp description). So if my understanding of the code is right, we either have to call HotkeyInputHandler when constructing the SDL_HOTKEYDOWN and SDL_HOTKEYUP event inside the HotkeyInputHandler function, or call it from CGUI::HandleEvent before processing it, chose your poison.

[elexis]

Rephrase the comment to make it more explicit which variable is outdated.

[elexis]

We should likely not return IN_HANDLED if the GUI says it didn't handle the key, needs exploration.

[elexis]

Reproduces the underlying bug that was addressed by the commit.

[elexis]

Passes blackbox testing, but seems wrong to never send IN_HANDLED, needs exploration.

[elexis]

Alternative code calling HotkeyInputHandler from HotkeyInputHandler when creating SDL_HOTKEYDOWN from SDL_KEYDOWN instead.

[elexis]

Requesting a review for the commit, cleanup and alternative patches, just to be sure.

[elexis]

Closing as fixed for now as an audit is requested in ​http://code.wildfiregames.com/rP19028

[elexis]

Closing as working for now. We'll enjoy the phabricator audit for the time being.

[elexis]

In r22909 by wraitii:

Fix hotkey events synching with hotkey state.

This is a semi-revert of r19028 and a correct fix for #3194.

The core issue is that the GUI handler must come behore the hotkey handler, otherwise typing in boxes can set off hotkeys, and the hotkey handler is repsonsible for updating the hotkey state. Thus the GUI handler never has an up-to-date hotkey state, since that's done later. r19028 fixed that by calling HotkeyInputHandler manually, but that was still broken in some (unused) cases and was hacky (indeed, it even looked hacky as noted by elexis).

The simplest fix is to split the 'hotkey creator' handler from the 'hotkey state change' handler, and run the 'hotkey state change handler' before any other handler. Thus the gui handler remains in front of the 'hotkey creator' handler, but it has a correct hotkey state at any time.

Differential Revision: ​​https://code.wildfiregames.com/D1839

[elexis]

In 23065:

Refactor diplomacy dialog to use object orientation paradigm using the class keyword, refs #5387.

Includes dilomacy colors, tribute buttons, spy request buttons, attack request buttons, minimap panel, idle worker button, ceasefire counter.
Differential Revision: ​https://code.wildfiregames.com/D2365

Use hotkey release event from rP19028 to remove input.js state and evil global nested closure hackery from rP13201, refs #1839, #3194.
Fixes masstribute selecitonchange issue subject of D1191.

[elexis]

In 23081:

Rewrite developer overlay to use class syntax, one class per checkbox, a class for the EntityState overlay and TimeWarp debug feature, refs #5387.

Using 22 classes instead of 1 class (refs rP22370/D1928) leverages more benefit of the paradigm.
In particular it means the checkboxes can own the EntityState and TimeWarp helpers and the DeveloperOverlay itself can remain independent.
Improve performance by 200 microseconds per turn by unsubscribing from onSimulationUpdate when the developer overlay is not opened, refs rP23076 / D2378.
Move TimeWarp from input.js from rP8803 to independent class using hotkey release event from rP19028, refs #3194.

Differential Revision: ​https://code.wildfiregames.com/D2383