Skip to content
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

Merged
merged 2 commits into from Jan 10, 2021

Conversation

glx22
Copy link
Contributor

@glx22 glx22 commented Mar 7, 2020

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.

@LordAro LordAro requested a review from PeterN March 24, 2020 21:38
@PeterN
Copy link
Member

PeterN commented Mar 30, 2020

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?

@nielsmh
Copy link
Contributor

nielsmh commented Mar 30, 2020

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.

@glx22
Copy link
Contributor Author

glx22 commented Mar 30, 2020

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.

@PeterN
Copy link
Member

PeterN commented Mar 30, 2020

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.

@floodious
Copy link

floodious commented Apr 7, 2020

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.

@floodious
Copy link

floodious commented Apr 7, 2020

perhaps reset the timer after the code is completed instead of before, i.e. move it to after the w->OnHundredthTick() loop.

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, with such a stand-by timer the failure case described above would remain identical with the function being called once per game tick. I am mistaken: obviously it would provide a span of three seconds during which UpdateWindows() calls would not execute w->OnHundredthTick() and prevent a runaway feedback process... but the OnHundredthTick() call would still have no time constraints and would execute at most once per tick if a tick took 3000 ms or more due to other calls.

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.

@TrueBrain TrueBrain added candidate: probably not This Pull Request will most likely be closed soon size: small This Pull Request is small, and should be relative easy to process candidate: most likely This Pull Request is probably going to be accepted and removed candidate: probably not This Pull Request will most likely be closed soon labels Dec 14, 2020
@TrueBrain
Copy link
Member

My two cents on this:

OnHundredthTick mostly just invalidates windows "once in a while" to keep CPU load away. For that, this PR seems sensible, as it scales with the general slowdown of the game. The game normally runs on 30fps, and in that case this function is called every 3 seconds. Now if the game runs on 15fps, it would make sense that it is called every 6 seconds. I can completely understand that it would stop the downwards spiral of game slowdown. And most (and yes, most) usage of this function will be completely fine with that, as they are meant to just be called "when-ever you see fit".

It would also mean the name of the function aligns with how it works again.

However, there is one exception. Isn't there always? error_gui.cpp has this beauty:

	void OnHundredthTick() override
	{
		/* Timeout enabled? */
		if (this->duration != 0) {
			this->duration--;
			if (this->duration == 0) delete this;
		}
	}

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?

@glx22
Copy link
Contributor Author

glx22 commented Dec 20, 2020

I think you're right. And

OpenTTD/src/console_gui.cpp

Lines 230 to 237 in e82333c

void OnHundredthTick() override
{
if (IConsoleLine::Truncate() &&
(IConsoleWindow::scroll > IConsoleLine::size)) {
IConsoleWindow::scroll = max(0, IConsoleLine::size - (this->height / this->line_height) + 1);
this->SetDirty();
}
}
may be an issue too.

@floodious
Copy link

floodious commented Dec 23, 2020

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:
for (auto i = events.begin(); i.due < current_time; i++) { i->execute(); events.remove(i); }

From the replacement for OnHundredthTick():
Suitably Named Timer Trigger Function(caller &reference) { ... done what must be done; reference.add(this, &thisfunction, due_time_offset); }

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.

@TrueBrain TrueBrain added this to the 1.11.0 milestone Jan 5, 2021
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--;
Copy link
Member

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.

Copy link
Member

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.

src/error_gui.cpp Outdated Show resolved Hide resolved
src/error_gui.cpp Outdated Show resolved Hide resolved
@TrueBrain TrueBrain added candidate: yes This Pull Request is a candidate for being merged and removed candidate: most likely This Pull Request is probably going to be accepted labels Jan 7, 2021
src/window.cpp Outdated Show resolved Hide resolved
@glx22 glx22 force-pushed the fix_7904 branch 2 times, most recently from 0594fb3 to 03dffc4 Compare January 7, 2021 20:45
@TrueBrain
Copy link
Member

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

src/window_gui.h Outdated Show resolved Hide resolved
TrueBrain
TrueBrain previously approved these changes Jan 8, 2021

Window *w;
FOR_ALL_WINDOWS_FROM_FRONT(w) {
if (!_network_dedicated && hundredth_tick_timeout == 0) w->OnHundredthTick();
Copy link
Member

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.

@TrueBrain TrueBrain dismissed their stale review January 8, 2021 18:57

What frosch says :P

@TrueBrain TrueBrain merged commit 1fb4ed8 into OpenTTD:master Jan 10, 2021
@glx22 glx22 deleted the fix_7904 branch January 10, 2021 13:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
candidate: yes This Pull Request is a candidate for being merged size: small This Pull Request is small, and should be relative easy to process
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants