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

fix crash on iOS 13 #33154

Merged
merged 1 commit into from Nov 5, 2019
Merged

fix crash on iOS 13 #33154

merged 1 commit into from Nov 5, 2019

Conversation

@ealataur
Copy link
Contributor

ealataur commented Oct 29, 2019

Fix crash on iOS 13 when app go to background and back it from

Bugsquad edit: Fixes #7966.

@akien-mga akien-mga requested review from BastiaanOlij and godotengine/ios and removed request for BastiaanOlij Oct 29, 2019
@akien-mga akien-mga added this to the 3.2 milestone Oct 29, 2019
platform/iphone/gl_view.mm Outdated Show resolved Hide resolved
@akien-mga

This comment has been minimized.

Copy link
Member

akien-mga commented Oct 30, 2019

Could you rebase the branch to remove the unnecessary merge commit? (just git pull --rebase upstream master should be sufficient if you have upstream pointing to godotengine/godot). See https://docs.godotengine.org/en/3.1/community/contributing/pr_workflow.html#mastering-the-pr-workflow-the-rebase

@akien-mga

This comment has been minimized.

Copy link
Member

akien-mga commented Nov 3, 2019

Still needs a rebase to squash commits, otherwise changes seem OK if others can confirm that it fixes the bug without introducing performance regressions.

@gerald1248

This comment has been minimized.

Copy link

gerald1248 commented Nov 4, 2019

@akien-mga which benchmark should we use? I have built iphone.zip with and without the change. I'm happy to run a comparison.

I'm not 100% confident performance is the same (I get weird dropped frames sometimes, but perhaps they were an issue before and I didn't notice?) but the thing is that this addresses a bug that's pretty serious. Every Godot app on iOS13 crashes every time it is closed.

Fixes #7966.
@akien-mga akien-mga force-pushed the ealataur:master branch from a514ca8 to 29bde8c Nov 5, 2019
@akien-mga

This comment has been minimized.

Copy link
Member

akien-mga commented Nov 5, 2019

Pushed a rebased commit that should be fine to merge.

which benchmark should we use? I have built iphone.zip with and without the change. I'm happy to run a comparison.

I'm not sure, but I guess the changes should be fine. The only performance-impacting change now is the reintroduction of the [drawView] call, which was removed in #32194 but this was likely not the right fix for the bug it tried to address (this PR looks like it might be).

@akien-mga akien-mga merged commit 2cf7f53 into godotengine:master Nov 5, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@akien-mga

This comment has been minimized.

Copy link
Member

akien-mga commented Nov 5, 2019

Thanks!

@akien-mga

This comment has been minimized.

Copy link
Member

akien-mga commented Nov 8, 2019

Cherry-picked for 3.1.2.

@akien-mga

This comment has been minimized.

Copy link
Member

akien-mga commented Nov 8, 2019

Cherry-picked for 3.0.7.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.