Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Backends: Vulkan: Make descriptor pool optional #8172

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

zeux
Copy link

@zeux zeux commented Nov 21, 2024

Instead of specifying DescriptorPool when initializing Vulkan backend, it is now possible to instead specify the descriptor pool size. An internal descriptor pool will be created and destroyed alongside other objects.

The size needs to accommodate the textures that will be added, thus it will be application dependent.

This makes integration into Vulkan renderers simpler. Not all renderers have an available descriptor pool with _FREE_DESCRIPTOR_SET bit that is not reset every frame; right now in many cases integration will require creating a separate pool and destroying it alongside imgui backend. With this change, the process is simplified by specifying the maximum number of descriptors.

See also: #4867

Instead of specifying DescriptorPool when initializing Vulkan backend, it
is now possible to instead specify the descriptor pool size. An internal
descriptor pool will be created and destroyed alongside other instances.

The size needs to accommodate the textures that will be added, as such it
will be application dependent.
@ocornut ocornut added this to the v1.92 milestone Nov 25, 2024
@ocornut
Copy link
Owner

ocornut commented Nov 26, 2024

Thanks Arseny. This coincide with work I've been doing which will require me to settle on a few related things.
May I stray on a few tangents to get your advice here?

TL;DR I am working on a small revamping of IO to allow imgui to create and update textures (for dynamic font atlas support etc.). As a result the backend will need (occasionally, and generally temporarily) to have multiple textures allocated, e.g. when needing to grow a texture we keep the previous one (for previous in-flight renders + for what has been submitted so far in the frame) and we create a second texture.

I've implemented this new API in the DX12 backend on my WIP branch. I came up with this earlier change in public backend API: 40b2286 (+ minor tweaks 08400f5, 142827f). Basically the DX12 backend user can pass alloc/free callbacks for when the backend needs a SRV descriptor which is currently our way to describe an image when e.g. using ImGui::Image(). Following this change, in 40b2286 the examples's main.cpp provide a simple allocator (basically a free list), which is supposed to mimic what a real DX12 application would use.

A. If you are familiar with DX12, do you think my change above is a viable design?
B. I will need to implement the texture stuff for Vulkan, but if I understand things correctly (and pardon me but I'm not a real user of neither DX12 neither Vulkan), the ExampleDescriptorHeapAllocator in DX12 example has no equivalent as Vulkan land as the VkDescriptorPool serves the equivalent purpose? And therefore there will be no need to add alloc/free callback, just the existing descriptor pool (+ your patch to make it optional).

Thanks!

@zeux
Copy link
Author

zeux commented Nov 26, 2024

Re: B: Yeah this is largely correct. There is a way to allocate descriptors from a heap manually in Vulkan when you use EXT_descriptor_buffer extension, but this extension is very optional (not implemented/supported on some hardware and drivers), and requires different setup in some places. In general in Vulkan the descriptor story is complicated as there's a few different options to manage them (e.g. descriptors don't necessarily need to be allocated at all when using KHR_push_descriptors), the only actual constant right now is descriptor pools (from Vulkan 1.0) and the newer mechanisms are in flux. So I would recommend to stay with pool allocation for now.

Re: A: Just so that I understand better, before DX12 backend only ever needed 1 SRV, but you want to allow using more than 1? How high do you want that number to be? The alloc/free callbacks will work. They also might be excessive: I think an SRV descriptor is something like 32 bytes of space in the resource descriptor heap. If you can eventually need thousands, or if the number is not bounded a-priori (for example, the UI allows the app to insert new generated textures into a list), then you want something very dynamic like the scheme you've added. In this case Vulkan scheme might need to also be reworked eventually, because you'd need an ability to allocate new pools as each pool can only allocate a finite number of descriptors. If you only need a few, however, then the alloc/free callbacks will work but it feels like you could also change the interface in a simpler, more compatible, way by asking the user to provide N descriptors (and the number N which needs to be within some bounds) instead of alloc/free callbacks. But your scheme will work as well of course, so that's up to you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants