Skip to content

Conversation

@pusewicz
Copy link
Contributor

This is the attempt to fix the issues with the samples on macOS.

I saw that in the s_draw_report, after calling cf_draw_elements(), we also call cf_commit(). Following that approach in the s_blit() makes the draw_to_texture work:

image

Opening this as draft, as I'm not sure this is the correct way to fix this problem, and I'm not sure what the implications are.

Additionally, the other shatter demos have the same issue. I wonder if cf_commit() should be part of cf_draw_elements()?

This extra commit ensures that the demos work on macOS

Same error as in RandyGaul#286
@RandyGaul
Copy link
Owner

RandyGaul commented Oct 15, 2025

Seems like we should perhaps merge draw elements and commit together. Historically commit came from copying sokol_gfx's API design and just mirroring it 1:1 without much independent design consideration.

I rather like @bullno1's suggestion:

cf_draw_elements also feels like it's basically forced since it's the only call that makes sense after cf_apply_shader. What if instead of cf_apply_mesh(mesh), it's cf_draw_elements(CF_Mesh mesh) after cf_apply_shader?

Putting a little more thought into it, commit at this time is acting like "I'm done with this pass" which not what it did in sokol_gfx. In CF we have:

void cf_sdlgpu_commit()
{
	SDL_EndGPURenderPass(g_ctx.canvas->pass);
}

I do agree the weirdness seems to be draw elements is separated from mesh. Though, in the case of static meshes it is entirely possible to want to apply a mesh once and then swap textures or maybe even shaders for different instances, but, perhaps that's just not worth an entire other API call.

From sokol_gfx API sg_commit meant end of frame, so I don't think we should use that name or have an equivalent public function.

In CF the "end of frame" that a commit style API would usually cover is automagically handled in cf_app_draw_onto_screen even when using the low level cute_graphics.h header directly, basically handled by our own blit function and end frame function, e.g. cf_gles_end_frame or cf_gles_blit_canvas. So we don't need a public commit.


My tentative rec (would appreciate comments from others):

  • Nix cf_commit
  • Nix cf_apply_mesh
  • Send in CF_Mesh to cf_draw_elements
  • Move old cf_commit code to end of cf_draw_elements

Thoughts?

@RandyGaul
Copy link
Owner

So it would be in summary comment form in cute_graphics.h

 *     for each canvas {
 *         cf_apply_canvas(canvas);
 *         for each material {
 *             cf_material_set_uniform_vs(material, ...);
 *             cf_material_set_uniform_fs(material, ...);
 *             for each shader {
 *                 cf_apply_shader(shader, material);
 *                 cf_draw_elements(mesh);
 *             }
 *         }
 *     }

@ogam
Copy link
Contributor

ogam commented Oct 15, 2025

Sounds good, majority (maybe all?) of cases you have a mesh to pass in anyways so cutting off a couple lines makes this less error prone.

This change inlines cf_commit into cf_draw_elements so that it does not need to be called separately.
@pusewicz pusewicz marked this pull request as ready for review October 15, 2025 05:14
@pusewicz
Copy link
Contributor Author

@RandyGaul The nixing of the cf_apply_mesh is more invasive, so going to hold on that. We can continue the discussion in #403.

@RandyGaul RandyGaul merged commit 85908f5 into RandyGaul:master Oct 15, 2025
6 checks passed
@pusewicz pusewicz deleted the draw_to_texture-fix branch October 15, 2025 16:57
theopechli pushed a commit to theopechli/cute_framework that referenced this pull request Oct 18, 2025
* Commit in blit

Closes RandyGaul#286

* Fix shatter samples

This extra commit ensures that the demos work on macOS

Same error as in RandyGaul#286

* Inline cf_commit

This change inlines cf_commit into cf_draw_elements so that it does not need to be called separately.

* Update docs after cf_commit removal
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.

3 participants