Navigation Menu

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

Implement partial multicore rendering #8481

Merged
merged 10 commits into from Apr 2, 2019

Conversation

ZehMatt
Copy link
Member

@ZehMatt ZehMatt commented Dec 18, 2018

For testing purpose, perhaps a feature at some point.

@ZehMatt
Copy link
Member Author

ZehMatt commented Jan 2, 2019

What shall happen to this PR? It seems to work well with OpenGL not a lot with the Software renderer but very noticable on debug builds.

@ZehMatt ZehMatt force-pushed the multicore-rendering branch 3 times, most recently from 6fc3c8e to bc79430 Compare January 2, 2019 12:37
data/language/en-GB.txt Outdated Show resolved Hide resolved
src/openrct2/interface/Viewport.cpp Outdated Show resolved Hide resolved
src/openrct2/localisation/Localisation.cpp Show resolved Hide resolved
@janisozaur
Copy link
Member

'shared_mutex' is unavailable: introduced in macOS 10.12

I guess the time has come to drop older versions then

@ZehMatt
Copy link
Member Author

ZehMatt commented Jan 7, 2019

Damn, what did they drop it for?

@janisozaur
Copy link
Member

janisozaur commented Jan 7, 2019

6e44334 should probably change these to 10.12 now

There are some more instances of MACOSX_DEPLOYMENT_TARGET in there now, most likely they all need to get updated.

@ZehMatt ZehMatt force-pushed the multicore-rendering branch 2 times, most recently from e43e921 to 560fcb1 Compare January 7, 2019 20:08
@janisozaur janisozaur self-assigned this Jan 14, 2019
@ZehMatt ZehMatt force-pushed the multicore-rendering branch 2 times, most recently from 4393a27 to 5243e49 Compare February 25, 2019 00:39
@AaronVanGeffen
Copy link
Contributor

AaronVanGeffen commented Feb 25, 2019

Builds cleanly, but segfaults on macOS soon after starting:

Process 20801 stopped
* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
    frame #0: 0x00000001004114f9 openrct2`OpenRCT2::Paint::Painter::CreateSession(rct_drawpixelinfo*, unsigned int) + 825
openrct2`OpenRCT2::Paint::Painter::CreateSession:
->  0x1004114f9 <+825>: movq   %rdx, (%rcx)
    0x1004114fc <+828>: movq   0x8(%rax), %rdx
    0x100411500 <+832>: movq   %rdx, 0x8(%rcx)
    0x100411504 <+836>: movq   0x10(%rax), %rax
Target 0: (openrct2) stopped.
$ clang --version
Apple LLVM version 10.0.0 (clang-1000.10.44.4)
Target: x86_64-apple-darwin18.2.0
Thread model: posix
InstalledDir: /Library/Developer/CommandLineTools/usr/bin

@tomlankhorst
Copy link
Contributor

tomlankhorst commented Feb 26, 2019

Mutex - the type of the shared mutex to lock. The type must meet the SharedMutex requirements
cppreference.com thread/shared_lock

... which std::mutex does not.

We could bump the API to 10.12... but that would support a mere 75% of worldwide users.

Alternatively, can't you use unique locks? PR

@ZehMatt
Copy link
Member Author

ZehMatt commented Feb 26, 2019

I'm not sure if the issue is the mutex, have a look at the build logs. Thats the mysterious part here.

@ZehMatt ZehMatt force-pushed the multicore-rendering branch 3 times, most recently from 7596f1e to e5b7339 Compare February 27, 2019 00:34
@ZehMatt
Copy link
Member Author

ZehMatt commented Feb 27, 2019

I've disabled the whole thing for mac to preserve my sanity. If anyone feels like giving this a shot feel free, I'm done trying.

@janisozaur janisozaur self-requested a review February 27, 2019 05:53
@tomlankhorst
Copy link
Contributor

tomlankhorst commented Feb 27, 2019

I'm not sure if the issue is the mutex, have a look at the build logs. Thats the mysterious part here.

You mean this error?

This is indeed the mutex... can’t shared-lock a non-shared mutex. Can only use unique lock here.
The PR I sent you builds fine.

Copy link
Member

@janisozaur janisozaur left a comment

Choose a reason for hiding this comment

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

static const utf8* large_scenery_sign_fit_text(const utf8* str, rct_large_scenery_text* text, bool height)
{
static utf8 fitStr[32];
utf8* fitStrEnd = fitStr;
safe_strcpy(fitStr, str, sizeof(fitStr));
int32_t w = 0;
uint32_t codepoint;
while (w <= text->max_width && (codepoint = utf8_get_next(fitStrEnd, (const utf8**)&fitStrEnd)) != 0)
{
if (height)
{
w += large_scenery_sign_get_glyph(text, codepoint)->height;
}
else
{
w += large_scenery_sign_get_glyph(text, codepoint)->width;
}
}
*fitStrEnd = 0;
return fitStr;
}

fitStr needs some locking or refactoring. This should be the last change required.

@ZehMatt
Copy link
Member Author

ZehMatt commented Apr 2, 2019

Done, made it pass a buffer to be filled rather than returning static one.

@AaronVanGeffen AaronVanGeffen added the changelog This issue/PR deserves a changelog entry. label Apr 2, 2019
@AaronVanGeffen
Copy link
Contributor

Could you add a line to the changelog as well?

@ZehMatt
Copy link
Member Author

ZehMatt commented Apr 2, 2019

Good?

@AaronVanGeffen
Copy link
Contributor

AaronVanGeffen commented Apr 2, 2019

Gave it a quick spin. Not noticing much of a difference on my old Linux pc, but all seems to work fine. Quick benchmark:

Off: Rendering 40 times with drawing engine Software (hardware display) took 13.34 seconds.
On: Rendering 40 times with drawing engine Software (hardware display) took 12.20 seconds.

On macOS, the same benchmark:

Off: Rendering 40 times with drawing engine Software (hardware display) took 14.53 seconds.
On: Rendering 40 times with drawing engine Software (hardware display) took 11.36 seconds.

@ZehMatt
Copy link
Member Author

ZehMatt commented Apr 2, 2019

Sadly its not too noticable without the OpenGL renderer but the gfxbenchmark is also not that good since theres nothing rendered really.
Edit: Also I had to introduce a mutex for the global font variables, once thats refactored it should squeeze out a little bit again, same goes for the scrolling text.

@ZehMatt
Copy link
Member Author

ZehMatt commented Apr 2, 2019

Should perhaps mention, this gives debug builds some nice performance under any renderer.

@janisozaur janisozaur merged commit 2fb3191 into OpenRCT2:develop Apr 2, 2019
AaronVanGeffen added a commit that referenced this pull request Jul 10, 2019
- Feature: [#485] Rides can now be simulated with ghost trains during construction.
- Feature: [#1260] Option for making giant screenshots have a transparent background.
- Feature: [#2339] Find local servers automatically when fetching servers.
- Feature: [#7296] Allow assigning a keyboard shortcut for the scenery picker.
- Feature: [#8029] Add the Hungarian Forint (HUF) to the list of available currencies.
- Feature: [#8481] Multi-threaded rendering.
- Feature: [#8558] Guest debugging tab.
- Feature: [#8659] Banner and sign texts are now shown in tooltips.
- Feature: [#8687] New multiplayer toolbar icon showing network status with reconnect option.
- Feature: [#8791] Improved tile element flag manipulation in Tile Inspector.
- Feature: [#8919] Allow setting ride price from console.
- Feature: [#8963] Add missing Czech letters to sprite font, use sprite font for Czech.
- Feature: [#9154] Change map toolbar icon with current viewport rotation.
- Change: [#7877] Files are now sorted in logical rather than dictionary order.
- Change: [#8427] Ghost elements now show up as white on the mini-map.
- Change: [#8688] Move common actions from debug menu into cheats menu.
- Change: [#9428] Increase maximum height of the Hypercoaster to RCT1 limits.
- Fix: [#2294] Clients crashing the server with invalid object selection.
- Fix: [#4568, #5896] Incorrect fences removed when building a tracked ride through
- Fix: [#5103] OpenGL: ride track preview not rendered.
- Fix: [#5889] Giant screenshot does not work while using OpenGL renderer.
- Fix: [#5579] Network desync immediately after connecting.
- Fix: [#5893] Looking at guest window tabs other than the main tab eventually causes assertion.
- Fix: [#5905] Urban Park merry-go-round has entrance and exit swapped (original bug).
- Fix: [#6006] Objects higher than 6 metres are considered trees (original bug).
- Fix: [#7039] Map window not rendering properly when using OpenGL.
- Fix: [#7045] Theme window's colour pickers not drawn properly on OpenGL.
- Fix: [#7323] Tunnel entrances not rendering in 'highlight path issues' mode if they have benches inside.
- Fix: [#7729] Money Input Prompt breaks on certain values.
- Fix: [#7884] Unfinished preserved rides can be demolished with quick demolish.
- Fix: [#7913] RCT1/RCT2 title sequence timing is off.
- Fix: [#7700, #8079, #8969] Crash when unloading buggy custom rides.
- Fix: [#7829] Rotated information kiosk can cause 'unreachable' messages.
- Fix: [#7878] Scroll shortcut keys ignore SHIFT/CTRL/ALT modifiers.
- Fix: [#8219] Faulty folder recreation in "save" folder.
- Fix: [#8480, #8535] Crash when mirroring track design.
- Fix: [#8507] Incorrect change in vehicle rolling direction.
- Fix: [#8537] Imported RCT1 rides/shops are all numbered 1.
- Fix: [#8553] Scenery removal tool removes fences and paths while paused.
- Fix: [#8598] Taking screenshots fails with some park names.
- Fix: [#8602] Wall piece collision detection deviates from vanilla
- Fix: [#8649] Setting date does not work in multiplayer.
- Fix: [#8873] Potential crash when placing footpaths.
- Fix: [#8882] Submarine Ride does not count as indoors (original bug).
- Fix: [#8900] Peep tracking is not synchronized.
- Fix: [#8909] Potential crash when invoking game actions as server.
- Fix: [#8947] Detection of AVX2 support.
- Fix: [#8988] Character sprite lookup noticeably slows down drawing.
- Fix: [#9000] Show correct error message if not enough money available.
- Fix: [#9067] Land/water tools show prices when money is disabled.
- Fix: [#9124] Disconnected clients can crash the server.
- Fix: [#9132] System file browser cannot open SV4 files.
- Fix: [#9152] Spectators can modify ride colours.
- Fix: [#9202] Artefacts show when changing ride type as client or using in-game console.
- Fix: [#9240] Crash when passing directory instead of save file.
- Fix: [#9245] Headless servers apply Discord Rich Presence.
- Fix: [#9293] Issue with the native load/save dialog.
- Fix: [#9322] Peep crashing the game trying to find a ride to look at.
- Fix: [#9324] Crash trying to remove invalid footpath scenery.
- Fix: [#9402] Ad campaigns disappear when you save and load the game.
- Fix: [#9411] Ad campaigns end too soon.
- Fix: [#9476] Running `simulate` command on park yields `Completed: (null)`.
- Fix: [#9520] Time Twister object artdec29 conversion problem.
- Fix: Guests eating popcorn are drawn as if they're eating pizza.
- Fix: The arbitrary ride type and vehicle dropdown lists are ordered case-sensitively.
- Improved: [#6116] Expose colour scheme for track elements in the tile inspector.
- Improved: Allow the use of numpad enter key for console and chat.
janisozaur added a commit to janisozaur/OpenRCT2 that referenced this pull request Jul 16, 2019
This takes multithreading into account as well
janisozaur added a commit to janisozaur/OpenRCT2 that referenced this pull request Feb 11, 2020
This takes multithreading into account as well
janisozaur added a commit to janisozaur/OpenRCT2 that referenced this pull request Feb 12, 2020
This takes multithreading into account as well
janisozaur added a commit that referenced this pull request Feb 12, 2020
* Fix #9559: benchspritesort is broken after #8481

This takes multithreading into account as well

* Apply review fixes

* Fix detection of newer Google Benchmark (>= 1.5.0)

* Review fix
@KKLD
Copy link

KKLD commented Mar 22, 2020

Thank you so much for working on this project and keeping the childhood dream alive!

I think you did a really great job with this feature. I get about 90% more FPS. It really helps for large parks. On that note, I curiously looked through #10664 and noticed that the maximum values will be increased and I think that will create more lag. Therefore, I was wondering if this multicore rendering only makes use of one additional core or if it can use more?

Looking through the code I noticed this: JobPool(size_t maxThreads = 255)
I also noticed that the picture is split up into 32x32 blocks to be rendered (as far as I can tell), however, I am unsure if it can really use 255 threads and not just 2 threads since I am getting 90% more performance and all of my CPU cores are not used. Therefore, I am wondering if there is an easy fix to use even more cores so we will have a smoother experience when going "limitless" in the new save format.

I hope that this is just a tiny bit useful. Once again thank you so much for working on this project, you have done an amazing job!

(I wonder why the sprite limits are only increased about 6.5 times since I can imagine that this will be the easiest limitation to reach, especially considering the fact that the other limits being raised allows one to easier reach that limit).

@ZehMatt
Copy link
Member Author

ZehMatt commented Mar 23, 2020

It will not use 255 threads unless your CPU has actually 255 cores, the value is clamped. See:

maxThreads = std::min<size_t>(maxThreads, std::thread::hardware_concurrency());

Its just defaulted so you don`t have to manually pass the correct amount from a implementation perspective.

@Gymnasiast
Copy link
Member

@KKLD Currently, not many people hit the sprite limit, so a 6.5 times increase should be sufficient. It also means that sprite IDs will continue to fit in 16-bit unsigned integers, which saves a lot of refactoring.

(If you have more questions about the save format, please ask them on Gitter, so we keep this PR clean of off-topic discussions.)

XplosiveLugnut pushed a commit to XplosiveLugnut/OpenRCT2 that referenced this pull request Apr 27, 2020
…nRCT2#9590)

* Fix OpenRCT2#9559: benchspritesort is broken after OpenRCT2#8481

This takes multithreading into account as well

* Apply review fixes

* Fix detection of newer Google Benchmark (>= 1.5.0)

* Review fix
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog This issue/PR deserves a changelog entry.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants