ocornut / imgui

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

Any plans to support bindless textures for OpenGL? #5656

Closed vdweller84 closed 2 years ago

vdweller84 commented 2 years ago

I have integrated Dear ImGui to an existing C++/OpenGL/SDL2 engine. Everything works as expected and I have to say, as a concept, immediate mode gui is impressive!

I am using bindless textures and I was wondering if there are any plans to make that an option in the drawing/backend code as well. I could tamper with the implementation code but this would make updating inconvenient as any changes on my part would be overwritten.

ocornut commented 2 years ago

I have no idea what they are so you'll need to explain and investigate and propose a solution. Most of the time we expect people to use ImDrawList draw callbacks (->AddCallback()) in order to alter rendering state.

vdweller84 commented 2 years ago

My apologies. Bindless textures in OpenGL (I believe DX also has an equivalent) is an ARB extension that, broadly speaking, helps reduce state changes related to binding textures. For each OpenGL texture generated, on can make it "resident", thus obtaining a GLuint64 handle. You can pass this handle in various way to the shader and convert it to a sampler2D there. The upshot is that you never have to call glBindTexture again, or break a drawing batch because the texture ID changed. OpenGL version 4.2+ is required.

Extensive info can be found here: https://www.khronos.org/opengl/wiki/Bindless_Texture

Due to the open-ended nature of ImTextureID , one can easily use it for getting the bindless handle. I changed a few lines and shader code in _imgui_implopengl3.cpp and it seems to be working fine. A simple #define should be sufficient to trigger the backend going bindless.

One issue is that, since you don't have to worry about texture IDs changing any more, I suspect that the related checks in _ImDrawList::OnChangedTextureID() of _imguidraw.cpp must be skipped when one goes bindless, else there's no point. I haven't gone through all the code so I don't know what other parts can/must be altered/omitted.

I am not very knowledgeable about github so I'm not sure what is the best way to propose/present these changes. In any case, if any of this sounds interesting, let me know how I can help.

ocornut commented 2 years ago

Interested in seeing the changes in imgui_impl_opengl3 for reference. Even if for reasons we can't easily merge just yet, at least we get to see them.

Not sure I understand how OnChangedTextureID() can be omitted, drawing with varying textures (e.g. drawing an image) would need some identifier stored and it is only stored in ImDrawCmd.

I'm particularly busy/overwhelmed with tasks and backend tasks in particular so I'll just need people to provide careful/thoughtful help if they want changes happening.

PathogenDavid commented 2 years ago

I'd be interested in seeing the changes too, but my initial reaction to this issue was: "Why though?"

Bindless doesn't feel like it'd really be a win for performance or usability in the context of Dear ImGui (especially in the OpenGL backend.) Almost certainly not enough of a win to justify the increased complexity of the backend.

One issue is that, since you don't have to worry about texture IDs changing any more, I suspect that the related checks in _ImDrawList::OnChangedTextureID() of _imguidraw.cpp must be skipped when one goes bindless

I agree with Omar, I'm not seeing how this could reasonably work. I'd be interested in seeing what you have in mind, but I don't think it'd be reasonable to have ImDrawList work differently based on the backend being used.

I am not very knowledgeable about github so I'm not sure what is the best way to propose/present these changes.

Click the Fork button at the top of this page, it'll make a copy of the Dear ImGui repo on your account. Push your changes to a branch there, and then create a pull request in this repository by following the instructions here.

vdweller84 commented 2 years ago

About the draw code. Here is as it is right now:

void ImDrawList::_OnChangedTextureID()
{
    // If current command is used with different settings we need to add a new command
    IM_ASSERT_PARANOID(CmdBuffer.Size > 0);
    ImDrawCmd* curr_cmd = &CmdBuffer.Data[CmdBuffer.Size - 1];
    if (curr_cmd->ElemCount != 0 && curr_cmd->TextureId != _CmdHeader.TextureId)
    {
        AddDrawCmd();
        return;
    }
    IM_ASSERT(curr_cmd->UserCallback == NULL);
    // Try to merge with previous command if it matches, else use current command
    ImDrawCmd* prev_cmd = curr_cmd - 1;
    if (curr_cmd->ElemCount == 0 && CmdBuffer.Size > 1 && ImDrawCmd_HeaderCompare(&_CmdHeader, prev_cmd) == 0 && ImDrawCmd_AreSequentialIdxOffset(prev_cmd, curr_cmd) && prev_cmd->UserCallback == NULL)
    {
        CmdBuffer.pop_back();
        return;
    }

    curr_cmd->TextureId = _CmdHeader.TextureId;
}

From what I understand, the check

if (curr_cmd->ElemCount != 0 && curr_cmd->TextureId != _CmdHeader.TextureId)

Means "If you detect a texture ID different from what is currently used, draw whatever you have right now." *** With bindless textures you won't have to perform any such check whatsoever.

You are correct in assuming that, in a typical use case, a couple more texture swaps for drawing the GUI don't strain the GPU all that much. I guess it might help on struggling systems.

Another scenario is when there are lots of user-imported images, for instance used as thumbnails in tree structures. image One can argue for using an atlas there too but, in a live editor, (re)constructing atlases every time the user adds/removes an image is not reasonable. By importing each image as a separate texture and using bindless textures, there is a lot to gain performance-wise.

*** I am not privy to all that the function above does, so my first naïve attempt was to comment out the entirety of this check. The program seemed to be working afterwards, of course more experienced people can guess whether I blew something up.

@PathogenDavid Thanks for suggesting the fork/PR, I will give it a go when I have the time.

ocornut commented 2 years ago

OK we need to get back to the beginning.

Are you requesting a change expecting it to bring an optimization, or are you requesting a change because the use of bindless textures on your side of the fence makes it an absolute necessity that the backend is changed in order to be able to draw your textures ?

ocornut commented 2 years ago

PS: you can use Demo>Tools>Metrics/Debugger->DrawLists to visualize the output, its highly probably that you haven’t actually disabled creating of draw cmd per texture otherwise there’s no way the render would work since this is the only place we have info about a texture. Proper bindless implementation would probably need use to pass the same info in eg a uniform instead of calling glBindTexture() and i don’t think it would make any meaningful difference.

PS.2: you didn’t actually post any of the draw code in your section stating “ About the draw code. Here is as it is right now:” , it would be useful to see it.

vdweller84 commented 2 years ago

OK we need to get back to the beginning.

Are you requesting a change expecting it to bring an optimization, or are you requesting a change because the use of bindless textures on your side of the fence makes it an absolute necessity that the backend is changed in order to be able to draw your textures ?

The former. You can have both bound and bindless textures at the same time.

ocornut commented 2 years ago

Then I am sorry but I have to close this. I know you mean well but this is a drain of our development energy for no good reason.

Do some measurements if you need but I suspect if you care about 10k/100k+ draw calls issues you’d be using Vulkan. Displaying 100 icons on screen even with varying textures is not going to be a problem.