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 https://github.com/ocornut/imgui/pull/914

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

[martty]

I updated my branch to reflect upstream changes, so people led here can more easily integrate this code while this PR is in limbo (I also added the ability to specify image layout, but otherwise the PR is unchanged).

[hanatos]

i found this PR really useful, thanks! it seems it doesn't directly apply to master, here is my adjusted commit in case anyone else finds it useful: https://github.com/hanatos/imgui/commit/6e39cb5991cd96c7f53ec1a3d1f25c83e82a5144

i put a descriptor pool inside imgui, not sure everybody would agree with this.

[lordalcol]

i found this PR really useful, thanks! it seems it doesn't directly apply to master, here is my adjusted commit in case anyone else finds it useful: hanatos@6e39cb5

i put a descriptor pool inside imgui, not sure everybody would agree with this.

Thanks, that seems reasonable. How would an application remove a texture (aka destroy the descriptor set)?

[hanatos]

thanks, i hadn't thought of that. i have a somewhat hacky answer to this, let me know if you have any better ideas.

1. added obvious final cleanup: https://github.com/hanatos/imgui/commit/3425334f66324d0ef6f39ce33797bb341a878400#diff-180a54b806063579542c0d0ae7ec1566R792

2. i'm now tearing down the whole descriptor pool in NewFrame: https://github.com/hanatos/imgui/commit/3425334f66324d0ef6f39ce33797bb341a878400#diff-180a54b806063579542c0d0ae7ec1566R827

which unfortunately means i need to re-create the descriptor set for the font image: https://github.com/hanatos/imgui/commit/3425334f66324d0ef6f39ce33797bb341a878400#diff-180a54b806063579542c0d0ae7ec1566R830

this seems to work but is arguably quite a bit ugly. but i'm also shying away from pushing all other textures into a vector and resetting the descriptors individually. i suppose going this way could be implemented by reusing _TextureIdStack on the DrawList (and would potentially be slower).

there is one more change i had to make to integrate with the rest of my code, which is to set the stageFlags to ALL, so i can use the descriptor in compute shaders, too: https://github.com/hanatos/imgui/commit/7f33caaae56a7a16d512601c4cf1f93bad9ec24c

this makes me wonder how generic this solution with the descriptor sets can ever be (maybe most people will want to write their own bindings after all). even so i think it would be great to have image support in the vulkan bindings out of the box.

On Fri, Jun 14, 2019 at 9:53 AM Lorenzo Dal Col [email protected] wrote:

i found this PR really useful, thanks! it seems it doesn't directly apply to master, here is my adjusted commit in case anyone else finds it useful: hanatos@6e39cb5 https://github.com/hanatos/imgui/commit/6e39cb5991cd96c7f53ec1a3d1f25c83e82a5144

i put a descriptor pool inside imgui, not sure everybody would agree with this.

Thanks, that seems reasonable. How would an application remove a texture (aka destroy the descriptor set)?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/ocornut/imgui/pull/914?email_source=notifications&email_token=AAMAKKNTNM4OMK2DMKHJPUDP2NE63A5CNFSM4CWIZ5F2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXWASEI#issuecomment-502008081, or mute the thread https://github.com/notifications/unsubscribe-auth/AAMAKKJ6FURCHLWQBQCLS7TP2NE63ANCNFSM4CWIZ5FQ .

[martty]

Rebased onto master.

[hanatos]

this is the minimal gist of martty's commit to make me happy for my use case: https://github.com/hanatos/imgui/commit/c91a96a2241e141d3f090cddacc9a5af0477599b

(i don't mind 32-bit and i don't want to create VkImageView + descriptors on imgui's side, as i already have them on the other side of the fence)

[meshula]

nice

[johannesugb]

@martty First of all, thanks for your efforts! I have successfully managed to display a texture quite easily with this PR.

Two questions:

Question 1) Descriptor Sets allocated within ImGui_ImplVulkan_AddTexture are not freed anywhere, are they? If I got it right from following the discussion, they are freed when the Descriptor Pool is destroyed, right? Although it is possibly a rare case, there could be applications where the pool runs out of memory.

Question 2) What do you think of making ImGui_ImplVulkan_AddTexture also an "immediate mode"-style function? Concretely, I mean implementing a cache which would check -- based on the parameters VkSampler sampler, VkImageView image_view, VkImageLayout image_layout -- whether or not a descriptor for these parameters has already been created and if so, return it from the cache. That would be more in line with how ImGui is generally being used. With the current state, one has to make sure to not invoke ImGui_ImplVulkan_AddTexture every frame in order to not create a descriptor set every frame, right?

The approach from question 2 would also provide a solution to question 1 if a proper cleanup was implemented. In particular, I am thinking of cleaning up the cache within ImGui_ImplVulkan_NewFrame by tracking the frame-id for each cache item and if the "last used frame-id" is < "current frame-id" minus ImGui_ImplVulkan_InitInfo::ImageCount, a cached item can be destroyed. What do you think?

[Ybalrid]

This PR sadly doesn't apply cleanly to the current docking branch...

I am not familiar enough with both Vulkan and ImGui's codebase to go look at the details (yet... 😅)

[figwig]

@Ybalrid I've got it with the current docking branch, I've got everything from docking with just the impl_vulkan files from martty and it works great

[Ybalrid]

@figwig I will try again to just drop these files in the repo checked out as the docking branch then ;-)

[sirus20x6]

so I have a pointer to RGBA data, the columns, the width, and how exactly am I supposed to get that into an ImTextureID? It's not clear at all

[Ybalrid]

@sirus20x6 You cannot do anything with ImGui and this raw bitmap data.

ImTextureID are "Graphic API specific" handle to a texture of some sort. So, you need to get your image data into a texture, via the graphics API used, you need to have that done before attempting to display the texture in an ImGui window.

In the very specific case of Vulkan (the subject matter of this pull request), ImGui upstream do not handle them (yet). This pull request propose to use a Vulkan descriptor set handle as waht is stored and passed via ImTextureID (ImTextureID itself is just a non-descript "pointer sized type") + an unity to create one adapted to the rendering code here from a Vulkan Image, ImageView and Sampler object.

If that's confusing for you, you should study the chapter of Alexander Overvoorde excelent Vulkan Tutorial here : https://vulkan-tutorial.com/Texture_mapping/Images

Hope this is helpful for you. (This issue comment section is probably not the best place to ask this question)