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

Update macOS/iOS implementations, fix queue family transfers, implement window resizing for all examples #965

Merged
merged 25 commits into from
Jul 30, 2022

Conversation

SRSaunders
Copy link
Contributor

@SRSaunders SRSaunders commented Jul 13, 2022

This is a fairly sizeable pull request that updates the macOS/iOS implementation for on-par functionality vs. Windows and Linux, but is still subject to Vulkan API limitations imposed by MoltenVK (e.g. no geometry shaders, no raytracing, etc). I probably should have broken this contribution into multiple smaller pull requests, but feature and regression testing on macOS, iOS (both sim and hardware), Windows, and Linux proved more efficient this way.

In addition, I have made some generic changes that hopefully helps for all platforms:

  1. implemented window resizing viewChanged() for all examples, including multiview and multisampling
  2. fixed queue family transfer operations (graphics <-> compute) in the compute* examples so they follow Vulkan sample code and no longer throw validation errors
  3. updated getQueueFamilyIndex() to support VkQueueFlags vs. VkQueueFlagBits for the possibility of requesting a queue with multiple flag bits set - while not strictly necessary, I made this change during testing and is a generalization
  4. fixed most Vulkan validation errors (with the exception of window resizing on linux where the validation layer's view of swapchain dimensions can get out of sync with the actual runtime and throw spurious false positives)
  5. implemented the ability to change the swapchain image count on window resize events - while this was motivated by a macOS vsync issue for windowed vs fullscreen modes, it was an interesting problem and the solution is generic
  6. enabled ImGui scaling for macOS retina displays - this could possibly be leveraged for other HiDPI situations
  7. updated the computecloth example to use an animation rate that is independent of frame rate
  8. updated the ImGui example to display the Vulkan API level + driver information and also defined initial window placement/sizing where possible

The macOS/iOS specific changes are:

  1. updated the vulkanExample project (cmake -G "Xcode" or VK_EXAMPLE_XCODE_GENERATED) to support max frame rate rendering by default (the command line -vs option now works to optionally enable vsync if requested), the macOS and iOS xcode example projects remain vsync only (DisplayLink-driven). Note: also fixes benchmarking (-b) on macOS.
  2. updated the macOS xcode example project to properly handle and respond to keyboard and mouse events
  3. updated the iOS xcode example project to properly handle and respond to touch, tap, pan, and pinch/zoom gestures
  4. updated the iOS xcode example project to support the Xcode iOS simulator
  5. implemented Vulkan / memory cleanup on shutdown for both vulkanExample and xcode example projects
  6. fixed OpenMP integration for macOS and iOS - this accelerates texture regeneration in the texture3d example
  7. vulkanExample windows now launch in front of Xcode vs behind it when debugging - an annoying issue fixed
  8. updated the VK_KHR_portability_subset and VK_KHR_portability_enumeration implementation to perform runtime checks in case the project is linked to MoltenVK directly vs. to the Vulkan loader - cannot assume VK_KHR_portability_enumeration is available unless it appears at runtime (note this extension is Vulkan loader only)
  9. fixed the glTF examples so they build and link properly for both vulkanExample and xcode example projects
  10. fixed the triangle example so validation errors are eliminated and window resizing is possible on macOS (changes done using #defines so that bare code remains in place for other platforms, including iOS)
  11. worked around a known MoltenVK vsync issue where frame rate appears to be double the monitor refresh rate - workaround is to manipulate swapchain image count to force image acquire frame rate to match display vsync frame rate
  12. allow mouse click-through in vulkanExample project to capture first mouse click during long load-time examples (e.g. glTFloading example)

All these changes have been tested on macOS Big Sur, iOS 15.3.1 on an iPhone 8, iOS Sim for iPhone 13, Windows 10, and Linux Manjaro.

While there are a lot of changes here, hopefully they are useful to others and could help the project.

SRSaunders added 21 commits July 7, 2022 09:57
…yboard for resizable window, set animation rate based on display refresh period
…zing, update macOS examples.h file

(cherry picked from commit 47061ff99446d8826ebe7fe187467ba638236a70)
…build issues on macOS

(cherry picked from commit d2f6713c418ea5bdd2c3fcee922def5854e534d4)
… sensitivity, use key chars vs. key codes in Xcode examples project
…date and use keycodes.hpp defines for iOS/macOS
…for Win & macOS portability, support vsync off rendering on macOS, support swapchain image count change on resize, handle macOS fullscreen; Fixes for xcode example: use PanGestureRecognizer on iOS, add macOS cursor tracking, cleanup Vulkan on shutdown
…x multiple validation layer errors on resize and quit, multiview now supports resize/fullscreen, computecloth deltaT now based on frame time, multisampling recreates attachments on resize, P key now pauses computeparticles, descriptorsets, and pushdescriptors
…ion specific to iOS/macOS xcode examples project
…ues, generalize getQueueFamilyIndex() to support VkQueueFlags vs. VkQueueFlagBits, computecloth deltaT now based on frameTimer
… handling, fix triangle example resizing on macOS
…ow in front of Xcode, accept mouse click-through on macOS
…lementations for iOS/macOS support with runtime checks
…+ConditionalRender+gltfSceneRendering examples now support macOS retina displays
@SaschaWillems
Copy link
Owner

Thank you so much for this comprehensive PR. This is very much appreciated!

The only small problem I see with this, is that it contains a lot of fixed and changes (e.g. validation and sync) that I already did in my sync rework branch. See #871.

So it'll probably take me some time to go through all the changes before merging this, and making sure that they match above mentioned changes I already did.

@SRSaunders
Copy link
Contributor Author

SRSaunders commented Jul 15, 2022

Thanks for looking at the pull request. I started off with the intention of making only Apple-specific changes since the macOS/iOS portions of the project had not been updated for a while. However, once I got into it I extended the scope to include validation and sync issues to hopefully improve things cross-platform. Unfortunately I was not aware of your work in those areas and in retrospect perhaps this was a mistake on my part. In any case, hopefully my proposed changes could be a point of comparison for your own modernization efforts. At least for the graphics <-> compute queue sync issues, most of my changes were concentrated within commit b2f501d and a few within commit 1216128 (specifically computenbody and computeparticles). Other differences in the latter commit relate to handling swapchain image count changes on resize, but I am not sure if that is overlapping work or not.

As an aside, this pull request also fixes benchmarking on macOS (I forgot to include that in my notes above). To that end I just added a small change updating the benchmark-all.py script to update the example list and recognize macOS. UPDATE: Also fixed Vulkan / memory cleanup on exit for macOS benchmarking case.

After reviewing the changes, if you need any assistance separating the macOS/iOS - specific parts from the generic work I would be happy to help. I could also do some post-merge testing for those platforms if you need it. Let me know.

@SaschaWillems
Copy link
Owner

Took a first deeper look at this. And while I can't judge the Apple side of things, Windows, Linux and Android look fine to me. Did a quick test with several samples on Android and they worked as expected.

I'll do a more detailed review on this PR very soon, but I think this is good to go :)

@SRSaunders
Copy link
Contributor Author

SRSaunders commented Jul 24, 2022

Glad to hear that Android seems to be working fine. I don’t have that platform to test on.

Would you be able to check the imgui, gltfscenerendering, and textoverlay examples on Android? I would like to make sure UI display scaling and (x,y) input scaling (for widget interaction) is working properly on that platform, and that I didn’t break anything when implementing UI scaling for macOS retina displays.

UPDATE: After looking at the Android-specific scaling code, I suspect the following #ifdefs and code should be removed from VulkanExampleBase::updateOverlay(). Note that style.ScaleAllSizes(scale) is now called by UIOverlay::prepareResources() to set up style scaling, and the following lines may no longer be required:

void VulkanExampleBase::updateOverlay()
{
...
#if defined(VK_USE_PLATFORM_ANDROID_KHR)
	ImGui::PushStyleVar(ImGuiStyleVar_ItemSpacing, ImVec2(0.0f, 5.0f * UIOverlay.scale));
#endif
...
#if defined(VK_USE_PLATFORM_ANDROID_KHR)
	ImGui::PopStyleVar();
#endif
...
}

Perhaps you could test using gltfscenerendering on Android with these lines in and out and let me know the outcome. If needed, I would be happy to push an update to the pull request to address the issue.

@SaschaWillems
Copy link
Owner

The samples mentioned look exactly the same on Android as on the master branch. So it seems that no further changes are required.

@SRSaunders
Copy link
Contributor Author

SRSaunders commented Jul 25, 2022

Good news - thanks for letting me know re Android gui scaling.

FYI - I found an additional issue with the master branch: the F1 gui overlay toggle does not work across most examples and platforms, in some cases generating validation errors (linux). I have a set of changes that will correct this but are dependent on this PR. Unless you request otherwise, I will wait until you are finished reviewing this pull request and hopefully merge it. Once this is done I can submit a new PR with the changes. Would that approach work for you?

@SaschaWillems
Copy link
Owner

A question on resizing things in case swap chain image count changes: Is that a thing on iOS or Mac? I intentionally did not implement that (see e.g. the multiview sample) cause I thought this should never happen during runtime of a sample. But if that can happen on one of these platforms, I'm fine with those changes.

@SaschaWillems
Copy link
Owner

I went through all the changes and only noticed very minor things, see comments above.

…in macOS/iOS termination handlers, c) remove OpenMP target_compile_options() in CMakeLists
@SRSaunders
Copy link
Contributor Author

SRSaunders commented Jul 27, 2022

I went through all the changes and only noticed very minor things, see comments above.

Thanks very much for your feedback. I have a new commit prepared with the above code review changes but I would like to test before submitting. I should be able to push it soon and revise the PR.

UPDATE: I have pushed two commits to hopefully finalize this PR:

  1. Code review changes as per discussion above (revert TinyGLTF #defines, add vkDeviceWaitIdle() to macOS/iOS termination handlers and revert from examples above, remove OpenMP target_compile_options() in CMakeLists)
  2. Update headless examples (computeheadless, renderheadless) with the supported, up-to-date validation layer name (VK_LAYER_KHRONOS_validation) and implement VK_KHR_portability_subset / VK_KHR_portability_enumeration for macOS. This omission from my original pull request is a bit embarrassing. Turns out the headless examples used an out-of-date validation layer name (VK_LAYER_LUNARG_standard_validation). This did not activate the validation layer for any platform and prevented me from seeing macOS validation issues with these examples. I finally caught it when retesting the code review changes above.

These changes have been re-tested on macOS, iOS where applicable (no headless examples), Linux, and Windows 10.

…KHR_portability_subset / VK_KHR_portability_enumeration
@SaschaWillems
Copy link
Owner

Changes look good to me. Will merge this now :)

@SaschaWillems SaschaWillems merged commit 857f028 into SaschaWillems:master Jul 30, 2022
@SRSaunders
Copy link
Contributor Author

Thanks for merging. I appreciate your effort in reviewing this and including in the main branch.

I am away for a few days but will submit a new PR with the F1 UI overlay toggle fixes when I return.

@luck-bear
Copy link

luck-bear commented Oct 11, 2022 via email

if (CMAKE_CXX_COMPILER_ID MATCHES "Clang")
SET(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fobjc-arc -ObjC++")
ELSE()
#if (CMAKE_CXX_COMPILER_ID MATCHES "Clang")

Choose a reason for hiding this comment

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

commenting this out breaks fixed issue #770
revert?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The issue here is the kind of cmake generator used for macOS builds. The comment-out was to meant to support Xcode builds. However, you are right that it appears to break macOS builds when using cmake's make or ninja command-line generators. I will fix this by testing for CMAKE_GENERATOR and selecting the correct objective c++ option. Note the previous test for "Clang" did not make sense since it has been the default macOS compiler for ages and is used for both Xcode and command line builds. I will link a pull request here when ready.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changes implemented in PR #1117. Please let me know if this fixes your issue.

@luck-bear
Copy link

luck-bear commented Apr 12, 2024 via email

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

Successfully merging this pull request may close these issues.

4 participants