Page MenuHomeWildfire Games

[ATLASUI] Main Thread Checker: UI API called on a background thread: -[NSView initWithFrame:]
Needs ReviewPublic

Authored by trompetin17 on Thu, Jun 27, 7:07 PM.

Details

Reviewers
historic_bruno
Group Reviewers
Windows Developers
Restricted Owners Package(Owns No Changed Paths)
Trac Tickets
#5470
Summary

Please refer the ticket about the problem.

This solution just move the
SDL_InitSubSystem(SDL_INIT_VIDEO);
SDL_GL_LoadLibrary(NULL);

To GameLoop.

In osx Mojave this solution works perfectly and remove all warnings about another thread, and also this patch allow to show AtlasUI menu even when you pass "--editor" flag

Here some documentation
https://developer.apple.com/documentation/code_diagnostics/main_thread_checker

Looks like from StackTrace that SDL_InitSubSystem call the method Cocoa_StartTextInput, and Input is an UI, and then the warning show

Test Plan

Please apply this patch an test over linux, window, and osx.

No warning about Background thread and atlasUI menu must be appear

Diff Detail

Repository
rP 0 A.D. Public Repository
Branch
/ps/trunk
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 8299
Build 13544: Vulcan BuildJenkins
Build 13543: arc lint + arc unit

Event Timeline

trompetin17 created this revision.Thu, Jun 27, 7:07 PM
trompetin17 edited the summary of this revision. (Show Details)Thu, Jun 27, 7:10 PM

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

Linter detected issues:
Executing section Source...

source/tools/atlas/GameInterface/Messages.h
| 211| »   »   ((std::wstring,·path))
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character '{' when these macros are defined: 'MESSAGESSETUP_NOTFIRST'.

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

source/tools/atlas/GameInterface/GameLoop.cpp
| 211| 
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character '{' when these macros are defined: 'MESSAGESSETUP_NOTFIRST'.

source/tools/atlas/GameInterface/Handlers/GraphicsSetupHandlers.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):
|    | Invalid number of character '{' when these macros are defined: 'MESSAGESSETUP_NOTFIRST'.
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/differential/1828/display/redirect

Stan added a subscriber: Stan.Thu, Jun 27, 7:45 PM
Stan added inline comments.
source/tools/atlas/GameInterface/GameLoop.cpp
311

Must be called [...] On osx :)

313

Not sure whether null or nullptr here.

trompetin17 edited the summary of this revision. (Show Details)Thu, Jun 27, 7:50 PM
trompetin17 updated this revision to Diff 8629.Thu, Jun 27, 7:52 PM

Finxing Typo

trompetin17 marked an inline comment as done.Thu, Jun 27, 7:53 PM

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

Linter detected issues:
Executing section Source...

source/tools/atlas/GameInterface/Messages.h
| 211| »   »   ((std::wstring,·path))
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character '{' when these macros are defined: 'MESSAGESSETUP_NOTFIRST'.

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

source/tools/atlas/GameInterface/GameLoop.cpp
| 211| 
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character '{' when these macros are defined: 'MESSAGESSETUP_NOTFIRST'.

source/tools/atlas/GameInterface/Handlers/GraphicsSetupHandlers.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):
|    | Invalid number of character '{' when these macros are defined: 'MESSAGESSETUP_NOTFIRST'.
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/differential/1829/display/redirect

Documentation for the fact that MacOS requires UI to happen on the main thread: https://developer.apple.com/documentation/code_diagnostics/main_thread_checker, https://www.quora.com/Why-must-the-UI-always-be-updated-on-Main-Thread

I believe what is happening is that SDL_INIT_VIDEO also inits the events system, as written in the documentation: SDL_INIT_VIDEO | video subsystem; automatically initializes the events subsystem, and that must call some UI-related functions.

Thus moving this — on the surface — looks good to me. I think we should keep the comments though.

What should be looked into:

  • Why was this done so in the first place - is there a particular reason or does it appear to have been chance / no particular reason.
  • Its it 'dangerous' to init the video system in the main thread and use it in the secondary thread (I find that unlikely, but who knows). SDL isn't thread-safe but we aren't calling video-related methods from the Atlas thread as far as I know, so it should be fine.
  • Should we perhaps just init the event system in the main thread, and the video subsystem in the secondary thread.
wraitii added reviewers: Windows Developers, Restricted Owners Package.Fri, Jun 28, 9:37 AM

So this does cut out the UI warnings, I'm still getting _createMenuRef called with existing principal MenuRef already associated with menu

Also getting a crash when quitting sometimes in Atlas_GLSwapBuffers but I'm not certain it's related.

The commit that introduced this is rP16356, which doesn't seem particularly relevant, nor the update before. I've looked at a number of resources online, and there are warnings about creating GL contexts in a main thread and using it in a secondary thread, but I believe we do things properly as the secondary thread calls Atlas_GLSetCurrent which calls wxGLContext.SetCurrent and that does what we want - tell OpenGL that this secondary thread is the right one.

We swap buffers in the secondary thread too, and all the rendering takes place there, so yeah, this seems to be working.

I've tried initing solely SDL_INIT_EVENTS in the main thread, but that doesn't fix it.

Needs to be tested on *nix and windows for sure though.

source/tools/atlas/AtlasUI/ScenarioEditor/ScenarioEditor.cpp
579

I would put the calls directly there though, not in GameLoop.cpp

vladislavbelov added a subscriber: vladislavbelov.EditedSat, Jun 29, 8:06 PM

So, I think we may have only a workaround solution here. Because we have 2 independent libraries each of them thinks that it process all events alone and calls OS API directly.

Probably possible solution would be to find a way to initialise SDL video system without the event one.

I need to know if we could commit this one, Im going to continue with the other bugs like this one _createMenuRef called with existing principal MenuRef already associated with menu.

So let me know what could i do

Probably possible solution would be to find a way to initialise SDL video system without the event one.

This sounds impossible. First - the API won't let you. Further, there are hints online that this isn't possible. It would make sense that most events are tied to a window, so initialising video probably calls some UI-related API on its own (and my above testing would seem to confirm that).

Could somebody (@Stan ?) test this on Windows, and someone else on Linux?
In my opinion, it looks like the only possible fix.

elexis added a subscriber: elexis.Thu, Jul 4, 10:48 PM

No warning about Background thread and atlasUI menu must be appear

Tested with wxgtk 3.0.4 on linux, didn't get background thread warnings before nor after, menu visible before and after.
I guess SDL features should be tested. WASD Camera movement works, ingame clicking and simulation too.

Angen added a subscriber: Angen.Fri, Jul 5, 8:07 PM

Windows 10:
menu visible with and without patch
no warnings with and without patch

Did someone test it with an old platform?

source/tools/atlas/GameInterface/GameLoop.cpp
313

If in doubt, check CppSupport and Coding_Conventions :)

We support nullptr, so there's no reason to avoid using it in our C++ code, if you want. In fact, it's the recommended convention.

source/tools/atlas/GameInterface/Handlers/GraphicsSetupHandlers.cpp
69–78

Doesn't most of this comment still apply? I'm not sure if the TODO is resolved either, nobody seems to know for certain.

historic_bruno requested changes to this revision.Fri, Jul 12, 12:49 AM
This revision now requires changes to proceed.Fri, Jul 12, 12:49 AM
trompetin17 updated this revision to Diff 8838.Fri, Jul 12, 5:29 AM

change null for nullptr

trompetin17 marked 2 inline comments as done.Fri, Jul 12, 5:32 AM
trompetin17 added inline comments.
source/tools/atlas/GameInterface/Handlers/GraphicsSetupHandlers.cpp
69–78

Our engine uses SDL_GL_GetProcAddress to load all GL libraries, even if wxWidget load OpenGL, our engine isn't going to use because our engine is waiting we load OpenGL with SDL_GL_LoadLibrary.
So We must use SDL_GL_LoadLibrary always

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

Linter detected issues:
Executing section Source...

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| »   »   ((std::wstring,·path))
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Unmatched '}'. Configuration: 'MESSAGESSETUP_NOTFIRST'.

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

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

source/tools/atlas/GameInterface/Handlers/GraphicsSetupHandlers.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'.
Executing section JS...
Executing section cli...

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