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_imgui.h: Automatically re-build sokol-gfx resources when Dear ImGui font data has changed. #1010

Merged
merged 2 commits into from
Mar 21, 2024

Conversation

elloramir
Copy link
Contributor

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
Copy link
Owner

floooh commented Mar 14, 2024

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
Copy link
Owner

floooh commented Mar 14, 2024

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: dd00e4d

...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
Copy link
Contributor Author

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
Copy link

Dvad commented Mar 18, 2024

@Dvad do you have an opinion on this PR?

I think this PR is a good idea!

@floooh
Copy link
Owner

floooh commented Mar 19, 2024

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

util/sokol_imgui.h Outdated Show resolved Hide resolved
util/sokol_imgui.h Outdated Show resolved Hide resolved
Copy link
Contributor Author

@elloramir elloramir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done!

@floooh
Copy link
Owner

floooh commented Mar 20, 2024

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

floooh added a commit that referenced this pull request Mar 21, 2024
@floooh floooh merged commit d70cba2 into floooh:master Mar 21, 2024
23 checks passed
@floooh
Copy link
Owner

floooh commented Mar 21, 2024

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!

@floooh floooh changed the title fix font loading to work with more fonts than default one sokol_imgui.h: Automatically re-build sokol-gfx resources when Dear ImGui font data has changed. Mar 21, 2024
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

3 participants