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

Reorganization of context/allocators: CreateContext(), DestroyContext(), SetAllocatorFunctions() #1565

Closed
ocornut opened this issue Jan 21, 2018 · 8 comments

Comments

@ocornut
Copy link
Owner

ocornut commented Jan 21, 2018

I am reorganizing context management functions to be more explicit.
I have created a branch "context" to test those changes, and would very much like feedback on them!
https://github.com/ocornut/imgui/tree/context

The purpose of those changes is:

  • To address issues with multiple context management being confusing for anyone trying to use them.
  • To address issues with people using DLL hot-reloading, which the statically created instances made more difficult.
  • To address issues with memory allocators being stored inside the context. The statically created instances made it worse, but generally it feels more healthy to have a shared allocator that helper classes such as ImVector<> can use.

Those topics are discussed in e.g. #586, #992, #1007, #1558
Those changes DO NOT address multi-threading issues from #586.
Also note that the incoming virtual viewport feature (#1542) will allow you to manage multiple OS windows with a single context, therefore multiple context will become less useful and less exercised onward :) but we should still support them.


  • YOU NOW NEED TO CALL ImGui::CreateContext() AT THE BEGINNING OF YOUR APP, AND CALL ImGui::DestroyContext() AT THE END.
  • Removed the statically constructed global context and font atlas instances, which were confusing for users of DLL reloading and users of multiple contexts.
  • Removed Shutdown() function, as DestroyContext() serve this purpose.
  • You may pass a ImFontAtlas* pointer to CreateContext() to share a font atlas between contexts. Otherwise CreateContext() will create its own font atlas instance and destroy it on DestroyContext().
  • Removed allocator parameters from CreateContext(), they are now setup with SetAllocatorFunctions(), and shared by all contexts. Added a void* user_data to allocators function.
  • Added a IMGUI_DISABLE_DEFAULT_ALLOCATORS imconfig.h directive for people who don't want to link with malloc/free at all.

Those changes are touchy because it will break existing codebase:

  • The user needs to call ImGui::CreateContext(), and possibly ImGui::DestroyContext()
  • Previously the imgui_impl_xxx binding were calling ImGui::Shutdown(), this was removed and the function doesn't exist as a public function any more. DestroyContext() calls the internal Shutdown() and delete the instance.

I think I should push those change to master as they are for the better but would really like feedback from anyone using multiple contexts, multiple font atlas or DLL reloading. If you could quickly try to merge the context branch, see if it works for you. Any feedback on the changes are welcome..

Tagging people involved or who may be interested in this.. Sorry for the tag-spam.
@aiekick @sonoro1234 @zx64 @thedmd @zlnimda @Pagghiu @itamago @galloman @matteomandelli @pdoane-blizzard

Thank you!

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 added a commit that referenced this issue Jan 21, 2018
…ve context yet, to be more flexible with creation order. (#1565)
ocornut added a commit that referenced this issue Jan 21, 2018
@thedmd
Copy link
Contributor

thedmd commented Jan 22, 2018

I love it.

@zlnimda
Copy link

zlnimda commented Jan 22, 2018

Hi there, thanks for the update !

I just pulled and tested your changes and it worked fine to me.
Initialization and destruction is far better intuitive this way.
Also, I really like the global Set of Alloc/Free callbacks SetAllocatorFunctions() and IMGUI_DISABLE_DEFAULT_ALLOCATORS . (By the way the define is missing in imconfig.h, so I added it)

I tested with a single shared font right now and the instance was not loaded from a dynamic library.

About break the existing codebase:

It didn't change much to me, I just removed the Get/Set of default context which was useless to my use case, removed the Shutdown(), called SetAllocatorFunctions() instead of set functions on each context, and pre-allocated my own shared ImFontAtlas.

@meshula
Copy link

meshula commented Jan 22, 2018

Coincidentally this weekend I was working around a shutdown crash due to the global font atlas getting destructed after a context was disposed. This mod is most welcome.

@Pagghiu
Copy link
Contributor

Pagghiu commented Jan 23, 2018

This looks a very nice addition!
I should admit though that I will be even happier when we will have an ImGui object to use instead of free functions operating on a globally set context, but I can wait some more time for such a breaking change ;)

Thanks @ocornut !

@JSandusky
Copy link

Works here.

Coalescing context shutdown is welcomed. I've made the goof twice of using ImGui::DestroyContext without first using ImGui::Shutdown.

@JaapSuter
Copy link

Works great for us, a welcome improvement, thanks!

@damucz
Copy link

damucz commented Feb 5, 2018

I did similar thing this weekend before I noticed this thread. :) I tried the official code from the branch and it works great. Good job!

ocornut added a commit that referenced this issue Feb 6, 2018
…alls CreateContext/DestroyContext + merged the massive Navigation branch (#1565, #787)
ocornut added a commit that referenced this issue Feb 6, 2018
@ocornut
Copy link
Owner Author

ocornut commented Feb 6, 2018

This is now merged, I've also merged the Navigation branch and bumped versions to 1.60.
Will make a more visible forum post to help people to know about CreateContext.

I've added asserts to make this more visible:

ImGuiIO& ImGui::GetIO()
{
    IM_ASSERT(GImGui != NULL && "No current context. Did you call ImGui::CreateContext() or ImGui::SetCurrentContext()?");
    return GImGui->IO;
}

ImGuiStyle& ImGui::GetStyle()
{
    IM_ASSERT(GImGui != NULL && "No current context. Did you call ImGui::CreateContext() or ImGui::SetCurrentContext()?");
    return GImGui->Style;
}

ocornut added a commit that referenced this issue Aug 15, 2018
…s-context global counters than we previously used. (#1565, #1599, #586)
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

8 participants