-
Notifications
You must be signed in to change notification settings - Fork 626
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
Fixes to iOS render loop and HUD overlay. #1120
Conversation
You should at least resolve the failing Quality Check. |
40ea4c8
to
57d03b0
Compare
If tried many times.
|
https://github.com/KhronosGroup/Vulkan-Samples/actions/runs/10461719961?pr=1120 If you scroll to the bottom you can grab the diff that can be directly run and hopefully fix the issue. |
Indeed, it should remove white spaces at the end of a line, and it should replace spaces with tabs. |
8cfcdc6
to
0a75d06
Compare
Thanks for the help on code formatting. I've got this formatting correctly now. Plus I've added a couple of other iOS fixes, and updated the title and description above. |
0a75d06
to
afa3773
Compare
* Refactor Platform::main_loop_frame() and main_loop(), to reduce fragility, and avoid unnecessary code duplication. * Avoid initial app_requested() test in main_loop_frame(), as it was causing looping to stop after one frame. * Since main_loop() is not a virtual function, remove declaration comment indicating it can be overridden. * Set iOS view to have a black background, to ensure transparency in HUD overlay displays in full on device. * Fix rendering scaling in IosWindow::get_dpi_factor(). Previous value was causing HUD overlay to render magnitudes too large. * Add macOS files to .gitignore.
afa3773
to
7d5f680
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By code inspecting and running on Windows: LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Did full batch runs for iOS + macOS and both main_loop_frame()
and main_loop()
worked correctly. Still need fixes for non-success return codes in iOS viewDidLoad()
and renderLoop()
, but these can be dealt with in a future PR.
@SRSaunders Would you mind giving this a formal approval, so it can be merged, please? Once the approvals are in, what is the process for merging? Do I do that? Or is there a designated person responsible for merging? |
We usually ask @marty-johnson59 to handle the merging. We usually merge on 3; but Marty, I think this is safe to merge on 2. |
@billhollings I don't have approval rights for this project. I am happy to provide feedback, but I cannot mark the PR as ready for merge. As you see above, @gpx1000 has that ability. |
Thanks. Given this is Mac (we have limited folks who can test), I'm happy to merge w/ 2. We can revert if needed. |
Refactor Platform::main_loop_frame() and main_loop(),
to reduce fragility, and avoid unnecessary code duplication.
Avoid initial app_requested() test in main_loop_frame(),
as it was causing looping to stop after one frame.
Since main_loop() is not a virtual function, remove
declaration comment indicating it can be overridden.
Set iOS view to have a black background, to ensure
transparency in HUD overlay displays in full on device.
Fix rendering scaling in IosWindow::get_dpi_factor().
Previous value was causing HUD overlay to render magnitudes too large.
Add macOS files to .gitignore.
I created this from a branch on this repo instead of a separate fork, because I was not expecting to need to make a fix.
Fixes #1121
Fixes #1131
General Checklist:
Please ensure the following points are checked:
Note: The Samples CI runs a number of checks including:
If this PR contains framework changes:
batch
command line argument to make sure all samples still work properlySample Checklist
If your PR contains a new or modified sample, these further checks must be carried out in addition to the General Checklist: