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

Font bug when using multiple instances #591

Closed
Cthutu opened this issue Apr 15, 2016 · 14 comments
Closed

Font bug when using multiple instances #591

Cthutu opened this issue Apr 15, 2016 · 14 comments

Comments

@Cthutu
Copy link

Cthutu commented Apr 15, 2016

This is briefly mentioned in #586 but when I switch state via the ImGui::SetInternalState() API, the fonts that is the not most recently created instance get corrupted. There is a work around. I run this code before the draw loops (OpenGL renderer):

unsigned char* pixels;
int width, height;
io.Fonts->GetTexDataAsRGBA32(&pixels, &width, &height);

And immediately I bind the texture:

glBindTexture(GL_TEXTURE_2D, (GLuint)(intptr_t)drawCmd.TextureId);
glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, (GLint)GL_LINEAR);
glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, (GLint)GL_LINEAR);
glTexImage2D(GL_TEXTURE_2D, 0, (GLint)GL_RGBA, width, height, 0, GL_RGBA, GL_UNSIGNED_BYTE, pixels);

I am essentially reloading the OpenGL textures. Is this because ImGui::SetInternalState() is moving the font texure around in memory and the OpenGL texture is referencing old memory? I haven't analysed the behaviour between switching internal states yet, but I thought I'd let you be aware of it.

@ocornut
Copy link
Owner

ocornut commented Apr 17, 2016

SetInternalState() is only assigning to a pointer so isn't affecting anything font related.
Now, I believe you that may be a bug but I would need more detailed information and ideally a repro. What may be obvious/trivial in your context may not be obvious everywhere.

How are you creating the second state? Are you reassigning the font atlas pointer?

@itamago
Copy link

itamago commented Apr 17, 2016

I had the same issue few time ago while using 2 different ImGui contexts with their own fonts. After reading the source code I understood that you need to have separate texture atlases too.
So, create an additional atlas for each additional context, and when you SetInternalState you must swap the atlases too. It works perfectly well.

@ocornut
Copy link
Owner

ocornut commented Apr 17, 2016

This isn't as intended, you should be able to share the same atlas for two instances.

@Cthutu
Copy link
Author

Cthutu commented Apr 17, 2016

I do create an additional atlas for each context. In fact, I have a
different OpenGL context per ImGui context. I will look at the code on
Monday and show you how I manage the ImGui contexts.

On Sun, 17 Apr 2016 at 07:08 omar notifications@github.com wrote:

This isn't as intended, you should be able to share the same atlas for two
instances.


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#591 (comment)

@ocornut
Copy link
Owner

ocornut commented Apr 17, 2016

Thanks!

Similarly, you should be able to reuse the same atlas if they are going to be the same (tho obviously you need to upload texture to your two opengl context and find a way to pass textureid that works for both).

This looks fishy so I'll need to give it a closer look.

@ocornut
Copy link
Owner

ocornut commented Apr 17, 2016

This worked for me:

    // Setup ImGui binding
    void* state1 = ImGui::GetInternalState();
    ImGui_ImplGlfw_Init(window, true);

    void* state2 = (void*)new char[ImGui::GetInternalStateSize()];
    ImGui::SetInternalState(state2, true);
    ImGui_ImplGlfw_Init(window, true);

    [...] Main Loop

        ImGui::SetInternalState(state2);

        ImGui_ImplGlfw_NewFrame();

        ImGui::Begin("HELLO");
        ImGui::Text("Hello");
        ImGui::End();

        ImGui::SetInternalState(state1);

        ImGui_ImplGlfw_NewFrame();
        [...] 

        ImGui::SetInternalState(state1);
        ImGui::Render();

        ImGui::SetInternalState(state2);
        ImGui::Render();

BUT I had to fix code in ImGui_ImplGlfw_NewFrame(), added the else statement else:

    ImGuiIO& io = ImGui::GetIO();
    if (!g_FontTexture)
        ImGui_ImplGlfw_CreateDeviceObjects();
    else
        io.Fonts->TexID = (void *)(intptr_t)g_FontTexture;

Maybe the sample code should be more aware of multiple contexts? Are you using this code as is, or something that made more sense for your own use of multiple contexts?

Also the InternalState function names/API sucks (will be renamed).

@Cthutu
Copy link
Author

Cthutu commented Apr 17, 2016

I will compare what you have to what I have on Monday and will report in!

On Sun, 17 Apr 2016 at 14:11 omar notifications@github.com wrote:

This worked for me:

// Setup ImGui binding
void* state1 = ImGui::GetInternalState();
ImGui_ImplGlfw_Init(window, true);

void* state2 = (void*)new char[ImGui::GetInternalStateSize()];
ImGui::SetInternalState(state2, true);
ImGui_ImplGlfw_Init(window, true);

[...] Main Loop

    ImGui::SetInternalState(state2);

    ImGui_ImplGlfw_NewFrame();

    ImGui::Begin("HELLO");
    ImGui::Text("Hello");
    ImGui::End();

    ImGui::SetInternalState(state1);

    ImGui_ImplGlfw_NewFrame();
    [...]

    ImGui::SetInternalState(state1);
    ImGui::Render();

    ImGui::SetInternalState(state2);
    ImGui::Render();

Apart from the fact that the function names/API sucks (will be fixed!).


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#591 (comment)

@Cthutu
Copy link
Author

Cthutu commented Apr 17, 2016

Immediately I notice that you're only using one OpenGL context, but if that
would affect anything, I couldn't tell you.

On Sun, 17 Apr 2016 at 17:22 Matt Davies mattie.davies@gmail.com wrote:

I will compare what you have to what I have on Monday and will report in!

On Sun, 17 Apr 2016 at 14:11 omar notifications@github.com wrote:

This worked for me:

// Setup ImGui binding
void* state1 = ImGui::GetInternalState();
ImGui_ImplGlfw_Init(window, true);

void* state2 = (void*)new char[ImGui::GetInternalStateSize()];
ImGui::SetInternalState(state2, true);
ImGui_ImplGlfw_Init(window, true);

[...] Main Loop

    ImGui::SetInternalState(state2);

    ImGui_ImplGlfw_NewFrame();

    ImGui::Begin("HELLO");
    ImGui::Text("Hello");
    ImGui::End();

    ImGui::SetInternalState(state1);

    ImGui_ImplGlfw_NewFrame();
    [...]

    ImGui::SetInternalState(state1);
    ImGui::Render();

    ImGui::SetInternalState(state2);
    ImGui::Render();

Apart from the fact that the function names/API sucks (will be fixed!).


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#591 (comment)

@ocornut
Copy link
Owner

ocornut commented May 8, 2016

@Cthutu Any news on that?

I still have to admit that contexts are confusing because a) imgui_impl_xxx files aren't very context aware. b) Font Atlas being shared by default makes things confusing.

Would definitively need to clarify the situation with font atlas.

    // Setup ImGui binding
    ImGuiContext* ctx1 = ImGui::GetCurrentContext();
    ImGui_ImplGlfw_Init(window, true);

    ImGuiContext* ctx2 = ImGui::CreateContext();
    ImGui::SetCurrentContext(ctx2);
    ImGui_ImplGlfw_Init(window, true);

...

        ImGui::SetCurrentContext(ctx1);
        ImGui_ImplGlfw_NewFrame();
        ImGui::Text("Hello");

        ImGui::SetCurrentContext(ctx2);
        ImGui_ImplGlfw_NewFrame();

...

        ImGui::SetCurrentContext(ctx1);
        ImGui::Render();

        ImGui::SetCurrentContext(ctx2);
        ImGui::Render();

ocornut added a commit that referenced this issue May 8, 2016
@Cthutu
Copy link
Author

Cthutu commented May 8, 2016

I haven't been able to check this out yet. I'm currently busy at ToJam but
when I get to work I will figure it out. But first I have to port my code
to use your tree nodes :) Sorry I can't be more timely on this but things
are a bit crazy at the moment. Expect a reply towards the end of the week.

On Sun, 8 May 2016 at 11:01 omar notifications@github.com wrote:

@Cthutu https://github.com/Cthutu Any news on that?

I still have to admit that contexts are confusing because a)
imgui_impl_xxx files aren't very context aware. b) Font Atlas being shared
by default makes things confusing.

Would definitively need to clarify the situation with font atlas.

// Setup ImGui binding
ImGuiContext* ctx1 = ImGui::GetCurrentContext();
ImGui_ImplGlfw_Init(window, true);

ImGuiContext* ctx2 = ImGui::CreateContext();
ImGui::SetCurrentContext(ctx2);
ImGui_ImplGlfw_Init(window, true);

...

    ImGui::SetCurrentContext(ctx1);
    ImGui_ImplGlfw_NewFrame();
    ImGui::Text("Hello");

    ImGui::SetCurrentContext(ctx2);
    ImGui_ImplGlfw_NewFrame();

...

    ImGui::SetCurrentContext(ctx1);
    ImGui::Render();

    ImGui::SetCurrentContext(ctx2);
    ImGui::Render();


You are receiving this because you were mentioned.

Reply to this email directly or view it on GitHub
#591 (comment)

@Cthutu
Copy link
Author

Cthutu commented May 13, 2016

FYI, things have been delayed to Monday so I will look at updating my imgui
repo on Tuesday.

On Sun, 8 May 2016 at 12:22 Matt Davies mattie.davies@gmail.com wrote:

I haven't been able to check this out yet. I'm currently busy at ToJam
but when I get to work I will figure it out. But first I have to port my
code to use your tree nodes :) Sorry I can't be more timely on this but
things are a bit crazy at the moment. Expect a reply towards the end of
the week.

On Sun, 8 May 2016 at 11:01 omar notifications@github.com wrote:

@Cthutu https://github.com/Cthutu Any news on that?

I still have to admit that contexts are confusing because a)
imgui_impl_xxx files aren't very context aware. b) Font Atlas being shared
by default makes things confusing.

Would definitively need to clarify the situation with font atlas.

// Setup ImGui binding
ImGuiContext* ctx1 = ImGui::GetCurrentContext();
ImGui_ImplGlfw_Init(window, true);

ImGuiContext* ctx2 = ImGui::CreateContext();
ImGui::SetCurrentContext(ctx2);
ImGui_ImplGlfw_Init(window, true);

...

    ImGui::SetCurrentContext(ctx1);
    ImGui_ImplGlfw_NewFrame();
    ImGui::Text("Hello");

    ImGui::SetCurrentContext(ctx2);
    ImGui_ImplGlfw_NewFrame();

...

    ImGui::SetCurrentContext(ctx1);
    ImGui::Render();

    ImGui::SetCurrentContext(ctx2);
    ImGui::Render();


You are receiving this because you were mentioned.

Reply to this email directly or view it on GitHub
#591 (comment)

@ocornut
Copy link
Owner

ocornut commented Sep 23, 2016

@Cthutu Any news of that? Still looking for a repro :)

@ocornut
Copy link
Owner

ocornut commented Aug 9, 2017

Could we get to the bottom of this topic?

AFAIK:

  • You can share a ImFontAtlas between two ImGuiContext (I might be wrong, maybe there is an issue in which case let me know)
  • If you have 2 OpenGL contexts sharing texture data is a solvable problem (out of the scope of imgui) but if for some reason it is causing issues you can effectively have 2 ImFontAtlas, or share 1 ImFontAtlas and somehow reinterpret the atlas TextureId to refer to a different OpenGL texture according to your OpenGL context.

@ocornut
Copy link
Owner

ocornut commented Feb 6, 2018

@Cthutu The issues discussed here should now be clarified and fixed by the changes discussed in #1565.
Context creation is now explicit, and have a parameter to pass in a font atlas created by the user.
Read #1565 for details.

@ocornut ocornut closed this as completed Feb 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants