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

macOS/iOS fixes plus other generic fixes for clang and validation warnings #1117

Merged
merged 20 commits into from
May 4, 2024

Conversation

SRSaunders
Copy link
Contributor

@SRSaunders SRSaunders commented Apr 23, 2024

This fixes a recently reported macOS command line build issue against merged PR #965. In addition this fixes some other macOS issues and adds generic fixes for clang warnings and some validation layer warnings in a few examples:

  1. Fixes macOS command line builds when using cmake's make and ninja generators (restore -ObjC++ compiler flag).
  2. Updates macOS versions of getAssetPath() and getShaderBathPath() to be tolerant of command line and Xcode builds with associated binary locations (e.g. in /bin or /bin/Debug or /bin/Release).
  3. Fixes debugUtilsMessageCallback() to be tolerant of cases when pMessageIdName is null, which can occur when validation is on and MoltenVK issues errors [mvk-error] or warnings [mvk-warn] such as using deprecated VK_USE_PLATFORM_IOS_MVK or VK_USE_PLATFORM_MACOS_MVK defines.
  4. Fixes clang type warning in getValueAsInt() and missing overrides in the variablerateshading example.
  5. Fixes validation layer warning on exit in the computeraytracing example - was missing the staging buffer destroy.
  6. Restores the spacebar text visibility toggle in the textOverlay example. This must have been lost during refactoring.
  7. Adds support for VK_USE_PLATFORM_METAL_EXT which supercedes the deprecated VK_USE_PLATFORM_IOS_MVK and VK_USE_PLATFORM_MACOS_MVK platform defines. This touches a lot of files but the changes are straight forward. The only issue is that VK_USE_PLATFORM_METAL_EXT requires initialization using a Metal layer vs. the view, so I had to make a few Apple-only changes to support this. I retained the deprecated platform options for backwards compatibility, but the default cmake setting is now VK_USE_PLATFORM_METAL_EXT.
  8. Check for supported dynamic state features in the dynamicstate example vs. just the presence of extensions since these are not necessarily the same thing. This is especially true for VK_EXT_extended_dynamic_state3.
  9. Fixed the vkCmdDraw() vertexCount argument in the particlesystem example since the previous code used the particle buffer size and not the particle count. This lead to multiple drawing artifacts as would be expected.
  10. Updated the examples list and restored the benchmarking script to the top level. This must have been lost when removing the bin directory. I still find the script useful when regression testing changes - it's a quick way to run through all the examples (with validation on) without manually running each one.
  11. Fixed validation warning in descriptorindexing example. Was missing the VK_KHR_maintenance1 extension.
  12. Added check for device max pipeline recursion depth to avoid validation warnings for limited capability devices in the raytracingcallable, raytracingintersection, raytracingreflections, and raytracingshadows examples.
  13. Fixed build settings for OpenMP in the texture3d example for all platforms. This used to work, but somehow things must have changed and the compiler flag settings and library location were not being set for the target.
  14. Updated and simplified build instructions for macOS.
  15. Updated CI script to account for updated build instructions re libomp on macOS.
  16. Added fix for combined image descriptor offset calculation in descriptorbuffer example (Problems with descriptorbuffer example on AMD GPU #1118).
  17. Add support for non-system level Vulkan SDK installs on macOS, with fallback to direct linkage to the MoltenVK library if the Vulkan loader is not found. This is helpful when testing against different official SDK versions, or locally built versions of MoltenVK. You can point VULKAN_SDK at a local install and cmake will use it if the system global installation is not available.

Tested on Windows 10, Manjaro Arch Linux, and macOS Ventura 13.6.6

@SRSaunders
Copy link
Contributor Author

SRSaunders commented Apr 24, 2024

The CI script is failing for macOS since the OpenMP_omp_LIBRARY path should now be /usr/local/opt/libomp/lib/libomp.dylib for homebrew on x86_64. See the updated (simpler) build instructions for macOS in commit 2057064.

Even though it was building before, OpenMP was not actually working for any platform: Win, Linux, or macOS. This is because the openmp compiler flags were not being set for the target and OpenMP_CXX_LIBRARY_DIRS is no longer defined or set by cmake's FindOpenMP.cmake module. Instead, you need to use OpenMP_CXX_LIBRARIES which provides the path for cmake's target_link_libraries() command

UPDATE: Finally got it working after three tries!

@SaschaWillems
Copy link
Owner

Thank you very much for your PR. This is very much appreciated.

I will take a look at this, but it may take a few weeks.

@SRSaunders
Copy link
Contributor Author

SRSaunders commented Apr 27, 2024

Thanks for agreeing to look at this. No rush so feel free to take your time.

There are a lot of small things collected in this PR. Each commit is a separate item (except VK_USE_PLATFORM_METAL_EXT, which is still one commit but a bit larger, and I added a second commit for particlesystem when I realized your pattern for checking vector sizes), so hopefully this will make it easier to review.

Note this PR has one non-duplicated commit 677b77b compared to #1119. If you are considering incorporating both PRs, then please merge this one #1117 first, then #1119 afterwards. Thanks.

@SaschaWillems SaschaWillems merged commit bdfd470 into SaschaWillems:master May 4, 2024
3 checks passed
@SRSaunders
Copy link
Contributor Author

Thanks for merging.

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

Successfully merging this pull request may close these issues.

2 participants