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

Move GameLoop into a thread (and no longer run Paint in a thread) #8741

Merged
merged 5 commits into from Mar 8, 2021

Conversation

TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented Feb 24, 2021

Requires #8747 to be merged first.

Motivation / Problem

During work on the video drivers, I noticed a few (SDL1, SDL2 and Win32) had a "draw thread" implementation, where drawing was moved to a thread. This to me is odd, as normally you want to do drawing in the main thread, and everything else in other threads.

This was also showing, as with OpenGL, "draw thread" could not be used. The thread the context is created in, has to be the thread that is updating it. And given this was done in the main thread, it could not be done in the "draw thread".

This PR introduced an alternative: do GameLoop in a thread. It works for all video drivers.

Description

This allows drawing to happen while the GameLoop is doing an
iteration too.
    
Sadly, not much drawing currently can be done while the GameLoop
is running, as for example PollEvent() or UpdateWindows() can
influence the game-state. As such, they first need to acquire a
lock on the game-state before they can be called.
    
Currently, the main advantage is the time spend in Paint(), which
for non-OpenGL drivers can be a few milliseconds. For OpenGL this
is more like 0.05 milliseconds; in these instances this change
doesn't add any benefits for now.
    
This is an alternative to the former "draw-thread", which moved
the drawing in a thread for some OSes. It has similar performance
gain as this does, although this implementation allows for more
finer control over what suffers when the GameLoop takes too
long: drawing or the next GameLoop. For now they both suffer
equally.

This is the inverse of draw_thread, where drawing was done in a thread. Now the GameLoop is instead.

There is currently very little benefit from doing this with OpenGL. The time spend in Paint, the only function that is allowed to run in parallel with the GameLoop for now, takes on my machine ~0.05ms. That won't change anything, really.
For non-OpenGL, it is a bit different. Paint can easily take 5ms, so that means the simulation speed can go slightly higher.

If I load complex games, like ProGame5 or a game with many Timberwolf's trains, using threads does indeed increase the simulation speed (with something between 10% and 20%, so noticeable), when using non-OpenGL. With OpenGL, again, no difference is to be spotted (or expected).

This PR is mainly meant to allow future work. Things to think about:

  • UpdateWindows now does everything while the game-state is locked. This is not required. Half-way through the viewport functions it can release the game-state lock, as it no longer will change the game-state. Sadly, this is not trivial to implement, but doing so would mean more time could be spend during the GameLoop, further improving the gameplay for complex games.
  • During resource starvation, currently both the simulation and draw fps suffer. But this doesn't have to be that way. I experimented a bit with this, but it is trivial to let one or the other suffer more. In the end, if GameLoop goes over 30ms, draw fps will always suffer, but till that time simulation can suffer. Or, if we like, we can always prioritize simulation over draw fps, and for example have drawing being done at 10fps so the simulation can run at 1x. I have fiddled a bit with this, but it is really hard to find a good compromise. So l left it at equal starvation for now.
  • The mouse is now non-responsive during GameLoop, but that is not needed. We can just keep the mouse reactive. This would also change how Modal windows work, for example. Already with this PR the AcquireBlitterLock doesn't do anything, and it is a small step to just remove it, and have a more reactive GUI.

Limitations

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 force-pushed the gameloop-thread branch 4 times, most recently from e5e2383 to 04c39e5 Compare February 24, 2021 17:49
@TrueBrain TrueBrain force-pushed the gameloop-thread branch 3 times, most recently from 1cc4aef to f9788a8 Compare February 26, 2021 23:44
@TrueBrain TrueBrain marked this pull request as ready for review February 26, 2021 23:52
@TrueBrain
Copy link
Member Author

TrueBrain commented Feb 27, 2021

Sometimes, when switching game-modes, my mouse with OpenGL transforms into this:

image

@michicc : happen to have any idea what could be causing that? It seems to be timing related, something to do with when GameLoop runs and when drawing is done.

@michicc
Copy link
Member

michicc commented Feb 27, 2021

@TrueBrain I would assume that the call to VideoDriver::ClearSystemSprites is somehow mistimed.

@TrueBrain
Copy link
Member Author

TrueBrain commented Feb 27, 2021

@TrueBrain I would assume that the call to VideoDriver::ClearSystemSprites is somehow mistimed.

W00p, tnx :) It was not this function, but the function populating the cache (OpenGL's DrawMouseCursor). This was done outside the lock on the game-state, but it was using GetRawSprite which is not valid to call when you don't own the lock. And indeed, GameLoop was rebuilding sprites, causing DrawMouseCursor to read random garbage instead.

Solution was to make a function that populated the cache for OpenGL, and call it while the game-state is still locked.

@TrueBrain TrueBrain force-pushed the gameloop-thread branch 2 times, most recently from 9d7f2f0 to 6f6cb42 Compare February 27, 2021 12:11
@andythenorth
Copy link
Contributor

andythenorth commented Feb 28, 2021

No obvious issues from a basic check on Mac.

It's 10-20% faster on ffwd for me than 1.11.0-beta-2 for same map area of a s specific savegame. Might not be significant result, might just be noise in the system.

@TrueBrain TrueBrain force-pushed the gameloop-thread branch 8 times, most recently from 759bb75 to f933592 Compare March 8, 2021 16:33
@TrueBrain TrueBrain added this to the 1.11.0 milestone Mar 8, 2021
@TrueBrain TrueBrain force-pushed the gameloop-thread branch 2 times, most recently from 28d1e69 to b137b08 Compare March 8, 2021 17:17
michicc
michicc previously approved these changes Mar 8, 2021
Copy link
Member

@michicc michicc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine 😇 (after a rebase).

Drawing in a thread is a bit odd, and often leads to surprising
issues. For example, OpenGL would only allow it if you move the
full context to the thread. Which is not always easily done on
all OSes.
In general, the advise is to handle system events and drawing
from the main thread, and do everything else in other threads.
So, let's be more like other games.

Additionally, putting the drawing routine in a thread was only
done for a few targets.

Upcoming commit will move the GameLoop in a thread, which will
work for all targets.
michicc and others added 3 commits March 8, 2021 19:07
There really is no need to make an extra call to the OS in
these cases.
This allows drawing to happen while the GameLoop is doing an
iteration too.

Sadly, not much drawing currently can be done while the GameLoop
is running, as for example PollEvent() or UpdateWindows() can
influence the game-state. As such, they first need to acquire a
lock on the game-state before they can be called.

Currently, the main advantage is the time spend in Paint(), which
for non-OpenGL drivers can be a few milliseconds. For OpenGL this
is more like 0.05 milliseconds; in these instances this change
doesn't add any benefits for now.

This is an alternative to the former "draw-thread", which moved
the drawing in a thread for some OSes. It has similar performance
gain as this does, although this implementation allows for more
finer control over what suffers when the GameLoop takes too
long: drawing or the next GameLoop. For now they both suffer
equally.
This because video-drivers might need to make changes to their
context, which for most video-drivers has to be done in the same
thread as the window was created; main thread in our case.
@TrueBrain TrueBrain merged commit 8946b41 into OpenTTD:master Mar 8, 2021
@TrueBrain TrueBrain deleted the gameloop-thread branch March 8, 2021 18:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants