Unify class VulkanSample and HPPVulkanSample. #910

asuessenbach · 2024-02-13T16:11:03Z

Description

First step to unifying C- and C++-bindings of vulkan in the framework: Unify the central classes VulkanSample and HPPVulkanSample.
Conceptually does the following:

replaces VulkanSample and HPPVulkanSample by a templated class template <bool CppBindings> VulkanSample.
internally, that new class uses the C++-bindings.
controlled by the bool template parameter CppBindings, it provides some interface functions using the C-bindings data types or the C++-bindings data types.
for now, all the other classes with two flavors (Device and HPPDevice, ...) still exist and their usage in this class' interface are also controlled by the bool template parameter CppBindings.
to gracefully handle that dichotomy, all members of that class are private now, and some accessor functions are provided.
especially due to that latest bullet point, literally all samples needed to be touched to use those accessor functions!

Build tested on Win10 with VS2022. Run tested on Win10 with NVidia GPU.

General Checklist:

Please ensure the following points are checked:

Note: The Samples CI runs a number of checks including:

I have updated the header Copyright to reflect the current year (CI build will fail if Copyright is out of date)
My changes build on Windows, Linux, macOS and Android. Otherwise I have documented any exceptions

Sample Checklist

If your PR contains a new or modified sample, these further checks must be carried out in addition to the General Checklist:

I have tested the sample on at least one compliant Vulkan implementation
If the sample is vendor-specific, I have tagged it appropriately
I have stated on what implementation the sample has been tested so that others can test on different implementations and platforms
Any dependent assets have been merged and published in downstream modules
For new samples, I have added a paragraph with a summary to the appropriate chapter in the samples readme
For new samples, I have added a tutorial README.md file to guide users through what they need to know to implement code using this feature. For example, see conditional_rendering
For new samples, I have added a link to the Antora navigation so that the sample will be listed at the Vulkan documentation site

framework/vulkan_sample.h

samples/tooling/profiles/profiles.cpp

framework/api_vulkan_sample.h

framework/CMakeLists.txt

framework/hpp_api_vulkan_sample.h

framework/hpp_vulkan_sample.h

samples/api/hpp_compute_nbody/hpp_compute_nbody.cpp

framework/vulkan_sample.h

asuessenbach · 2024-02-27T13:40:50Z

Somehow other peoples changes has sneaked into this one. Maybe due to some rebasing?
And I don't know, how to get them out of the PR again. Can anybody help me with that?

tomadamatkinson · 2024-03-01T09:17:00Z

Somehow other peoples changes has sneaked into this one. Maybe due to some rebasing?
And I don't know, how to get them out of the PR again. Can anybody help me with that?

@asuessenbach as it has been a few days it likely wont be achievable with git reflog. Something you might try is

Checkout your branch
Checkout a new branch (temp to test the changes without editing your current work)
Squash your commits
Rebase again on main - override any changes present in your branch with those in main

I tend to be able to fix these changes locally if it happens to me but its quite hard to recall the precise steps i normally take. The steps above may not work for you but its something to try at least

tomadamatkinson · 2024-03-01T09:42:29Z

I will take a more thorough review once the rebase issues are sorted. I reviewed vulkan_sample.h and the sample changes. Unsure which changes have leaked in from other commits.

Im happy with the overall interface. I would have suggested something like get_device and get_c_device to keep things ultra simple, but I didn't realise just how large the VulkanSample interface had grown. Is the idea to slowly phase out the C interfaces using this same technique?

asuessenbach · 2024-03-04T13:30:54Z

I will take a more thorough review once the rebase issues are sorted. I reviewed vulkan_sample.h and the sample changes. Unsure which changes have leaked in from other commits.

It seems, all the alien changes have gone with my latest force-push. So feel free to review in more depth ;)

tomadamatkinson

In general very happy with the change. Following this pattern in the future should let us condense a lot of the duplicate interfaces. In time this should help simplify things again but also mean that we transition to the C++ api seamlessly.

Will approve when ive tested locally. Hopefully tomorrow! If i don't end up reviewing soon please @ me

tests/system_test/test_framework/vulkan_test.h

SaschaWillems

Tested this on Windows 11 with an RTX 4070 and on Android 13 with a Google Pixel 4a (build from Windows 11). Compiles and runs fine on both.

I'm not much of a C+++ expert, but this looks like a nice solution and a step in the right direction and gets a thumbs up from me :)

Thank you very much for this PR!

JoseEmilio-ARM · 2024-03-06T20:45:52Z

third_party/vulkan

was this updated intentionally?

Yes, that was intentional.
For some unknown reason, one of the compiler in the CI complained about some template magic in Vulkan-Hpp. Updating that to a newer version resolved that.

tomadamatkinson

LGTM! Thanks for all the work on this

Introduce type aliases on conditional types; refactor dual interface functions to single ones using those conditional types. Plus slightly adjust a few functions in VulkanSample for consistency. Fix indentation in CMakeLists.txt; simplify body of VulkanSample::create_instance; fix device usage in profiles sample Introduce Parent for class VulkanSample. Unify class VulkanSample and HPPVulkanSample. Introduce type aliases on conditional types; refactor dual interface functions to single ones using those conditional types. Plus slightly adjust a few functions in VulkanSample for consistency. Fix indentation in CMakeLists.txt; simplify body of VulkanSample::create_instance; fix device usage in profiles sample Introduce Parent for class VulkanSample. Fix typo. Adjust hpp_timestamp_queries and oit_depth_peeling. Fix color_write_enable. Fix validation errors in timeline_semaphores sample (KhronosGroup#927) * Fix validation errors in timeline_semaphores sample The command buffer was being implicitly reset, but wasn't resettable. * Review feedback addressed Used a much simpler approach to resolve the error Allow explicit skips in batch mode (KhronosGroup#914) * Allow explicit skips in batch mode * Fix bug in CLI11 wrapper that inhibits multi-value flag parsing Unify class VulkanSample and HPPVulkanSample. Unify class VulkanSample and HPPVulkanSample. Introduce type aliases on conditional types; refactor dual interface functions to single ones using those conditional types. Plus slightly adjust a few functions in VulkanSample for consistency. Fix indentation in CMakeLists.txt; simplify body of VulkanSample::create_instance; fix device usage in profiles sample Introduce Parent for class VulkanSample. Introduce enum class vkb::BindingType to select between C- and C++-bindings of vulkan; replaces the boolean template parameter of VulkanSample. Adjustments after rebasing. Update vulkan Adjustments due to rebasing. Unify class VulkanSample and HPPVulkanSample. Introduce type aliases on conditional types; refactor dual interface functions to single ones using those conditional types. Plus slightly adjust a few functions in VulkanSample for consistency. Fix indentation in CMakeLists.txt; simplify body of VulkanSample::create_instance; fix device usage in profiles sample Introduce Parent for class VulkanSample. Unify class VulkanSample and HPPVulkanSample. Introduce type aliases on conditional types; refactor dual interface functions to single ones using those conditional types. Plus slightly adjust a few functions in VulkanSample for consistency. Fix indentation in CMakeLists.txt; simplify body of VulkanSample::create_instance; fix device usage in profiles sample Introduce Parent for class VulkanSample. Fix typo. Adjust hpp_timestamp_queries and oit_depth_peeling. Fix color_write_enable. Fix validation errors in timeline_semaphores sample (KhronosGroup#927) * Fix validation errors in timeline_semaphores sample The command buffer was being implicitly reset, but wasn't resettable. * Review feedback addressed Used a much simpler approach to resolve the error Allow explicit skips in batch mode (KhronosGroup#914) * Allow explicit skips in batch mode * Fix bug in CLI11 wrapper that inhibits multi-value flag parsing Unify class VulkanSample and HPPVulkanSample. Unify class VulkanSample and HPPVulkanSample. Introduce type aliases on conditional types; refactor dual interface functions to single ones using those conditional types. Plus slightly adjust a few functions in VulkanSample for consistency. Fix indentation in CMakeLists.txt; simplify body of VulkanSample::create_instance; fix device usage in profiles sample Introduce Parent for class VulkanSample. Introduce enum class vkb::BindingType to select between C- and C++-bindings of vulkan; replaces the boolean template parameter of VulkanSample. Adjustments after rebasing. Update vulkan

marty-johnson59 · 2024-03-12T17:19:47Z

Merging per agreement on the call - now that conflicts are resolved..

asuessenbach force-pushed the unify branch 7 times, most recently from 2f347ab to 051056e Compare February 13, 2024 22:33

jherico reviewed Feb 13, 2024

View reviewed changes

framework/vulkan_sample.h Show resolved Hide resolved

jherico reviewed Feb 13, 2024

View reviewed changes

framework/vulkan_sample.h Outdated Show resolved Hide resolved

jherico reviewed Feb 13, 2024

View reviewed changes

framework/vulkan_sample.h Outdated Show resolved Hide resolved

asuessenbach force-pushed the unify branch 6 times, most recently from e491abd to caa7e1f Compare February 14, 2024 15:41

SaschaWillems reviewed Feb 15, 2024

View reviewed changes

samples/tooling/profiles/profiles.cpp Outdated Show resolved Hide resolved

SaschaWillems reviewed Feb 15, 2024

View reviewed changes

framework/api_vulkan_sample.h Outdated Show resolved Hide resolved

jherico reviewed Feb 15, 2024

View reviewed changes

asuessenbach marked this pull request as ready for review February 19, 2024 14:25

asuessenbach force-pushed the unify branch 7 times, most recently from 5049451 to 265a888 Compare February 27, 2024 13:08

asuessenbach force-pushed the unify branch 2 times, most recently from e6bc330 to 5c17bb5 Compare February 29, 2024 14:01

tomadamatkinson reviewed Mar 4, 2024

View reviewed changes

tests/system_test/test_framework/vulkan_test.h Show resolved Hide resolved

SaschaWillems previously approved these changes Mar 5, 2024

View reviewed changes

JoseEmilio-ARM reviewed Mar 6, 2024

View reviewed changes

JoseEmilio-ARM previously approved these changes Mar 6, 2024

View reviewed changes

tomadamatkinson previously approved these changes Mar 10, 2024

View reviewed changes

asuessenbach dismissed stale reviews from tomadamatkinson, JoseEmilio-ARM, and SaschaWillems via 2805da5 March 12, 2024 09:45

asuessenbach force-pushed the unify branch from 2805da5 to bdb3ccb Compare March 12, 2024 11:19

SaschaWillems approved these changes Mar 12, 2024

View reviewed changes

marty-johnson59 merged commit 6fb2196 into KhronosGroup:main Mar 12, 2024
25 checks passed

This was referenced Mar 12, 2024

Add Image Compression Control Sample (no-op merge, please ignore) #963

Merged

Access violation when recreate swapchain #981

Closed

jherico mentioned this pull request Mar 13, 2024

Fix for crash on swapchain recreation #982

Merged

12 tasks

SaschaWillems mentioned this pull request Mar 15, 2024

No way to access instance from samples anymore #985

Closed

gary-sweet mentioned this pull request Apr 18, 2024

wait_idle is failing at startup with ERROR_NATIVE_WINDOW_IN_USE_KHR #1033

Closed

asuessenbach mentioned this pull request Apr 20, 2024

Fix wrong swapchain creation in performance sample wait_idle #1034

Merged

19 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unify class VulkanSample and HPPVulkanSample. #910

Unify class VulkanSample and HPPVulkanSample. #910

asuessenbach commented Feb 13, 2024 •

edited

Loading

asuessenbach commented Feb 27, 2024

tomadamatkinson commented Mar 1, 2024

tomadamatkinson commented Mar 1, 2024

asuessenbach commented Mar 4, 2024 •

edited

Loading

tomadamatkinson left a comment

SaschaWillems left a comment

JoseEmilio-ARM Mar 6, 2024

asuessenbach Mar 7, 2024

tomadamatkinson left a comment

marty-johnson59 commented Mar 12, 2024

Unify class VulkanSample and HPPVulkanSample. #910

Unify class VulkanSample and HPPVulkanSample. #910

Conversation

asuessenbach commented Feb 13, 2024 • edited Loading

Description

General Checklist:

Sample Checklist

asuessenbach commented Feb 27, 2024

tomadamatkinson commented Mar 1, 2024

tomadamatkinson commented Mar 1, 2024

asuessenbach commented Mar 4, 2024 • edited Loading

tomadamatkinson left a comment

Choose a reason for hiding this comment

SaschaWillems left a comment

Choose a reason for hiding this comment

JoseEmilio-ARM Mar 6, 2024

Choose a reason for hiding this comment

asuessenbach Mar 7, 2024

Choose a reason for hiding this comment

tomadamatkinson left a comment

Choose a reason for hiding this comment

marty-johnson59 commented Mar 12, 2024

asuessenbach commented Feb 13, 2024 •

edited

Loading

asuessenbach commented Mar 4, 2024 •

edited

Loading