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

Fix for shared handles for pool deleter #1925

Merged
merged 3 commits into from
Aug 5, 2024
Merged

Conversation

Agrael1
Copy link
Contributor

@Agrael1 Agrael1 commented Jul 22, 2024

No description provided.

@Agrael1
Copy link
Contributor Author

Agrael1 commented Jul 22, 2024

@asuessenbach Is it ok to leave it at that?
I could do a mandatory pool argument, but it will require another bunch of template metaprogramming,
Also I'm not able to test that, since generator breaks with
caught exception: VulkanHppGenerator: Spec error on line 2: missing required element <commands>

@asuessenbach
Copy link
Contributor

asuessenbach commented Jul 22, 2024

I think, just using that check could result in a resource leak, as allocated command buffers would not be freed.
That is, I would prefer to have some template magic to resolve that

Besides that: You might have modified your vk.xml, somehow. The <commands> section is required and definitely part of every vk.xml. In the current version (1.3.291), it's on line 11321.

And a side question: is it somehow assured or verified, that the CommandPool passed into the SharedCommandBuffer constructor is the very same that is used to actually allocate the CommandBuffer?

@Agrael1
Copy link
Contributor Author

Agrael1 commented Jul 22, 2024

I think, just using that check could result in a resource leak, as allocated command buffers would not be freed. That is, I would prefer to have some template magic to resolve that

Template magic it is then.

And a side question: is it somehow assured or verified, that the CommandPool passed into the SharedCommandBuffer constructor is the very same that is used to actually allocate the CommandBuffer?

No way unfortunately, only if we had functions to make shared handles. In such case there would need to be all of the functions for shared handles. I dropped that idea, because both approaches looked inconvenient and ugly (operator-> and raw functions)

added special constructor for pool types
@Agrael1
Copy link
Contributor Author

Agrael1 commented Jul 23, 2024

Ok, so I added the magic, should not compile without the explicit pool argument now

Copy link
Contributor

@asuessenbach asuessenbach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, so far.
Just two questions.

vulkan/vulkan_shared.hpp Outdated Show resolved Hide resolved
@@ -346,8 +346,8 @@ namespace VULKAN_HPP_NAMESPACE
public:
void destroy( DestructorType parent, HandleType handle ) const VULKAN_HPP_NOEXCEPT
{
VULKAN_HPP_ASSERT( m_destroy && m_dispatch );
( parent.*m_destroy )( handle, m_allocationCallbacks, *m_dispatch );
if ( m_destroy && m_dispatch )
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you mind to run the generation for vulkansc as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

About that, what do you think about making github action for generating the code on PR merge to master?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I saw, there is one already :D

@Agrael1
Copy link
Contributor Author

Agrael1 commented Aug 1, 2024

@asuessenbach So, what do you think, can it be merged?

@asuessenbach
Copy link
Contributor

Looks good now!
Thanks a lot for your contribution.

@asuessenbach asuessenbach merged commit 0f4e59b into KhronosGroup:main Aug 5, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants