add callbacks for default gl framebuffer.

[danielchasehooper]

fixes #892

There is a simpler fix of adding this function, but I figured you might not want to add backend-specific public apis.

void sg_set_default_gl_framebuffer(GLuint framebuffer) {
    _sg.gl.cur_context->default_framebuffer = framebuffer;
}

This is nicer for the end user than the callback approach, because you don't have to create the callback and a global. and set the global before calling sg_begin_default_pass.

[danielchasehooper]

Ok looks like I'll have to edit gen_zig.py to fix the GitHub action error...before I invest more time in this, does this seem like you'll merge this PR once the tests pass?

[floooh]

I don't like this solution because it adds yet another fairly random backend-specific public API function. I think it's better to put this stuff as callbacks into the sg_context_desc struct, by reintroducing an sg_gl_context_desc gl; nested struct. That way it's consistent with the other platforms.

[danielchasehooper]

Ok, I think you misunderstood my PR message. This PR actually does reintroduce sg_gl_context_desc, check out the files changed. The PR message mentioned a simpler approach while acknowledging that you probably wouldn't like it.

[floooh]

You can omit the == NULL, or at least replace the NULL with 0 (yeah I know theoretically a NULL pointer doesn't have to be zero, but that's a very esoteric assumption).

(I would prefer removing the == NULL alltogether.

[danielchasehooper]

removing == NULL changes the meaning. I think you're thinking of != NULL being removable

[floooh]

ah right... I was confused because in other places I have asserts like this:

SOKOL_ASSERT(desc->context.d3d11.render_target_view_cb || desc->context.d3d11.render_target_view_userdata_cb);

...but in this case, one of the two callbacks must be set.

TL;DR: just replace the NULL with 0 and it's fine ;)

[danielchasehooper]

Replaced NULL with 0. I noticed there are 19 other places that NULL is used in sokol_gfx.h, how do you decide when to use NULL and when to use 0?

[floooh]

Ok, I think you misunderstood my PR message. This PR actually does reintroduce sg_gl_context_desc, check out the files changed. The PR message mentioned a simpler approach while acknowledging that you probably wouldn't like it.

Ah right, I didn't look at the actual changes, only saw that function. The simplified PR looks alright (but please check out my request above).

(PS: I probably shouldn't review PRs in the middle of the night lol)

Cheers!

[danielchasehooper]

I'll let you revisit this PR after a full night's rest before I dig into making changes to gen_zig.py :)

[floooh]

I'll let you revisit this PR after a full night's rest before I dig into making changes to gen_zig.py :)

Hmm, from what I'm seeing, this shouldn't be necessary any longer with that setter function removed (this had a GLuint type, which the bindings generator scripts would reject - but in any case, the correct solution in that case would be to remove the GLuint with an uint32_t, since no backend specific types must show up in the public API).

PS: CI is still failing though, weird. Anyway, I'm off for today :)

[floooh]

(note to self: sg_begin_default_pass_with_context(pass_action, width, height, sg_context_desc* ctx))?

PS: was thinking about getting rid of associating resource objects with 'sokol-gfx contexts', but this would break the previous "one GL context per sokol-gfx context" behaviour, since AFAIK not all GL object types can be shared between GL contexts.

...could probably add a config flag to sg_desc though, and then we could use the existing context functions to switch between different sets of callbacks and swapchain attributes (pixel formats and sample count).

Requires quite a bit of reworking the internals though :/

[danielchasehooper]

the correct solution in that case would be to remove the GLuint with an uint32_t, since no backend specific types must show up in the public API).

PS: CI is still failing though, weird. Anyway, I'm off for today :)

The PR as of right now does not have GLuint in the public API. The CI is failing because gen_zig.py doesn't support function pointers that return uint32_t, as is required by uint32_t (*default_framebuffer_cb)(void); I'll look into fixing that now.

[danielchasehooper]

(note to self: sg_begin_default_pass_with_context(pass_action, width, height, sg_context_desc* ctx))?

I like this idea. Much nicer than callbacks.

[floooh]

I need to think about this idea a bit more, but unrelated to that change, if we would make the association of resource objects with sg_contexts optional, would that help your use case?

In that situation, a context would basically just describe a separate default framebuffer by pixel formats, sample count and a separate set of callback functions and would only need to be set before a default pass (although that assumption might be a bit optimistic, and may require more untangling in the sokol-gfx implementation).

You would then create an sg_context for every 'output view' and switch to it right before the default pass:

    sg_activate_context(ctx);
    sg_begin_default_pass(...);
    ...
    sg_end_pass();

...unlike the GLFW multiwindow-sample there should only be a single sg_commit() call though (not sure yet if this would require more changes).

The sg_begin_default_pass_with_context() would then just be 'API sugar' which would take an sg_context handle instead, and just get rid of the sg_activate_context() pass.

But TBH, this might require more internal changes than I'm currently ready to integrate ;)

So I'd actually first like to go with a 'minimally invasive' procedure that works for your use case, and after that maybe continue working on a cleaner overall structure.

(e.g. another thing that popped into my mind was having a single sg_begin_pass() function which takes a parameter struct, which would be configured differently for different types of passes, e.g. an offscreen pass would be:

sg_begin_pass(&(sg_pass_options){
    .pass = pass_object,
    .action = { ... },
});

A vanilla default pass would look like this:

sg_begin_pass(&(sg_pass_options){
    .width = sapp_width(),
    .height = sapp_height(),
    .action = { ... },
});

...and a default pass into a specific context would look like this:

sg_begin_pass(&(sg_pass_options){
    .width = sapp_width(),
    .height = sapp_height(),
    .action = { ... },
    .context = ctx,
});

Also, a better name for sg_context and sg_context_desc would actually be sg_swapchain and sg_swapchain_desc.

These are big API changes that I don't want to do in a random 'drive-by' PR though, but I feel like this is would provide a much cleaner overall structure.

...btw, on GL, are you actually using a single GL context? If yes how does that work in the window system glue, what I've seen so far is that each 'view' has its own GL context (like in GLFW for instance).

[danielchasehooper]

If you want a minimally invasive approach that handles my use case, the changes in this PR as it stands now is that, and brings the GL API in line with the others (in terms of all having callbacks).

If we want to go with a more meaningful change, I think a single "begin pass" function that can be used for both offscreen and default passes is the nicest solution. That would allow user functions to accept a sg_pass_options parameter and draw to either offscreen or default framebuffer without special cases. In my codebase I have a function kind of like that called sg_begin_surface_pass(Surface *surface, sg_pass_action *action), where the surface can represent either the default framebuffer or an offscreen pass. Actually now that I'm thinking about it, we could add a sg_begin_pass_options(sg_pass_options *, sg_pass_action *) function (fairly non-invasively?) that sets a few internal state variables and then calls _sg_begin_pass. I feel like the pass action should be a separate parameter from the options struct since it's required and "what we're drawing to" is separate from "what we draw to it (load/clear)".

I think altering the behavior of sokol_gfx's contexts is less appealing because drawing to a different destination is conceptually more like a pass and less like GL's concept of a context. And like you said, contexts are sort of an old idea that doesn't fit well with newer apis, so you don't really want to invest more in that concept. Rendering to multiple destinations is something that I want to do with Metal too.

So in summery, (Depending on how much you want to alter the API) we could do one of these 3:

• stick with callbacks; accept this PR as is.
• add sg_begin_pass_options(sg_pass_options *, sg_pass_action *) (non-api-breaking)
• update sg_begin_pass to accept options struct, and remove sg_begin_default_pass (api-breaking)

---

...btw, on GL, are you actually using a single GL context? If yes how does that work in the window system glue, what I've seen so far is that each 'view' has its own GL context (like in GLFW for instance).

Yes, I use a single GL context. I overrode this function in CAOpenGLLayer to return the same global CGLContextObj for all layers

// from CAOpenGLLayer.h

/* Called by the CAOpenGLLayer implementation when a rendering context
 * is needed by the layer. Should return an OpenGL context with
 * renderers from pixel format 'pf'. The default implementation
 * allocates a new context with a null share context. */

- (CGLContextObj)copyCGLContextForPixelFormat:(CGLPixelFormatObj)pf;

Later I get called in - (void)drawInCGLContext:(CGLContextObj)glContext pixelFormat:(CGLPixelFormatObj)pixelFormat forLayerTime:(CFTimeInterval)timeInterval displayTime:(const CVTimeStamp *)timeStamp to do the actual drawing. the glContext is the same for all layers, but when I call CGLSetCurrentContext(glContext);, a unique GL_DRAW_FRAMEBUFFER_BINDING is set.

---

...unlike the GLFW multiwindow-sample there should only be a single sg_commit() call though (not sure yet if this would require more changes).

Given that each CAOpenGLLayer calls me when it needs redrawing, I'd have to find a way to run sg_commit() at the end of the current event loop in order to guarantee it is called after any/all windows did their drawing. Sort of messy, but I don't see a way to share mutable sg resources between windows without that requirement.

On another note, thanks for taking my feedback seriously and wanting to get this right.

[floooh]

If you want a minimally invasive approach that handles my use case, the changes in this PR as it stands now is that, and brings the GL API in line with the others (in terms of all having callbacks).

Oki doki, then let's do this as the first step! I guess I'll take another week for catching up on PRs and issues, before going back to the WebGPU backend.

Thanks for your input btw, it helps me to look differently at old ideas that had been baked into the API from the start. Thinking back I never was quite happy with the sg_begin_default_pass() function, I think a unified sg_begin_pass() with flexible options, and some sort of swapchain abstraction/wrapper is the way forward (maybe that will even motivate me to pick up the multiwindow branch again lol)

[floooh]

Looking into this now...

[floooh]

...and merged! I also upated a small blurb to the changelog.

[floooh]

...oops, something broke in the Rust bindings, need to figure out why this doesn't trigger in the regular tests. I'm on it...

https://github.com/floooh/sokol/actions/runs/6285075508/job/17067124611#step:5:113

[floooh]

...ok, this was just a missing -> before the return type. Still, I need to figure out why this didn't show up in the CI pipeline.

[floooh]

...heh ok the test-rust step in the Rust bindings GH Actions workflow file used the wrong artifact download path, also explains some weird behaviour I saw sporadically in the past.

[danielchasehooper]

Thanks for accepting the PR! Looking forward to helping with #904 next!