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

ImGuiIntegration memory corruption when destroying context with custom font loaded via AddFontFromMemoryTTF #42

Closed
matusnovak opened this Issue Feb 22, 2019 · 3 comments

Comments

Projects
2 participants
@matusnovak
Copy link

matusnovak commented Feb 22, 2019

The problem

I apologise if this is not MagnumImGuiIntegration problem but ImGui problem instead. I am not sure what is the exact cause of this problem so I have started here.

I am following the ImGui example from here: https://doc.magnum.graphics/magnum/examples-imgui.html

I am trying to load a custom font from a ttf file. Had to slightly modify the original example below into the following:

IMGUI_CHECKVERSION();
context = ImGui::CreateContext();

const Utility::Resource rs{ "fonts" };
const auto ttfRaw = rs.getRaw(<font name here>);
ImGuiIO& io = ImGui::GetIO();
imguiFont = io.Fonts->AddFontFromMemoryTTF(reinterpret_cast<void*>(const_cast<char*>(ttfRaw.data())), ttfRaw.size(), 18.0f);
IM_ASSERT(imguiFont != NULL);
io.Fonts->Build();

// Passing ImGui context into the constructor!
_imgui = ImGuiIntegration::Context(*context, Vector2{windowSize()}/dpiScaling(), windowSize(), framebufferSize());

I am able to draw the ImGui with the custom font without issues. However when the _imgui falls out of scope (on program exit), ImGui tries to delete invalid memory.

The exact traceback:

MagnumImGuiIntegration-d.dll!FreeWrapper(void * ptr, void * user_data) Line 1065
MagnumImGuiIntegration-d.dll!ImGui::MemFree(void * ptr) Line 2966
MagnumImGuiIntegration-d.dll!ImFontAtlas::ClearInputData() Line 1462
MagnumImGuiIntegration-d.dll!ImFontAtlas::Clear() Line 1500
MagnumImGuiIntegration-d.dll!ImFontAtlas::~ImFontAtlas() Line 1452
[External Code]	
MagnumImGuiIntegration-d.dll!IM_DELETE<ImFontAtlas>(ImFontAtlas * p) Line 1534
MagnumImGuiIntegration-d.dll!ImGui::Shutdown(ImGuiContext * context) Line 3570
MagnumImGuiIntegration-d.dll!ImGui::DestroyContext(ImGuiContext * ctx) Line 3035
MagnumImGuiIntegration-d.dll!Magnum::ImGuiIntegration::Context::~Context() Line 137
magnum-imgui.exe::ImGuiExample::~ImGuiExample()

Inside of the FreeWrapper it is trying to call free(); with pointer that comes from my exe. (Visual Studio reports that the address of the pointer belongs to the exe, not to the Magnum DLL.)

Workaround

It seems that if I add io.Fonts->ConfigData.clear(); before creating ImGuiIntegration::Context it works without issues. So I get the following:

ImGuiIO& io = ImGui::GetIO();
imguiFont = io.Fonts->AddFontFromMemoryTTF(reinterpret_cast<void*>(const_cast<char*>(ttfRaw.data())), ttfRaw.size(), 18.0f);
IM_ASSERT(imguiFont != NULL);
io.Fonts->Build();
io.Fonts->ConfigData.clear();

No memory corruption on exit. But why?

Additional info

OS: Windows 10
Compiler: Visual Studio 15 2017 Win64 (14.16.27023)
Configuration: Debug
Magnum integration version: Commit 31ccefc
ImGui version: Commit ocornut/imgui@79f7778
Haven't tried Linux or OSX

@mosra mosra added this to the 2019.0b milestone Feb 22, 2019

@mosra mosra added this to TODO in GUI via automation Feb 22, 2019

@mosra

This comment has been minimized.

Copy link
Owner

mosra commented Feb 22, 2019

Zdravím! :)

I ran into this as well when adding a HiDPI-aware font for the WebGL version. According to the docs, it takes ownership of the memory by default and you have to explicitly tell it to not do that -- like this. In your case that would be (from the top of my head):

+ImFontConfig fontConfig;
+fontConfig.FontDataOwnedByAtlas = false;
 ImGuiIO& io = ImGui::GetIO();
-imguiFont = io.Fonts->AddFontFromMemoryTTF(reinterpret_cast<void*>(const_cast<char*>(ttfRaw.data())), ttfRaw.size(), 18.0f);
+imguiFont = io.Fonts->AddFontFromMemoryTTF(reinterpret_cast<void*>(const_cast<char*>(ttfRaw.data())), ttfRaw.size(), 18.0f, &fontConfig);

I'll add a mention about this to the font loading documentation.

@matusnovak

This comment has been minimized.

Copy link
Author

matusnovak commented Feb 22, 2019

Ah, I have missed that, it fixed the problem for me!

Ďakujem! :)

@matusnovak matusnovak closed this Feb 22, 2019

GUI automation moved this from TODO to Done Feb 22, 2019

@mosra

This comment has been minimized.

Copy link
Owner

mosra commented Feb 23, 2019

For the record, the documentation is now updated mentioning this: 6b4de97

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.