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

Add updated iOS App support to the Vulkan examples #1119

Merged
merged 27 commits into from
May 22, 2024

Conversation

SRSaunders
Copy link
Contributor

@SRSaunders SRSaunders commented Apr 25, 2024

This PR depends on and includes #1117 due to those dependencies - only the final commits here are new. However, I am submitting this as a separate PR since I am not sure if you want to include iOS in your project. I would be happy to provide support from time-to-time for new updates if you need it. I would be interested in @billhollings reaction and if he would like this added to your repository, or if he has other plans. Most of the original code was his, and credits/copyright remain in place but perhaps dates should be updated.

Similar to before, it is an Xcode project contained within a separate sub-folder (but named apple this time) that depends on the base project. It is like the previous xcode sub-folder, but with documentation and functionality emphasis on iOS and the iOS Simulator. macOS standalone app support is also present but not emphasized. Most users should use the base cmake project in the main folder for macOS.

Details are:

  1. Updated to use VK_USE_PLATFORM_METAL_EXT by default vs. deprecated VK_USE_PLATFORM_IOS_MVK
  2. Updated to use MoltenVK.xcframework from the Vulkan SDK (requires SDK 1.3.275.0 and Xcode 14 or later).
  3. Added support for single and multi-touch gestures on iOS and the iOS Simulator, allowing select and rotation of the scene, plus translation and zoom. Virtual keyboard support remains for imgui overlay toggle and animation pausing.
  4. Updated example.h file with categories and comments on which examples currently work and don't work on iOS.
  5. Updated to work with your current main project baseline.
  6. Updated docs.

Let me know if you are interested in this and if so, I will do my best to help out. If not, then no worries. I realize that automated CI support is not implemented at this time, but perhaps could be added.

Here is a pic of the bloom example running on the iOS simulator...
Screenshot 2024-04-21 at 1 26 16 PM

SRSaunders and others added 19 commits April 21, 2024 20:16
@SaschaWillems
Copy link
Owner

Thank you very much for your PR. This is very much appreciated. I don't own any apple devices to test this on, so all I can do is a code review.

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

@SRSaunders
Copy link
Contributor Author

SRSaunders commented Apr 27, 2024

Again thanks for agreeing to look at this. Only commits 31f98f9 and later are different from the other PR.

Given your comments above about testing on Apple devices, I have gone ahead and added CI support for iOS so you can at least be assured of build sanity.

@SRSaunders
Copy link
Contributor Author

SRSaunders commented May 4, 2024

Fixed merge conflicts.

Thanks for catching my redundant code for getting rayTracingPipelineProperties. I originally missed that common code in VulkanRayTracingSample.

Also, it seems that you don't want the benchmarking script. Note it is present in this PR too, so please remove if desired.

Copy link
Owner

@SaschaWillems SaschaWillems left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I can't test this looks fine to me and CI is in place. Aside from a minor nitpick I'll merge once I get green light from @billhollings with regards to copyright.

benchmark-all-validate.py Outdated Show resolved Hide resolved

#MoltenVK Vulkan Examples

Copyright (c) 2016-2024 [The Brenwill Workshop Ltd.](http://www.brenwill.com).
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would like to get @billhollings feedback if it's okay to put this into my repo, due to the copyright line.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. And as I mentioned before, if @billhollings has concerns about maintaining it I can help out.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay.

Yes...any code Brenwill contributes (or has contributed) is open-source, and respects the open-source license of this repo.

@SaschaWillems
Copy link
Owner

Thank you very much for the confirmation. Merging this now :)

@SaschaWillems SaschaWillems merged commit ba927ea into SaschaWillems:master May 22, 2024
4 checks passed
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.

None yet

3 participants