Opened 5 years ago

Closed 3 years ago

Last modified 3 months ago

#3194 closed enhancement (fixed)

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

Reported by: elexis Owned by: wraitii
Priority: Should Have Milestone: Alpha 22
Component: Core engine Keywords: patch
Cc: Patch:

Description (last modified by 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.)

Attachments (10)

t3194_proof_of_concept.patch (634 bytes) - added by elexis 5 years ago.
t3194_display_units_to_queue_on_same_turn.patch (630 bytes) - added by elexis 5 years ago.
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).
gui_hotkey_release_event_v1_debug.patch (6.2 KB) - added by elexis 4 years ago.
With all debug warnings to investigate the bug when not calling HotkeyInputHandler.
gui_hotkey_release_event_v1.patch (3.0 KB) - added by elexis 4 years ago.
release candidate
cleanup_rephrase.patch (693 bytes) - added by elexis 3 years ago.
Rephrase the comment to make it more explicit which variable is outdated.
cleanup_return_handled_only_if_handled.patch (1.0 KB) - added by elexis 3 years ago.
We should likely not return IN_HANDLED if the GUI says it didn't handle the key, needs exploration.
reproduce_bug.patch (692 bytes) - added by elexis 3 years ago.
Reproduces the underlying bug that was addressed by the commit.
reproduce_bug_worse.patch (1.0 KB) - added by elexis 3 years ago.
alternative_working_but_seems_wrong.patch (747 bytes) - added by elexis 3 years ago.
Passes blackbox testing, but seems wrong to never send IN_HANDLED, needs exploration.
alternative.patch (2.6 KB) - added by elexis 3 years ago.
Alternative code calling HotkeyInputHandler from HotkeyInputHandler when creating SDL_HOTKEYDOWN from SDL_KEYDOWN instead.

Download all attachments as: .zip

Change History (34)

comment:1 Changed 5 years ago by elexis

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

comment:2 Changed 5 years ago by sanderd17

Milestone: Backlog
Resolution: wontfix
Status: newclosed

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

comment:3 Changed 5 years ago by elexis

Milestone: Backlog
Resolution: wontfix
Status: closedreopened

Changed 5 years ago by elexis

comment:4 Changed 5 years ago by 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.

comment:5 Changed 5 years ago by elexis

Summary: Allow queuing units with shift-click without waiting for another turnDisplay queued units without waiting for another turn

comment:6 Changed 5 years ago by elexis

Same goes for bartering at the market.

Changed 5 years ago by 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).

comment:7 Changed 4 years ago by elexis

In 18416:

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

comment:8 Changed 4 years ago by elexis

Keywords: patch review added
Milestone: BacklogAlpha 21
Summary: Display queued units without waiting for another turn[PATCH] Display units to queue without waiting for another turn

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.

Changed 4 years ago by elexis

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

Changed 4 years ago by elexis

release candidate

comment:9 Changed 4 years ago by elexis

Description: modified (diff)

comment:10 Changed 3 years ago by Itms

Keywords: rfc added; review removed

Move tickets from the review queue to the rfc one.

comment:11 Changed 3 years ago by elexis

Milestone: Alpha 21Alpha 22

Feature freeze in 2 days.

comment:12 Changed 3 years ago by wraitii

I'd say this can be committed.

comment:13 Changed 3 years ago by elexis

Milestone: Alpha 22Work In Progress

Moving to the new WIP milestone.

comment:14 Changed 3 years ago by wraitii

Owner: set to wraitii
Resolution: fixed
Status: reopenedclosed

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

comment:15 Changed 3 years ago by wraitii

Keywords: patch rfc removed
Milestone: Work In ProgressAlpha 22
Summary: [PATCH] Display units to queue without waiting for another turnDisplay units to queue without waiting for another turn

comment:16 Changed 3 years ago by 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.

comment:17 Changed 3 years ago by wraitii

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

comment:18 in reply to:  17 Changed 3 years ago by 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.

Changed 3 years ago by elexis

Attachment: cleanup_rephrase.patch added

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

Changed 3 years ago by elexis

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

Changed 3 years ago by elexis

Attachment: reproduce_bug.patch added

Reproduces the underlying bug that was addressed by the commit.

Changed 3 years ago by elexis

Attachment: reproduce_bug_worse.patch added

Changed 3 years ago by elexis

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

Changed 3 years ago by elexis

Attachment: alternative.patch added

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

comment:19 Changed 3 years ago by elexis

Keywords: patch review added
Priority: Nice to HaveShould Have
Resolution: fixed
Status: closedreopened
Summary: Display units to queue without waiting for another turn[PATCH] Hotkey status outdated, Hotkey release event and display units to queue without waiting another turn

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

comment:20 Changed 3 years ago by elexis

Keywords: review removed

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

comment:21 Changed 3 years ago by elexis

Component: UI & SimulationCore engine
Description: modified (diff)
Resolution: fixed
Status: reopenedclosed

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

comment:22 Changed 4 months ago by 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

comment:23 Changed 3 months ago by 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.

comment:24 Changed 3 months ago by 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

Note: See TracTickets for help on using tickets.