pthom / hello_imgui

Hello, Dear ImGui: unleash your creativity in app development and prototyping
https://pthom.github.io/hello_imgui
MIT License
606 stars 91 forks source link

When should ImageGl be destructed? #14

Closed codecat closed 3 years ago

codecat commented 3 years ago

If I use HelloImGui::ImageGl, when am I supposed to destruct the object? It's currently happening when the program exits, but it seems like (on Linux at least) this is too late as glDeleteTextures is pointing to invalid memory.

pthom commented 3 years ago

Hello,

Hum, this is not an easy issue.

glDeleteTextures is pointing to invalid memory.

Do you mean that the function pointer is then invalid?

If so, here is a possible analysis (I'm sorry if I drown you under technical details).

Based on what I saw in your code, this is want I understood:

class Explorer
{
public:
    static Explorer* Instance;  // You have a static pointer that contains an ImageGlPtr
private:
    HelloImGui::ImageGlPtr m_imgBackground;

...

int main()
{
    make_test_page();
    Explorer::Instance = new Explorer();

       // I suppose this launches hello_imgui (which in turn inits the glad openGl Loader)
    Explorer::Instance->Run();  

        // Do you encounter the bug here (which would mean that the OpenGL loader 
       // was "unloaded" ?
    delete Explorer::Instance;   
    Explorer::Instance = nullptr;
    return 0;
}

Below, more details about the internals of hello_imgui:

By default, you should be using glad, see external/hello_imgui/CMakeLists.txt:

if (IOS OR EMSCRIPTEN)
    set(need_opengl_loader OFF)
else()
    set(need_opengl_loader ON)
endif()
option(HELLOIMGUI_USE_GLAD "Use Glad OpenGl loader" ${need_opengl_loader})

In this case, glad is loaded here (and this should make the opengl function pointers ok), in the case of the glfw backend:

    void RunnerGlfwOpenGl3::Impl_InitGlLoader()
    {
#ifndef __EMSCRIPTEN__
        // Initialize OpenGL loader
#if defined(IMGUI_IMPL_OPENGL_LOADER_GL3W)
        bool err = gl3wInit() != 0;
#elif defined(IMGUI_IMPL_OPENGL_LOADER_GLEW)
        bool err = glewInit() != GLEW_OK;
#elif defined(IMGUI_IMPL_OPENGL_LOADER_GLAD)
                                                     // You code should run this part 
                                                     // this is were the glad openGl loader is inited
        bool err = gladLoadGLLoader(reinterpret_cast<GLADloadproc>(glfwGetProcAddress)) == 0;
#else
        bool err = false;  // If you use IMGUI_IMPL_OPENGL_LOADER_CUSTOM, your loader is likely to requires
                           // some form of initialization.
#endif
        if (err)
        {
            HIMG_THROW("RunnerGlfwOpenGl3::Impl_InitGlLoader(): Failed to initialize OpenGL loader!");
        }
#endif  // #ifndef __EMSCRIPTEN__
    }

Below is an analysis of what happens once the glfw backend is stopped.

    void RunnerGlfwOpenGl3::Impl_Cleanup()
    {
        ImGui_ImplOpenGL3_Shutdown();
        ImGui_ImplGlfw_Shutdown();
        ImGui::DestroyContext();

        glfwDestroyWindow(mWindow);
        glfwTerminate();

       // Could you test whether the glDeleteTextures function pointer 
       // was invalidated by the call to glfwTerminate ?
    }

I suspect the glDeleteTextures function pointer might have been invalidated by the call to glfwTerminate() inside RunnerGlfwOpenGl3::Impl_Cleanup().

I'm still pondering what is the best solution for that. In the meantime, may be you could try to add an exit button? This way you could trap the exit, and handle the destruction before OpenGL is deinited;

For a more complete approach, may be we could add a PreExit callback that would be called before destroying the openGl context (its rule would be symmetrical to PostInit). May be something like this:

struct RunnerCallbacks
{
    VoidFunction ShowGui = {};
    VoidFunction ShowMenus = {};
    VoidFunction ShowStatus = {};
    VoidFunction PostInit = {};

    VoidFunction PreExit = {};    // Add this new callback
...
}

// And this new callback could be called here:
void AbstractRunner::TearDown()
{
    params.callbacks.PreExit();
    Impl_Cleanup();
}
pthom commented 3 years ago

I added a BeforeExitcallback: see https://github.com/pthom/hello_imgui/commit/b7d78def75ff7a2781ff86582a25721533f66d1e

I think it should be enough to delete your ImageGl in this callback.

codecat commented 3 years ago

Thanks for the detailed rundown! You are right, it is indeed crashing on delete Explorer::Instance;. A callback like that is exactly what I was looking for, thank you!