ocornut / imgui

Dear ImGui: Bloat-free Graphical User interface for C++ with minimal dependencies
MIT License
61.39k stars 10.34k forks source link

Multiple instances of Dear ImGui in the same program #586

Open nsf opened 8 years ago

nsf commented 8 years ago

It's not a bug report but more like a question/discussion topic.

So, I have this engine of mine where I have a rendering thread which runs C++ code and scripting environment which runs C# code. They talk to each other via queue and triple buffers to pass rendering lists. Of course I want ImGui to be available on both sides and I can't really just lock it with a mutex, because it will block quite a lot. So what I did:

Converted GImGui to a thread local variable, initialize ImGui from main thread before running mono scripts, also force creation of a font atlas via GetTexDataAsRGBA32 and... it kind of works. Mono renders stuff when it wants to, passes the contents of ImDrawData via triple buffer to the rendering thread, rendering thread renders both GUIs, its own and the one from mono scripting environment.

But I've noticed there are a few places where functions don't look reentrant, for example ImHash function has a static LUT which is initialized on first access. Perhaps there are some other places where implicit global state is used?

In general what do you think about that kind of usage (or should I say abusage :D)?

nsf commented 8 years ago

Yeah, it segfaults somewhere. I guess my other option is to just copy & paste imgui into two translation units, maybe wrap them into different namespaces and use it this way. After all I need only 2 instances of it.

ocornut commented 8 years ago

I can't really just lock it with a mutex, because it will block quite a lot.

Depending on how/where you use it it may not be that bad.

Perhaps there are some other places where implicit global state is used?

I don't know! You'd have to list them and we can see if it is reasonable or out of reach to fix all of them and maintain it. I would suppose there aren't so many.

Yeah, it segfaults somewhere.

Where/what? A very quick skimming shows one for the CRC32 lut, one static const array in RoundScalar (const may not be a problem?), and one in the default clipboard implementation for Win32. Off-hand I don't think there's anything else?

In general what do you think about that kind of usage (or should I say abusage :D)?

If it works for you I don't mind :) I have never tried to do that. Locking mixed with SetInternalState() so far.

nsf commented 8 years ago

Yeah, sorry, this segfault was actually my fault. But anyways I decided to split imgui into two translation units. This way I'm 100% sure the instances have nothing shared and it does work well.

Thanks for awesome lib.

ocornut commented 8 years ago

Some discussions about this on twitter today. Adding copies here for reference.

https://twitter.com/Donzanoid/status/720548646601797633

Don Williamson ‏@Donzanoid 11h11 hours ago @ocornut @andrewwillmott @daniel_collin worth checking out how CUDA API handles this without explicit context param http://docs.nvidia.com/cuda/cuda-c-programming-guide/#context

https://twitter.com/Donzanoid/status/720552966957199360

Rest of the discussion was about the possibility of having an explicit state API (e.g. c-style first parameter passed as state) which could be handled by a script generating code. I have no issue with that as long as it doesn't have pollute or add noise to the basic API, so it should be invisible to the casual user (different file/folder?).

Frankly I don't feel we need a stack but I would like to modify the code to allow the user to make the global pointer a TLS variable.

Also naming needs to be fixed see https://github.com/ocornut/imgui/issues/269

Cthutu commented 8 years ago

What is the problem with having a ImGui object? The instance can keep track of it's own IO etc.

On Thu, 14 Apr 2016 at 17:06 omar notifications@github.com wrote:

Some discussions about this on twitter today. Adding copies here for reference.

https://twitter.com/Donzanoid/status/720548646601797633

Don Williamson ‏@Donzanoid 11h11 hours ago @ocornut https://github.com/ocornut @andrewwillmott https://github.com/andrewwillmott @daniel_collin worth checking out how CUDA API handles this without explicit context param http://docs.nvidia.com/cuda/cuda-c-programming-guide/#context

https://twitter.com/Donzanoid/status/720552966957199360

Rest of the discussion was about the possibility of having an explicit state API (e.g. c-style first parameter passed as state) which could be handled by a script generating code. I have no issue with that as long as it doesn't have pollute or add noise to the basic API, so it should be invisible to the casual user (different file/folder?).

Frankly I don't feel we need a stack but I would like to modify the code to allow the user to make the global pointer a TLS variable.

Also naming needs to be fixed see #269 https://github.com/ocornut/imgui/issues/269

— You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub https://github.com/ocornut/imgui/issues/586#issuecomment-210148960

nsf commented 8 years ago

Well, if you ask me, I would definitely prefer explicit context variable. But when it comes to having both APIs, global style and the one with context variable, it's a tricky question.

Maybe you can even do that with macros, i.e. make it a compile-time feature. But not sure if you would like that solution or not.

Something like:

IMGUI_API bool          RadioButton(IMGUI_CONTEXT const char* label, bool active);

And in implementation:

GImGui *g = IMGUI_USE_CONTEXT;

Probably pretty ugly though. So one can leave IMGUI_CONTEXT empty and say that IMGUI_USE_CONTEXT simply refers to global var. Or fill it with the right content.

Or the same, but wrap all the API into a class conditionally with a macro. But again some macro pressure on the implementation side, because funcs/methods look different: void X::Foo() vs void Foo().

Whatever works I'm happy with all variants. I don't like much TLS usage though. On my side it was a quickest hack possible.

ocornut commented 8 years ago

That's actually a neat solution from a technical perspective, would save us a lot of work. It's mainly ugly because of the missing comma, but it also mean that within functions the parameter has to be passed around which adds another layer of missing-comma-fugliness inside the functions themselves.

I really don't know what's the best solution. Ultimately it isn't a much desired feature, people sort of prefer if it was there but nobody absolutely needs it so I may put that in the backburner until someone comes up with a better idea. I'd really love it if some macro trick could save us from that missing comma.

Should still fix that CRC32 static table too.

EDIT Not helping much, but another solution for an alternative reality would be to always have context pointer and allow 0/NULL as a shortcut for global state pointer. ImGui::Button(0, "blah").

emoon commented 8 years ago

For me personally I would rather have this:

void RadioButton(context, ...)

(or, like talked about in the twitter thread)

void RadioButton(...); // Global Edition

void RadioButton(...)
{ 
  RadioButton(g_context, ...); // defaults to global context, defined in imgui_context.h or such
}

I know this would result in lots of cut'n'paste of all the functions but I really think it's the cleanest way to do it. Also as @nsf points out I really don't like the TLS stuff. I would personally see that dear imgui only deals with a context and and let the application deal with anything threading.

As always threading is a data issue and not a code one so as long as dear imgui would only play with a given context (and global in the causal use-case) everything would be fine.

ocornut commented 8 years ago

TLS is the crap workaround obviously but as long as it's not enabled by default (because of performance) it's probably a 2-lines source change (probably some optional macro in imconfig.h) and not causing harm.

Now on discussing proper solution. There's about 330 functions in imgui.h Potentially the "no-context" versions could be inlined in a header file (If the global pointer can be declared extern there?) meaning the overhead would be 1 line per function and in a single place. @nsf how did you "split imgui into two translation units" without duplicates at link time?

They would perhaps need to be in different namespace in order to avoid end-user seeing 2 versions of each.

nsf commented 8 years ago

@ocornut wrapped the second copy into a different namespace

emoon commented 8 years ago

Yes all of the non-context functions would be inlined as you say so there wouldn't be any extra overhead to the change. Also the TLS change doesn't help if you want multiple instances on the same thread (I haven't used TLS much but is it possible to have more instances per thread? Then it would work also)

nsf commented 8 years ago

No, with TLS - one instance per thread, unless you do that push/pop trick from cuda API. Also TLS is bad for coroutine-like situation where your jobs can be scheduled onto different threads over time. Whether it's bad or not is a different topic, but it's possible. E.g. languages like Go and coroutines will come into C++ sooner or later.

ocornut commented 8 years ago

@emoon In that case the thread could always call e.g. SetCurrentContext() or some equivalent Push/Pop to change context.

Looks like adding the non-context inline function sounds fairly reasonable, just need to swallow the bullet of making imgui.h less good looking with extra arg everywhere. Or, I replace namespace ImGui by struct ImGuiContext for the context version and make them method? Would that be any less friendly, have subtle side-effects?

nsf commented 8 years ago

Struct is okay, but the main problem with structs is that even private methods have to be in the header file. Or you'll have to pass the context around as an argument for private functions.

DanielGibson commented 8 years ago

probably for migrating code, search and replace on ImGUi:: to imguiCtx. (or however one calls their local context) is a bit easier than adding a parameter on every call?

emoon commented 8 years ago

Yeah for my use-case I think I will be fine with SetCurrentContext as I don't run my UI stuff multi-threaded (yet)

I assume you still want to keep your namespace ImGui::Function(...) syntax? Otherwise the other approach would be to actually use a struct/class like g_imgui->Function(...) I much prefer the current style though.

emoon commented 8 years ago

@ocornut remember that you can still keep the current layout exactly as in the header file. All you need is to include a "some.inl" that actually does the inline implementation of the non-context functions.

ocornut commented 8 years ago

Struct is okay, but the main problem with structs is that even private methods have to be in the header file. Or you'll have to pass the context around as an argument for private functions.

I hate this, but tempted to still do it if I cold. I was mainly wondering if using method would make a slight difference to people working on bindings or other languages. Perhaps explicit parameters can be easier to bind to other languages without adding another set of wrapper functions? In term of how mangled names can be accessed?

(..Second paragraph completely backtracking on the first one..)

However, if there is an actual struct + methods now we need to also shove members into header OR have an additional indirection (bad) OR have a private derived type and cast this in the private code. None of those solution are exciting. Any better idea that doesn't suck?

I don't actually care the ImGui:: namespace too much (personally mostly because of the two uppercase letters :) tho changing it may be more of a v2.0 thing? DanielGibson has a point that at least it could be search-and-replaced safely, so it's not out of consideration and may be part of solution.

Now clearly: ctx.Button() is shorter than ImGui::Button(ctx)

But later also feels a little less C++, perhaps more bindings-friendly, more search-and-replace friendly, and I can't find a satisfactory solution to use member functions/variables (paragraph above).

nsf commented 8 years ago

Well, you can pass context as an argument to private functions. I agree there is no pretty solution in C++. As for bindings, I don't think any C++ solution works there. Name mangling is already bad enough and if you have it, only brave people use C++ .so/.dll directly. So, one way or another there will be a C wrapper also and it's typically easier to use for bindings. As for me, for my C# bindings I use wrapper functions, so the C++ API isn't a big deal.

I would say that passing context argument explicitly is a true procedural way, but also it's not pretty at all. But that's the only way how C api will look like at the end. So, eh, I don't know what's the best solution here.

Cthutu commented 8 years ago

I would definitely prefer the ctx.Button() way that ImGui::Button(ctx). I don't agree with bindings-friendly as every scripting language generally deals with C++ object bindings. It would get rid of the SetInternalState() calls that I am currently doing to support multiple instances (on the same thread) of ImGui. It would also solve the multi-threaded issue too.

On Thu, 14 Apr 2016 at 19:00 nsf notifications@github.com wrote:

Well, you can pass context as an argument to private functions. I agree there is no pretty solution in C++. As for bindings, I don't think any C++ solution works there. Name mangling is already bad enough and if you have it, only brave people use C++ .so/.dll directly. So, one way or another there will be a C wrapper also and it's typically easier to use for bindings. As for me, for my C# bindings I use wrapper functions, so the C++ API isn't a big deal.

I would say that passing context argument explicitly is a true procedural way, but also it's not pretty at all. But that's the only way how C api will look like at the end. So, eh, I don't know what's the best solution here.

— You are receiving this because you commented.

Reply to this email directly or view it on GitHub https://github.com/ocornut/imgui/issues/586#issuecomment-210194622

emoon commented 8 years ago

I do all my wrapping in C++ to a C API in the end also so I really don't care that much how the context is being sent. Regarding the data in the header. I would just go with

struct PrivateData;

struct ImGui {
  void RadioButton(...);
  ...
  PrivateData* private_data;
}

And have the PrivateData declared in the C++ code. If you really want to hide it you have can of course have a void pointer here but then you would need to do more casting inside you other code to use it.

Problem with this approach is if the user wants to modify/get data from the private struct it has to be done using functions which may be a bit annoying. Of course the private data can be stuffed in another header also to keep the regular header a bit clear.

Pagghiu commented 8 years ago

I am doing multi context in my apps as well as multi-thread and I'm now using locks to avoid undefined behaviour/data races. I am also trying to do some fancy experiments with multiple concurrent UI "Sessions", one for each user connected through the browser ... My preference is to make everything ctx.Method(...) or at the very minimum ImGui::Method(&ctx,...)

I don't see so much of a value to save any casual newbie programmer from thinking where he should declare this state/context variable to access where he needs it. Any real c++ coder will know where to declare it :laughing:

You can also expose the "default static context" as global variable from imgui.h If you call this variable "ImGui" and you transform ImGui namespace to a struct ImGuiContext,

in imgui.cpp

ImGuiContext ImGui;

in imgui.h

extern ImGuiContext ImGui

everything that the casual programmer will need to do is to replace

ImGui::Button()

with

ImGui.Button()

Would that be acceptable as a minimal search&replace update to a potential 2.0 Api?

andrewwillmott commented 8 years ago

Another vote for context->Blah() and so on here, rather than Blah(context). If there's a backwards compatibility header with inline wrappers, it's not going to affect users who don't change their code either way, and I think that's a cleaner more C++ approach. (Though sadly the meaning of 'more C++ approach' has degenerated into insanity these days so maybe ignore that comment :P.)

The changes on the ImGui side can be pretty minimal, thanks to the state already being well encapsulated. Daniel's already alluded to this but

struct ImGuiContext
{
    void NewFrame();
    // ...

    ImGuiContext();
    ~ImGuiContext();
protected:
    ImGuiState& g;
};

would reduce changes in many cases to just taking out the dereference lines:

void ImGui::NewFrame()
{
    ImGuiState& g = *GImGui;

    if (!g.Initialized)
    {
    }
}

// Becomes

void ImGuiContext::NewFrame()
{
    if (!g.Initialized)
    {
    }
}

The constructor/destructor is just

ImGuiContext::ImGuiContext() : g(*new ImGuiState)
{
}
ImGuiContext::~ImGuiContext()
{
    delete &g;
}

// or better

ImGuiContext::ImGuiContext() : g(*new(MemAlloc(sizeof(ImGuiState))) ImGuiState)
{
}
ImGuiContext::~ImGuiContext()
{
    g.~ImGuiState();
    MemFree(&g);
}

// or even better avoid the double-alloc by having CreateContext()/DestroyContext() that allocate one block.

One thing that would need thinking about is memory allocation. This either has to be separated out of the context, or the code needs to be updated to call per-context allocs rather than MemAlloc(), and pass the memory allocator into the context constructor or creation function.

Some of the functions don't reference the context internally at all of course. I guess the easiest thing would be for these to remain as ImGui::ColorConvertRGBtoHSV (say) and hence not needed in the optional backwards compat. header.

ocornut commented 8 years ago

Some extra ideas

or even better avoid the double-alloc by having CreateContext()/DestroyContext() that allocate one block.

This is desirable to avoid be touching 1 extra cache line on every call accessing the context. Will probably use a scheme like that. That and perhaps changing the way allocators are specified might lead us to require an explicit one-liner Initialization of ImGui.

I will let those ideas simmer for a bit, afaik there is not urgency. I would like to point out that it is a topic where everyone would naturally want to say "of course it is better if there is an explicitly context" because obviously it is better, but it's not a requirement, it has a cost of change, and I don't think it is blocking many people to not have it soon.

1-thread 1-context : not affected by change. (most common) 1-thread 2-contexts : that change will help to avoid calling SetInternalState/SetContext so code will be a little nicer, but it's not total a deal-breaker to not have it. N-threads 1-context : that change won't help at all, user still need a lock. N-threads 1-context per-thread : that change will allow that situation. note that the TLS workaround also allows it, albeit with extra cost of TLS access.

In other words, there's no situation that's 100% blocked right now. I want to do this but I don't think it is urgent.

ocornut commented 8 years ago

(Note that for a known number of instances,e.g. 2, the namespacing trick with 2 copies of ImGui code in different namespaces also works)

Soonish tasks:

ocornut commented 8 years ago

Cleaned up the messy/confusing Set/GetInternalState functions, and added

ImGuiContext* CreateContext(void* (*malloc_fn)(size_t) = NULL, void (*free_fn)(void*) = NULL);
void          DestroyContext(ImGuiContext* ctx);
ImGuiContext* GetCurrentContext();
void          SetCurrentContext(ImGuiContext* ctx);

Decided against changing io.MemAllocFn etc. to imconfig.h IM_MALLOC because editing anything in imconfig.h has big mental and practical barrier cost for many. Instead, to solve the fact that CreateContext() needs to do an allocation the functions are passed here and stored immediately within the context.

This is breaking API for users who were using SetInternalState GetInternalState but they are very rare and those users are expert users so they will figure it out. Added the information in the API Breaking Changes section of the documentation.

I also also renamed the internal ImGuIState struct to ImGuiContext.

Cthutu commented 8 years ago

I'll take the compliment!

On Sat, 7 May 2016 at 13:59 omar notifications@github.com wrote:

Cleaned up the messy/confusing Set/GetInternalState functions, and added

ImGuiContext* CreateContext(void* (_malloc_fn)(size_t) = NULL, void (_freefn)(void) = NULL); void DestroyContext(ImGuiContext_ ctx); ImGuiContext* GetCurrentContext(); void SetCurrentContext(ImGuiContext* ctx);

Decided against changing io.MemAllocFn etc. to imconfig.h IM_MALLOC because editing anything in imconfig.h has big mental and practical barrier cost for many. Instead, to solve the fact that CreateContext() needs to do an allocation the functions are passed here and stored immediately within the context.

This is breaking API for users who were using SetInternalState GetInternalState but they are very rare and those users are expert users so they will figure it out. Added the information in the API Breaking Changes section of the documentation.

— You are receiving this because you commented.

Reply to this email directly or view it on GitHub https://github.com/ocornut/imgui/issues/586#issuecomment-217656384

itamago commented 8 years ago

Oh f...k, I just grabbed the latest version with so much code to change on my side... :( But, clearly, this is a lot better with this new version (even if it will cost me one night)

ocornut commented 8 years ago

Sorry :/ Do a manual rename of ImGuIState to ImGuiContext in your branch BEFORE merging and it may be much easier?

EDIT Did you mean merge issues? Which local changes do you have that weren't pushed as PR? I wonder which local changes you actually need.

itamago commented 8 years ago

Not any issues for the moment, I didn't start the merge. This won't be very painful, that's a mechanic job keeping my mind in automatic mode... and that's for a better meaning of ImGuIState as ImGuiContext.

EDIT: in my private engine, I use 2 imgui instances, a first one for my own "simulator" of iOS, and second one for the editor. That's why I use 2 ImGuIStates.

mojmir-svoboda commented 8 years ago

I am SO lucky :) This change came just in time for me :) Many thanks!

Pagghiu commented 8 years ago

I've just updated ImGuIState to ImGuiContext and it looks good so far.

jslee02 commented 8 years ago

I've tried to render two windows with the new context API modifying opengl3_example. I observed two problems: (1) the ImGui widgets in the first window don't respond to mouse actions and (2) the second window doesn't render the ImGui widgets.

Here is my test code. I would appreciate if someone could point out what I'm doing wrong here.

Edit: I modified the while statement only.

galloscript commented 8 years ago

From #688

Currently, a lot of functions access the ImGuiState (aka context) in that way:

ImGuiState& g = *GImGui;

That's fine for a single context, or multiple context (windows) updating on the same thread changing it with ImGui::SetInternalState, but that doesn't work well if we have different windows on different threads. Changing the entire interface to require a ImGuiState* as a parameter for every function is madness, it not only implies functions that directly access to state but functions that also call those, which means the entire imgui interface. So, a solution could be just replacing the *GImGui; (~245) access with a call to GetInternalState() .

This way the user (programmer) can modify GetInternalState function to manage how to access the context and use a unique thread id like the c++11 one http://en.cppreference.com/w/cpp/thread/get_id, to access a per-thread ImGuiState, just think in a std::map<std::thread_id, ImGuiState*>. Ok, it asumes that every imgui call is performed in the thread that holds the ImGuiState where it wants to draw something, but makes sense, and it doesn't mean that a thread can only hold one ImGuiState, It can even be mixed with threads that hold multiple context just modifying GetInternalState to work that way with those threads, similar to how we are currently working calling SetInternalState to change it.

For some functions (mainly little functions used internally) where a call to a complex version of GetInternalState may suppose a performance impact, well, maybe for those there is no better solution than passing the context as a parameter...

What do you think about this possible solution for multi-threaded/windows applications?

ocornut commented 8 years ago

@Galloman Your solution can already be implemented by setting GImGui to be a thread-local-storage variable, which would be a 1 line modification locally.

PS: Note that following discussion in this thread, one month ago the GetInternalState etc function have been changed to GetCurrentContext() with various modification to enable context creation.

This thread still entertain the idea that we could make the context an implicit this and change existing functions to this-call methods, which wouldn't be so hard to implement but break a few things.

galloscript commented 8 years ago

@ocornut I was trying to set the GImGui as local storage but Apple doesn't want it... http://stackoverflow.com/a/29929949/2484187 So, it's a good solution when you are not using the Apple LLVM compiler :(

guysherman commented 8 years ago

I'm thinking about using ImGui for audio plugin guis. The key difficulty is around having multiple contexts, because you could have many plugin instances in a single host process, and you don't have any control over which threads own which plugins etc, so you need to be able to scope separate gui instances quite tightly.

galloscript commented 8 years ago

You can check how I'm doing it in a single thread for GTF here: https://github.com/Galloman/GTF/tree/master/src/gtf/iosys

sonoro1234 commented 7 years ago

Couldn't all this be managed making ImGUI a class? The class would have the imgui context as a member so we dont have to overwrite the global with SetCurrentContext. Multiple windows and multiple threads also.

ocornut commented 7 years ago

That's also what's discussed here and rather probable it'll go this route. Creates a few issues a) for backward compatibility (it will break everyones code everywhere but can be Search&Replaced) and b) populating ImGui:: with new functions which is the thing I'm the most annoyed with.

sonoro1234 commented 7 years ago

Which new functions you mean?

ocornut commented 7 years ago

It is currently convenient to add functions to the ImGui namespace (overloads for own types etc.) which c++ allows for namespace but not for class unless inheriting or hackery

sonoro1234 commented 7 years ago

Perhaps the TLS way should tried before as less code intrusive. Did anyone experimented with it?

sonoro1234 commented 7 years ago

Just tried TLS with the GLF3 implementation in https://github.com/sonoro1234/cimgui/tree/lua_build/extras/impl_glfw3 for switching ImGuiContext.

Fonts appear strange in one of the two threads But only if I add a font from file with ImFontAtlas_AddFontFromFileTTF and previous AddFontDefault

Otse6olectric commented 7 years ago

Hi everyone,

I've been working with ImGui for a while now (awesome job @ocornut btw). I managed to do compose a simple bezier curve editor, node editor and other gadgets, but now I am at a turning point and got stuck. What I want to do is to create a new independant floating window. This secondary window will contain a texture reference and just be drawn as it is (with resizing) - the drawList tricks.

I've also read plenty of forums here, but still got stuck.

The whole idea: main application has a node editor where you set float value which then sets radius value for a red circle and draws everything to a fbo. I got it working in the main window and now I want to display this (updating) texture in this new window.

Many thanks!

matteomandelli commented 7 years ago

I just published my experiments for N-threads 1-context per-thread with TSL access. Not ideal but for the moment 3 lines of code change and seems to work.

https://github.com/matteomandelli/ssb

ocornut commented 7 years ago

To everybody using overload for malloc/free, I would like to fix the bug discussed in #992, but here seems like the thread where I'd get better feedback.

Basically: Will remove MemAllocFn/MemFreeFn OUT of the context structure.

(I think it was a mistake I had it there, was hoping it would allow different allocators to be affected to different imgui instances, but I don't think anybody care for that, it mostly causes problems e.g. at destruction time, and helpers that aren't tied to an imgui instance and often allocated inside tools are sitting in the middle of this and most affected by those problems, it makes it harder to have a simple ImGuiTextFilter declared static in a function because it tends to crash on destruction if you don't keep an ImGui instance alive.)

This will break the build for only the people reassigning those (which is good, most people won't be affected).

Looking for feedback on those two possibilities:

1) Expose defines (documented in imconfig.h), e.g. #define IMGUI_MALLOC malloc, #define IMGUI_FREE free. Pros: User can avoid linking with malloc/free altogether if they wish so. Cons: A little harder to setup, user needs to modify imconfig.h (probably ok for the the sorts of people who don't use malloc/free).

2) Expose two globals, e.g. ImGui::SetAllocators(xxx,xxx), Pros: Easier to setup, no need to touch imconfig.h Cons: Enforce linking with malloc/free (but we can also solve that with another layer of defines). (edit TO CLARIFY THOSE GLOBALS WOULD NOT BE STORED IN AN IMGUICONTEXT)

Anything else? Suggestions?

thedmd commented 7 years ago

From my point of view problem with allocator functions is aftermath of having global contex/atlas instance. I added a define allowing me to disable default instances of context and font atlas. Problem vanished since construction and destruction time was explicitly managed.

As to choice of API I do not have strong preference. Either will work. First one will be harder to break at run-time.

ocornut commented 7 years ago

From my point of view problem with allocator functions is aftermath of having global contex/atlas instance.

It also makes it trickier for code that tries to manage multiple contexts to ever use those functions, making such code harder to share as an extension.

Anyone else, feedback on my post above? (#define IMGUI_MALLOC vs off-context ImGui::SetAllocators() setting globals).

godlikepanos commented 7 years ago

I've been monitoring this thread for quite some time because because memory allocation and global state prevent me from using imgui.

Global context This is a problem because I might want to "draw" the UI in thread A and "render" it in thread B.

Memory allocation IMHO the 2nd solution is better but it should be more fine grained. I'm not sure how imgui works internally but there might be a need for more allocators. You may need an allocator that matches the context's lifetime and another allocator that is per frame. The motivation for a per-frame alocator is to have a super fast linear allocator that is reset and reused at the beginning of the frame.

Other objects like the font atlas might need their own allocator because their lifetime might be different.

Also the allocator callbacks should accept some user data:

using AllocCallback = void* (*)(void* userData, size_t size, unsigned alignment);
using FreeCallback = void (*)(void* userData, void* ptr);

Final thoughts Removing a global stuff and adding a more fine grained allocator scheme will provide much more flexibility to the existing scheme. The context change will break the interfaces but the memory allocators can be optional.