[PATCH] Make Atlas work on OS X

[Philip Taylor]

When running ./pyrogenesis_dbg -editor on OS X, it starts up the editor window and then the window freezes (though all the code threads continue running).

In the current design, pyrogenesis_dbg starts up first, via SDL's main function, which calls [NSApplication run] and eventually ends up in our main (source/main.cpp).

We then detect the -editor flag, load libAtlasUI_dbg.dll, start up a new thread, and run Atlas_StartWindow (source/tools/atlas/AtlasUI/Misc/DLLInterface.cpp) in the new thread, which calls wxEntry to start the wxWidgets event loop.

As far as I can tell, the problem is that we have two Cocoa event loops, one in SDL and one in wxWidgets, and the GUI thread doesn't get the input events it needs. (See ​here etc for some stuff about Cocoa threading.)

If that's true, we need to do, uh, something to fix it. (I have no idea what, but it might involve a redesign of the game/editor startup process). If that's not true (I'm not an OS X developer so I may easily be mistaken), we need to work out what the problem really is and then do something to fix it.

[(none)]

Milestone Unclassified deleted

[Philip Taylor]

A thing to try: When we launch Atlas, perhaps we could run wxWidgets on the main thread (so it should receive Cocoa events) instead of a new UI thread, and then run the rest of the engine in a new thread. So the engine code decides to run Atlas and then gets shunted across into a new thread instead of continuing as normal. I'm not sure how crazy/possible that will be.

[historic_bruno]

Attaching a patch which runs Atlas window in the main thread and shifts the game loop into a new thread. This has been tested and works on Windows (wxMSW 2.8.12) and OS X 10.6 (wxOSX/Cocoa 2.9.2).

[historic_bruno]

Oh and to clarify the problem on OS X without this patch: wxWidgets ties into various parts of the Cocoa API which do thread-safety checks, requiring the UI code to run on the main thread, and these cause fatal errors.

Can't test this patch with wxWidgets 2.8 and Carbon, because it won't build on a 64-bit system.

[historic_bruno]

Just noticed that the patch seems to break opening Atlas from the main menu.

[historic_bruno]

The associated error and callstack:

First-chance exception at 0x75a9b9bc in pyrogenesis_dbg.exe: 0x800401F0: CoInitialize has not been called.
wmi.cpp(97): Function call failed: return value was -1 (Function failed (no details available))

pyrogenesis_dbg.exe!sys_display_error(const wchar_t * text, unsigned int flags)  Line 315 + 0x1d bytes	C++
pyrogenesis_dbg.exe!CallDisplayError(const wchar_t * text, unsigned int flags)  Line 377 + 0xd bytes	C++
pyrogenesis_dbg.exe!debug_DisplayError(const wchar_t * description, unsigned int flags, void * context, const wchar_t * lastFuncToSkip, const wchar_t * pathname, int line, const char * func, volatile int * suppress)  Line 468 + 0xd bytes	C++
pyrogenesis_dbg.exe!debug_OnError(__int64 err, volatile int * suppress, const wchar_t * file, int line, const char * func)  Line 545 + 0x2c bytes	C++>
pyrogenesis_dbg.exe!wmi_GetClass(const wchar_t * className, std::map<std::basic_string<wchar_t,std::char_traits<wchar_t>,std::allocator<wchar_t> >,tagVARIANT,std::less<std::basic_string<wchar_t,std::char_traits<wchar_t>,std::allocator<wchar_t> > >,std::allocator<std::pair<std::basic_string<wchar_t,std::char_traits<wchar_t>,std::allocator<wchar_t> > const ,tagVARIANT> > > & wmiMap)  Line 97 + 0x1a bytes	C++
pyrogenesis_dbg.exe!wgfx_CardName(wchar_t * cardName, unsigned int numChars)  Line 141 + 0xe bytes	C++
pyrogenesis_dbg.exe!gfx::CardName()  Line 43 + 0x11 bytes	C++
pyrogenesis_dbg.exe!RunHardwareDetection()  Line 190 + 0x10 bytes	C++
pyrogenesis_dbg.exe!InitGraphics(const CmdLineArgs & args, int flags)  Line 867	C++
pyrogenesis_dbg.exe!AtlasMessage::fInitGraphics(AtlasMessage::mInitGraphics * msg)  Line 78 + 0xd bytes	C++
pyrogenesis_dbg.exe!AtlasMessage::fInitGraphics_wrapper(AtlasMessage::IMessage * msg)  Line 56 + 0x5f bytes	C++
pyrogenesis_dbg.exe!RunGame(void * data)  Line 169 + 0x19 bytes	C++
pyrogenesis_dbg.exe!thread_start(void * param)  Line 585 + 0x9 bytes	C++
msvcr100d.dll!_callthreadstartex()  Line 314 + 0xf bytes	C
msvcr100d.dll!_threadstartex(void * ptd)  Line 297	C

​Reason why this would break when moving graphics init to a new thread:

CoInitialize Function
Initializes the COM library on the current thread and identifies the concurrency model as single-thread apartment (STA).

There is no error when using the -editor flag because the COM library has not been initialized yet, it does so on the new thread when InitGraphics() is called. Maybe we should shut down the associated module when exiting the game and before starting Atlas, so it enters the uninitialized state. I don't know if that would have other unintended consequences, but is worth testing.

[Philip Taylor]

If it's complex/messy to move the engine thread on Windows, we could keep the old Atlas behaviour on Windows and do the thread-swapping only on non-Windows (since we use a load of low-level Win32 API stuff, unlike on other OSes). (Best to keep Linux and OS X consistent if possible so that that code path gets regular testing). Would be nicer if the engine can shut down and start up again on a different thread without these errors if it's not too much of a pain, though.

[historic_bruno]

To shut down properly we have to balance each (successful) CoInitialize with a CoUninitialize, which seems simple enough as we don't use them very often. In fact we haven't been calling wmi_Shutdown at all. Attaching a new patch that does that and fixes the Atlas loading.

[ben]

(In [10299]) Moves Atlas UI to main thread while engine loop runs in new thread. Fixes Atlas compatibility with OS X (wxOSX/Cocoa requires the UI to run in the main thread). Fixes #500. Fixes Windows shutdown to close COM library properly (and WMI).

[wraitii]

In 24361:

Remove the Engine threading when using Atlas

MacOS requires some UI-related API calls to happen on the main thread. There are several SDL functions that call UI-things, and those were, since #500 / r10299, in a separate thread.
This crashes on Catalina, instead of simply warning (see #5470).

It is not the first time we had such issues, as originally the Engine was on the main thread and AtlasUI on a separate thread.
That didn't work on MacOS, so the threading was inverted in #500 / r10299, with AtlasUI on the main thread and the Engine in another thread.
Unfortunately, this still wasn't enough.

This formally unthreads the engine, running it on a wxTimer, to avoid these issues.

Future work should focus on:

• Further decoupling the simulation from the engine itself, as what Atlas really needs is a threaded simulation, not a threaded engine.
• Making the simulation itself more threaded
• Making it possible to do tasks asynchronously under Atlas.

Refs #500
Fixes #5470

Differential Revision: ​https://code.wildfiregames.com/D2752