Backend Vulkan: Make descriptor set pool optional

[Thalhammer]

In order to use ImGui with Vulkan you need to provide a descriptor set pool, which in many cases will get exclusively allocated for ImGui. This PR makes the parameter optional and automatically creates/destroys an internal pool if non is provided.

I oriented the creation code on the examples, but the pool is obviously waaayy oversized for ImGui alone, it should probably get adopted to the real sizes needed.

[ocornut]

Hello,

Thanks for the PR.
This feels like adding extra burden to the backend, more so as those buffers may need proper sizing and right now there has been no thought about how to do this weel.

Also linking to #914 - which is still dangling as my questions were not answered and I am not familiar enough with how "textures" are managed in a variety of engine.

[Thalhammer]

I am by no means an experienced Vulkan developer, but I can share some of my experience so far.
In my opinion:

• 32 bit is dead (a)
• The value passed to ImGui::Image should be an VkImage (b)
• The datatype of ImTextureID should be uint64_t (c)

(a) Don't get me wrong, 32bit is still a thing (as is 16 and 8bit) but the latest Intel cpu that did not include 64bit support was Pentium 4 Northwood (desktop) and Atom Z6xx (mobile) which were released 20 and 11 years ago[1]. Apple discontinued 32bit a while ago [2] (as in you can't run 32bit code anymore) and ARM announced to stop creating new 32bit designs for their Cortex-A lineup last year [3]. Imho its safe to assume that anything capable of running ImGUI with a vulkan renderer will also be 64bit capable.

(b) VkImage is (roughly) what a Texture is in opengl and since the ogl backend accepts textures I think it makes a lot of sense for Vulkan to do the same. It is also the easiest for both user and ImGui to manage. Passing in a VkDescriptorSet would restrict the vulkan backend and increase burden on the user.

(c) Since changing it from void* to uint64_t would break all existing code I propose a small wrapper function in the header that still accepts a pointer, reinterpret_casts it to uint64_t and passes it to the real function. This should allow safely switching over without breaking too much code. Another option would be to introduce a new typedef ImImageID and marking the old as deprecated.

So much for my two cents. I hope this helps. Whatever the decision is: There needs to be one. Vulkan doesn't support images isn't a valid response for most people, so they roll their own modified version of imgui (just like I and many before me did) which inherently becomes out of date, cause incaptibilies and missing bugfixes.

[dpwiz]

Not only this is oversized, it contains all kinds of resources never to be used by dear-imgui.

[Thalhammer]

As far as I could tell IMGUI only ever allocates a single descriptor of type VK_DESCRIPTOR_TYPE_COMBINED_IMAGE_SAMPLER but I don't know enough about imgui to judge that. But thats pretty much exactly my point: The maintainers of the vulkan backend will know a lot better which descriptors are needed than any value a user of the library could ever come up with.

[dpwiz]

The value passed to ImGui::Image should be an VkImage (b)

VkImage is just a bunch of bytes. You need at least a VkImageView to make sense of it. Of course dear-imgui can create it by itself, but then it can do all kinds of stuff. An engine should already have that image view on hands, since it is using them everywhere too.

(b) VkImage is (roughly) what a Texture is in opengl and since the ogl backend accepts textures I think it makes a lot of sense for Vulkan to do the same.

OpenGL does a lot of resource tracking by itself. Vulkan is different in that regard and you must register your stuff before using it.

(And I feel this is off-topic for this PR, and the implementation discussion should move to 914).

[ocornut]

Hello,
Any specific reason for closing this? Open PR are valuable to track information/ideas that are still pertinent.

[Thalhammer]

Any specific reason for closing this?

No I just cleaned up the forked repos in my profile and didn't remember I still had this PR open cause its been so long.
The changes are still accessible in the "Files changed" tab, feel free to use them if you want.

[ocornut]

Does the "Reopen Pull Request" button works for you?

[Thalhammer]

Does the "Reopen Pull Request" button works for you?

Unfortunately not, because the head repo doesn't exist anymore.

[ocornut]

Makes tracking trickier, it's an unfortunate issue with GitHub issue, as people commonly accidentally close old PR when cleaning up their repo (it used to keep PR open, they changed behavior).

[ocornut]

@SaschaWillems @Thalhammer

Yes, yes it is. Which is kinda my point. The application writer has no idea
or way to figure out what imgui needs and the example provides useless
values. This is why stuff like that should be managed by imgui itself.

Happy to steer toward this change. Whatever change the TL;DR; is we want to minimize user involvement but maximize compatibility with users/existing Vulkan codebases.

After 1.90 is shipped one on my hope is to focus on finishing #3761 which would make backend able to update and recreate textures, one difficulty being the possibility of occasional multiple in-flight texture (when e.g. a full texture rewrite is necessary, we need to keep old one for as long as needed by past rendering, while creating a new one). Once #3761 is done for all backends we can engage in the dynamic atlas work.

[SaschaWillems]

Yes, yes it is. Which is kinda my point. The application writer has no idea
or way to figure out what imgui needs and the example provides useless
values. This is why stuff like that should be managed by imgui itself.

I disagree. It's usually the other way round. The UI library shouldn't need to know anything about what the application's api backend requires, as that may force the host application to work around global assumptions made in the UI library. The Vulkan implementation of ImGui e.g. creates a combined image sampler, but what if you don't want to use that?

From my experience it's best to leave this to the host application.

[ocornut]

This is now solved by #8172.