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

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

Closed
rune-scape opened this issue Dec 21, 2019 · 6 comments

Comments

@rune-scape
Copy link
Contributor

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!

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

rune-scape commented Dec 21, 2019

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

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:

  • Access to the texture in memory after ImFontAtlas::Build()
    or
  • Access to the GL::Texture2D after it is sucked into the GPU memory

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

@rune-scape
Copy link
Contributor Author

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 mosra added this to the 2019.1c milestone Dec 22, 2019
@mosra
Copy link
Owner

mosra commented Dec 22, 2019

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?

auto userTexture = static_cast<GL::Texture2D*>(pcmd->TextureId);
_shader.bindTexture(userTexture ? *userTexture : _texture);

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

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

mosra commented Jan 3, 2020

Fixed by #61, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging a pull request may close this issue.

2 participants