Restoring OpenGL state after using callbacks

[xor2k]

Compare #984.

What this pull request changes: for the OpenGL3-examples, (sdl_opengl3_example, opengl3_example) glVertexAttribPointer is now called every time the draw lists are drawn and not only when the draw list shaders are compiled. This allows the user to use custom glVertexAttribPointer in a draw_callback callback specified in

ImGui::GetWindowDrawList()->AddCallback(draw_callback, NULL);

This e.g. allows to execute a user defined OpenGL-program per window and have draw lists be executed on top: in the figure below, the black triangle is drawn by a different program than the yellow line, which is drawn by the draw list program.

[ocornut]

@xor2k I understand your intent but I don't understand your solution at all.

There are hundreds of GL state, it should be up to the callback to restore whatever state they changed. Setting/restoring a small selection of state because this what your application changes doesn't provide a good solution.

If restoring modified state is too much work, you could even provide your own callback to reset the render state to the one required for the imgui render loop, and queue that callback.

[xor2k]

Well, I agree with you about that. However, as far as I can see, there is no way to backup the glVertexAttribPointer state (glGetVertexAttribPointerv at least will not do it) and the configuration, i.e. how many indices there are, and all their other arguments are specific to the draw list shaders, so to properly restore them, the restoration has to be done in the draw list shader code, i.e. the imgui_impl*.cpp.

Or am I missing something?

[ocornut]

I guess we could have the renderer expose a function to restore specific state, or split the function to have a full separate backup/setup/restore, or decide of a way for callbacks to communicate that they want a full gpu state restore.

For now I'd suggest you add yourself a function with those 6 lines and you can call the function BELOW the pcmd->UserCallback() statement.

I'm interested in those problem because I'm currently refactoring the imgui_impl_xxx files (separating renderer (opengl, directx, vulkan etc.) from platform (glfw, sdl, win32, etc.) and until now I was telling people it is ok to modify those _impl file, and it still is, but I would eventually like those files to cater for more use cases. If you have a fancy engine presumably it wouldn't be a problem for you to just copy that render function in your code and modify it.

[xor2k]

If you make certain changes to the draw list shaders, e.g. you modify an attribute, my 6 lines restore code will be invalidated. Not that I expect this to happen often, but there should be a canonical way to restore certain states of the draw list shaders. The glVertexAttribPointer is a very good starting point: when it is restored, the draw lists can actually be used alongside custom shaders. Below is the test code I am currently using. As you can see, it is directly derived from the draw list code. In addition to this pull request, this code is a compact example of what I would expect the user callback to do. I am thinking of some header listed functions that can be called to save/restore the draw list shader state.

static bool         g_Initialized = false;
static int          g_ShaderHandle = 0, g_VertHandle = 0, g_FragHandle = 0;
static int          g_AttribLocationPosition = 0;
static unsigned int g_VboHandle = 0, g_ElementsHandle = 0;

bool Custom_CreateDeviceObjects()
{
    if(g_Initialized){
        return true;
    }
    // Backup GL state
    GLint last_texture, last_array_buffer;
    glGetIntegerv(GL_TEXTURE_BINDING_2D, &last_texture);
    glGetIntegerv(GL_ARRAY_BUFFER_BINDING, &last_array_buffer);

    const GLchar *vertex_shader =
        "attribute vec2 vPosition;\n"
        "void main()\n"
        "{\n"
        "    gl_Position = vec4(vPosition, 0, 1);\n"
        "}\n";

    g_ShaderHandle = glCreateProgram();
    g_VertHandle = glCreateShader(GL_VERTEX_SHADER);
    glShaderSource(g_VertHandle, 1, &vertex_shader, 0);
    glCompileShader(g_VertHandle);
    glAttachShader(g_ShaderHandle, g_VertHandle);

    const GLchar* fragment_shader =
    "void main()\n"
    "{\n"
    "    gl_FragColor = vec4(0.0, 0.0, 0.0, 1.0); \n"
    "}\n";

    g_FragHandle = glCreateShader(GL_FRAGMENT_SHADER);
    glShaderSource(g_FragHandle, 1, &fragment_shader, 0);
    glCompileShader(g_FragHandle);
    glAttachShader(g_ShaderHandle, g_FragHandle);

    glLinkProgram(g_ShaderHandle);

    g_AttribLocationPosition = glGetAttribLocation(g_ShaderHandle, "vPosition");

    // Restore modified GL state
    glBindTexture(GL_TEXTURE_2D, last_texture);
    glBindBuffer(GL_ARRAY_BUFFER, last_array_buffer);

    g_Initialized = true;
    return true;
}

void handleGl (const ImDrawList* cmd_list, const ImDrawCmd* pcmd){
    Custom_CreateDeviceObjects();

    GLint last_program, last_texture, last_array_buffer, last_element_array_buffer;
    glGetIntegerv(GL_CURRENT_PROGRAM, &last_program);
    glGetIntegerv(GL_TEXTURE_BINDING_2D, &last_texture);
    glGetIntegerv(GL_ARRAY_BUFFER_BINDING, &last_array_buffer);
    glGetIntegerv(GL_ELEMENT_ARRAY_BUFFER_BINDING, &last_element_array_buffer);

    glUseProgram(g_ShaderHandle);

    static const ImDrawVert g_vertex_buffer_data[] = {
        {ImVec2(-1.0f, -1.0f), ImVec2(0, 0), ImU32(0)},
        {ImVec2(1.0f, -1.0f), ImVec2(0, 0), ImU32(0)},
        {ImVec2(0.0f, 1.0f), ImVec2(0, 0), ImU32(0)}
    };

    GLuint vertexbuffer;
    glGenBuffers(1, &vertexbuffer);
    glBindBuffer(GL_ARRAY_BUFFER, vertexbuffer);
    glBufferData(GL_ARRAY_BUFFER, sizeof(g_vertex_buffer_data), g_vertex_buffer_data, GL_STATIC_DRAW);

    glEnableVertexAttribArray(g_AttribLocationPosition);
    glBindBuffer(GL_ARRAY_BUFFER, vertexbuffer);

#define OFFSETOF(TYPE, ELEMENT) ((size_t)&(((TYPE *)0)->ELEMENT))
    glVertexAttribPointer(g_AttribLocationPosition, 2, GL_FLOAT, GL_FALSE, sizeof(ImDrawVert), (GLvoid*)OFFSETOF(ImDrawVert, pos));
#undef OFFSETOF

    glDrawElements(GL_TRIANGLES, 3, GL_UNSIGNED_SHORT, 0);

    glUseProgram(last_program);
    glBindTexture(GL_TEXTURE_2D, last_texture);
    glBindBuffer(GL_ARRAY_BUFFER, last_array_buffer);
    glBindBuffer(GL_ELEMENT_ARRAY_BUFFER, last_element_array_buffer);

}

and of course

ImDrawList* draw_list = ImGui::GetWindowDrawList();
draw_list->AddCallback(&handleGl, NULL);

[mellinoe]

Why do you need a custom shader to render DrawLists, let alone multiple ones? From the linked thread, it does seem like rendering your "custom stuff" to a framebuffer and then drawing that with ImGui seems like the sensible approach. Perhaps I don't understand what you are trying to accomplish, though.

[xor2k]

The custom shader is not rendering draw lists and cannot render draw lists, because when the call to the custom UserCallback in the _impl-files occurs

pcmd->UserCallback(cmd_list, pcmd);

cmd_list is always empty. Therefore, draw lists are always rendered by the draw list shader. The custom shader renders custom graphics, in my example above that is a simple triangle, but it could also be a game engine or some other graphics producing application logic.

Maybe I overlooked somethink, but what is the UserCallback else made for?

I think it is absolutely necessary for the whole Dear ImGui-concept to have those UserCallbacks or at least to somehow (enable to user to) ensure that the draw list shader does not break during custom shader operation. Currently however, which is the reason for this pull request, custom shader operations (e.g. user callbacks) cause side effects on the draw list shader, because the draw list shader state is not restored by making enough glVertexAttribPointer-calls resp. there is no function I can call to do so. Even if I did not use the UserCallback to do custom graphics, e.g. rendering graphics to a texture instead (I think that is what you proposed) a single call to glVertexAttribPointer would already break the draw list shader.

[ocornut]

I am not sure why you say, "cmd_list is always empty" probably you meant "pcmd is empty" in the sense that it has a zero vertex count. We're passing pcmd so you can access the void* UserCallbackData field.

Most people are happy with rendering their engine/contents into an offscreen buffer because it's more practical and then from the ImDrawList point of view it's just a regular 2D textured quad. Of course, you are totally right that you can save on bandwidth by rendering directly without an intermediate buffer, and we ought to allow that.

Even if I did not use the UserCallback to do custom graphics, e.g. rendering graphics to a texture instead (I think that is what you proposed) a single call to glVertexAttribPointer would already break the draw list shader.

Not sure what you mean here, as I understand it glVertexAttribPointer essentially setup the bound VAO, it's not a GL global state. I am wrong?

FYI I have just merged in change related to an old issue (#1217) to create the VAO locally in the draw function instead of creating it in e.g. ImGui_ImplGlfwGL3_CreateDeviceObjects(). Because VAO are not shared by multiple resource-sharing GL contexts, it makes it easier to reuse that function for multiple simultaneous viewports.
d749d49

The problem with the PR are that:

• It fixes 1 piece of state (that happens to be commonly problematic) without resetting the other 25.
• Resetting even that one piece of state for every draw call is ill-advised, callbacks can be used for variety of reason (e.g. push a transformation matrix, select shader, blend state) which shouldn't necessary lead to setting various GPU API state again, sometimes for perf reason, sometimes because that would cancel the desirable effect of the callback. (I personally used ImDrawList this way for my entire game and you'd be surprised as how many people are using them for quick/easy custom rendering here and there).

ANYWAY, going toward a solution: Someone whispered an idea into my ear yesterday, which is to standardize callback special values:

#define ImDrawCallback_ResetState reinterpret_cast<ImDrawCallback>(-1)

            if (pcmd->UserCallback)
            {
                if (pcmd->UserCallback == ImDrawCallback_ResetState)
                {
                      // Explicitly set shaders, viewport, scissors, buffers, textures, etc
                }
                else
                {
                    pcmd->UserCallback(cmd_list, pcmd);
                }
            }

And we can expend this list with specific ones on a per-state basis.
The small problem I have is that handling those in the example impl_ code would make the example renderer a little more odd or complex to read, but it's doable.

[xor2k]

First of all, sorry for my late reply. I double checked whether everything works as I expect it to work and indeed, it does. Those VAOs are really great & thank you a lot!

[ocornut]

I am reopening this because although the PR won’t be taken as is, the discussion below should lead us to standardize a set of callback defines to reset the gpu state.

[xor2k]

Maybe we should move it to a new issue, this pull request thing does not really allow reopening as far as I can see.

[ocornut]

Branch has been merged in now.
Using ImDrawList::AddCallback(ImDrawCallback_ResetRenderState) it is possible to request the renderer to reset render state. This is now supported by all example renderers.