Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sokol multi-window support #437

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

stbachmann
Copy link

@stbachmann stbachmann commented Nov 30, 2020

This pull request adds basic multi-window support across sapp/sg/glue. Definitely still in a bit of a rough state and needs testing. Since this would be a pretty substantial change to sapp, I can totally understand if you don't want to merge this. But I wanted to at least present it as an option and open up the discussion around it. I tried to stick to most of your thoughts presented in #229

Basic usage

sapp_window second_window = sapp_create_window(&(sapp_window_desc){
        .window_title = "Second Window!"
});

sg_context_desc desc = sapp_window_sgcontext(second_window);
sg_context second_context = sg_setup_context(&desc);

sapp_destroy_window(second_window);

Events now contain an additional window variable for identifying. I've added a barebones multi-window example here multi-window-sapp.c

Some things to note/discuss

  • What's working: win/uwp/macos/emsc/linux have all been tested and are working with d3d/gl/mtl. WGPU and android/ios compile, but I haven't tested things properly yet. I currently can't get the wgpu backend to compile (probably just a wrong sdk version).
  • What's missing: There are some missing functions we probably want to add to make it properly useful such as show/hide window, etc. I've basically just taken the shortest path to getting multi-window support across all backends for now.
  • Backwards compatibility: I've kept the API completely backwards compatible. This means all the samples compile out of the box without any changes required. But, it also means the API is quite a bit more bloated than it needs to be. Most functions that are window related now have a _window version. The old functions simply uses the main_window to call into the new _window variant. So for example sapp_width() will essentially call the new function sapp_window_width(main_window). If you're happy to make some breaking changes, we could simplify the API significantly and clean up quite a bit of bloat. For example we could push all the window specific stuff in sapp_desc into its own sapp_window_desc struct.
  • macOS: This ended up being the most involved change. The NSOpenGLView that was previously used does not play nice when using multiple gl contexts. I ended up re-implementing the backend with a custom NSView similar to what glfw does. As a consequence of this, we now have a similar core while(!_sapp.quit_ordered) loop like the other backends (rather then doing the update via drawRect). Again, most of this is inspired by the glfw implementation.
  • Pools: the windows use a slightly simplified version of your sg pools (I'm not using resource states). This still feels a little bit bloaty for something that most likely doesn't really happen a lot in an applications life cycle? Happy to adapt this to something simpler (like a linked list) if you'd prefer. Especially on backends that don't permit multi-window, it seems like a wasteful approach.
  • I've had to forward declare functions in some spots (sokol_gfx.h). If we want to avoid that, it'd mean shuffling around the code order quite a bit in some places. Happy to do so, but felt like the forward declaration was the better path until discussed.

I'm sure there's quite a few things that can be improved and need fixing/discussion. But I think it's a solid starting point for further work. Happy to hear any thoughts/feedback :)

@floooh
Copy link
Owner

floooh commented Dec 1, 2020

Cool stuff :) I only had a guick glance over the chances, but for now here's the first round of feedback:

I currently can't get the wgpu backend to compile

Yes that's expected, the WGPU code is way behind the latest API changes. I'll wait until the WGPU API has settled a bit and the new shading language is implemented before updating the WGPU backend code, otherwise it's just chasing lots of little changes.

Backwards compatibility...

I think breaking backward compatibility is justified in this case because it's essentially a sokol_app.h 2.0

...NSOpenGLView...

I wonder if we can finally kick out GL support on Mac completely... I already had the macOS GL backend removed after it was broken with Catalina, but then @gustavolsson brought it back from the dead early 2019 ;) The more the GL backend code differs from the Metal backend, the harder it will be to maintain (but I haven't looked in detail yet how much the code differs, just had a quick glance over the changes).

...But looking at the code the Metal backend is now also using the new event loop? If it works it's fine I guess, but I had quite a few problems with this approach in the past (like stuttering frame rate, high CPU usage, or problems with window resizing), mostly after OS updates where Apple changed things around. This is definitely an area I'll need to test and meditate about a bit :)

Are those event-loop changes only required because of NSOpenGLView, or did MTKView also have other problems with the multi-window changes? If MTKView would also work with the previous approach I'd prefer to separate the GL- and Metal-code more so that only the GL backend uses the custom draw/event-loop, or otherwise, this would also be an argument to remove GL support from the macOS backend.

Pools...

Yeah I'm aware of the problem because I've also copied the pool code from sokol_gfx.h into various other headers (the latest version is here in sokol_debugtext.h:

sokol/util/sokol_debugtext.h

Lines 3407 to 3512 in f047abc

/*=== CONTEXT POOL ===========================================================*/
static void _sdtx_init_pool(_sdtx_pool_t* pool, int num) {
SOKOL_ASSERT(pool && (num >= 1));
/* slot 0 is reserved for the 'invalid id', so bump the pool size by 1 */
pool->size = num + 1;
pool->queue_top = 0;
/* generation counters indexable by pool slot index, slot 0 is reserved */
size_t gen_ctrs_size = sizeof(uint32_t) * pool->size;
pool->gen_ctrs = (uint32_t*) SOKOL_MALLOC(gen_ctrs_size);
SOKOL_ASSERT(pool->gen_ctrs);
memset(pool->gen_ctrs, 0, gen_ctrs_size);
/* it's not a bug to only reserve 'num' here */
pool->free_queue = (int*) SOKOL_MALLOC(sizeof(int)*num);
SOKOL_ASSERT(pool->free_queue);
/* never allocate the zero-th pool item since the invalid id is 0 */
for (int i = pool->size-1; i >= 1; i--) {
pool->free_queue[pool->queue_top++] = i;
}
}
static void _sdtx_discard_pool(_sdtx_pool_t* pool) {
SOKOL_ASSERT(pool);
SOKOL_ASSERT(pool->free_queue);
SOKOL_FREE(pool->free_queue);
pool->free_queue = 0;
SOKOL_ASSERT(pool->gen_ctrs);
SOKOL_FREE(pool->gen_ctrs);
pool->gen_ctrs = 0;
pool->size = 0;
pool->queue_top = 0;
}
static int _sdtx_pool_alloc_index(_sdtx_pool_t* pool) {
SOKOL_ASSERT(pool);
SOKOL_ASSERT(pool->free_queue);
if (pool->queue_top > 0) {
int slot_index = pool->free_queue[--pool->queue_top];
SOKOL_ASSERT((slot_index > 0) && (slot_index < pool->size));
return slot_index;
}
else {
/* pool exhausted */
return _SDTX_INVALID_SLOT_INDEX;
}
}
static void _sdtx_pool_free_index(_sdtx_pool_t* pool, int slot_index) {
SOKOL_ASSERT((slot_index > _SDTX_INVALID_SLOT_INDEX) && (slot_index < pool->size));
SOKOL_ASSERT(pool);
SOKOL_ASSERT(pool->free_queue);
SOKOL_ASSERT(pool->queue_top < pool->size);
#ifdef SOKOL_DEBUG
/* debug check against double-free */
for (int i = 0; i < pool->queue_top; i++) {
SOKOL_ASSERT(pool->free_queue[i] != slot_index);
}
#endif
pool->free_queue[pool->queue_top++] = slot_index;
SOKOL_ASSERT(pool->queue_top <= (pool->size-1));
}
static void _sdtx_setup_context_pool(const sdtx_desc_t* desc) {
SOKOL_ASSERT(desc);
/* note: the pool will have an additional item, since slot 0 is reserved */
SOKOL_ASSERT((desc->context_pool_size > 0) && (desc->context_pool_size < _SDTX_MAX_POOL_SIZE));
_sdtx_init_pool(&_sdtx.context_pool.pool, desc->context_pool_size);
size_t pool_byte_size = sizeof(_sdtx_context_t) * _sdtx.context_pool.pool.size;
_sdtx.context_pool.contexts = (_sdtx_context_t*) SOKOL_MALLOC(pool_byte_size);
SOKOL_ASSERT(_sdtx.context_pool.contexts);
memset(_sdtx.context_pool.contexts, 0, pool_byte_size);
}
static void _sdtx_discard_context_pool(void) {
SOKOL_ASSERT(_sdtx.context_pool.contexts);
SOKOL_FREE(_sdtx.context_pool.contexts);
_sdtx.context_pool.contexts = 0;
_sdtx_discard_pool(&_sdtx.context_pool.pool);
}
/* allocate the slot at slot_index:
- bump the slot's generation counter
- create a resource id from the generation counter and slot index
- set the slot's id to this id
- set the slot's state to ALLOC
- return the resource id
*/
static uint32_t _sdtx_slot_alloc(_sdtx_pool_t* pool, _sdtx_slot_t* slot, int slot_index) {
/* FIXME: add handling for an overflowing generation counter,
for now, just overflow (another option is to disable
the slot)
*/
SOKOL_ASSERT(pool && pool->gen_ctrs);
SOKOL_ASSERT((slot_index > _SDTX_INVALID_SLOT_INDEX) && (slot_index < pool->size));
SOKOL_ASSERT((slot->state == SG_RESOURCESTATE_INITIAL) && (slot->id == SG_INVALID_ID));
uint32_t ctr = ++pool->gen_ctrs[slot_index];
slot->id = (ctr<<_SDTX_SLOT_SHIFT)|(slot_index & _SDTX_SLOT_MASK);
slot->state = SG_RESOURCESTATE_ALLOC;
return slot->id;
}
/* extract slot index from id */
static int _sdtx_slot_index(uint32_t id) {
int slot_index = (int) (id & _SDTX_SLOT_MASK);
SOKOL_ASSERT(_SDTX_INVALID_SLOT_INDEX != slot_index);
return slot_index;
}
), but I prefer this over other solutions even for a small number of items. I'd rather come up with a more compact and simpler pool implementation which is easier to copy and adapt for other libraries (however I don't want to move the pool code into a separate header which other headers then need to depend on, at least for the 'core headers').

I've had to forward declare functions in some spots (sokol_gfx.h).

I had a guick glance over the changes and didn't see any forward decls(?) But if it's all within the IMPL block that's fine.

Another very minor thing:

sg_context sg_setup(const sg_desc* desc);

I haven't made up my mind yet whether the return value is a good idea. In sokol_debugtext.h I have a special constant for the default context instead (which is simply hardwired to the first created resource handle, so there's no special detection needed in the rest of the code - except for a check in the destroy-function which doesn't allow to destroy the default context):

/* the default context handle */
static const sdtx_context SDTX_DEFAULT_CONTEXT = { 0x00010001 };

@stbachmann
Copy link
Author

Yes that's expected, the WGPU code is way behind the latest API changes. I'll wait until the WGPU API has settled a bit and the new shading language is implemented before updating the WGPU backend code, otherwise it's just chasing lots of little changes.

Good to know.

I think breaking backward compatibility is justified in this case because it's essentially a sokol_app.h 2.0

In that case I'll make a couple of changes to clean up some bits.

How do you feel about keeping two versions of window related functions around (sapp_width() & sapp_window_width(window_id))? My concern is that if we drop the old api, we'll add a lot of boiler plate code to applications that only use a single window as they'd have to call all the window functions with something like sapp_window_width(sapp_main_window())

Something else (maybe) worth considering since we're breaking api compatibility: It would keep quite a few things tidier on the front-end (and in some cases backend) if each window were to have separate callbacks (their own init_cb, frame_cb). I think from an API user point of view, this makes sense in most situations as you'd want to handle events for a specific window separately anyway.

I wonder if we can finally kick out GL support on Mac completely...

Can totally understand if you want to kick it out, it'd certainly make things easier. The differences aren't huge right now (the NSView changes maybe led to an additional 50 loc, mostly the event loop which currently both gl+mtl use). I think quite a few people would miss the gl option on mac.

...But looking at the code the Metal backend is now also using the new event loop?

Yes. The MTKView didn't make any problems and we could keep using it as the main loop driver. It'd mean having a bit of an awkward separation of logic in the macos backend. At that point it'd probably make sense to just separate the two entirely. More code, but probably easier to maintain. I opted for a unified approach as I didn't have any immediate issues, but I've certainly not done any extensive testing. It feels "hack-ish" for sure.

Pools ..

Cool, would you like me to add the more complete version with SG_RESOURCESTATE/slot? I've basically removed the slot from
your implementation and currently just store the slot->id directly.

Fwd decl

sokol/sokol_gfx.h

Lines 8420 to 8422 in 777b510

// DISCUSS: Had to to forward declare this method here :/
_SOKOL_PRIVATE _sg_context_t* _sg_lookup_context(const _sg_pools_t* p, uint32_t ctx_id);
_SOKOL_PRIVATE void _sg_d3d11_begin_pass(_sg_pass_t* pass, const sg_pass_action* action, int w, int h) {

Places like this ^ It's a couple of pool access functions that are declared later.

I haven't made up my mind yet whether the return value is a good idea.

Happy to change it :) Let me know what you'd like to do.

In general, happy to massage this whole thing for a while. Definitely quite a few changes that need thought/testing.

@floooh
Copy link
Owner

floooh commented Dec 1, 2020

How do you feel about keeping two versions of window related functions around

Yeah I think it makes sense to have two functions there, one "shortcut" function with implicit default window handle, and one that takes a window handle.

...each window were to have separate callbacks

Yes, totally makes sense. sapp_window_desc should have callback pointers like in sapp_desc, and if those per-window callbacks are zero, the main-window-callbacks should be taken instead (I guess?).

Another thing that makes sense is to move the sapp_window_desc struct for the main window into sapp_desc (I think that's what you mean with "For example we could push all the window specific stuff in sapp_desc into its own sapp_window_desc struct."?). Same way it is done with the 'default context' in sg_desc:

sokol/sokol_gfx.h

Lines 2210 to 2212 in f047abc

int sampler_cache_size;
sg_context_desc context;
uint32_t _end_canary;

would you like me to add the more complete version with SG_RESOURCESTATE/slot?

If you don't need the resource state just remove it along with the slot struct, looking at the sokol_debugtext.h header, the resource state is kinda pointless there because it only toggles between two states, ALLOC and INITIAL.

Places like this ^ It's a couple of pool access functions that are declared later.

Ah alright, that's fine. If it bugs me later for some reason I can still change it around in some random code cleanup session.

About the sg_setup() return value, I think we should definitely do this default-context thing anyway:

static const sdtx_context SDTX_DEFAULT_CONTEXT = { 0x00010001 };

This doesn't collide with the sg_setup() return value, but with such a default-context constant application code doesn't need to keep the return value around.

...there's also some overlap with the sapp_main_window() function... this could also be a hardwired constant. But I guess it's better if I wait a bit (before insisting on a change) and play around with the API a bit to get a feel for it :)

@stbachmann
Copy link
Author

Oh another thing: I didn't bother looking into multi-window support for UWP. After reading a bit up on the api and looking at the backend I decided to run the other way. I did update everything to work with the new structure, just doesn't support >1 windows.

@floooh
Copy link
Owner

floooh commented Dec 2, 2020

Yeah, multi-window on UWP definitely isn't a priority. I guess the only interesting use case for UWP is running on Xbox One anyway. Don't worry about this stuff.

@serkonda7
Copy link

Any estimate when this might be merged? I'm really looking forward to multiple window support ❤️

@stbachmann
Copy link
Author

@serkonda7 there's quite a bit of work left to do on my side. I'm going to get back to this over the Christmas break and hopefully bring it closer to a mergeable state.

@serkonda7
Copy link

@stbachmann thanks for your engagement

@ikrima
Copy link
Contributor

ikrima commented Jan 31, 2021

This is fantastic. I finally got around to looking at it today after wasting a day on a bug caused by own commented hack that i forgot about
Is the repo branch the correct one to send some minor fixes? (some bugs from the existed baseline I found e.g.

if (IsIconic(window->win32.hwnd)) {
  Sleep(16 * window->swap_interval);
}

probably isn't what's intended since that'll sleep on every minimized window and serially

@stbachmann
Copy link
Author

This is fantastic. I finally got around to looking at it today after wasting a day on a bug caused by own commented hack that i forgot about
Is the repo branch the correct one to send some minor fixes? (some bugs from the existed baseline I found e.g.

if (IsIconic(window->win32.hwnd)) {
  Sleep(16 * window->swap_interval);
}

probably isn't what's intended since that'll sleep on every minimized window and serially

Ah yea, that definitely doesn't look right.

I've unfortunately not had much time to work on this lately so it's been dormant :( But any contributions/fixes are definitely welcome. Looks like it'd also need some fixes to be in line with the current master again.

@floooh
Copy link
Owner

floooh commented Apr 14, 2021

Ok, I think the time has come to tackle this PR ;)

I'll use the PR as a starting point for a new branch where I'll update the current version to the latest sokol_app.h changes, and also try to make up my mind how to proceed.

One minor thing I've been thinking about is whether those additional functions which take a window id are actually needed. Since sokol_app.h communicates with user code exclusively through a few callbacks, an implicit "current window" could be set before the user callbacks are invoked (e.g. sapp_width() wouldn't return the width of the main window, but the width of the "current window" the callback has been called for).

The explicit functions which take a window id might still be useful when looking up properties of another (than the current callback's) window, but I think they are not essential.

Maybe it's a stupid idea, but I guess I'll find that out when tinkering with the code ;)

@floooh
Copy link
Owner

floooh commented Apr 15, 2021

Ufff, resolving merge collisions manually all at once is too brittle and dangerous, the master branch has evolved quite a bit since the PR :D

I'll try to come up with a way to start from current master and incrementally 'retrace' your steps @stbachmann, while still properly tracking you as contributor in the git history. Maybe I can cherry-pick your unique commits and fix collisions on those instead of doing one big collision cleanup on the end result, that seems a lot safer.

@stbachmann
Copy link
Author

No problem @floooh - I don't specifically care about being in there. So do whatever makes most sense :) I have a bit of time opening up in the next few weeks, so I might be able to help with some of this as well!

@floooh
Copy link
Owner

floooh commented Apr 21, 2021

Btw, one thing I noticed yesterday while toiling away on the macOS/Metal backend in sokol_app.h is that the frame-semaphore in sokol_gfx.h which synchronizes CPU/GPU resource access needs to be per context (or rather per swapchain / MTKView), otherwise the frame synchronization will get confused when rendering in multiple windows - thankfully I'm getting error messages from Metal about invalid resources on the debug console, otherwise I probably would have missed this.

I've moved the init/frame/cleanup/event callbacks into the window-desc btw, so that each window can have its own callbacks (if the user choses so), and before the callbacks are called, the "current window" is set. This allows to leave the public sokol_app.h API largely unchanged, e.g. functions don't need to be duplicated for taking an explicit window handle, instead all functions called from inside the callbacks work on the "current window". So far this seems to work quite well, but I'm also quite early in the process.

I think I'll also need to do something about how sokol-gfx contexts work. Currently all resources are per-context, but that's not all that useful, and only "required" for OpenGL (where each window has its own GL context), but on the other hand, AFAIK only buffers and textures can be shared between GL contexts. I will need to rummage around in the Dear ImGui multi-viewports branch how the examples there actually handle that. But I'll first do Metal and D3D11 I think.

@stbachmann
Copy link
Author

stbachmann commented Apr 21, 2021

I've moved the init/frame/cleanup/event callbacks into the window-desc btw, so that each window can have its own callbacks (if the user choses so), and before the callbacks are called, the "current window" is set. This allows to leave the public sokol_app.h API largely unchanged, e.g. functions don't need to be duplicated for taking an explicit window handle, instead all functions called from inside the callbacks work on the "current window". So far this seems to work quite well, but I'm also quite early in the process.

I did actually consider this as well as an alternative. But I wasn't really sure whether it'd be better in the end. I figured there'd be situations where you still want to be able to explicitly access another window. But this approach would certainly keep the api cleaner and probably still cover most use cases.

Since you are making some bigger changes to the backend already.. Would it be worth considering separating the ogl macOS implementation? It could maybe live in an optional header in /utils that can be used by people who want to use opengl on mac (might need some thought how to separate it so it's not too messy). But it'd certainly allow for a much cleaner macOS backend in the core. I'm a bit worried about the changes I made with the custom NSView implementation and it causing issues in the future since it's breaking the idiomatic way to handle the app lifetime cycle.

Is there a meaningful way to contribute with all this atm?

@floooh
Copy link
Owner

floooh commented Apr 21, 2021

Would it be worth considering separating the ogl macOS implementation?

It might make sense to separate the macOS GL backend more instead of interleaving the code directly with the Metal backend. I wouldn't put it into a separate header, but I'll think about a good approach when getting to the GL multiwindow stuff.

Is there a meaningful way to contribute with all this atm?

Currently not, I'm very much in a quite chaotic experimentation phase still :)

@floooh
Copy link
Owner

floooh commented Apr 21, 2021

Hmm, moving the Metal semaphore into _sg_context_t, so that each swapchain gets its own semaphore doesn't fix that error I'm seeing, strange.

I'm getting this 4x when resizing a window in a "multi-window-scenario":

Execution of the command buffer was aborted due to an error during execution. Invalid Resource (IOAF code 9)

I'll investigate this first (also I need to check if I'm also seeing this also when using your branch and example code).

@floooh
Copy link
Owner

floooh commented Apr 22, 2021

Hmm... having multiple begin_default_pass/end_pass/commit per "frame" causes all sorts of complications in the sokol-gfx Metal backend I haven't thought of (mainly related to the synchronizatin needed for dynamic resources, currently this shouldn't work at all, because the "frame count" increases too fast (in each sg_commit), so that dynamic resources that are still in flight will be overwritten).

Moving all this "global" per-frame stuff into the context struct theoretically works, but is deeply "unsatisfying", and requires rewriting the entire garbage collection code for ObjC objects (among other things), plus this couples dynamic resource to their contexts (e.g. resource objects must be updated and destroyed on the same context they've been created on)... this "resources only work in the context they've been created in" restriction isn't all that great to begin with...

And I still see this mysterious Metal error message when resizing windows, despite also having moved the uniform buffers into the context struct.

I feel like I need to go back a few steps and rethink completely how sokol-gfx could work in a multi-window environment. Maybe it's better to render window content into offscreen render targets, and then just blit them into the window's swapchain.

And I need to have a closer look at the Dear ImGui multi-viewport examples and rendering backends (unfortunately I haven't found much information / examples about rendering with multiple MTKViews so far).

@stbachmann
Copy link
Author

And I still see this mysterious Metal error message when resizing windows, despite also having moved the uniform buffers into the context struct.

Is there a specific sample you're running? I tried it yesterday on my (outdated OS) macbook and didn't encounter this issue.

FWIW, I totally understand if you decide not to add multi-window to sokol_app. It obviously increases the complexity significantly.

@floooh
Copy link
Owner

floooh commented Apr 23, 2021

Is there a specific sample you're running?

It's happening in my own experimental branch, I haven't checked on your branch and with your example yet, but that's what I'll do next (it could also be a side effect of running the latest macOS Betas, there's a couple other new Metal warnings at startup which don't really make sense to me and which I currently put aside as "must be a temporary Beta problem").

I also checked in the Dear ImGui docking branch (which has the multi-viewport feature), but this only has multi-window examples for GL on macOS, the Metal example doesn't support multiple windows.

I definitely want to support multi-window in sokol-app, because I really want sokol_app.h, sokol_gfx.h and Dear ImGui to fit perfectly together.

But I need to put more time than expected into making sokol_gfx.h properly "multi-context-aware" (or maybe "multi-swapchain-aware" is the better word), mainly in the area how resources and contexts relate to each other, and how the restrictions for updating dynamic resources are going to work. Especially the Metal backend will need some work.

@floooh
Copy link
Owner

floooh commented Apr 23, 2021

PS: I think my idea to render into offscreen render targets and then only blit those into the target window wasn't all that great in hindsight, that's also not how the Dear ImGui samples are working.

@floooh
Copy link
Owner

floooh commented Apr 23, 2021

Alright, I'm not getting any errors when running your branch as is, with your example and the Metal backend. This is very encouraging :)

I'll dig into the differences to my code next. The only obvious difference I'm seeing at the moment is the different draw- and event-loop model.

@floooh
Copy link
Owner

floooh commented Apr 24, 2021

Just stumbled over another unexpected problem both with the GLFW-style render loop (explicitely calling [MTKView draw], and the "traditional method" of running MTKView automatically: with two windows open, rendering runs at half the refresh rate (presumably because each window waits for vsync). E.g. when I'm running your demo, the "clear loop" doubles in speed when closing one window (it also happens in my code without switching the explicit render loop).

Hrmpf... this is becoming quite the unexpected rabbit hole :D

I guess I'll have a look at dropping MTKView next, and using CAMetalLayer directly.

PS: ...hmm... or maybe it's because I'm using one MTLCommandQueue for both windows(?) if the presentDrawable command blocks until vsync, this would also cut the frame rate in half.

I'll check that first by giving each sokol-gfx context its own queue.

@floooh
Copy link
Owner

floooh commented Apr 24, 2021

Yep, that was it. Each window/context needs its own MTLCommandQueue. Bullet dodged ;)

Next problem I need to solve: Using the GLFW-style explicit render loop, rendering stops while resizing the window. Using MTKView in "automatic mode" somehow manages that rendering continues while resizing.

I'd like to switch to the GLFW-style render loop because that guarantees that each window is rendered "round-robin-style", which I'm now using in sokol-gfx to check when a frame is actually "complete" in sg_commit() (I'm keeping track of each context's/windows's sg_commit() in a bitmask, and when each context had their sg_commit() called, this means a new frame is about to start, this is needed for properly keeping track of resources that are currently in flight).

Letting the MTKViews do their own timing sometimes leads to the per-window frame-callback of the same window being called twice (or the other window's frame callback being "skipped" from time to time).

@floooh
Copy link
Owner

floooh commented Apr 24, 2021

Also interesting... using one MTLCommandQueue per window also makes that original error go away, even if not using the GLFW-style render loop... I guess using one queue per MTKView is a good thing nonetheless.

@stbachmann
Copy link
Author

stbachmann commented Apr 24, 2021

Also interesting... using one MTLCommandQueue per window also makes that original error go away, even if not using the GLFW-style render loop... I guess using one queue per MTKView is a good thing nonetheless.

Oh interesting! The kind of things you'd think should be documented somewhere ..

Next problem I need to solve: Using the GLFW-style explicit render loop, rendering stops while resizing the window. Using MTKView in "automatic mode" somehow manages that rendering continues while resizing.

Certainly nice to have, but I could definitely live without that.

Hrmpf... this is becoming quite the unexpected rabbit hole :D

Sorry! :D

@floooh
Copy link
Owner

floooh commented Apr 25, 2021

Ok, slight change of plans...

I think I'll go back to "application-wide callbacks", not per-window callbacks.

The frame callback would need to look like this:

void frame(void) {
    for each window:
        sg_activate_context(window.context);
        sg_begin_default_pass(...);
        ...
        sg_end_pass();
    }
    sg_commit()
}

The important part is that there's only a single sg_commit() call as the one central place to tell sokol-gfx that all work for all windows of the current "application frame" has been submitted. If this works as intended I can keep the resource-tracking in sokol-gfx (which depends on a single frame counter) as is (because keeping track of resources per context is quite the hassle).

This implies switching to the "GLFW-style" renderloop in sokol_app.h though, and for this I really need to figure out how to continue rendering during window resize (maybe I should also look into using CAMetalLayer and CVDisplayLink directly instead of MTKViews - but if at all possible I'd rather keep MTKView).

@edam
Copy link

edam commented Sep 12, 2022

This work may be at risk of getting a bit stale... Are there still plans to merge it @floooh? Anything I can do to help?

(Interested for V's UI library!)

@floooh
Copy link
Owner

floooh commented Sep 13, 2022

I had started a separate branch here, based on ideas from this PR: #526, but it turned out to be way too much work on the Metal backend, which also leaked into sokol_gfx.h's context handling.

I gave up when I encountered an 'unfixable issue' on macOS where the entire thread is blocked for about one second when one window becomes fully obscured (this also happens right now in sokol_app.h, but since the single app window is obscured, the 1 second pause isn't visible)... and there was also a crash, as detailed in the open todo points (PS: this one second pause when a window is obscured also happens in the "official" Metal sample - at least when I checked this at the time, so it's not actually sokol_app.h specific).

Because of that weird pause I also tinkered with dropping MTKView and going down to CAMetalLayer, but it turned out that the problems is actually caused by CAMetalLayer (the nextLayer method runs into a timeout).

Eventually I want to port some of the new window manipulation functions over (for instance to programmatically change the window size and position, enable/disable window chrome, etc...), and maybe drop MTKView and go down to CVDisplayLink and CAMetalLayer, but still only manage a single window. This would at least ease the way a bit in case there's a new multi-window approach.

@kochol
Copy link
Contributor

kochol commented Apr 11, 2024

Do you have any plans for multi-window support in the future?

@floooh
Copy link
Owner

floooh commented Apr 11, 2024

Do you have any plans for multi-window support in the future?

Not in the immediate future. But the recent 'begin pass unification' changes should make the next attempt a bit easier.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants