#3194 closed enhancement (fixed)
[PATCH] Hotkey status outdated, Hotkey release event and display units to queue without waiting another turn — at Version 21
Reported by: | elexis | Owned by: | wraitii |
---|---|---|---|
Priority: | Should Have | Milestone: | Alpha 22 |
Component: | Core engine | Keywords: | patch |
Cc: | Patch: |
Description (last modified by )
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.)
Change History (31)
comment:1 by , 9 years ago
comment:2 by , 9 years ago
Milestone: | Backlog |
---|---|
Resolution: | → wontfix |
Status: | new → closed |
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 by , 9 years ago
Milestone: | → Backlog |
---|---|
Resolution: | wontfix |
Status: | closed → reopened |
by , 9 years ago
Attachment: | t3194_proof_of_concept.patch added |
---|
comment:4 by , 9 years ago
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 by , 9 years ago
Summary: | Allow queuing units with shift-click without waiting for another turn → Display queued units without waiting for another turn |
---|
by , 9 years ago
Attachment: | t3194_display_units_to_queue_on_same_turn.patch added |
---|
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:8 by , 8 years ago
Keywords: | patch review added |
---|---|
Milestone: | Backlog → Alpha 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.
by , 8 years ago
Attachment: | gui_hotkey_release_event_v1_debug.patch added |
---|
With all debug warnings to investigate the bug when not calling HotkeyInputHandler
.
comment:9 by , 8 years ago
Description: | modified (diff) |
---|
comment:10 by , 8 years ago
Keywords: | rfc added; review removed |
---|
Move tickets from the review queue to the rfc one.
comment:15 by , 7 years ago
Keywords: | patch rfc removed |
---|---|
Milestone: | Work In Progress → Alpha 22 |
Summary: | [PATCH] Display units to queue without waiting for another turn → Display units to queue without waiting for another turn |
comment:16 by , 7 years ago
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.
follow-up: 18 comment:17 by , 7 years ago
To clarify: are you saying there's a problem with the commit or clarifying what it does?
comment:18 by , 7 years ago
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:
- Remove the
HotkeyInputHandler
call and compile - Start a game and select the civic center
- 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.
by , 7 years ago
Attachment: | cleanup_rephrase.patch added |
---|
Rephrase the comment to make it more explicit which variable is outdated.
by , 7 years ago
Attachment: | cleanup_return_handled_only_if_handled.patch added |
---|
We should likely not return IN_HANDLED
if the GUI says it didn't handle the key, needs exploration.
by , 7 years ago
Attachment: | reproduce_bug.patch added |
---|
Reproduces the underlying bug that was addressed by the commit.
by , 7 years ago
Attachment: | reproduce_bug_worse.patch added |
---|
by , 7 years ago
Attachment: | alternative_working_but_seems_wrong.patch added |
---|
Passes blackbox testing, but seems wrong to never send IN_HANDLED
, needs exploration.
by , 7 years ago
Attachment: | alternative.patch added |
---|
Alternative code calling HotkeyInputHandler
from HotkeyInputHandler
when creating SDL_HOTKEYDOWN
from SDL_KEYDOWN
instead.
comment:19 by , 7 years ago
Keywords: | patch review added |
---|---|
Priority: | Nice to Have → Should Have |
Resolution: | fixed |
Status: | closed → reopened |
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 by , 7 years ago
Keywords: | review removed |
---|
Closing as fixed for now as an audit is requested in http://code.wildfiregames.com/rP19028
comment:21 by , 7 years ago
Component: | UI & Simulation → Core engine |
---|---|
Description: | modified (diff) |
Resolution: | → fixed |
Status: | reopened → closed |
Closing as working for now. We'll enjoy the phabricator audit for the time being.
Correction: 5 are queued, but its not displayed before the next turn!