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

DisplayServer: Don't fallback to headless #62556

Merged

Conversation

akien-mga
Copy link
Member

@akien-mga akien-mga commented Jun 30, 2022

Unless users requested the headless driver specifically, they expect to either
see a window, or that the process terminates if there's an error.

Currently it would fallback to headless so they'd unexpectedly get a valid headless
instance if their DisplayServer failed initializing (e.g. missing Vulkan support).


Depends on #62555, otherwise it's crashing on exit in the case where it couldn't initialize a DisplayServer.
With #62555, it prints an error instead (which itself is likely a bug that could be fixed independently):

$ VK_ICD_FILENAMES= godot-git
Godot Engine v4.0.alpha.custom_build.e7197cb6f - https://godotengine.org
ERROR: No surface extension found, is a driver installed?
   at: _initialize_extensions (drivers/vulkan/vulkan_context.cpp:353)
ERROR: Could not initialize Vulkan
   at: DisplayServerX11 (platform/linuxbsd/display_server_x11.cpp:4684)
ERROR: Unable to create DisplayServer, all display drivers failed.
   at: setup2 (main/main.cpp:1700)
ERROR: BUG: Unreferenced static string to 0: interface_added
   at: unref (core/string/string_name.cpp:131)

The last ERROR here is printed using the fallback.

@Calinou
Copy link
Member

Calinou commented Jun 30, 2022

How does this PR compare to #59074?

@akien-mga
Copy link
Member Author

How does this PR compare to #59074?

Ah thanks, I knew I remembered a PR that did something like this but couldn't find it again.

I think my solution is more elegant, poke @RandomShaper for cross-review.

@akien-mga akien-mga requested a review from RandomShaper June 30, 2022 17:09
@RandomShaper
Copy link
Member

To do a recap on how this PR compares to mine:

  1. This one assumes that headless will always correspond to the highest index. Mine checks by name, but to do that it has first to create and destroy it if turned out to the headless one, since there's no API to check the name beforehand.
  2. Mine adds an abrupt exit (exit(255);) in case no display driver works. If I remember correctly, that was to avoid the engine startp ending in a crash that could cause further confusion to the user.

Regarding 1), I can't really tell which approach is better. Regarding 2), if it's still relevant for its purpose, I'd include it in whatever PR we end up merging.

@akien-mga
Copy link
Member Author

akien-mga commented Jun 30, 2022

For 1., this is seems to be the intended design that headless should always be last:

// Headless display server is always last

But might need double checking. I could add e.g. an assert that the name of the last index DisplayServer is headless.

For 2., I had a crash but I fixed it with #62555. I think it's nicer to let Godot try to exit gracefully than doing a hard exit, so we can properly do finalize steps.

@RandomShaper
Copy link
Member

Fair enough.

If we can somehow check under DEV_ENABLED that headless is the last one, cool. If not possible, I'm happy as well now I know it's like a design rule.

Unless users requested the headless driver specifically, they expect to either
see a window, or that the process terminates if there's an error.

Currently it would fallback to headless so they'd unexpectedly get a valid headless
instance if their DisplayServer failed initializing (e.g. missing Vulkan support).

Fixes godotengine#58414.
@akien-mga akien-mga force-pushed the displayserver-no-headless-fallback branch from 11a978b to 833c786 Compare July 1, 2022 21:52
@akien-mga
Copy link
Member Author

Added a couple DEV_ASSERT checks to validate that the headless DisplayServer is the last entry, and likewise for the dummy AudioDriver.

@akien-mga akien-mga merged commit 2b31c88 into godotengine:master Jul 2, 2022
@akien-mga akien-mga deleted the displayserver-no-headless-fallback branch July 2, 2022 08:54
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.

Vulkan: After giving error about missing driver, Godot doesn’t exit
3 participants