Fix vsg::DeviceFeatures.cpp to return a VkPhysicalDeviceFeatures2 to pass into VkDeviceCreateInfo.pNext.

[St0fF-NPL-ToM]

Pull Request Template

Description

Problem: Map-internal comparison does not assure VkPhysicalDeviceFeatures2 being "first", as e.g. VkPhysicalDeviceMultiviewFeatures gets sorted up front ( 1000059000 vs. 1000053001 → see vulkan_core.h)

But: specs state "For all features, including the Core 1.0 Features, use VkPhysicalDeviceFeatures2 to pass into VkDeviceCreateInfo.pNext"

Type of change

• Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

• manual overriding:
  • create ref_ptrvsg::WindowTraits tr, e.g. from command line parameters,
  • tr->deviceExtensionNames.push_back( VK_KHR_MULTIVIEW_EXTENSION_NAME );
  • use tr->validate() so deviceFeatures are initialized, create vsg::Instance and vsg::PhysicalDevice from those traits and a surface
  • add the feature

	tr->deviceFeatures->get< VkPhysicalDeviceMultiviewFeatures,
				 VK_STRUCTURE_TYPE_PHYSICAL_DEVICE_MULTIVIEW_FEATURES >()
					.multiview = VK_TRUE;

• create vsg::Device
• check using debugger:
  • without fix multiview feature will be returned inside device creation
  • with fix VkPhysicalDeviceFeatures2* will be returned inside device creation

Checklist:

• My code follows the style guidelines of this project
• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas

Trivia

I just stumbled over this. Didn't actually need Multiview, but enabled it and ASAP NSight went into Validation Violation mode ;)

[robertosfield]

I have done a first pass review and it looks a bit convoluted. Engineers coming after us and perhaps ourselves in a few months will look at the code and be perplexed why it's doing what it's doing.

Could you provide a code snippet that illustrates the problem usage that you came across, I can't have a think about this and the public API and implementation of vsg::DeviceFeatures to avoid issues.

[St0fF-NPL-ToM]

The problem itself: original assumption

VK_STRUCTURE_TYPE_PHYSICAL_DEVICE_FEATURES_2 < ANY_OTHER_FEATURE_STRUCT_TYPE
1000059000                                   > 1000053001
                                               VK_STRUCTURE_TYPE_PHYSICAL_DEVICE_MULTIVIEW_FEATURES

simply does not hold anymore due to Vulkan additions.

Which means: the original intention behind using a map for AUTOMATIC ORDERING BY VK_STRUCTURE_TYPE doesn't work anymore.

My intent - no change upon the public API - lead to fixing the output function and adding a safety-net (that "get();" call in the CTor).

I strongly agree this is far from perfect and it should be at least "outstandingly documented in code" - or completely rewritten. This only fixes the symptoms …

… just had a quick review myself and found a bug:

• the comment inside data() "returns nullptr if nothing was setup" doesn't even hold.
  (Always returns at least the default struct with an empty chain …)

Question

What would you choose, now that map is not the right container type?
Simple vector of pointers, using an emplace-iterator-hint depending on type?

[St0fF-NPL-ToM]

About "problem usage" - i guess you mean a real world use case - that is why I do not understand the question. The whole interface is made for customization of yet unknown upcoming vulkan features.

You did not need a use case for creating it in the first place. Now a central assumption was flipped by such "an upcoming update". Why do you need a motivation, now?

[robertosfield]

I need bit of example code that illustrates the problem you are seeing.

I could guess at what is causing the problem on your system and then see if the suggested changes, or my own changes fixes this, but I may well have not recreated the problem you are seeing, so my testing might well be completely off.

So, it's best to have a concrete bit of code that fails on your system, that I can test with myself and use for investigating the issue and then confirming that the issue is resolved by what ever changes are finally checked in.

[St0fF-NPL-ToM]

Please understand the problem at hand instead.

Something happened (even some time ago) that you:

• neither did expect,
• nor realize it actually happened:

→ Vulkan/Khronos added VK_STRUCTURE_TYPE_PHYSICAL_DEVICE_MULTIVIEW_FEATURES with a smaller value

• its numerical ID is LESS than the numerical ID of VK_STRUCTURE_TYPE_PHYSICAL_DEVICE_FEATURES_2
  Which was not to be expected! (I'd also have expected only larger IDs to be newly added to Vulkan)

Therefore using a map to assume the first entry being the VkPhysicalDeviceFeatures2 is not possible anymore, because

VK_STRUCTURE_TYPE_PHYSICAL_DEVICE_MULTIVIEW_FEATURES < VK_STRUCTURE_TYPE_PHYSICAL_DEVICE_FEATURES_2

I'm sorry if I am not able to translate something very clearly logically that is said in 1.5 sentences in German into understandable English.

[St0fF-NPL-ToM]

And for testing code:

• take the first example that requires some extra feature enabled (like the raytracing you're working on)
• append a multiview-enabler to the deviceFeatures prior to vsg::Device-creation:

   windowTraits->deviceFeatures->get<VkPhysicalDeviceMultiviewFeatures,
                                     VK_STRUCTURE_TYPE_PHYSICAL_DEVICE_MULTIVIEW_FEATURES>()
                                    .multiview = VK_TRUE;

• enable validation layers

expected outcome (my results within NSight on Windows): validation layers will report violations upon using the extra features: feature is not enabled.

At first, I did not understand the meaning of the specs. It seems the VkPhysicalDeviceFeatures2 structure inside the pNext chain starts a feature-enabling mechanism during device creation. Any other Feature-Structs seem to get ignored. Furthermore, they seem to get handled as if "nullptr", ignoring VkPhysicalDeviceFeatures2 further down the chain.

So my final conclusion on how to read the specs:

• it is required, if any features need to be enabled, to supply VkPhysicalDeviceFeatures2 within the pNext chain.
• of ALL feature-enable-structs within one pNext-chain, VkPhysicalDeviceFeatures2 must be the first, directly followed by any further feature-enable-structs. No interleaving with non-feature-enable-structs is allowed AT ALL.
• FURTHERMORE:
  • if a feature can be enabled via VkPhysicalDeviceVulkan1XFeatures, and such a Feature_X_struct is present, that specific feature's enabling-structure MUST NOT BE USED! (validation layer violation with nsight)
  • but I actually believe this must be within the user application's responsibility

Which - at the end - leaves only "VkPhysicalDeviceFeatures2 needs to exist at the top of the chain".