Fix Vulkan examples descriptor pool setup

[SaschaWillems]

This PR fixes the descriptor pool setup for the Vulkan SDL2 and GLFW examples. The original code passed an unnecessary large set of pool sizes including descriptor types never actually used. It also passed a huge no. of max sets that could be allocated.

This has been changed with this PR, which adjusts the pool to what's actually used in the samples: One combined image sampler descriptor type for the font image and one descriptor set.

[ocornut]

Thank you.
For the purpose of facilitating quick use of the AddTexture helper function, maybe it is worth making this a slightly bigger value eg 10 with a comment as to why we are picking this?
Full app would require to do something better anyway but this would remove a little bit of friction with experimenting with loading images.

[SaschaWillems]

A bigger value wouldn't make any sense with regards to the sample. If an application uses ImGui it'll most probably use it's own pool anyway, and the setup for that pool would be pretty specific to that application.

I also added a command above the setup as to why the pool is setup like it is.

[ocornut]

The idea is that if someone wants to try something like https://github.com/ocornut/imgui/wiki/Image-Loading-and-Displaying-Examples in examples or as part of a test bed derived from it, which is subject to many types of user errors in the first place, this remove one point of failure for the “quick attempt at getting this working”.

[SaschaWillems]

Sure, but the current approach of creating an insanely large pool may fail on implementations with smaller pool size limits and also teaches bad practices. On the other hand, if you allocate with a pool that's too small you'll get a validation error and (depending on the implementation) an error at allocation time that tells you that you need to increase the pool size.

For a low-level api like Vulkan, there is no solution that fits all use cases, so this needs to be handled at the documentation level.

Besides, if someone wanted to display a Vulkan image inside ImGui that person already would've needed to allocate that image from their apps pool.

The "correct" way is to have the host application setup a pool and the pass that to the Vulkan functions of ImGui. or don't use the Vulkan functions provided by ImGui at all and have the host application deal with it, which is the most flexible way.

[ocornut]

I may misunderstand something but surely it wouldn’t hurt to change that 1 to eg 5 or 10 and comment explicitly that the app as-is needs only 1, but it lower barrier for newcomer to try adding a texture. They’ll see the barrier soon enough if they want to scale to more, but if it’s not an initially blocking barrier it decreases likehood of newbies giving up.

Basically we want a simple attempt to use ImGui_ImplVulkan_AddTexture() to be more likely to work without making mods to existing code, just adding code.

For better or worse, many people use those samples as defacto starter apps so there is a little responsibility to make that as friendly as we can. I understand the old setup with 1000 of everything was absurd but I am arguing for 10 of one type for the purpose of making things easier for newcomers toying with things they don’t yet understand.

[SaschaWillems]

5 or 10 are just different arbitrary limits. I still think this is a documentation issue, and if the sample contains proper comments that tell people about having to adjust the descriptor pool size for their needs that's (at least imo) fine.

I think I noted it in the original PR, but trying to get ImGui_ImplVulkan_AddTexture to work in any case for any app simply won't work as descriptor handling and image/sampler creation should be the apps responsibility. But that's just my opinion, feel free to discuss ;)

[ocornut]

I have merged your PR (with 1 additional comment) as fa2f1bf.
I have updated https://github.com/ocornut/imgui/wiki/Image-Loading-and-Displaying-Examples#example-for-vulkan-users

think I noted it in the original PR, but trying to get ImGui_ImplVulkan_AddTexture to work in any case for any app simply won't work as descriptor handling and image/sampler creation should be the apps responsibility. But that's just my opinion, feel free to discuss ;)

I don't understand what this means or imply. I am not a Vulkan user and I kept #914 open for about 5 years hoping for people to come to a right solution and consensus and explain/convey it to me but this didn't happen.
If you have any idea for a better solution I am open to it.
EDIT the aim is that e.g. Vulkan users should be able and encouraged to use imgui_impl_vulkan.cpp without modification, while being able to integrate with their application/engine and use textures in e.g. ImGui::Image().

My intuition is that we should adopt an idea similar to #2697 aka let user app register a custom method for binding a ImTextureID.
This would need the font atlas texture id created by the backend to be either marked as different, either the user app would need to override this ID with the corresponding one. This ties to some of the work for #3761

[ocornut]

Closing this as merged.

As per my last comment, still (maybe) in need of someone more pro-actively helping with the design of using textures/images as I'm not competent enough to do it.