KhronosGroup / Vulkan-Samples

One stop solution for all Vulkan samples
Apache License 2.0
4.36k stars 650 forks source link

Regression in Platform::main_loop_frame() that causes all samples to only render one frame on iOS #1131

Closed billhollings closed 3 months ago

billhollings commented 3 months ago

Platform::main_loop_frame() has code at the beginning:

    if (!app_requested())
    {
        return ExitCode::NoSample;
    }

That halts execution after the first frame, because Platform::start_app() sets requested_app to nullptr, and Platform::main_loop_frame() is called on every frame.

Removing the code above fixes the issue.

I've submitted PR #1120 that fixes this, and also consolidates Platform::main_loop_frame() into Platform::main_loop() to eliminate the difficult-to-maintain code-duplication. However, I'm having trouble passing the CI code formatting tests on that PR, so in case that PR doesn't make it in, or if there is a better solution, I'm adding this here, so someone more familiar with this environment can fix it.

SRSaunders commented 3 months ago

@billhollings I am planning to submit a PR with other iOS improvements and can include your simple fix above if required (i.e. remove the piece of offending code at the start of Platform::main_loop_frame()). I have been testing my changes for a while using this fix and it works fine. This of course depends on the outcome of your PR #1120.

FYI, to address the CI code formatting issue, you can run Homebrew's clang-format from the project's top level dir:

clang-format framework/platform/platform.cpp --style=file:.clang-format --dry-run to see the changes clang-format framework/platform/platform.cpp --style=file:.clang-format -i to make the changes

Since this operates at the file level, the trick I have found is to afterwards manually revert any line changes in the file that are not related to your specific changes. The current CI process seems to get hung up on formatting fixes that are not related to your previous git changes.

billhollings commented 3 months ago

PR #1120 fixes this issue.

billhollings commented 3 months ago

@billhollings I am planning to submit a PR with other iOS improvements and can include your simple fix above if required (i.e. remove the piece of offending code at the start of Platform::main_loop_frame()). I have been testing my changes for a while using this fix and it works fine. This of course depends on the outcome of your PR #1120.

We might be on parallel paths. 😉

I've updated #1120 so that it fixes an issue I found where the HUD overlay was being displayed WAY too big and not with the correct transparency.

My involvement in these fixes was just to ensure that MoltenVK was not the culprit on them. Feel free to swap in any similar fixes you have, instead of using mine.

FYI, to address the CI code formatting issue, you can run Homebrew's clang-format from the project's top level dir:

clang-format framework/platform/platform.cpp --style=file:.clang-format --dry-run to see the changes clang-format framework/platform/platform.cpp --style=file:.clang-format -i to make the changes

Since this operates at the file level, the trick I have found is to afterwards manually revert any line changes in the file that are not related to your specific changes. The current CI process seems to get hung up on formatting fixes that are not related to your previous git changes.

Thanks for your help. I've got #1120 formatting correctly now.

SRSaunders commented 3 months ago

I've updated https://github.com/KhronosGroup/Vulkan-Samples/pull/1120 so that it fixes an issue I found where the HUD overlay was being displayed WAY too big and not with the correct transparency.

Thanks for adding the Storyboard fix your PR. I will delete this from my pending changes 😉

Regarding UI Overlay scaling I solved it slightly differently - see my comments in your PR.

FYI - I already added .DS_Store to the .gitignore file. If you rebase to current main, you will see it.

My involvement in these fixes was just to ensure that MoltenVK was not the culprit on them. Feel free to swap in any similar fixes you have, instead of using mine.

I am just happy you have eyes on this. Thanks for bringing your perspective and experience here.