mosra / magnum-integration

Integration libraries for the Magnum C++11 graphics engine
https://magnum.graphics/
Other
97 stars 44 forks source link

ImFontAtlas::TexID remains unset after a call to ImGuiIntegration::Context::relayout #60

Closed rune-scape closed 4 years ago

rune-scape commented 4 years ago

ImGuiIntegration::Context::relayout() creates the GL::Texture2D for the font atlas, but does not set the TexID in the ImFontAtlas object.

I may have just missed something, but I couldn't find any way to access the imgui font atlas texture.

Simple fix tho!

https://github.com/mosra/magnum-integration/blob/43e86c2a1e83caf9b4da42e00c8e74ccd3253d89/src/Magnum/ImGuiIntegration/Context.cpp#L228

Around there this line should fix it:

io.Fonts->SetTexID(reinterpret_cast<ImTextureID>(&_texture)); or io.Fonts->SetTexID(&_texture);

I am unsure if the reinterpret cast is needed, I would expect it, because ImTextureID is void*.

It seems like static_cast<ImTextureID>() is used elsewhere, so idk.

rune-scape commented 4 years ago

I'll submit a pull request

I'll also add a function to ImGuiIntegration::Context to get the atlas texture: GL::Texture2D &getAtlasTexture();

rune-scape commented 4 years ago

I should explain what I'm doing too, probably helpful (maybe good for documentation?).

Big picture I'm trying to use color emojis, and do it seamlessly. I should be able to put an emoji in a raw string literal and pass it to ImGui::Text() and it renders a color emoji.

I'm very close. I can create a custom glyph with a codepoint in the atlas and it renders(yay!), but I need:

Preferably the latter, because this library makes GL texture stuff not painful!

rune-scape commented 4 years ago

The first option is not possible becuase ImGuiIntegration::Context::relayout() both calls ImFontAtlas::Build() and creates a texture out of that, so none of my code can edit the built (and in CPU memory) texture data

mosra commented 4 years ago

Hi again!

Apologies if I'm misunderstanding what you want to do (not using ImGui that much myself, definitely not on the advanced level you're on, haha) -- so you are able to add extraneous glyphs together with their position in the atlas to ImGui, but it doesn't let you upload the data for it, and thus you need to do that manually by accessing the texture after, right?

Adding a texture() getter is a no-brainer, not sure why that was not done already. I'm not sure what exactly would io.Fonts->SetTexID(reinterpret_cast<ImTextureID>(&_texture)); do, but maybe it simplifies the render to not need this branching here? https://github.com/mosra/magnum-integration/blob/43e86c2a1e83caf9b4da42e00c8e74ccd3253d89/src/Magnum/ImGuiIntegration/Context.cpp#L301-L302

none of my code can edit the built

did you try calling io.Fonts->GetTexDataAsRGBA32(&pixels, &width, &height, &pixelSize) on your side after you have all fonts set up and before calling relayout()? It should cause the texture to be built slightly earlier, but then you'd somehow need to reset the IsLoaded() flag so relayout() knows it needs to repopulate the GPU texture ... and I don't know if/how is that possible. Adding some sort of callback or splitting relayout() into two parts to make the access possible would be overcomplicating things for everyone else I'm afraid :/ So this texture access is probably the best way to go about it.

maybe good for documentation?

Definitely! If you open a PR, would be great to expand the Context docs with this -- useful for lots of other users :) Thanks in advance.

(Please note I'm going to be without an access to a computer for the whole next week, so won't be able to do any merging or reviewing until I get back.)

rune-scape commented 4 years ago

Howdy,

Yes, ImFontAtlas::AddCustomRectFontGlyph lets me specify a Rect to pack into the atlas and map it to a codepoint.

https://github.com/ocornut/imgui/blob/ebe79bbed00a13fd4455f04131b63d49c28ebd5d/imgui.cpp#L230-L234 That is from release v1.69 which is what I think magnum-integration v2019.01 was tested against? (I am using the unicode branch, which is v1.75 wip) The comment from the above link says This will be passed back to your via the renderer. So I tested simplifying that ternary statement and it works! (obviously doesn't work without TexID being set) And the tests pass on my machine too.

I agree that splitting relayout would be weird, but what about optionally delaying repopulating the GPU texture until newframe? Im not sure. This isn't the issue i'm trying to solve, i'm just spitballing.

Cool! I'll try to write a snippet or two.

mosra commented 4 years ago

Fixed by #61, thank you!