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

Split vulkan.hpp into multiple sub-headers #979

Merged
merged 11 commits into from
Jun 15, 2021

Conversation

asuessenbach
Copy link
Contributor

No description provided.

Copy link

@lunarpapillo lunarpapillo left a comment

Choose a reason for hiding this comment

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

I'm looking at how this change affects the Vulkan SDK. The changes look good to me (though I'm not savvy enough in this code to click Approve).

One unexpected change caught my eye. I wanted to bring it to your attention to ensure that it was deliberate.

@@ -3144,7 +3163,7 @@ void VulkanHppGenerator::appendHandle( std::string & str, std::pair<std::string,
assert( commandIt != m_commands.end() );
for ( auto const & parameter : commandIt->second.params )
{
if ( handleData.first != parameter.type.type ) // the commands use this handleData type !
if ( isHandleType( parameter.type.type ) && ( handleData.first != parameter.type.type ) ) // the commands use this handleData type !

Choose a reason for hiding this comment

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

There's an implicit behavior change here that isn't explained by the commit title. Was it intentional? Can you elaborate on it?

Choose a reason for hiding this comment

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

Also, completely obliquely, I noticed from this change that Vulkan-Hpp seems to generate another header file vulkan_raii.hpp that is not currently included in KhronosGroup/Vulkan-Headers and is not shipped in the Vulkan SDK. Should it be?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That code change is needed for the separation into multiple files.
Before this change, structure and handle wrapper classes were listed interleaved in vulkan.hpp. With this change, they are separated into two files, vulkan_structs.hpp and vulkan_handles.hpp.
In this part of the code, I determine which handles need to be listed first (any handle A, that is part of any command of handle B needs to be listed before handle B). But there are also non-handles (like structs) part of the commands, that should not be listed here. Therefore I check for handles here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

vulkan_raii.hpp introduces additional functionalities (a set of raii-compliant handle wrapper classes) and is "on-top-of" vulkan.hpp.
It is not (yet) part of Vulkan-Headers, but I plan to add it soon.
If it's substiantially easier to add it as well right now, I think that would be fine.

@lunarpapillo
Copy link

I'm concerned about the loss of backward compatibility; after this change, vulkan.hpp is 547K, and before the change it was 5.7M., over 10x larger. So any application that had been using vulkan.hpp that was using some element that was broken out into another header will suddenly fail to compile until modified, correct?

Is this an intentional breaking change?

If not, please consider alternatives that won't break backward compatibility, e.g. giving the remaining contents of vulkan.hpp a new name (e.g. vulkan_core.hpp) and leaving vulkan.hpp as a monolithic include file, or perhaps making vulkan.hpp a shell that just includes the other headers, or some other equivalent change that will not require users to modify their source.

@lunarpapillo
Copy link

And I note the repository includes files vulkan/vulkan.hpp and vulkan/vulkan_raii.hpp, which appear to be the results from the last header generation. Should this change also include the new result files created in the vulkan/ directory, or will that be another change?

@jeremy-lunarg
Copy link

I'm concerned about the loss of backward compatibility; after this change, vulkan.hpp is 547K, and before the change it was 5.7M., over 10x larger. So any application that had been using vulkan.hpp that was using some element that was broken out into another header will suddenly fail to compile until modified, correct?

Is this an intentional breaking change?

If not, please consider alternatives that won't break backward compatibility, e.g. giving the remaining contents of vulkan.hpp a new name (e.g. vulkan_core.hpp) and leaving vulkan.hpp as a monolithic include file, or perhaps making vulkan.hpp a shell that just includes the other headers, or some other equivalent change that will not require users to modify their source.

vulkan.hpp does include the other headers: vulkan_enums.hpp, vulkan_funcs.hpp, vulkan_structs.hpp, and vulkan_handles.hpp.

As a quick sanity check I took the changes in this PR and inserted them into vulkan-tools. vkcubepp built and ran as expected.

@lunarpapillo
Copy link

vulkan.hpp does include the other headers: vulkan_enums.hpp, vulkan_funcs.hpp, vulkan_structs.hpp, and vulkan_handles.hpp.

Verified! Thanks... I should have been more thorough in my own checking.

I'm still concerned about the unexpected behavior change, and that the generated files aren't also checked into the vulkan/ directory.

@jeremy-lunarg
Copy link

vulkan.hpp does include the other headers: vulkan_enums.hpp, vulkan_funcs.hpp, vulkan_structs.hpp, and vulkan_handles.hpp.

Verified! Thanks... I should have been more thorough in my own checking.

I'm still concerned about the unexpected behavior change, and that the generated files aren't also checked into the vulkan/ directory.

I'm confused. I do see the generated files checked into the vulkan directory.

@lunarpapillo
Copy link

I'm confused. I do see the generated files checked into the vulkan directory.

Blast it, I'm batting no better than .333... You're right, they are there, and clearly in the PR as well.

For some reason I didn't see them when I was investigating - perhaps I was on the wrong branch, or exploring some other rookie mistake.

All that's left is the behavior change.

@asuessenbach
Copy link
Contributor Author

@lunarpapillo And that behavior change is needed for the listing of structs and handles in separate files, as explained above.

@asuessenbach asuessenbach merged commit 18ec7ad into KhronosGroup:multifile Jun 15, 2021
@asuessenbach asuessenbach deleted the multifile branch June 23, 2021 15:16
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.

3 participants