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

Abort on startup with a visible alert if required Vulkan features are missing #73999

Merged

Conversation

Calinou
Copy link
Member

@Calinou Calinou commented Feb 26, 2023

See #70013 (comment).

Preview

Screenshot_20230226_200928

@Calinou Calinou requested a review from a team as a code owner February 26, 2023 19:08
@Calinou Calinou added this to the 4.1 milestone Feb 26, 2023
@Calinou Calinou force-pushed the vulkan-context-abort-on-missing-features branch from 5787add to 2a146f9 Compare February 26, 2023 19:10
@clayjohn
Copy link
Member

Amazing! Does this work on mobile devices as well?

@Calinou
Copy link
Member Author

Calinou commented Feb 26, 2023

Amazing! Does this work on mobile devices as well?

It should work as alert() seems to be implemented on Android and iOS, but I haven't tested.

@RedworkDE
Copy link
Member

At least on Android alert() is not blocking #49969, so it won't actually be possible to read the dialog.

@lostminds
Copy link

At least on Android alert() is not blocking #49969, so it won't actually be possible to read the dialog.

I just tested and it's not blocking on iOS either.

OS.alert("This is our message","Alert!")
OS.crash("Crash message")

Causes a crash before the alert is visible on screen, so I assume it would be the same in this case of the alert about required vulkan features.

@lostminds
Copy link

Given the current situation that on mobile alert() isn't blocking, perhaps a compromise could be to simply not crash/exit on mobile. As I understand it that would leave it at a black screen once the user closes the alert message with the info, but that's fine and much better than the alternative (crash with no explanation) or current situation (black screen with no explanation).

@Calinou Calinou force-pushed the vulkan-context-abort-on-missing-features branch from 2a146f9 to eece2b3 Compare March 19, 2023 20:40
@Calinou
Copy link
Member Author

Calinou commented Mar 19, 2023

I removed the CRASH_NOW_MSG() call, so the previous indefinite black screen behavior is restored.

@clayjohn
Copy link
Member

clayjohn commented Apr 5, 2023

I think this still needs to return ERR_CANT_CREATE otherwise it will try to continue initializing Vulkan

@clayjohn clayjohn modified the milestones: 4.1, 4.x May 23, 2023
@clayjohn clayjohn modified the milestones: 4.x, 4.2 Jul 9, 2023
@Calinou Calinou force-pushed the vulkan-context-abort-on-missing-features branch from eece2b3 to 70cd272 Compare July 17, 2023 06:59
@Calinou
Copy link
Member Author

Calinou commented Jul 17, 2023

I think this still needs to return ERR_CANT_CREATE otherwise it will try to continue initializing Vulkan

Done 🙂

I've also modified the dialog to mention the GPU model name (useful for systems with multiple GPUs to choose from).

@Calinou Calinou force-pushed the vulkan-context-abort-on-missing-features branch from 70cd272 to a9d0862 Compare July 17, 2023 07:02
@clayjohn
Copy link
Member

Tested locally and this seems to work fine now. The popup works as expected and then falls back to the normal "failed to initialize vulkan" error which points the user to using the compatibility renderer.

@Calinou Calinou force-pushed the vulkan-context-abort-on-missing-features branch from a9d0862 to e65d2d7 Compare August 17, 2023 12:48
@Calinou Calinou force-pushed the vulkan-context-abort-on-missing-features branch from e65d2d7 to ce57c23 Compare August 17, 2023 12:48
@akien-mga akien-mga merged commit ed8b92a into godotengine:master Aug 17, 2023
@akien-mga
Copy link
Member

Thanks!

@Calinou Calinou deleted the vulkan-context-abort-on-missing-features branch August 17, 2023 13:50
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.

5 participants