Page MenuHomeWildfire Games

Do not thread the game in Atlas
Needs RevisionPublic

Authored by wraitii on May 19 2020, 9:53 PM.

Details

Reviewers
trompetin17
Stan
Trac Tickets
#5470
Summary

This was much easier than I thought :P.


This is a fix for a crash on Mac OS Catalina. D2019 reported issues with video setup in a separate thread, but it turns out it actually crashes, and D2019 is not enough to fix it (in fact I think it's partly the issue, by virtue that further graphics call must be on the initialiser thread).

The simplest solution I find to fixing these issues, which actually date back to #500, is to not put Atlas and the game in separate threads.
The reason for this architecture seems to be found here (staff forum thread). Basically, it was reasoned that it would help the editor not lag even if the game itself lagged.

The implementation of messages and such in Atlas makes it rather easy to not actually do threading. With the need for threading removed, a (much?) simpler implementation is likely possible, but this is a working first step (cleanup pending).

In my tests, I haven't noticed anything broken or obviously not working.


Pros of unthreading:

  • Code will be slightly easier to maintain and we might actually end up removing some magic form Atlas.
  • Less concern about the editor burning 100% of CPU if we reduce the frame rate.
  • Less concern about threading in other parts of the code because Atlas is no longer particularly special.
  • Any threading-UI error that might pop up in the future is preventively squashed.

Cons of unthreading:

  • Does reduce performance of Atlas tools slightly (though I don't think we had many tools that didn't run in the engine thread anyways).
  • Might be a slightly worse experience if the game frame really lags.
  • To an extent, it puts the whole DLL architecture in question too.

Makes D2019 and D1922 irrelevant (and ergo fixes #5470)


User wik has a different fix at https://wildfiregames.com/forum/index.php?/topic/28183-trunk23664-cant-open-atlas-editor-on-osx-catalina-10154/&tab=comments#comment-396649, which involves patching wxWidgets.

Test Plan

Test Atlas, run tools and other commands.

Event Timeline

wraitii created this revision.May 19 2020, 9:53 PM
Owners added a subscriber: Restricted Owners Package.May 19 2020, 9:53 PM
wraitii updated this revision to Diff 11931.May 19 2020, 10:01 PM

Slightly less crap version.

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/tools/atlas/GameInterface/Messages.h
|   1| /*·Copyright·(C)·2019·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2020" year instead of "2019"

source/tools/atlas/GameInterface/Messages.h
|  25| #include·<vector>
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'namespaceAtlasMessage{' is invalid C code. Use --std or --language to configure the language.

source/tools/atlas/GameInterface/Messages.h
| 211| »   »   ,
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Unmatched '}'. Configuration: 'MESSAGESSETUP_NOTFIRST'.

source/tools/atlas/GameInterface/MessagePasserImpl.cpp
|   1| /*·Copyright·(C)·2019·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2020" year instead of "2019"

source/tools/atlas/GameInterface/MessagePasserImpl.cpp
| 211| The line belonging to the following result cannot be printed because it refers to a line that doesn't seem to exist in the given file.
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Unmatched '}'. Configuration: 'MESSAGESSETUP_NOTFIRST'.

source/tools/atlas/GameInterface/GameLoop.cpp
|   1| /*·Copyright·(C)·2019·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2020" year instead of "2019"

source/tools/atlas/GameInterface/GameLoop.cpp
| 211| »   »   //////////////////////////////////////////////////////////////////////////
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Unmatched '}'. Configuration: 'MESSAGESSETUP_NOTFIRST'.

source/tools/atlas/AtlasUI/ScenarioEditor/ScenarioEditor.cpp
|   1| /*·Copyright·(C)·2019·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2020" year instead of "2019"

source/tools/atlas/GameInterface/Handlers/GraphicsSetupHandlers.cpp
|   1| /*·Copyright·(C)·2019·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2020" year instead of "2019"

source/tools/atlas/GameInterface/Handlers/GraphicsSetupHandlers.cpp
| 211| »   g_Input.ProcessInput(g_AtlasGameLoop);
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Unmatched '}'. Configuration: 'MESSAGESSETUP_NOTFIRST'.
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/2166/display/redirect

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/tools/atlas/GameInterface/MessagePasser.h
|   1| /*·Copyright·(C)·2009·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2020" year instead of "2009"

source/tools/atlas/GameInterface/MessagePasser.h
|  25| 
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'namespaceAtlasMessage{' is invalid C code. Use --std or --language to configure the language.

source/tools/atlas/GameInterface/MessagePasserImpl.h
|   1| /*·Copyright·(C)·2019·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2020" year instead of "2019"

source/tools/atlas/GameInterface/MessagePasserImpl.h
|  25| #include·<queue>
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'namespaceAtlasMessage{' is invalid C code. Use --std or --language to configure the language.

source/tools/atlas/GameInterface/Messages.h
|   1| /*·Copyright·(C)·2019·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2020" year instead of "2019"

source/tools/atlas/GameInterface/Messages.h
|  25| #include·<vector>
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'namespaceAtlasMessage{' is invalid C code. Use --std or --language to configure the language.

source/tools/atlas/GameInterface/Messages.h
| 211| »   »   ,
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Unmatched '}'. Configuration: 'MESSAGESSETUP_NOTFIRST'.

source/tools/atlas/GameInterface/MessagePasserImpl.cpp
|   1| /*·Copyright·(C)·2019·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2020" year instead of "2019"

source/tools/atlas/GameInterface/MessagePasserImpl.cpp
| 211| The line belonging to the following result cannot be printed because it refers to a line that doesn't seem to exist in the given file.
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Unmatched '}'. Configuration: 'MESSAGESSETUP_NOTFIRST'.

source/tools/atlas/AtlasUI/ScenarioEditor/ScenarioEditor.cpp
|   1| /*·Copyright·(C)·2019·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2020" year instead of "2019"

source/tools/atlas/GameInterface/GameLoop.cpp
|   1| /*·Copyright·(C)·2019·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2020" year instead of "2019"

source/tools/atlas/GameInterface/GameLoop.cpp
| 211| The line belonging to the following result cannot be printed because it refers to a line that doesn't seem to exist in the given file.
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Unmatched '}'. Configuration: 'MESSAGESSETUP_NOTFIRST'.

source/tools/atlas/GameInterface/Handlers/GraphicsSetupHandlers.cpp
|   1| /*·Copyright·(C)·2019·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2020" year instead of "2019"

source/tools/atlas/GameInterface/Handlers/GraphicsSetupHandlers.cpp
| 211| 
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Unmatched '}'. Configuration: 'MESSAGESSETUP_NOTFIRST'.
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/2167/display/redirect

wraitii added a subscriber: vladislavbelov.EditedMay 20 2020, 8:44 AM

Edit: the below fixed by wik at https://wildfiregames.com/forum/index.php?/topic/28183-trunk23664-cant-open-atlas-editor-on-osx-catalina-10154/&tab=comments#comment-396649
somewhat. It's fixable anyways.


This does fix the UI thread issues on Catalina, however I get an SDL assertion failure:

Assertion failure at SDL_GetWindowWMInfo_REAL (/Users/Shared/github_repos/0ad/libraries/osx/sdl2/SDL2-2.0.12/src/video/SDL_video.c:3760), triggered 1 time:
  'window && window->magic == &_this->window_magic'

and further the screen is indeed only displayed on ¼ of the screen. @vladislavbelov reports this happens on other platforms too, so that may or may not be a separate issue.

(Ignoring the above assertion failure "works" as in the game doesn't crash)

wraitii edited the summary of this revision. (Show Details)May 20 2020, 8:46 AM
wraitii edited the summary of this revision. (Show Details)May 20 2020, 8:54 AM
trompetin17 added a subscriber: trompetin17.EditedMon, Jul 6, 3:56 AM

Hi, I tried this path, I saw something interesting in my processor, it was always in 20% and this happen only starting.

I'm wondering if we are missing the "sleep" part in this implementation, I mean this part
"// Be nice to the processor (by sleeping lots) if we're not doing anything"

I will try to find if I could do something about it, BTW thx for the patch

PD: Also I'm getting this message in console:
CGContextSaveGState: invalid context 0x0. If you want to see the backtrace, please set CG_CONTEXT_SHOW_BACKTRACE environmental variable.

trompetin17 added a comment.EditedThu, Jul 9, 7:18 PM


@wraitii with your patch when start atlas I see a black window in the right hand, is this for "RendererIncrementalLoad"?


@wraitii with your patch when start atlas I see a black window in the right hand, is this for "RendererIncrementalLoad"?

Yes is about RendererIncrementalLoad, I set similar from gameLoop and now I saw green texture :D

wraitii updated this revision to Diff 12815.Mon, Jul 20, 2:50 PM
wraitii retitled this revision from Do not thread the game in Atlas [WIP] to Do not thread the game in Atlas.
wraitii edited the summary of this revision. (Show Details)
wraitii edited the test plan for this revision. (Show Details)

Re-implement RendererIncrementalLoop. AFAIK, this makes the patch complete.

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/tools/atlas/GameInterface/GameLoop.h
|   1| /*·Copyright·(C)·2012·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2020" year instead of "2012"

source/tools/atlas/GameInterface/MessagePasser.h
|   1| /*·Copyright·(C)·2009·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2020" year instead of "2009"

source/tools/atlas/GameInterface/MessagePasser.h
|  25| 
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'namespaceAtlasMessage{' is invalid C code. Use --std or --language to configure the language.

source/tools/atlas/GameInterface/Messages.h
|  25| #include·<vector>
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'namespaceAtlasMessage{' is invalid C code. Use --std or --language to configure the language.

source/tools/atlas/GameInterface/Messages.h
| 211| »   »   ,
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Unmatched '}'. Configuration: 'MESSAGESSETUP_NOTFIRST'.

source/tools/atlas/GameInterface/MessagePasserImpl.h
|   1| /*·Copyright·(C)·2019·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2020" year instead of "2019"

source/tools/atlas/GameInterface/MessagePasserImpl.h
|  25| #include·<queue>
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'namespaceAtlasMessage{' is invalid C code. Use --std or --language to configure the language.

source/tools/atlas/GameInterface/MessagePasserImpl.cpp
|   1| /*·Copyright·(C)·2019·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2020" year instead of "2019"

source/tools/atlas/GameInterface/MessagePasserImpl.cpp
| 211| The line belonging to the following result cannot be printed because it refers to a line that doesn't seem to exist in the given file.
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Unmatched '}'. Configuration: 'MESSAGESSETUP_NOTFIRST'.

source/tools/atlas/GameInterface/GameLoop.cpp
|   1| /*·Copyright·(C)·2019·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2020" year instead of "2019"

source/tools/atlas/GameInterface/GameLoop.cpp
| 211| The line belonging to the following result cannot be printed because it refers to a line that doesn't seem to exist in the given file.
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Unmatched '}'. Configuration: 'MESSAGESSETUP_NOTFIRST'.

source/tools/atlas/AtlasUI/ScenarioEditor/ScenarioEditor.cpp
|   1| /*·Copyright·(C)·2019·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2020" year instead of "2019"

source/tools/atlas/GameInterface/Handlers/GraphicsSetupHandlers.cpp
|   1| /*·Copyright·(C)·2019·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2020" year instead of "2019"

source/tools/atlas/GameInterface/Handlers/GraphicsSetupHandlers.cpp
| 211| 
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Unmatched '}'. Configuration: 'MESSAGESSETUP_NOTFIRST'.
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/2725/display/redirect

Stan added a subscriber: Stan.Sun, Jul 26, 5:21 PM
Stan added inline comments.
source/tools/atlas/GameInterface/GameLoop.cpp
251

Should we keep this ?

wraitii added inline comments.Sat, Aug 1, 5:32 PM
source/tools/atlas/GameInterface/GameLoop.cpp
251

It's not completely trivial with the new code -> we'd have to modify the timer.

I don't think it's worth keeping.

Stan requested changes to this revision.Mon, Aug 3, 10:16 AM

This patch breaks the elevation tool on Windows and weirdly caps the max FPS to 32 (144hz screen)

This revision now requires changes to proceed.Mon, Aug 3, 10:16 AM