floooh / sokol

minimal cross-platform standalone C headers
https://floooh.github.io/sokol-html5
zlib License
6.54k stars 469 forks source link

sokol_imgui.h: Automatically re-build sokol-gfx resources when Dear ImGui font data has changed. #1010

Closed elloramir closed 3 months ago

elloramir commented 3 months ago

Every time you invoke the io->AddFontXXX functions, imgui invalidates the default atlas texture, as it rebuilds the texture packer and needs to reload the new atlas on the GPU.

floooh commented 3 months ago

Hmm... that change would leak textures and samplers because they wouldn't be destroyed anywhere.

I think the intention is to call the newly exposed functions simgui_create_fonts_texture() and simgui_destroy_fonts_texture() functions around the ImGui AddTexture...() calls, e.g.

    simgui_destroy_fonts_texture();
    // ...
    io.Fonts->AddFontFromMemoryTTF(dump_font, sizeof(dump_font), 16.0f, &fontCfg)
    simgui_create_fonts_texture(...);

...apart from that, it would probably be a good idea though if simgui_create_fonts_texture() either destroys the previous texture and samplers, or at least asserts that there currently is no sampler and texture.

I think I'll add the assert, otherwise it's to easy to accidentially leak textures.

@Dvad do you have an opinion on this PR?

floooh commented 3 months ago

I have added a couple of assert to protect from textures leaking in when simgui_create_fonts_texture() is called when the texture already exists: https://github.com/floooh/sokol/commit/dd00e4d3a5af2461e117f714ee8fce432fc37c2f

...I'm tending towards requiring the user to call those functions explicitly around AddFontXXX() instead of automatically in the simgui_new_frame() though, but I'm open for discussion ;)

elloramir commented 3 months ago

Oh, it did not come to mind that kind of leak 😅. I think the automatic way is more aligned with other official implementations present in the DearImgui project, but calling it explicitly is also a fair choice. I do prefer the implicit one because the way DearImgui presents the API suggests that the implementation might know how to handle these cases, but any choice for me sounds totally okay.

Dvad commented 3 months ago

@Dvad do you have an opinion on this PR?

I think this PR is a good idea!

floooh commented 3 months ago

I think this PR is a good idea!

Ok noted :) Then we should come up with a solution that works both implicitly during simgui_new_frame() (how this PR implements it), or by manually calling the simgui_create/destroy_fonts_texture()

@elloramir: I'm going to add a couple of minor change requests

floooh commented 3 months ago

Looks good, thanks! I'll try to do a bit of testing and the merge by tomorrow evening.

floooh commented 3 months ago

Ok merged! I tested with imgui-highdpi-sapp by removing the explicit call to simgui_create_fonts_texture() and it works as expected. I also added a changelog item.

Thanks for the PR!