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

Memory stomping with ImGuiKey_XXX elements after ImGuiKey_COUNT_ #2063

Closed
scottmudge opened this issue Sep 5, 2018 · 7 comments
Closed

Memory stomping with ImGuiKey_XXX elements after ImGuiKey_COUNT_ #2063

scottmudge opened this issue Sep 5, 2018 · 7 comments

Comments

@scottmudge
Copy link

scottmudge commented Sep 5, 2018


Version/Branch of Dear ImGui:

1.63

Back-end file/Renderer/OS:

Back-ends: imgui_impl_opengl3.cpp + imgui_impl_glfw.cpp
OS: Windows 10
Compiler: MSVC 2015

My Issue/Question:

After upgrading from 1.50 to 1.63, and correspondingly upgrading the GLFW/OpenGL backends and their integration into my application, I am having trouble at ImGui_ImplOpenGL3_CreateFontsTexture(). At the following line (line 309 of imgui_impl_opengl3.cpp below), io.Fonts is undefined, and thus the GetTexDataAsRGBA32() function fails:

image

I'm using the default backends, and have even tried writing my own, but I keep getting stuck at this problem.

Because I am using multiple contexts, I am using CreateContext() and DestroyContext() appropriately, and am switching context appropriately using SetCurrentContex() before each implementation call. I have tried to manually create a new FontAtlas* before calling ImGui_ImplOpenGL3_CreateFontsTexture(), which allows the application to continue execution, but it yields a blank render with no ImGui content, so I'm assuming it's some sort of context issue.

Does anyone have any insight?

Standalone, minimal, complete and verifiable example:

See image above. Default example.

@ocornut
Copy link
Owner

ocornut commented Sep 5, 2018

Hello,

I am not sure what is happening but here's a few hints.

  • Do you have asserts enabled? (Try to call IM_ASSERT(0) and make sure it crashes/abort on you)

I have manually created a new FontAtlas*

  • When using multiple context the code in opengl3.cpp expect that the font atlas is shared between them.
    Did you pass your font atlas to all your CreateContext() calls?

  • CreateContext() create a new font atlas if you don't provide one, so it is surprised that io.Fonts can be incorrect or NULL at all. Specifically in your callstack it looks like it is not NULL but carries and an incorrect or dangling pointer, which hints at a whole different problem. (This is assuming that your screenshot was taken from a Debug build where the io pointer shows reliably).

@scottmudge
Copy link
Author

Hi Omar,

Thanks for the tips.

  • The asserts are enabled and it fails when IM_ASSERT(0) is called.

  • I should have noted (corrected now) that I had tried to create a new FontAtlas* object since the one created by CreateContext() was lost, to at least get the application to run, but doing so exhibited the issue described. I've opted not to share a FontAtlas, and instead have a unique one for each context.

  • Yes, that's what's puzzling me as well. The screenshot is an accurate representation of the pointer value at that point in execution. As you note, it's not the default NULL, but something else. It is also reliably that value at every execution. I'm going to try and do a deeper trace to see if I can isolate the issue, but it's peculiar.

I'm not sure if it's necessarily relevant to this, but I have linked ImGui dynamically to my application (stored in a DLL). In the older 1.49-1.50 version, I had to get a little creative to get the multi-context switching to work right, but the new version seems like it should be more turn-key in that regard.

@ocornut
Copy link
Owner

ocornut commented Sep 5, 2018

I've opted not to share a FontAtlas, and instead have a unique one for each context.

Mind that imgui_impl_opengl3.cpp currently won't work with that setup (it stores a single texture) but it's not too hard to mod it.

I'm not sure if it's necessarily relevant to this, but I have linked ImGui dynamically to my application (stored in a DLL).

The API are cleaner and there's no default global context instance any more (only a pointer which needs to be set explicitely) which makes it much less error prone (especially as DLL hot-loading would affect the global) but aside from that the logic should be the same.

@scottmudge
Copy link
Author

  • Definitely, I'll have to make those modifications at some point, but hopefully I'll be able to get one context working first.

So I've narrowed it down to ImGui_ImplGlfw_InitForOpenGL(). The IO->Fonts instance is a valid pointer before entering that function, but is turned into the dangling/undefined value after. Still not sure why, but that narrows it down a lot:

image

@scottmudge
Copy link
Author

Found it!

So the root of the problem is that I have defined extra ImGuiKey_ enums in my user header, and in the previous version, the ImGuiIO::KeyMap array was something like 512 elements wide. Now it is narrowed to the default key count:

image

However, I am mapping extra keys than are defined by default:

image

So at a certain point, the assignment operations were overwriting values outside the array. In this case, it was the Fonts pointer. This explains why it was the same value each time.

Do you have a recommendation for how to tenably modify the KeyMap size to accommodate user-specified keys? Or should I just set it back to some arbitrarily large number like 512?

@ocornut ocornut added the inputs label Sep 5, 2018
@ocornut
Copy link
Owner

ocornut commented Sep 5, 2018

KeyMap[] was always [ImGuiKey_COUNT] sized, you must have modified it locally before.
You could patch your local copy with e.g.

ImGuiKey_COUNT = 512

The initial intent behind this mechanism was to encourage people using their own native key enumeration (so your code instead of using an hypothetical ImGuiKey_P would use GLFW_KEY_P directly, or more generally MYENGINE_KEY_P.)

In reality there are pros and cons of both ways.
Using ImGuiKey_XXX makes it easier to make imgui-using code portable across code-base.
In some codebases if you don't have your own enum (MYENGINE_KEY_P) you may feel that ImGuiKey_XXX is something you are more acquainted with than an equivalent enum from another third-party library e.g. GLFW_KEY_XXX..

TL;DR; I agree this enum should be populated at some point. Also see: #2005, #1924.

@ocornut ocornut changed the title ImGuiIO::FontAtlas object undefined? Memory stomping with ImGuiKey_XXX elements after ImGuiKey_COUNT_ Sep 5, 2018
@scottmudge
Copy link
Author

You're right, I must have modified it a year ago and forgot. Changing the array size to a sufficient width fixed the problem.

Yes I'm creating a sort of internal SDK, so using the ImGuiKey_ method just helps with overall cohesiveness. But it's an easy fix.

Thanks again for the help. Issue closed.

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

2 participants