global context issues

[sinkingsugar]

First of all thanks for this great library!

I have a few issues related to the static global variable gp

I use a custom allocator and static globals init/deinit order is not guaranteed, and so my app will crash at exit cos my custom allocator (that imgui and in turn implot uses) will be destroyed before the gp.

Also within ImPlotContext destructor the call to SetColormap will access gp! that is a bug...

I am hacking the library to fix my needs but I wonder if a more clean design would be a better option for everyone's use?

Cheers

[epezent]

Thanks for the info! I was wondering when this would become an issue for someone.

I went with static initialization of ImPlotContext gp to remove the burden of initialization for the user and make ImPlot integration easy. Perhaps it is necessary to require manual initialization similar to what ImGui does with CreateContext and DestroyContext. It wouldn't be particularly hard for engine maintainers to add these lines where they initialize ImGui, but it could cause complications where users are adding ImPlot to an application that is using a pre-compiled library that exposes ImGui.

@ocornut, do you have any recommendations on how to handle this neatly? As an alternative, might it be possible for ImGui to expose callbacks whereby users could hook up ImPlot and other 3rd-party library initialization code? Just a random thought...

@sinkingsugar , what exact modifications did you make to get this working for yourself?

[sinkingsugar]

I opened a draft here with my changes I used to make it work in my project, I hope they can help
#79

In short they are kinda inspired by what ImGui already does indeed.

[JamesBoer]

Just wanted to mention that I seem to be having the same sort of issue. I'm using a custom allocator, and I judiciously avoid any implicit deallocation in static data, because as sinkingsugar stated, you can't guarantee shutdown order. In my case, it shows up as leaks on shutdown. Technically, they're not really leaks, but they get detected as such.

Anyhow, so I just tried out sinkingsugar's fork, and it fixed the issue I was having. No more errant leaks. I changed my own code to this, which is more consistent with ImGui:

ImPlotContext *CreateContext() {
    ImPlotContext * context = IM_NEW(ImPlotContext)();
    if (gp == NULL)
        SetCurrentContext(context);
    return context;
}

void DestroyContext(ImPlotContext *context) {

    if (context == NULL)
        context = gp;
    if (gp == gp)
        SetCurrentContext(NULL);
    IM_DELETE(context);
}

void SetCurrentContext(ImPlotContext *context) {
    gp = context;
}

I definitely think you should avoid implicit deallocations in static data as a matter of principle due to issues like this. Explicit initialization and shutdown is just conceptually cleaner in every respect.

Thanks for making such a cool add-on library for ImGui. I'm just starting to use it, but I can already see it's going to be very useful for showing the internal state of my game engine at runtime.

[sinkingsugar]

@JamesBoer glad it was useful to share my code.
Yeah I did't really polish it well, just quickly wrapped it up for my usage.

Overall global statics can be useful, but indeed their usage is complex.. they should be definitely self sustaining, if for some reason they have any side effect or mutation (custom allocators etc) they become a land mine :)

P.s.
Was also crashing for me on global destruction.

[JamesBoer]

Yeah, figured it was just a quick proof-of-concept hack. I'm just sort of pedantic about stuff like that. :) Appreciate you doing the bulk of the work so that I didn't have to! Hopefully we can get a similar fix in the main branch at some point, but in the meantime, it now shuts down cleanly.

[ocornut]

@ocornut, do you have any recommendations on how to handle this neatly? As an alternative, might it be possible for ImGui to expose callbacks whereby users could hook up ImPlot and other 3rd-party library initialization code? Just a random thought...

I'm open to it yes.

I thought we could have a system (exposed in imgui_internal.h) to (1) register a pointer stored in imgui context + a callback for destruction of the context? Would return an index so you can retrieve your pointer back from the index, easy and fast.

But I'm not sure if/how that would work with multiple imgui context, if that index is stored on your side it would defeat the purpose there, unless we decide to make it a requirement that multiple imgui context should all have the same extension installed and then the index would match.

Otherwise you'd need to request a slot given a key (e.g. your key would be imu32 encoded "PLOT") but it means access to your context pointer would probably not be as fast as we'd have to resolve that key multiple times, or imgui could optimized to fast return same slot if multiple requests are done with same key.

Feel free to open a topics in imgui repo if you have further thought, ideally even a PR. That's something we can work on together.

[epezent]

I've officially added explicit context creation and destruction to v0.5. The API exactly mirrors ImGui's API. See #48 and the updated README for more info.

@sinkingsugar and @JamesBoer , please let me know if your issues are resolved.

[JamesBoer]

I just replaced my custom solution with the latest source, and it worked flawlessly. Zero code changes, and only a tiny change to my build script to accommodate the refactored code. I like the simplicity of mirroring the way ImGui handles context management. Oh, and I'm glad you didn't replicate my bug in DestroyContext(). :-)

Thanks again for such a great library.

[sinkingsugar]

@epezent Cool! Thanks for pinging! I will integrate it once I get some time!