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

Add: make modal windows update more smooth and fix "closing the app" issues closely related to this. #8830

Merged
merged 5 commits into from Mar 10, 2021

Conversation

TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented Mar 9, 2021

  • This invalidates the first three commits in #8828, but not the fourth. That is, this also fixes the bug mentioned, but not in the way you think: it simply continues the NewGRF scan till it is finished, while the window is closed.
  • EXTRA EXTRA EXTRA: the second commit in this PR also fixes #8760, and stops the NewGRF scan as soon as possible, similar to #8828 (but without all the complex thread-stuff, as it is now running in the game-thread anyway).
  • And while at it, fixed similar issue for world-gen: that is now also aborted when you close the game.
  • #8833 was closely related to all this, and now also part of this PR

(okay, this might be growing a bit too much; if it is easier if I PR them separately (a few depend on each other, but I can deal with that), just let me know!)

Closes #8828
Fixes #8760
Fixes #8833

Motivation / Problem

With the gameloop now threaded, we can get ride of the ugliness that the NewGRFScan and GenerateWorld threads are. They are a huge hack dating back when threads were still scary and not many people had more than one core. Pretty sure those days are over :P

By removing the special code for those two modal windows, the code becomes a lot simpler. Having such massive chunks of code not running in different threads makes things easier to debug and understand.

Purely by accident, this solves most issues mentioned in #8712, with the exception of the palette.

But mostly ... it now looks so much more smooth to generate a world ...

genworld.mp4

Description

Basically, modal windows had their own thread-locking for what
drawing was possible. This is a bit nonsense now we have a
game-thread. And it makes much more sense to do things like
NewGRFScan and GenerateWorld in the game-thread, and not in a
thread next to the game-thread.

This commit changes that: it removes the threads for NewGRFScan
and GenerateWorld, and just runs the code in the game-thread.
On regular intervals it allows the draw-thread to do a tick,
which gives a much smoother look and feel.

It does slow down NewGRFScan and GenerateWorld ever so slightly
as it spends more time on drawing. But the slowdown is not
measureable on my machines (with 700+ NewGRFs / 4kx4k map and
a Debug build).

Running without a game-thread means NewGRFScan and GenerateWorld
are now blocking.

All the extra MarkDirty calls are because formally it just made the whole screen dirty every time, but that is very expensive. So now only that what changed is marked dirty, which is a lot nicer on the CPU.

Limitations

  • Slightly slower NewGRF scanning and map generation, because time is spend on drawing (and not on scanning). On my machine it is so small, I could not measure this outside the noise. I would estimate it at: 0.05ms per frame, 60 frames per second, 3ms per 1000ms, so 0.3% speed loss.
  • Running with -vwin32-opengl:no_threads means you don't see those two modals; similar to running without threads before this commit. I think this is reasonable, to make it work this way.
  • The game used to mark the whole screen dirty during map generation; but doing that now would be dreadfully slow. So instead I tried to find all the places that modify the map, and mark the files dirty. To me it seems I found them all, but I might have missed some. Nothing we can't fix over time. (Basically, the old implementation was lazy and just did EVERYTHING, instead of what is needed).

Checklist for review

Some things are not automated, and forgotten often. This list is a reminder for the reviewers.

  • The bug fix is important enough to be backported? (label: 'backport requested')
  • This PR affects the save game format? (label 'savegame upgrade')
  • This PR affects the GS/AI API? (label 'needs review: Script API')
    • ai_changelog.hpp, gs_changelog.hpp need updating.
    • The compatibility wrappers (compat_*.nut) need updating.
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')

@TrueBrain TrueBrain added this to the 1.11.0 milestone Mar 9, 2021
@TrueBrain TrueBrain force-pushed the smoother-modals branch 4 times, most recently from 6728f5e to deb94ec Compare Mar 9, 2021
@TrueBrain TrueBrain added the preview label Mar 9, 2021
@DorpsGek DorpsGek temporarily deployed to preview-pr-8830 Mar 9, 2021 Inactive
@ldpl
Copy link
Contributor

@ldpl ldpl commented Mar 9, 2021

Some stuff I noticed after testing (some may not be related to this pr but whatever):

  • Doesn't crash if closed during newgrf scan
  • Reaches 100% way before finishing the scan. Few times I've seen it starting with 100% and doing entire scan on there.
  • Landscape generation part of mapgen still lags as before
  • Sleeping cloud cursor is still quite confusing to use (when clicking abort button).
  • Mapgen seems to count as world tick for frame rate window so it initializes avg with some insanely high number.

@TrueBrain
Copy link
Member Author

@TrueBrain TrueBrain commented Mar 9, 2021

Tnx for testing!

* Doesn't crash if closed during newgrf scan

Good! That is the important bit :D

* Reaches 100% way before finishing the scan. Few times I've seen it starting with 100% and doing entire scan on there.

Are you sure it is doing the scan at 100%? How do you tell?

For me it is working as expected, but one important observation: at the end of the NewGRF Scan, it stalls for a bit of time. There is a pretty hefty function being called when the Scan is complete, which stalls for anywhere between 300ms up to 2seconds for me.

* Landscape generation part of mapgen still lags as before

The first 5 steps are indeed laggy. This is because TGP does a massive calculation at once, and prints that on the map. There are no in-betweens there, and it doesn't allow mouse movement while doing. I think future-extensions can be to allow mouse movements, even during times game-tick has a lock on the game-state. But we are a bit away from that :)

* Sleeping cloud cursor is still quite confusing to use (when clicking abort button).

shrug. Not disagreeing, but indeed out of scope of this PR. Maybe worth an issue on its own?

* Mapgen seems to count as world tick for fps so it initializes avg with some insanely high number.

Haha, I noticed that too :D I will fix this one, as it is really silly :P

@DorpsGek DorpsGek temporarily deployed to preview-pr-8830 Mar 9, 2021 Inactive
@TrueBrain TrueBrain changed the title Add: make modal windows update more smooth Add: make modal windows update more smooth and fix "closing the app" issues closely related to this. Mar 9, 2021
@TrueBrain
Copy link
Member Author

@TrueBrain TrueBrain commented Mar 9, 2021

* Reaches 100% way before finishing the scan. Few times I've seen it starting with 100% and doing entire scan on there.

Basically, the game remembers how many NewGRFs you had last round, and assumes you will have that again. So the 100% is at the last known amount you had (last_newgrf_count in the config). My PR used to store the value when you closed the window, but in the next push it will no longer do that, and only remember the number of a finished scan.

@DorpsGek DorpsGek temporarily deployed to preview-pr-8830 Mar 9, 2021 Inactive
@TrueBrain TrueBrain removed the preview label Mar 9, 2021
Copy link
Member

@LordAro LordAro left a comment

I like the look of the code in general, all sorts of mutex & global removals. As for it's correctness...

src/openttd.h Outdated Show resolved Hide resolved
src/video/video_driver.cpp Outdated Show resolved Hide resolved
src/video/video_driver.hpp Outdated Show resolved Hide resolved
TrueBrain added 5 commits Mar 9, 2021
Basically, modal windows had their own thread-locking for what
drawing was possible. This is a bit nonsense now we have a
game-thread. And it makes much more sense to do things like
NewGRFScan and GenerateWorld in the game-thread, and not in a
thread next to the game-thread.

This commit changes that: it removes the threads for NewGRFScan
and GenerateWorld, and just runs the code in the game-thread.
On regular intervals it allows the draw-thread to do a tick,
which gives a much smoother look and feel.

It does slow down NewGRFScan and GenerateWorld ever so slightly
as it spends more time on drawing. But the slowdown is not
measureable on my machines (with 700+ NewGRFs / 4kx4k map and
a Debug build).

Running without a game-thread means NewGRFScan and GenerateWorld
are now blocking.
Otherwise the numbers are all over the place when a modal window
just closed.
This prevents the window from "freezing" when you close it during
the scanning of NewGRFs, as it first would continue the action.
This prevents the window from "freezing" when you close it during
world generation, as it first would continue the action.
Otherwise that might cause calls to the video-driver, which are
already shut down by now. This causes, depending on the video-driver
crashes or weird effects.
Copy link
Member

@LordAro LordAro left a comment

It's been tested reasonably thoroughly, and removing mutexes & global variables is always nice

YOLO

@TrueBrain TrueBrain merged commit 14b61bf into OpenTTD:master Mar 10, 2021
12 checks passed
@TrueBrain TrueBrain deleted the smoother-modals branch Mar 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants