KhronosGroup
Handling "vulkan owned" objects (DescriptorSet and CommandBuffer) in vk::raii
When using the vk::raii interface it is currently impossible to handle descriptorsSets allocated from a descriptorPool created without vk::DescriptorPoolCreateFlagBits::eFreeDescriptorSet set.
Regardless of the order these objects get destroyed it violates the vulkan specification:
-
Destroy pool first: The pool gets destroyed and destroys all its sets. The sets then get destroyed. This is invalid for two reasons: First the descriptorPool handle that gets passed to vkFreeDescriptorSets is no longer valid and second the descriptorSet already got destroyed.
-
Destroy sets first: This is invalid in itself without vk::DescriptorPoolCreateFlagBits::eFreeDescriptorSetset for the pool.
Something similar applies to commandBuffers and commandPools. While they can be destroyed manually it is also valid to just destroy the commandPool they belong to and all its commandBuffers will get destroyed automatically.
Destroying a pool object implicitly frees all objects allocated from that pool. Specifically, destroying VkCommandPool frees all VkCommandBuffer objects that were allocated from it, and destroying VkDescriptorPool frees all VkDescriptorSet objects that were allocated from it.
From here (almost at the end): https://www.khronos.org/registry/vulkan/specs/1.2-extensions/html/vkspec.html#fundamentals-objectmodel-lifetime
While just using the plain vulkan.hpp handles in this case is possible, I think it is not a good solution to the problem.
I think the easiest and cleanest solution to this would be to add non owning versions of these objects to vk::raii. Then the user can decide to use the approriate one. A disadvantage would be, that it is possible to try and use a vk::raii object which is no longer valid, which rarely if ever happens in the rest of vk::raii. However validation layers should catch this. Another solution could be to add a function that releases the underlying handle to just those two objects. This would have the advantage that the user can decide at runtime wether the vulkan object still needs to be destroyed. However I don't like this solution, because it would be less clear from an interface perspective and also much less transparent in code.
Edit: For the solution of adding special non owning versions of these objects you could also add explicit conversion constructors between the owning and non-owning version, so the user still has the freedom to choose the actually needed type at runtime.
You're right, this is the main drawback of the vk::raii-classes: you can't get rid of all DescriptorSets of a DescriptorPool with just one call (and equally for CommandBuffers and CommandPool). You have to destroy them one by one, which might have performance implications in certain circumstances. That's the reason why I hesitated so long to introduce them at all. That is, you have to set vk::DescriptorPoolCreateFlagBits::eFreeDescriptorSetset on vk::raii:DescriptorPool creation. Maybe I should extent the vk::raii::DescriptorPool constructor to enforce that flag.
Right now there is actually a neat workaround for this: If you are storing these objects in unique pointers anyway you can create a unique pointer with a custom deleter that doesn't call the destructor. I had hoped for versions of DescriptorSet and CommandBuffer that behave similar to this: Exactly the same as the normal one, except for an empty destructor.
It would be extremely nice to be able to use the vk::raii API when the lifetime of the Vulkan object isn't attached to it. I run into a similar situation with a unified Texture class is used to hold all VkImages but only a subset need to be destroyed (due to the lifetime of some being handled by the Vulkan Swapchain), constructing a Texture object from vk::raii::Image is not possible without using std::variant<vk::Image, vk::raii::Image> to store the object which requires a downcast to vk::Image for any usage which is problematic since the non-RAII API has to be dealt with rather than the RAII API, it's not that big of a deal with vk::Image in particular though.
Could something like this be an acceptable solution of the problem?
class DescriptorSet
{
public:
DescriptorSet(VkDescriptorSet descriptorSet, std::shared_ptr<const VkDescriptorPool> descriptorPool)
: m_descriptorSet{descriptorSet}
, m_descriptorPool{std::move(descriptorPool)}
{
}
~DescriptorSet()
{
if (*m_descriptorPool == VK_NULL_HANDLE) {
return; // already freed, do nothing
}
// free set
}
private:
VkDescriptorSet m_descriptorSet;
std::shared_ptr<const VkDescriptorPool> m_descriptorPool;
};
class DescriptorPool
{
public:
DescriptorPool(VkDescriptorPool descriptorPool)
: m_descriptorPool{std::make_shared<VkDescriptorPool>(descriptorPool)}
{
}
~DescriptorPool()
{
// free pool
*m_descriptorPool = VK_NULL_HANDLE;
}
DescriptorSet createDescriptorSet() const
{
VkDescriptorSet descriptorSet{};
// alloc descriptorSet
return {descriptorSet, m_descriptorPool};
}
private:
std::shared_ptr<VkDescriptorPool> m_descriptorPool;
};
I don't think the code would work as is, because in its current state the destructor of DescriptorPool will only be called once all DescriptorSets created from it are destroyed. That means the check in DescriptorSets destructor can't actually do anything. You also have to keep in mind, it is possible to create a DescriptorPool in which it is allowed to free single sets. So you need some kind of distinction between sets from pools with and without this.
@Kasys in current state calling of ~DescriptorPool not depend on destruction of DescriptorSets.
I keep that in mind. That case is out of scope. Here I show how to handle problematic case. It is trivial to modify the example to handle the case, when DescriptorPool created with flags, that allows destruction of individual DescriptorSets, by just tracking the creation flags.
If there were a manner of querying the VkDescriptorPool handle to see if it had been created with the VK_DESCRIPTOR_POOL_CREATE_FREE_DESCRIPTOR_SET_BIT bit set then it would be easy enough to add a boolean to any vk::raii::DescriptorSet allocated from it indicate ownership or not. However in my perusal of the specification there is no manner of querying the VkDescriptorPool to see how it was created. In the case of vk::raii::CommandBuffer(s) it would be easy enough to add a boolean to CommandBuffers constructor which got passed to each vk::raii::CommandBuffer container that indicated ownership or not.
Another workaround is to create the descriptor sets without the RAII capabilities (vk::raii::DescriptorSet has pretty limited capabilities anyway):
std::vector<vk::DescriptorSet> descriptorSets = (*device).allocateDescriptorSets(vk::DescriptorSetAllocateInfo(*descriptorPool, ...));
This works well when VK_DESCRIPTOR_POOL_CREATE_FREE_DESCRIPTOR_SET_BIT is not set, which is often the case. If it is set, the RAII descriptor set class can be used, and care must only be taken that the descriptor sets' destructors are called before the pool's.
As there seems to be no acceptable approach on how to handle "vulkan owned" objects here, and there's no mean to enforce correct usage, I'll close this issue for now.
With #1978, I add some assertion to the vk::raii::DescriptorPool creation function to have vk::DescriptorPoolCreateFlagBits::eFreeDescriptorSet set, but that's all that could be done here.
All the rest would be caught by the Vulkan Validation Layers.