-
Notifications
You must be signed in to change notification settings - Fork 510
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
base: master
Are you sure you want to change the base?
Conversation
…se to window specific struct
Cool stuff :) I only had a guick glance over the chances, but for now here's the first round of feedback:
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.
I think breaking backward compatibility is justified in this case because it's essentially a sokol_app.h 2.0
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.
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: Lines 3407 to 3512 in f047abc
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:
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): Lines 426 to 427 in f047abc
|
Good to know.
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 ( 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
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.
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.
Cool, would you like me to add the more complete version with
Lines 8420 to 8422 in 777b510
Places like this ^ It's a couple of pool access functions that are declared later.
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. |
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.
Yes, totally makes sense. Another thing that makes sense is to move the Lines 2210 to 2212 in f047abc
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.
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: Line 427 in f047abc
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 |
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. |
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. |
Any estimate when this might be merged? I'm really looking forward to multiple window support ❤️ |
@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. |
@stbachmann thanks for your engagement |
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 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. |
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 ;) |
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. |
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! |
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. |
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? |
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.
Currently not, I'm very much in a quite chaotic experimentation phase still :) |
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":
I'll investigate this first (also I need to check if I'm also seeing this also when using your branch and example code). |
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). |
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. |
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. |
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. |
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. |
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. |
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). |
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 ..
Certainly nice to have, but I could definitely live without that.
Sorry! :D |
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:
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). |
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!) |
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. |
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. |
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
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
_window
version. The old functions simply uses themain_window
to call into the new_window
variant. So for examplesapp_width()
will essentially call the new functionsapp_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 insapp_desc
into its ownsapp_window_desc
struct.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.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 :)