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

FI: Vulkan Instance #466

Open
TomAtkinsonArm opened this issue May 31, 2022 · 5 comments
Open

FI: Vulkan Instance #466

TomAtkinsonArm opened this issue May 31, 2022 · 5 comments
Labels
framework This is relevant to the framework

Comments

@TomAtkinsonArm
Copy link
Contributor

What is the Problem?

Samples will need to create a Vulkan Instance. The usual route of passing vectors of layers and extensions to toggle is not very dynamic and tends to lead to bad code in the future. It would be great to have some form of InstanceBuilder to simplify this process.

What is the proposed Solution?

Add an instance builder class which has some helpful API patterns to simplify the process of enabling and disabling layers and extensions.

Examples

Example API usage. Allows a sample to have full control over instance creation with the least amount of code.

InstanceBuilder builder;

builder
    .target_vulkan_version(1, 2, 0);

builder
    .enable_layer_if_exists("VK_LAYER_NAME")
    .enable_layer_if_exists("VK_LAYER_NAME_1")
    .enable_layer_if_exists("VK_LAYER_NAME_2")
    .enable_layer_or_error("VK_LAYER_NAME_3");

builder
   // maintain a library of common features that samples need to enable - default_sample_features_func
   .apply(some_builder_mutation_func_declared_somewhere_else)
   // some custom mutation function
   .apply([](InstanceBuilder& builder) {
      builder.enable_extension<SomeExtension>([](const VkExtensionFeatures& supported_features, VkExtensionFeatures* requested_features){
          // check for device support and enable features
         // we need some useful error handling here StackError?

      });
   });

Instance instance = builder.build();
@TomAtkinsonArm TomAtkinsonArm added the framework This is relevant to the framework label May 31, 2022
@TomAtkinsonArm
Copy link
Contributor Author

@gpx1000 what do you think of this idea to replace instance creation? Something similar for device creation would also help simplify things

@TomAtkinsonArm
Copy link
Contributor Author

We can handle default validation layers and debug utils

builder
   .apply(enable_validation_layers)
   .apply(enable_debug_utils);

Or these can be inside a default_sample_config function

@gpx1000
Copy link
Collaborator

gpx1000 commented May 31, 2022

I definitely like it. Abstraction of complexity is ALWAYS a good thing for samples. I do however, want to ensure we maintain a sample that explicitly shows how to create an instance.

As an odd tangent to this concern is that the OpenXR runtime will be responsible for instance creation wholly on it's own. Thus, doing this might make creating an OpenXR sample easier to essentially "look" more like the other samples as this is also abstracted away.

@TomAtkinsonArm
Copy link
Contributor Author

@gpx1000 see #458.

A sample will have full control over everything. No more platform, no VulkanSample or ApiSample wrappers.

So if a sample wants to not use these helpers then they have the options to use pure vulkan.

ApiSample and VulkanSample will be replaced by default_performance_sample_fn and default_api_sample_fn in this case.

@TomAtkinsonArm TomAtkinsonArm linked a pull request Jun 1, 2022 that will close this issue
@tomadamatkinson tomadamatkinson added this to the Framework v2.0 milestone May 4, 2023
@tomadamatkinson tomadamatkinson removed this from the Framework v2.0 milestone May 11, 2023
@SaschaWillems
Copy link
Collaborator

How about we close this? Samples can (and some do) override the create_instance function (at least for api samples) and do their own instance setup. Not sure if we really not more or something different.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
framework This is relevant to the framework
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants