Improve thread safety

[melody-rs]

Right now, mkxp will segfault at any point if you try to call Graphics.update or the like from another thread. This is quite irritating if you want to unhook game logic from the framerate. From what I can tell, it looks like this is in part due to any thread aside from the main thread lacking the OpenGL context.

I'm not sure just yet what the solution to this would look like, or if its even possible. More likely than not, someone with more experience (@GamingLiamStudios or @thehatkid?) may be better suited to look at this problem.

[servantoftestator]

Would disabling error handling in opengl make any difference https://stackoverflow.com/questions/56824376/can-i-disable-automatic-error-handling-in-opengl ? The simplistic solution would be disabling error handling in opengl and hoping the undefined behaivor doesn't have the context exit.

Otherwise you'd have to decouple the sdl video functions from the input functions in eventthread.h and make a second struct in eventthread.h for seperation followed by calling it appropriately throughout the codebase. Graphics::update and the likes manipulate RGSSThreadData which is defined at src/threads/headers/eventthread.h . RGSSThreadData takes input, is declaring the window functions, and using sdl to tie the thread to the framelimit. List of places RGSSThreadData is called..... https://pastebin.pl/view/5fe87dbd

[melody-rs]

If you call print in ruby somewhere down the line it ends up calling Graphics.update on a thread that doesn't hold the OpenGL context, leading to a segfault

[zimberzimber]

Wouldn't hijacking Graphics.update to check if it's on the main thread before continuing solve this?

[servantoftestator]

in src/graphics/source/graphics.cpp One of the functions of the class Graphics, is Graphics::Graphics(RGSSThreadData *data) which calls the private struct GraphicsPrivate . Even if you checked if you are in RGSSThreadData you'd have to exit RGSSThreadData and re-enter it to cleanly make that transition. Which means forfieting input, framerate, and more importantly the window that is tied up in RGSSThreadData while you exit/re-enter. I'd imagine the usecase for calling the main thread, RGSSThreadData, from ruby would still desire user input with a window, just without the framerate tied to it.

So you'd have to decoupled the portions of RGSSThreadData that you do not desire in that context in eventthread.h and not store it on line 673 where it brings up opengl context in src/graphics/source/graphics.cpp. Otherwise you'd have to get the OpenGL context to save to a buffer temporarily and then re-call it on seperately from what RGSSThreadData does using sdl. Problem is that the functions RGSSThreadData as a struct and graphics.cpp uses are in a private struct for the opengl context bringup so you'd have to seperate them out anyways at line 673 in graphics.cpp.

[melody-rs]

Wouldn't hijacking Graphics.update to check if it's on the main thread before continuing solve this?

No, because it calls the C++ side equivalent. At some point down the line print calls this function which calls shState->graphics().repaintWait(), which is effectively a gimped Graphics.update

[servantoftestator]

Yea at

ModShot-Core/src/graphics/source/graphics.cpp

Line 1274 in 718e6d5

void Graphics::repaintWait(const AtomicFlag &exitCond, bool checkReset)

Its retuning an undefined parameter since exitCond is already met when the graphics context is not available. So when shState->graphics().repaintWait(msgBoxDone); is called it repaints a undefined value since the graphics haven't been brought up for this usecase.

[servantoftestator]

In EventThread::showMessageBox you might be able to get away with a if; then statement for shState->graphics().repaintWait(msgBoxDone); . Not sure what you would if for though.... You could also make Graphics::repaintWait Return something instead of a undefined value as it currently stands. See the bottom example https://www.geeksforgeeks.org/return-statement-in-c-cpp-with-examples/

The real solution here is rewriting in rust instead of dealing with memory management like this ;p .

[melody-rs]

There is a rewrite in rust ongoing, but it is very slow unless we get more people contributing.

[servantoftestator]

repaintWait only has two users and then its defines in graphics.h/cpp
binding-mri/binding-mri.cpp: shState->graphics().repaintWait(shState->rtData().rqResetFinish,
src/thread/source/eventthread.cpp: shState->graphics().repaintWait(msgBoxDone);
Looks like your ruby print call is routed through binding-mri.cpp specifically called at static void processReset(). You also could put a if; then statement in binding-mri.cpp to only call repaintWait if something....

[melody-rs]

This really isn't addressing the main problem, putting an if is more of a bandaid than anything

[melody-rs]

I do appreciate the effort though

[GamingLiamStudios]

The real solution here is rewriting in rust instead of dealing with memory management like this ;p .

but then you have to deal with the syntax of Rust

[GamingLiamStudios]

Right now, mkxp will segfault at any point if you try to call Graphics.update or the like from another thread. This is quite irritating if you want to unhook game logic from the framerate. From what I can tell, it looks like this is in part due to any thread aside from the main thread lacking the OpenGL context.

OpenGL isn't very good at running in multiple threads, so you've either gotta keep ALL graphical calls in the main thread or rewrite in something that is a bit more multithreading friendly, like Vulkan(but that comes with its own set of challenges).

I think the best solution rn is to make a very basic event system for the Graphical context. You'd have a thread-safe queue, which the main graphical thread would be constantly waiting for information from, and the other threads would add a graphical event it wants, like moving a sprite or showing some text.

Would be a pain in the ass to create, but once you've got the ruby boilerplate shit in, it should be mostly seamless.

[GamingLiamStudios]

You could also make the Graphics.update only run on the main thread, the other calls sending a hint to the main thread to run it. Not sure how much OpenGL will like that tho.

[servantoftestator]

You could also make the Graphics.update only run on the main thread, the other calls sending a hint to the main thread to run it. Not sure how much OpenGL will like that tho.

If you implemented it like that you would have to make all the calls into RGSSThreadData into thread safe futex's or windows equivielent to play nicely with opengl. I know that in x86/64 assembly for linux that spinlock implementations are much slower then plain register transfer logic. As futex/mutex uses a clear which is slow in x86 due to the nature of OOE processor memory models. Also see https://www.khronos.org/opengl/wiki/OpenGL_and_multithreading