bkaradzic / bgfx

Cross-platform, graphics API agnostic, "Bring Your Own Engine/Framework" style rendering library.
https://bkaradzic.github.io/bgfx/overview.html
BSD 2-Clause "Simplified" License
14.94k stars 1.94k forks source link

macOS metal deadlock (multithreaded renderer) #2036

Open sinkingsugar opened 4 years ago

sinkingsugar commented 4 years ago

since https://github.com/bkaradzic/bgfx/pull/2001 my framework stopped working properly on macOS. as explained in the comment of the PR above there is a deadlock. I saw that the fix got reverted but that's really confusing. CC: @attilaz

I will try to workaround by using different threads, but the easy and usual behavior is definitely broken.

Workaround: Revert ecc8f791f2ed53225e000eb57f8109660c011ee0 (https://github.com/bkaradzic/bgfx/commit/ecc8f791f2ed53225e000eb57f8109660c011ee0)

kattkieru commented 4 years ago

I think I just hit the same issue-- working on using bgfx with an SDL2 window, the app locks hard unless I disable threading in the build.

loryxia commented 4 years ago

i stuck at the same issue too, single thread is ok.

mfxdeveloper commented 4 years ago

I have this too. Apart from disabling multithreading on Mac (which lowers performance), is the workaround to go back to an earlier version of the bgfx metal renderer which did work with multithreading?

sinkingsugar commented 4 years ago

Fyi the workaround is to create a .mm file and instead of passing a window or view to bgfx, pass directly a metal layer. Basic solution (no cleanups):


#import <QuartzCore/CAMetalLayer.h>
#import <Metal/Metal.h>
#import <MetalKit/MetalKit.h>
#import <Cocoa/Cocoa.h>

namespace BGFX {
void *cbSetupMetalLayer(void *wnd) {
  NSWindow *window = (NSWindow*)wnd;
  NSView *contentView = [window contentView];
  [contentView setWantsLayer:YES];
  CAMetalLayer *res = [CAMetalLayer layer];
  [contentView setLayer:res];
  return res;
}
}

Passing directly a metal layer prevents the (a bit silly semaphore and workaround) deadlock

mfxdeveloper commented 4 years ago

Thank you, that metal layer fix looks pretty easy!

Kurapikov commented 11 months ago

Fyi the workaround is to create a .mm file and instead of passing a window or view to bgfx, pass directly a metal layer. Basic solution (no cleanups):

#import <QuartzCore/CAMetalLayer.h>
#import <Metal/Metal.h>
#import <MetalKit/MetalKit.h>
#import <Cocoa/Cocoa.h>

namespace BGFX {
void *cbSetupMetalLayer(void *wnd) {
  NSWindow *window = (NSWindow*)wnd;
  NSView *contentView = [window contentView];
  [contentView setWantsLayer:YES];
  CAMetalLayer *res = [CAMetalLayer layer];
  [contentView setLayer:res];
  return res;
}
}

Passing directly a metal layer prevents the (a bit silly semaphore and workaround) deadlock

This deadlock issue still persists in the most recent version of bgfx on macOS 14.0, and the workaround by sinkingsugar still effectively addresses it. Given that this issue has been present for over three years and many people (including myself) have wasted a lot of time on it, I believe it should be clearly labeled as either a bug or a feature.

If this issue is identified as a bug, then I think sinkingsugar's workaround should be integrated into the main code branch. If it's deemed a feature, then I believe it should be prominently highlighted multiple times in the documentation and examples, providing more information for those who encounter the problem.

Between the two, I would prefer it to be classified as a bug and have the workaround incorporated within the bgfx library. This ensures a consistent appearance of the API. Moreover, for pure C++ projects that don't use Objective-C, retaining an Objective-C file solely for the workaround is an unsightly implementation.

bkaradzic commented 11 months ago

@Kurapikov Create PR with fix you think it's appropriate...