ocornut / imgui

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

OpenGL error when restoring the state of the OpenGL context #6220

Closed Cyphall closed 1 year ago

Cyphall commented 1 year ago

Version/Branch of Dear ImGui:

Version: 1.89 Branch: docking

Back-end/Renderer/Compiler/OS

Back-ends: imgui_impl_glfw.cpp + imgui_impl_opengl3.cpp Compiler: MSVC Operating System: Windows 11

My Issue:

There is a bug in imgui_impl_opengl3.cpp at line 579 that may generate the following OpenGL error: GL_INVALID_VALUE error generated. Program handle does not refer to an object generated by OpenGL.

This error is caused by the following situation:

1) Create an OpenGL shader (we will call it 123). 2) Bind the shader and do some rendering 3) Delete the shader as it is no longer needed. The shader is flagged for deletion but not directly deleted as it is currently bound to the state machine. 4) Render the ImGui UI. In this step, the backend will first backup the currently bound shader using glGetIntegerv(GL_CURRENT_PROGRAM, ...). This will return 123 since our shader is not deleted yet (as it is bound), but is pending deletion. The backend will then do its internals, which includes binding its own shaders. This will unbind our shader 123 and since it is pending deletion and no longer used by OpenGL, it will actually be deleted there. Once the backend is done with its tasks, it will attempt to rebind the previously bound shader, which is our shader 123. The problem is that in the meantime, this shader has been deleted. This call ends up binding a deleted shader and causing the mentionned OpenGL error.

A way to fix this is to check if the shader is still valid before trying to rebind it:

if (glIsProgram(last_program))
    glUseProgram(last_program);

Note: This bug may also apply to other resources in the backup/restore steps.

ocornut commented 1 year ago

Interesting find. Thanks for the careful report.

I am wondering if it might not be simpler than we call glGetError() once rather than attempt to deal with all possible edge cases here? Otherwise I’d rather fix situations one by one as they are encountered by real people, like you just did for texture.

Cyphall commented 1 year ago

The glGetError() idea might not work for cases where people use the Debug Output Callback feature. In my case, my program is designed to crash at the first OpenGL error caught by this callback (since it is a personal project and does not need any robustness).

Since this bug can only occur for OpenGL objects (shader programs, textures, buffers and a few others), it may be cleaner (but still quick) to just add the handful of necessary if (glIsXXX()).

Cyphall commented 1 year ago

So it turns out that only Program and Shader objects can be in a "pending deletion" state. Other objects will simply set the current binding to 0 if they are deleted while being bound.

ocornut commented 1 year ago

Thank you! Merged your fix as 66b7625.