New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix #7904: Don't use a timer for hundredth tick determination #8032
Conversation
What does this resolve? IIRC the hundreth-tick was a UI thing and therefore it's logical to use a timer so that UI things don't speed up. Hundreth-tick effectively became "every 3 seconds". If a non-UI thing is using hundreth-tick, that should perhaps be fixed another way? |
I remember some IRC discussions about certain GUI occasionally having so long updates that they end up executing on every tick. E.g. a large list of towns that takes 5 seconds to sort/render would effectively make the game run at 0.2 fps or similar, since the timer would elapse every tick and trigger a re-sort. |
If the game loop is already slow (because many towns, vehicles, huge map, ...), things get worse as windows are updated more frequently than needed. Most of the intensive updates like sorting are done on hundredth tick to prevent the overload, and using a timer based hundredth tick we get the opposite effect. |
Then that applies to any use of the timer. If it's the code that is triggered by the timer causing a delay, then perhaps reset the timer after the code is completed instead of before, i.e. move it to after the w->OnHundredthTick() loop. |
It's a complex problem formed from the combination of many issues due to recursive haywire fixes applied over time. Ultimately one must decide whether the function is intended to be called in real-time (seconds) or game-time (ticks.) The name of the function implies it is called on a by-ticks basis rather than periodically. This confusion leads to further problems as the function is utilized incorrectly or inappropriately to solve issues it was not designed to handle. In the case mentioned it triggered "heavy lifting" including sorting with no time constraints which should be a queued/threaded process. This lead to a feedback process where the timer's trigger length approached once per call (per-tick). With a correct implementation it should have asymptotically approached zero, but this was limited by the fact the timer executes at most once per tick or per 3000 ms, whichever is greater. That behavior was definitely not intentional as the timer then failed to execute both once per 100 ticks and once per three seconds, instead executing once per some arbitrary period greater than three seconds or 100 ticks. This is ultimately reducible to "cutting haywire" where when you find yourself lifting yourself up by your haywire straps, cutting them could lead to a dangerous fall. |
You mean transform it from a periodic timer to a stand-by timer. Such that it ensures there is minimally a three second delay between any two executions. If I'm not mistaken, This would definitely work in terms of a w->MostOncePerThreeSeconds() call, but may worsen the confusion due to the misnomer. It could also interfere with any code that relies upon OnHundredthTick() calls to have any relationship to ticks. |
My two cents on this:
It would also mean the name of the function aligns with how it works again. However, there is one exception. Isn't there always?
This does depend on the 3 seconds, not the 100 ticks. At least, that is my interpretation of it :) So I think we need to fix that function to still be called every 3 seconds. What do you think @glx22? |
I think you're right. And Lines 230 to 237 in e82333c
|
If the primary purpose of the function is to trigger periodically it should then be replaced with a periodic timer system. Timers could be added/removed using a per-tick mutex FIFO/queue into a sorted list. Each tick the next events would be checked iteratively and events due for execution would be executed and removed from the list: From the replacement for OnHundredthTick(): The timer itself would maintain 'current_time' measured where event iteration begins to ensure everything is synced correctly. I'm not familiar enough with the existing timer implementation; without an existing implementation I would suggest a member notifier/handler template object to store and execute the member function pointer, eliminating the need for over-complicated multiple inheritance. This would make the per-tick-timer implementation a generic abstract which could be applied anywhere. In addition, the add() function could be passed tags or other instance data to define an event in the list as "GUI" or "deferred" and similar. This would allow the timers to be further genericized and categorized into multiple sorted lists automatically. For example the "GUI" type would be defined to be handled on a per-tick basis within a single GUI thread while a "deferred" type may be limited to longer periodicity and handled in a separate thread. |
src/window.cpp
Outdated
@@ -1944,6 +1945,7 @@ void ResetWindowSystem() | |||
static void DecreaseWindowCounters() | |||
{ | |||
if (_scroller_click_timeout != 0) _scroller_click_timeout--; | |||
if (_hundredth_tick_timeout != 0) _hundredth_tick_timeout--; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DecreaseWindowCounters() is called via a GUITimer, so I think OnHundredthTick is called as often as before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, nevermind. If OnHundredthTick() stalls the game, _hundredth_tick_timeout won't continue. So there is still 99 DecreaseWindowCounters-ticks before the next invocation.
0594fb3
to
03dffc4
Compare
Looks good to me! @frosch123 mentioned something on IRC about adding a comment about the behaviour in normal speed, fast-forward, and slow-forward? Might be good, to document what this is intended to do! But I am sure he can fill you in better than I can :D |
|
||
Window *w; | ||
FOR_ALL_WINDOWS_FROM_FRONT(w) { | ||
if (!_network_dedicated && hundredth_tick_timeout == 0) w->OnHundredthTick(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OnHundredthTick() is now called before ProcessScheduledInvalidations().
That looks dangerous to me.
I'm not really sure about how to test this change. But in theory, now, OnHundredthTick() will be called every 100 ticks when a tick is 30 ms or more. Call frequency will be reduced in fast forward as a tick is less than 30 ms in this case, but I don't think it should be an issue.