KhronosGroup / Vulkan-Samples

One stop solution for all Vulkan samples
Apache License 2.0
4.23k stars 634 forks source link

Add Layer Settings API to framework, improve batch mode error recovery, fix macOS issues #1084

Closed SRSaunders closed 1 month ago

SRSaunders commented 2 months ago

Description

This PR provides solutions to #1080, #1081, #1082, and #1083.

  1. Fixes defects in CMakeLists.txt find dxc logic. Supports system-level Vulkan SDK installations that include dxc.
  2. Adds layer settings capability to framework with new add_layer_setting(VkLayerSettingEXT layerSetting), and get_layer_settings() API calls in vulkan_sample.h. Modified Instance() and HPPInstance() parameter lists to include required_layer_settings so they can be activated during instance creation. Protected all changes with #if defined(VK_EXT_layer_settings) for backwards compatibility with Vulkan SDKs earlier than 1.3.272 where the extension may not be defined.
  3. Modified shader_debugprintf sample to override get_validation_layers() and use layer settings to activate the Validation Layer's VK_VALIDATION_FEATURE_ENABLE_DEBUG_PRINTF_EXT feature using the standard framework. The sample's custom create_instance() is now only used if VK_EXT_layer_settings is not available at runtime.
  4. Modified descriptor_indexing sample to use the layer settings API on macOS to activate MoltenVK's MVK_CONFIG_USE_METAL_ARGUMENT_BUFFERS feature which is necessary to increase the available descriptors for the sample. Also worked around a macOS/MoltenVK limitation in the variable descriptor count feature. This may be fixed in future versions of MoltenVK, but the workaround will function for past, current and future MoltenVK versions.
  5. Enabled VK_ENABLE_BETA_EXTENSIONS on Apple platforms for access to the VK_KHR_portability_subset extension and features. Activating VK_KHR_portability_subset is required on all portability implementations where the extension is defined and available. Also activated the mutableComparisonSamplers portability feature for the async_compute and multithreading_render_pass samples.
  6. Improved batch mode error recovery by: a) using throw std::runtime_error() vs. ExitCode::FatalError in main_loop() and main_loop_frame(), b) modifying VK_CHECK to use throw std::runtime_error() to gracefully recover from more subtle issues such as pipeline creation problems (e.g. SPIR-V to Metal cross-compilation errors due to feature limitations) as well as queue submission errors, c) resetting the shader compiler's target environment settings prior to each sample to avoid stickiness from earlier samples in batch mode, d) allowing VK_SUBOPTIMAL_KHR which avoids early termination of some samples when exiting while in batch mode, and e) fixing specific issues in async_compute, texture_mipmap_generation, and fragment_shader_barycentric samples to permit graceful failure and continuation while in batch mode.
  7. Changed packaging to build a command line macOS executable (not an app bundle) to match the existing online build/run instructions on the web site (i.e. use ./build/mac/app/bin/Release/<x86_64|arm64>/vulkan_samples vs. ./build/mac/app/bin/Release/<x86_64|arm64>/vulkan_samples.app/Contents/MacOS/vulkan_samples). App bundles are needed for iOS, but get in the way on macOS for command line applications like vulkan_samples.
  8. Added .DS_Store to the .gitignore file for git cleanliness on Apple.

I have other improvements pending for Xcode project generation for macOS/iOS, running on iOS and the iOS simulator (x86_64 and arm64 hosts), and standalone MoltenVK support (i.e. without the vulkan loader, useful when working on the MoltenVK dev stream), but will submit those in a separate PR.

General Checklist:

Please ensure the following points are checked:

I have tested using batch runs on Windows and macOS. Depending on CI for now on linux - will test later Batch mode testing on Manjaro now complete. Cannot test on Android - don't have the environment. I have also tested on iOS but have additional changes to add in a separate PR.

If this PR contains framework changes:

CLAassistant commented 2 months ago

CLA assistant check
All committers have signed the CLA.

SRSaunders commented 2 months ago

I added commit https://github.com/KhronosGroup/Vulkan-Samples/pull/1084/commits/7c0195acae6536b1f6ac63f8922cb5c7a818d8f6 to fix the Doxygen and Copyright date issues found by CI.

I am not sure what to do with clang-issues.diff as it does not appear to be properly formatted git patch file.

I have also added commit https://github.com/KhronosGroup/Vulkan-Samples/pull/1084/commits/8caca90df210eb9c1c8c77c89e7141c2b54b8c92 to fix clang-format issues found by CI.

SRSaunders commented 2 months ago

The remaing CI issues (Doxygen, clang-format) appear to be upstream issues, not related to this PR.

SRSaunders commented 2 months ago

I don't plan to make any more changes here unless there are additional review comments or issues found.

I will submit changes in a separate PR to remove macOS workarounds within the _descriptorindexing sample once improvements are made to MoltenVK in this area (currently in development).

SRSaunders commented 2 months ago

Hopefully all issues raised by @asuessenbach and @SaschaWillems are now resolved. Again, I don't plan any more updates and re-review of recent changes / resolutions can now proceed.

SRSaunders commented 1 month ago

@SaschaWillems are there any further changes needed to proceed with this PR? I have tried to respond to your review comments with resolutions but have not heard back. If there is anything else please let me know. I would like to submit some iOS fixes and Xcode improvements (in a new PR) that are dependent on this one. Thanks.

SaschaWillems commented 1 month ago

Two things: I'm currently lacking time for PRs and I don't own any Apple device so I can't actually test if this works. We discussed this on our recent call (how we'll review Apple related PRs in the future), so hopefully someone else can green light this.

SRSaunders commented 1 month ago

We discussed this on our recent call (how we'll review Apple related PRs in the future), so hopefully someone else can green light this.

Given your constraints, if you could nominate or assign an Apple-knowledgeable reviewer that would be great. Thanks.

SaschaWillems commented 1 month ago

We are already working on that and hopefully have another official Mac reviewer soon ;)

SRSaunders commented 1 month ago

Thanks @SaschaWillems for approving.

After this is merged, I may update the logic for picking the API version for _shaderdebugprintf. Given that the VVL is now fixed and will be picked up in the next SDK, the API version should be set to 1.1 starting with the next SDK release. I can add a bit of logic to detect this at runtime and dynamically set the correct API version based on the running SDK instance. I would put this change in a supplementary PR. What do you think?

gpx1000 commented 1 month ago

@marty-johnson59 ready to merge here.

@SRSaunders we typically try to be careful about updating the API so older versions which might still be out there in the wild continue to work. However, I certainly am on board with considering an update. Ideally we try to keep it to just the iPhone or Mac platforms. Lemme know if you need any help with it. I do Mac development and can be reliable for any help you might need.

SRSaunders commented 1 month ago

@gpx1000 thanks for approving the merge.

Regarding the _shaderdebugprintf sample, there has been a platform-independent bug in the Vulkan Validation Layers that manifests itself as an-almost unusable performance issue when using the debugPrintfEXT feature with the correct Vulkan API 1.1 setting. This happens on all platforms, not just macOS. See defect https://github.com/KhronosGroup/Vulkan-ValidationLayers/issues/7562. This PR changes the sample to Vulkan API 1.0 as a temporary workaround, but it should be moved back to Vulkan 1.1 when the fixed VVL ships, which should be in the next SDK version. I was just offering to make a follow-on change to this PR that recognizes this and allows the _shaderdebugprintf sample code to adapt to the SDK instance it is running. Note the VVL fix came in after this PR was frozen for review so I did not change it here.

Your offer to assist with macOS issues is very welcome - thank you! In fact, I do have a pending fix that I would like to get feedback on - relating to swapchain recreation with MoltenVK. I will open a separate issue and ask for your feedback there. Thanks again.