Conditionally enabling device features

[spnda]

Some physical device features can be useful for an application, but not necessary. For example, in my application, I'd like to enable the BC, ASTC, or the ECS texture compression features if they are available. If I would do add_required_extensions the physical device selection would fail as often not all three features are available. Therefore, I'd like to see some functionality on the physical device selector or the device builder to specify optional features or features to be enabled, if they are available.

I would propose a function on the device builder that enables a feature if it is available, using std::mem_fn. This would look something like this:

// The user, enabling a feature
deviceBuilder.enableFeatureIfPresent(std::mem_fn(&VkPhysicalDeviceFeatures::textureCompressionBC));

// Implementation, could also move the std::mem_fn in here.
template <typename Func>
bool vkb::DeviceBuilder::enableFeatureIfPresent(Func function) {
    if (function(availableFeatures)) {
        function(enabledFeatures) = VK_TRUE;
        return true;
    }
    return false;
}

@charles-lunarg Does this look like a good idea to you? I would implement a PR for this right now I have your blessing.

Also, another question, but is it intended that physical_device.features only represents the enabled features?

[charles-lunarg]

physical_device.features represents the features to enable on the device yes, so the 'available' features would need to be added to vkb::PhysicalDevice, which is kinda annoying because of all the feature structs that exist. Might make sense to query the available features on demand instead of bloating vkb::PhysicalDevice even more.

As far as implementation goes, I'm not sure a callback is necessary or even desired. In my mind I would just want to pass in the features struct with the features I optionally want set to true, and get back a bool telling me if they are enabled.

That said, being able to directly modify the user's features struct would be nice. What I worry about is making VkPhysicalDeviceFeatures2 work with the large number of optional structs there. Not impossible I'm sure, I just want the API to be as 'easy' to use as is reasonably possible.

Yes, we probably should move mem_fn inside the implementation since templates are easy to misuse.

Implementation for my 'basic bool check'

bool enable_all_features_if_available(VkPhysicalDeviceFeatures feats) {
    bool can_enable = true;
    can_enable =  feats.f1 ? can_enable && physical_device.available_feats.f1 : can_enable; 
    // Early exit if false
    if (!can_enable) return false;
    
    physical_device.feats_to_enable.f1 = physical_device.feats_to_enable.f1 && feats.f1;
    // repeat for all features
    return true;
}

I guess the issues I would like to see solved is being able to specify several optional features at the same time, and handling all of the VkPhysDevFeats2 structs.

[spnda]

physical_device.features represents the features to enable on the device yes, so the 'available' features would need to be added to vkb::PhysicalDevice, which is kinda annoying because of all the feature structs that exist. Might make sense to query the available features on demand instead of bloating vkb::PhysicalDevice even more.

In my local implementation, the availableFeatures struct gets created and queried in the constructor of DeviceBuilder. Though, perhaps, we should avoid calling physical device properties and features more than necessary, and could instead move the features and properties to the heap to avoid bloating PhysicalDevice more.

As far as implementation goes, I'm not sure a callback is necessary or even desired. In my mind I would just want to pass in the features struct with the features I optionally want set to true, and get back a bool telling me if they are enabled.

Actually, for me I'd prefer if I could specify a few features I'd like optionally and then see if they were enabled through device.physical_device.features, as that's what currently reflects the enabled features. Of course, returning a bool on the function itself is a nice addition but given that it's optional I think it'd be fine if the user only knows if it was enabled after device creation.

Implementation for my 'basic bool check'

How would this even work, when you'd need to dynamically know what f1 is. That was the whole reason I went for the std::mem_fn alternative, as it allows to dynamically access single members of the feature structs in a single function. To me it looks like your implementation would require some sort of reflection, or at least something akin to std::mem_fn.

I guess the issues I would like to see solved is being able to specify several optional features at the same time, and handling all of the VkPhysDevFeats2 structs.

As for the several optional features, using a parameter pack could work but I kinda don't like specifying them in bulk, seeing as there might be other conditions for individual features.
Yes, I have no concrete solution on how to do that tbh. The problem here is that the DeviceBuilder only has the pNext chain to work with, but we could theoretically just iterate through that and apply the same procedure on the correct struct as I originally suggested. The important thing I'd like to note here is that the current code does not change any values of any of the feature structs passed into the PhysicalDeviceSelector, so to match current behavior I'd just like to then set the newly enabled features in physical_device.features, which is I guess what most people would use to check enabled features at runtime.

[charles-lunarg]

I've gone ahead and implemented what I thought was a reasonable interface for 1.0 features as well as 1.1+/extension features. I probably should document that this wont enable the extensions along side it, cause that's a different can of worms.

[spnda]

My main issue with your proposal and the implementation found in #273 is that I cannot do this:

	VkPhysicalDeviceFeatures textureCompressionFeatures {
		.textureCompressionETC2 = VK_TRUE,
		.textureCompressionASTC_LDR = VK_TRUE,
		.textureCompressionBC = VK_TRUE,
	};
	physicalDevice.enable_features_if_present(textureCompressionFeatures);

and instead, need to do this, because the combine_features function is only called when all features are present:

	VkPhysicalDeviceFeatures textureCompressionETCFeatures {
		.textureCompressionETC2 = VK_TRUE,
	};
	physicalDevice.enable_features_if_present(textureCompressionETCFeatures);
	VkPhysicalDeviceFeatures textureCompressionASTCFeatures {
		.textureCompressionASTC_LDR = VK_TRUE,
	};
	physicalDevice.enable_features_if_present(textureCompressionASTCFeatures);
	VkPhysicalDeviceFeatures textureCompressionBCFeatures {
		.textureCompressionBC = VK_TRUE,
	};
	physicalDevice.enable_features_if_present(textureCompressionBCFeatures);

That seems overly complicated to me, quite frankly. However, it is still possible to add my proposal with std::mem_fn on top of this, so I might do that myself if you don't want that in the library itself, which would simplify this use case a lot.

Also, the current PR modifies the features2 struct, and not the features struct in PhysicalDevice. Therefore, it is currently not possible to check if a feature has been enabled besides the returned bool, which would make me store a third copy of everything currently. Perhaps that could be deduplicated anyway?

[charles-lunarg]

My main holdup with mem_fn is that it appears to only work with the 1.0 features struct, and there are way more features than just 1.0.

Yes, combine only works when “all” features are present - this allows setting groups of settings all at once and know if they all are supported. Allowing ”any” feature to be enabled would be difficult to know which features are actually supported, unless the struct was taken as non const and I assumed users will check the structs contents - which is not as good. Of course both could be supported with a function for all and another for “any”.

Also really appreciate the review and feedback, seems you are finding bugs in the implementation as well as design issues with it. Being able to enable a single feature with mem_fn is nice, I just wish it supported multiple structs gracefully (or maybe it can and i just don’t know how yet).

[charles-lunarg]

Also, the current PR modifies the features2 struct, and not the features struct in PhysicalDevice. Therefore, it is currently not possible to check if a feature has been enabled besides the returned bool, which would make me store a third copy of everything currently. Perhaps that could be deduplicated anyway?

That was definitely an oversight. No reason to have a VkPhysicalDeviceFeatures2 struct anyhow, so I'll remove it in the PR too.

[spnda]

@charles-lunarg Just tested the PR locally with the new changes, and it works great. Thank you very much.