Vulkan: switching between images for rendering

[martty]

As described previously, I implemented ImTextureID with a VkDescriptorSet.

• I added a utility function for allocating the ImTextureIDs. This allows the user to set both sampler and image(view). Can be removed if not needed. (Note: freeing is not necessary, since the DescriptorSet comes from a pool)
• Not 100% sure if the cast to void* is legal, someone please correct me if it is not.
• Let me know if I messed up the formatting!

[ratchetfreak]

non-dispatchable handles (like the vkDescriptorSet) are 64 bit handles. So on 32bit this will create issues.

[MrSapps]

TextureId appears to be the size of a pointer, so should work

[martty]

This is what I was afraid of. It is not apparent for me how to best solve this.

We could shove the VkDescriptorSets in an array and hand out indices (+ an indirection to allow deletion).

Or the user could be responsible for management: Put a dynamic array field in the init struct containing the VkDescriptorSets and then ImTextureID is the index into this array. User is responsible for putting the VkDescriptorSets in this array and keeping the ImTextureIDs up to date.

Also there is some difficulty of freeing the DescriptorSets (which I didn't touch in this PR). Maybe the DescriptorPool should be internal, so we can use VK_DESCRIPTOR_POOL_CREATE_FREE_DESCRIPTOR_SET_BIT to free individual Sets, otherwise it will be more difficult to recreate the ones still needed..

Any ideas?

[ocornut]

Are many people going to use Vulkan in 32-bits?

I could add a #ifndef ImTextureId block to allow overriding that type in imconfig.h, or somehow find a way to reword its declaration so that it would fit 64-bits (not sure how yet), if any useful?

[martty]

I think redefining the ImTextureID as unsigned long long (or redefining behind an ifndef if someone wants both Vulkan and 32 bits) would do the trick. As for the first question, I have no idea.

[ocornut]

Question to @martty, @ratchetfreak @SaschaWillems @ParticlePeter and other Vulkan users:

Would it perhaps make more sense (and solve this issue) to make ImTextureId a pointer to descriptor set, aka VkDescriptorSet*, instead of the actual structure? Would the user be able to provide pointers to them that would be valid for the frame?

I'm asking because I don't really know the typical lifetime of a VkDescriptorSet nor whether its size is supposed to be opaque.

[SaschaWillems]

Not sure about this. A descriptor set contains information beyond what the actual image/texture is about.

Having ImTextureID point to a VkDescriptorSet is something I personally wouldn't do and leave that part to the application. With the change proposed here a ImTextureID would always point to a descriptor set with a fixed image view, fixed image layout (shader read, some ppl. might want to use something else). Fixing the descriptor type to a combined image sampler is also something not everybody may want to use (separating image and sampler is something very common). This would also force the application to allocate such descriptor types when creating the pool.

So IMO I'd prefer the current way with the ImTextureID pointing to VkImage.

[ocornut]

OK, so there are two topics now here. :) Initially Martty suggested VkDescriptorSet and the problem we have at the moment is that ImTextureId is a void* so VkDescriptorSet would NOT fit in a 32-bit app. And I'm hesitant to turn ImTextureId storage into a long long because ImTextureId has already been a huge source of confusion (across all backends) for newer programmers, so I suggested VkDescriptorSet* instead of VkDescriptorSet.

I'd prefer the current way with the ImTextureID pointing to VkImage.

Now you are suggesting to use VkImage. However note that the current backend does NOT support either. It is not the "current way", in fact the render loop currently completely ignore the pcmd->TextureID value making it impossible with the current code to draw other images with ImGui or ImDrawList. So this is what we are aiming at sorting out here!

I'd be happy if you Vulkan users can have that discussion, would be super helpful as I'm not using Vulkan myself yet.
And maybe there isn't a perfect answer. It is semi-expected that people who are building a rendering layer in their engine would eventually replace the render loop. The proposed default is for the Vulkan examples and it's meant to be the "best" reasonable default for Vulkan users who aren't building their own rendering layer.

[martty]

I was really trying to make this change as non-intrusive and user friendly as possible. For users wanting slightly more control can simply make their own ImTextureIDs (adding an extra parameter for layout to AddTexture seems sensible however). Any further would require redoing the shaders, and at that point you could write your own rendering backend. This all being said, I am not against a different implementation.

I'm not sure why putting the VkDescriptorSet simply on the heap didn't occur to me back then, but that could be a solution to the pointer size problem. We should provide a deallocation function then, though.

[ocornut]

I'm not sure why putting the VkDescriptorSet simply on the heap didn't occur to me back then, but that could be a solution to the pointer size problem. We should provide a deallocation function then, though.

Wouldn't the user already have VkDescriptorSet instances they can just point too?

[martty]

Currently, I made the AddTexture function, which, with the proposed change would do a heap allocation. I think it is reasonable to have a pair function that frees that allocation.

Otherwise, the user can use any pointer to VkDescriptorSet, but allocating & setting a proper VkDescriptorSet requires "inside" knowledge at the moment (the VkDescriptorSetLayout).
(eg. if you meant: would the users reuse their own descriptor sets: they can't)

[meshula]

I went with something similar to @martty 's solution, because one needs to bind an image and sampler to the descriptor set to pass to the cmd within ImGui, and the only ways I thought of to do it, that worked, are to either to do it persistently and pass a descriptor around via the TextureId, or to recreate the descriptor sets on the fly on demand inside of ImGui. I think I understand @SaschaWillems' concern, but it's hard to imagine how to keep ImTexture referring only to a VkImage. My solution was to provide ImGui with a VkImage, and for ImGui to hand me back an ImTexture which is backed by an object that has got the descriptor in it, so that ImGui can manage the sampler and descriptor set persistently on its own. I don't have any expectation that I can unpack the ImTexture and magically use it outside of ImGui.

[mua]

Descriptors worked well for me. I keep descriptors on my texture objects, pass descriptors to ImGui::Image.

[meshula]

@mua Would you mind posting what you did for comparison with other solutions? It would be great if we could get consensus on a solution and merge it.

[mua]

@meshula I did not want ImGui to persist any resources, I just put descriptor set handle into ImTextureID just like @martty did, bind it in ImGui_ImplVulkan_RenderDrawData. And add a lazy generator for descriptor set on my texture objects. just a method returns guiDescriptorSet if not allocated previously, using the same descriptor set layout as ImGui. So I can tie lifetime of descriptor set to my texture, buffer, image, sampler etc.

[gviot]

I merged this PR into my own binding (loosely based on the SDL/Vulkan binding). I automatically create the imgui descriptor set (using the ImGui_ImplGlfwVulkan_AddTexture function from the PR) for every texture that I load into my engine because I use imgui to browse the list of all loaded textures anyway.
It works nicely and I didn't care about 32 bits so I used VkDescriptSet as the ImTextureID directly to make things simple.
I don't think using VkDescriptorSet* for everyone as a default would be great because it makes things more complex and potentially even more confusing for newbies. Not sure how to handle it nicely for 32 bits users though.

[aCuria]

Hi,
I am new to imgui, and ran into a problem where passing a VkImage handle to ImGui functions does nothing. Eg: the following code: ImGui::Image(vulkanTexture_.handle(), ImVec2(1000, 1000)); will render the font atlas

I found the following comment in imgui_impl_vulkan.cpp which led me to this post, however there doesnt seem to be a solution explained in this post for newbies yet.
// Missing features:
// [ ] Renderer: User texture binding. Changes of ImTextureID aren't supported by this binding! See #914

Is there an interim solution I can use before this is fixed in v1.65? (Will it be fixed in 1.65? =x)

[ocornut]

Finally merged.

Here's what I did:

• Rebased and merged @martty commit as 29f1043
• Added ceb26ba on top with various comments, niceties, build files changes.

The later commit makes example build files compile with #define ImTextureID ImU64.
Among other thing I did this at the time of binding:

#if UINTPTR_MAX == 0xffffffff
if (sizeof(ImTextureID) < sizeof(ImU64))
{
    // We don't support texture switches if ImTextureID hasn't been redefined to be 64-bit. Do a flaky check that other textures haven't been used.
    IM_ASSERT(pcmd->TextureId == (ImTextureID)bd->FontDescriptorSet);
    desc_set[0] = bd->FontDescriptorSet;
}
#endif

The backend will compile on 32-bits platform without a imconfig directives, but support for that will then be disabled at runtime with an assertion to detect other uses (which is unperfect but should catch most). The small niggle with doing this is that to allow for the check to be done, ImGui_ImplVulkan_AddTexture() now returns VkDescriptorSet but there are comments above. I believe it makes the features somehow easier to grasp and makes the design also easier to challenge.

As mentioned countless times, it appears there's no consensus on how to do things right. so this might change. I would still really like people to chime in and provide a lengthy explanation of what could be done. I believe runtime override of texture binding like #2697 is probably the way to go.

Thanks @martty and everyone for your patience.

[rolandhill]

Thanks everybody for this. Is it possible to get an example of how to use the Vulkan ImTextureID included on the wiki page https://github.com/ocornut/imgui/wiki/Image-Loading-and-Displaying-Examples?

[filnet]

A similar descriptor set approach was taken, after a bit of erring, in imgui-rs-vulkan-renderer.
The main difference being that it is the responsibility of the application to provide the descriptor set.

See https://github.com/adrien-ben/imgui-rs-vulkan-renderer/pull/3/files#diff-d683eee04fe76db84204abb0d0ac423596f368fd20f604ad52266fec2a21c2e8R167

PS : please overlook Lenna, will submit a PR someday...

[dpwiz]

Drawing a bunch of images this way can kneecap performance, since each draw will be wrapped in a descset switch.

dear-imgui can have a image descriptor array instead and pass texture IDs in push constants.

layout(set=0, binding=0) { .... generic stuff ... }
layout(set=0, binding=1) uniform sampler2D textures[];

Backend would reserve some DESCRIPTOR_TYPE_COMBINED_IMAGE_SAMPLER descriptors.
Then it can update everything at once, putting all the known textures in there, or issue write in new elements as needed.

[LogiCoal]

Good point! @dpwiz

Put all textures in an array, then just get the specific texture by index without rebinding, which can improve performance.

But the implementation could be a little complex.

Suppose we need to change textures frequently( sometimes, we just don't know all textures will be used or maybe it will take too many memory to load all textures), then we'll need a way to update the texture array, which might cause the texture index changed for a specific texture.

A possible solution here (Just add an indirect layer):

                                                       +--------------------------------------------------------+
                                                       |                                                        |
                                                       |                                                        |
                                                       |                                                        |
                                                       |                                                        |
                                                       |                                                        |
                                                       |                                                        |
                                                       |                                                        |
                     +--------------------------------------------------+                                +------+--------+
                     |                                 |                |                                |               |
                     |        +-----------+       +----v------+         |                                |   Render ObjX |
raw texture file     |        |           |       |           |         |                                |               |
                     |        |  image 0  |       |  image i  |         |                                +---------------+
                     |        |           |       |           |         |
                     |        +----+------+       +------+----+         |                       ObjX uses texture "image i",
                     |             |                     |              |                       the index of "image i" is dynamic,
                     +--------------------------------------------------+                       we can get it by
                                   |                     |
                                   |    +----------------+                                      getImageTextureIndex("image i")
                                   |    |                    map out of order
                                   +--------------------+                                       Then set the index by pushconstant
                                        |               |
                                        |               |
                                        |               |
                                        |               |
                     +------------------------+-------------------------+
                     |                  |     |         |               |
                     |        +---------v-+   |   +-----v-----+         |
                     |        |           |   |   |           |         |
texture array        |        | texture0  |   |   | texturei  |         |
                     |        |           |   |   |           |         |
                     |        +-----------+   |   +-----------+         |
                     |                        |                         |
                     +------------------------+-------------------------+

[dpwiz]

You can treat the texture array as a pool for some texture id slots. Nothing to reinvent here.

Allocate a bunch of "slots", fill in with all the initially known ImageViews (fonts, etc.), add remaining to a free list. Adding a texture would mean picking one slot from the free list and writing a new ImageView to its array index. Evicting a texture is simply returning its index to the free list. Let the texture owner manage its resources.

Also, using image views means you can update the data without updating the descriptor. If you really need to shuttle new image data without blocking to rebuild the descriptor array, just copy your data to the image you've already registered.

[JakubChomicz]

Someone maybe know when will this be documented in here: https://github.com/ocornut/imgui/wiki/Image-Loading-and-Displaying-Examples ? It would be really helpfull.

[ghost]

Good point! @dpwiz

Put all textures in an array, then just get the specific texture by index without rebinding, which can improve performance.

But the implementation could be a little complex.

Suppose we need to change textures frequently( sometimes, we just don't know all textures will be used or maybe it will take too many memory to load all textures), then we'll need a way to update the texture array, which might cause the texture index changed for a specific texture.

A possible solution here (Just add an indirect layer):

                                                       +--------------------------------------------------------+
                                                       |                                                        |
                                                       |                                                        |
                                                       |                                                        |
                                                       |                                                        |
                                                       |                                                        |
                                                       |                                                        |
                                                       |                                                        |
                     +--------------------------------------------------+                                +------+--------+
                     |                                 |                |                                |               |
                     |        +-----------+       +----v------+         |                                |   Render ObjX |
raw texture file     |        |           |       |           |         |                                |               |
                     |        |  image 0  |       |  image i  |         |                                +---------------+
                     |        |           |       |           |         |
                     |        +----+------+       +------+----+         |                       ObjX uses texture "image i",
                     |             |                     |              |                       the index of "image i" is dynamic,
                     +--------------------------------------------------+                       we can get it by
                                   |                     |
                                   |    +----------------+                                      getImageTextureIndex("image i")
                                   |    |                    map out of order
                                   +--------------------+                                       Then set the index by pushconstant
                                        |               |
                                        |               |
                                        |               |
                                        |               |
                     +------------------------+-------------------------+
                     |                  |     |         |               |
                     |        +---------v-+   |   +-----v-----+         |
                     |        |           |   |   |           |         |
texture array        |        | texture0  |   |   | texturei  |         |
                     |        |           |   |   |           |         |
                     |        +-----------+   |   +-----------+         |
                     |                        |                         |
                     +------------------------+-------------------------+

Just like using some tech like descriptor indexing to get a unbound descriptor set, and it's easy to update this set async safe, i think it is a good idea for imgui texture binding.

[ocornut]

Although likely to be a bit bulky, it would be nice if someone could add an example for loading textures in Vulkan in this Wiki page: https://github.com/ocornut/imgui/wiki/Image-Loading-and-Displaying-Examples

[timprepscius]

Hey there, I'm reading through this and the vulkan backend code, this question probably has a super obvious answer but, here goes:

I see calls to vkAllocateDescriptorSets, and I get they are allocated from pools.
When are those pools freed? Is it per frame?

Do I need to worry about descriptor sets reaching some maximum?

[DangitBen]

Although likely to be a bit bulky, it would be nice if someone could add an example for loading textures in Vulkan in this Wiki page: https://github.com/ocornut/imgui/wiki/Image-Loading-and-Displaying-Examples

As @ocornut, @rolandhill, and @Chomiczjakub asked, I have added an example to the above page for Vulkan after struggling with this for a while. I am far from a Vulkan/ImGui expert, so if someone would be willing to look it over for any egregious errors/malpractice I would appreciate it. If anyone wants to test it out, that would be great too!

I made some assumptions for the sake of "brevity":

• A starting point is one of the vulkan example files (uses g_Device, g_Allocator, g_MainWindowData, g_PhysicalDevice, and check_vk_result()).
• It is acceptable to use one of the command pools from the g_MainWindowData.
• User is responsible for cleaning up/destroying any Vulkan objects in a proper implementation.

[CarloWood]

Thanks to @DangitBen for taking the effort. I definitely thought about writing up such an example, but Vulkan is just too bulky. In my opinion it wasn't possible to write something self-contained without falling into the same pitfall that every other tutorial on vulkan out there has fallen into: you can't use their code without being severely limited - even making grave mistakes in terms of real-life code (ie, baking into the example a 1:1 relationship between two objects that doesn't exist). A beginner, when using such code as a base for their own code almost MUST run into serious problems later on when they try to do something that is more real-life, more practical (e.g. try to write an actual game). For example, I could not use my own code to show how to upload a texture to the GPU - because that is useless without using my vulkan engine. And it is not possible to rip out those parts of the engine that are being used and include them in the example-- vulkan is just too darn complex.

PS Here is code that I use to upload a texture:

      vk_utils::stbi::ImageData texture_data(m_application->path_of(Directory::resources) / "textures/vort3_128x128.png", 4);
      // Create descriptor resources.
      { 
        static vulkan::ImageKind const sample_image_kind({
          .format = vk::Format::eR8G8B8A8Unorm,
          .usage = vk::ImageUsageFlagBits::eTransferDst | vk::ImageUsageFlagBits::eSampled
        });
  
        static vulkan::ImageViewKind const sample_image_view_kind(sample_image_kind, {});

        m_sample_texture = vulkan::shader_resource::Texture("vort3", m_logical_device,
            texture_data.extent(), sample_image_view_kind,
            { .mipmapMode = vk::SamplerMipmapMode::eNearest,
              .anisotropyEnable = VK_FALSE },
            graphics_settings(),
            { .properties = vk::MemoryPropertyFlagBits::eDeviceLocal }
            COMMA_CWDEBUG_ONLY(debug_name_prefix("m_sample_texture")));
     
        auto copy_data_to_image = statefultask::create<task::CopyDataToImage>(m_logical_device, texture_data.size(),
            m_sample_texture.m_vh_image, texture_data.extent(), vk_defaults::ImageSubresourceRange{},
            vk::ImageLayout::eUndefined, vk::AccessFlags(0), vk::PipelineStageFlagBits::eTopOfPipe,
            vk::ImageLayout::eShaderReadOnlyOptimal, vk::AccessFlagBits::eShaderRead, vk::PipelineStageFlagBits::eFragmentShader
            COMMA_CWDEBUG_ONLY(true));
  
        copy_data_to_image->set_resource_owner(this);   // Wait for this task to finish before destroying this window, because this window owns the texture (m_sample_texture).
        copy_data_to_image->set_data_feeder(std::make_unique<vk_utils::stbi::ImageDataFeeder>(std::move(texture_data)));
        copy_data_to_image->run(vulkan::Application::instance().low_priority_queue());
      }

[ocornut]

Today added ImGui_ImplVulkan_RemoveTexture() for api symmetry a588f00

@DangitBen Thanks for a lot for this addition to the tutorial. I confirmed it works on my side, but realized using LoadTextureFromFile() as provided would lead to debug layer emitting leak info on exit.

Do you think you or someone) could refactor this?

(1) Bundling all data into a structure e.g.

struct MyTextureData
{
    VkDescriptorSet DS;         // Descriptor set: this is what you'll pass to Image()
    int             Width;
    int             Height;
    // + missing fields

    MyTextureData() { memset(this, 0, sizeof(*this)); }
};

Aptly named this way to emphasis this is an example and likely not a base for more advanced stuff, but I'm also sure some casual users will be happy using this as it so it is nice if it's usable as it.

(2) Adding a RemoveTexture(MyTextureData*) in the example freeing all the right Vulkan objects (include a call to newly added ImGui_ImplVulkan_RemoveTexture()` but more stuff needs to be done).

Aim being that example should be able to exit without any report from debug layer.

[DangitBen]

I confirmed it works on my side, but realized using LoadTextureFromFile() as provided would lead to debug layer emitting leak info on exit.

Yeah I did call this out in my comment above, but not in such specific language.

Do you think you or someone) could refactor this?

@ocornut done! While I am not certain about what the lifetime of the objects should be, the validation layers seem to be silent and happy now.

[ocornut]

@DangitBen Thank you for updating the wiki!
https://github.com/ocornut/imgui/wiki/Image-Loading-and-Displaying-Examples

[Nemirtingas]

Hi, instead of the ImGui_ImplVulkan_AddTexture/ImGui_ImplVulkan_RemoveTexture, you could do something analogous to DirectX12, where you send the CPU and GPU handles, and passing a VkDescriptorSet in the init function?