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

iOS runtime fixes #1173

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

Conversation

SRSaunders
Copy link
Contributor

@SRSaunders SRSaunders commented Sep 23, 2024

Description

This PR fixes multiple runtime issues for iOS physical device and iOS Simulator targets:

  1. General: Fixes error handling for main_loop_frame() and eliminates redundancy with main_loop(). Moved window close logic from main_loop() to main_loop_frame() so iOS can handle this condition properly. This does not change behaviour for other platforms since main_loop() calls main_loop_frame().
  2. General: Asserts for valid render context before calling render() within [HPP]ApiVulkanSample::update(). This is primarily defensive code but the issue was visible before improving error detection in main_loop_frame().
  3. iOS only: Adds main_loop_frame() return code handling within iOS renderLoop() - this was previously missing. Also adds exit handling when an error is encountered or batch mode is complete. The latter is unconventional for iOS, but is justified here since this is an Xcode-controlled developer application.
  4. iOS only: Adds support for setting and getting iOS window content scale factor, which is important for the rendering of certain samples on retina displays.
  5. General: Fixes [HPP]Gui::draw() to work with iOS Simulator with its vkCmdDrawIndexed() vertex offset limitations. The iOS Simulator does not support vertex offets > 0, so I made a modification that rebinds the vertex buffer with an offset when running on the iOS Simulator. Functionality is unchanged when running on real iOS targets or other platforms.
  6. General: Initializes pipeline variables to null handles for better startup error recovery in oit* samples. This issue shows up when a platform does not support the required sample features, and teardown crashes with uninitialized pipelines.
  7. General: Use Vulkan API 1.0 support funcs for Vulkan API 1.0 sample host_image_copy. The sample originally used vkGetPhysicalDeviceFormatProperties2() (API 1.1) but should use vkGetPhysicalDeviceFormatProperties2KHR(). This issue shows up when using MoltenVK which is more sensitive to the Vulkan API version than the Vulkan loader. Moved to a separate PR.
  8. iOS Simulator only: Disable MoltenVK's Metal Argument Buffers on the iOS Simulator for samples: layout_transitions, pipeline_barriers, subpasses. Otherwise the samples can display blank screens on the iOS Simulator. Reverted for now until issue resolved in MoltenVK project.

Tested on iOS 17 with an iPhone 15, on the iOS Simulator, and macOS Ventura.

General Checklist:

Please ensure the following points are checked:

  • My code follows the coding style
  • I have reviewed file licenses
  • I have commented any added functions (in line with Doxygen)
  • I have commented any code that could be hard to understand
  • My changes do not add any new compiler warnings
  • My changes do not add any new validation layer errors or warnings
  • I have used existing framework/helper functions where possible
  • My changes do not add any regressions
  • I have tested every sample to ensure everything runs correctly
  • This PR describes the scope and expected impact of the changes I am making

Note: The Samples CI runs a number of checks including:

  • I have updated the header Copyright to reflect the current year (CI build will fail if Copyright is out of date)
  • My changes build on Windows, Linux, macOS and Android. Otherwise I have documented any exceptions

If this PR contains framework changes:

  • I did a full batch run using the batch command line argument to make sure all samples still work properly

Sample Checklist

If your PR contains a new or modified sample, these further checks must be carried out in addition to the General Checklist:

  • I have tested the sample on at least one compliant Vulkan implementation
  • If the sample is vendor-specific, I have tagged it appropriately
  • I have stated on what implementation the sample has been tested so that others can test on different implementations and platforms
  • Any dependent assets have been merged and published in downstream modules
  • For new samples, I have added a paragraph with a summary to the appropriate chapter in the readme of the folder that the sample belongs to e.g. api samples readme
  • For new samples, I have added a tutorial README.md file to guide users through what they need to know to implement code using this feature. For example, see conditional_rendering
  • For new samples, I have added a link to the Antora navigation so that the sample will be listed at the Vulkan documentation site

Copy link
Contributor

@asuessenbach asuessenbach left a comment

Choose a reason for hiding this comment

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

I can't tell anything about the iOS Simulator stuff.
But it seems, you have three identical code sequences introduced in layout_transition.cpp, pipeline_barriers.cpp, and subpasses.cpp. Maybe introduce a little helper function, instead?

framework/api_vulkan_sample.cpp Outdated Show resolved Hide resolved
framework/gui.cpp Show resolved Hide resolved
framework/hpp_api_vulkan_sample.cpp Outdated Show resolved Hide resolved
framework/hpp_gui.cpp Show resolved Hide resolved
framework/platform/platform.cpp Show resolved Hide resolved
samples/performance/subpasses/subpasses.cpp Outdated Show resolved Hide resolved
samples/extensions/host_image_copy/host_image_copy.cpp Outdated Show resolved Hide resolved
@@ -653,7 +668,13 @@ void HPPGui::draw(vk::CommandBuffer command_buffer) const
command_buffer.drawIndexed(cmd->ElemCount, 1, index_offset, vertex_offset, 0);
index_offset += cmd->ElemCount;
}
#if defined(PLATFORM__MACOS) && TARGET_OS_IOS && TARGET_OS_SIMULATOR
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm a bit torn on "polluting" code with a special case as the iOS simulator. Can this be solved in a more elegant way? I wouldn't mind if we'd simply disable the UI on the iOS simulator instead of adding specific code for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I debated this myself, and actually tried the UI disable route initially. However, I didn't like the experience, especially in batch mode where you really can't tell what is going on as the sample runs through its various configurations. And also in the near future I would to add gesture and key input handling for iOS and without the UI present on the Simulator that would be completely inaccessible.

The reason I like the iOS Simulator is that it is easy to run and does not require an Apple Development Certificate nor a unqiue bundle identifier for iOS. I figured this would make your project easily accessible for iOS testing without adding overhead for potential users. In addition, the Simulator allows me to try different devices that I don't own (e.g. larger screen and various generation iOS devices).

If you don't like the #ifdef special case handling, the change that I made is completely generic and could be applied to all platforms, i.e. simply don't use vertex_offset > 0 and use my rebinding code for all platforms. Not sure if you would like that but it would make the code generic. Please let me know.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe if we combine them into a new macro that's APPLE specific. Say SAMPLES_PLATFORM_APPLE or similar. I think the path of having macros is fine but there are other platforms in the Apple ecosystem that we haven't added support for yet.

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

Successfully merging this pull request may close these issues.

4 participants