KhronosGroup / Vulkan-Samples

One stop solution for all Vulkan samples
Apache License 2.0
4.34k stars 648 forks source link

Unify class VulkanSample and HPPVulkanSample. #910

Closed asuessenbach closed 8 months ago

asuessenbach commented 9 months ago

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:

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

General Checklist:

Please ensure the following points are checked:

Sample Checklist

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

asuessenbach commented 8 months ago

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 commented 8 months ago

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

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 commented 8 months ago

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 commented 8 months ago

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 ;)

marty-johnson59 commented 8 months ago

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