-
Notifications
You must be signed in to change notification settings - Fork 618
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
Refactor getting/adding extension features to create a VkDevice with. #1013
base: main
Are you sure you want to change the base?
Conversation
37787b5
to
a6ce335
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we can just assert in all these cases can we?
It seems to prevent batch mode from running as soon as it finds an unsupported case.
You're kind of right. As I stated in the Description above, that should be replaced by some meaningful error or fall-back handling. |
Well throwing a I'm also not really sure what was wrong with just having a single call to request_extension_features? I can't see why unrolling that code everywhere is better. Maybe I'm just missing the issue that's being solved here. |
The problem, as described in #938, was, that currently you get the supported extension features bits, possibly set some of them to true, no matter if they are supported at all, and store that structure in the structure chain for device creation.
With this PR, you just set the flags you're interested in and when they're supported at all. |
Sounds good. I can change that accordingly. |
309c259
to
5ed9f04
Compare
I think you'll need to rebase this over #1031. A lot of the samples don't run without that, so testing this isn't really possible without it. |
5ed9f04
to
ff84e5a
Compare
Tbh. I'm kinda torn over the syntax that now needs to be used. Will that actually work with e.g. code completion/intellisense? Or will it fail at compile time when I e.g. request a member of a feature that isn't present or that I did misspell? I'm referring to this:
The old syntax has you access that member (graphicsPipelineLibary) directly, which works fine with intellisense/code compleation and is easier to understand/follow:
(though I don't really like that syntax either) I fear that this makes working on samples harder, hurts readability and foremost makes it harder to follow our samples. Is there a different way to handle this that looks more similar to the old syntax? |
As it is with all macros, you probably don't have any support by code completion or intellisense when you use
But as described in #938 it's just wrong!
I don't know of any. Actually, I provided three different levels of complexity to handle that correctly. If you have an idea for an additional approach, I'd be happy to add that as well. |
As discussed on the call, here is the way I do it in my samples:
An example: class VulkanExample : public VulkanExampleBase
{
VkPhysicalDeviceTimelineSemaphoreFeaturesKHR enabledTimelineSemaphoreFeaturesKHR{};
VulkanExample() : VulkanExampleBase()
{
...
enabledTimelineSemaphoreFeaturesKHR.sType = VK_STRUCTURE_TYPE_PHYSICAL_DEVICE_TIMELINE_SEMAPHORE_FEATURES_KHR;
enabledTimelineSemaphoreFeaturesKHR.timelineSemaphore = VK_TRUE;
pNextDevice = &enabledTimelineSemaphoreFeaturesKHR;
}
} It might not be the best approach from a C++ language point of view (esp. how pNextDevice is implemented), but it makes it very easy for people to follow my samples and you can copy that 1:1 into your own application without having to follow any templates or macros. It would probably be a more substantial change, so before we take this further we should agree on what way we want to follow. |
@SaschaWillems I assume, that From a sample point of view, I think, it's important that the reader of the sample can easily see what features are requested. But I would not consider the details on how that is handled to be important for a sample. Besides for a sample that actually demonstrates how feature requesting is done, of course. |
I actually don't do that. If one requests a feature that isn't supported, instance or device creation will fail with an error code hinting at a feature not supported. I want to deliberately keep samples as simple as possible. |
Interesting approach! That is, you would get some |
Description
As described in #938, requesting and enabling extension features is currently incorrect. With this PR, this is done similar to how it's done for the simple PhysicalDeviceFeatures: one function to get the supported bits (
get_extension_features
) and one to actually set them into the structure chain (add_extension_features
).The perfect handling would look like this:
To ease usage,
[HPP]PhysicalDevice
got two new template functions:request_optional_feature
andrequest_required_feature
, which log a message or throw an exception, respectively, if the requested feature is not supported.Requesting a required feature would now look like
Where the last two arguments to that functions are just for error reporting and maybe could omitted.
To ease usage even more, some macros have been added, so that requesting a required feature would now look like
REQUEST_REQUIRED_FEATURE(gpu, VkExtensionFeaturesStruct, VK_STRUCTURE_TYPE_EXTENSION_FEATURES_STRUCT, featureBit);
Build tested on Win10 with VS2022. Run tested on Win10 with NVidia GPU.
Fixes #938
General Checklist:
Please ensure the following points are checked:
Note: The Samples CI runs a number of checks including:
Sample Checklist
If your PR contains a new or modified sample, these further checks must be carried out in addition to the General Checklist: