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

Multiple Contexts shutdown leak or not clear #1558

Closed
zlnimda opened this issue Jan 18, 2018 · 2 comments
Closed

Multiple Contexts shutdown leak or not clear #1558

zlnimda opened this issue Jan 18, 2018 · 2 comments

Comments

@zlnimda
Copy link

zlnimda commented Jan 18, 2018

Hello, I am using imgui and my app has multiple windows.
The version used is one year old but the weird problem is still present in last version.
I used as recommended since there the use of multiple context, one by windows.

I hooked our memory management system on Alloc & Free callbacks and it seems there was some leaks.
At first my code was looking like that:

auto defaultContext = ImGui::GetCurrentContext(); 
[...]
ImGui::CreateContext(...); // multiple times 
[...]
ImGui::DestroyContext(...); // multiple times
[...]
ImGui::SetCurrentContext(defaultContext);
ImGui::Shutdown();

I found that each allocation by context was not freed, so I changed my code in this :

auto defaultContext = ImGui::GetCurrentContext(); 
[...]
wContext = ImGui::CreateContext(...); // multiple times
[...]
ImGui::SetCurrentContext(wContext);
ImGui::Shutdown();
ImGui::DestroyContext(...);
[...]
ImGui::SetCurrentContext(defaultContext);
ImGui::Shutdown();

But it crashed because It seems that I use a single Font which is shared between context.

// imgui.cpp line:716
static ImFontAtlas      GImDefaultFontAtlas;
// imgui.cpp line:796
ImGuiIO::ImGuiIO()
{
    Fonts = &GImDefaultFontAtlas;
}

So now everything is working (no leak anymore) with this change:

ImGui::SetCurrentContext(wContext);
+ ImGui::GetIO().Fonts = null;
ImGui::Shutdown();

But this looks so unexpected and not documented in comments. I'm not sure this is the way it should be uninitialized, so I prefer to refer my code here to know what it is the real purpose of Shutdown() and DestroyContext(). The destructor ~ImGuiContext doesn't exist but is explicitly called in DestroyContext().

Please let me know what you think about this case.

@ocornut
Copy link
Owner

ocornut commented Jan 18, 2018

Thank you for the detailed post.

There are several issues related to this (#591, #1007, #586, #992).

I don't have the exact answer right now but those issues have been lingering and I will try to tackle some of them soon.

Also note that I am working on a new system to use a single imgui context for multiple windows (see #1542). If you are not looking for multi-threading or rendering at different frequency, using that new system will be much better :)

ocornut added a commit that referenced this issue Jan 20, 2018
…now setup with SetMemoryAllocators() and shared by all contexts. (#586, #992, #1007, #1558)
ocornut added a commit that referenced this issue Jan 21, 2018
…now setup with SetMemoryAllocators() and shared by all contexts. (#1565, #586, #992, #1007, #1558)
ocornut added a commit that referenced this issue Jan 21, 2018
- YOU NOW NEED TO CALL ImGui::CreateContext() AT THE BEGINNING OF YOUR APP, AND CALL ImGui::DestroyContext() AT THE END.
- removed Shutdown() function, as DestroyContext() serve this purpose.
- you may pass a ImFontAtlas* pointer to CreateContext() to share a font atlas between contexts. Otherwhise CreateContext() will create its own font atlas instance.
- removed allocator parameters from CreateContext(), they are now setup with SetAllocatorFunctions(), and shared by all contexts.
- removed the default global context and font atlas instance, which were confusing for users of DLL reloading and users of multiple contexts
(#1565, #586, #992, #1007, #1558)
ocornut added a commit that referenced this issue Jan 21, 2018
…ntext(), DestroyContext() in example code. Removed Shutdown() from binding code. (#1565, #586, #992, #1007, #1558)
@ocornut
Copy link
Owner

ocornut commented Feb 6, 2018

@zlnimda With the changes included in #1565 (now merged) all the issues here should be solved :)

@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

2 participants