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

Try all known video drivers if the requested one fails to initialize #13284

Merged
merged 2 commits into from Jun 25, 2023

Conversation

numberZero
Copy link
Contributor

@numberZero numberZero commented Mar 5, 2023

Sometimes, “best” available video driver isn’t at all best, or even working. With this PR, if the configured video driver fails to initialize, Minetest will try each other supported video driver, in order of decreasing preference, until it finds one that works (initializes successfully).

@rubenwardy rubenwardy added Rebase needed The PR needs to be rebased by its author. WIP The PR is still being worked on by its author and not ready yet. labels Apr 13, 2023
@Zughy
Copy link
Member

Zughy commented May 14, 2023

@numberZero any updates on this?

Comment on lines 156 to 157
if (!m_device)
throw std::runtime_error("Could not initialize the device with any supported video driver");
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part would be good to get in; without it the process crashes on the next line when createDeviceEx() fails.

I might throw another type of exception because runtime_error results in dumping a core, which is useless for this error and a bit severe. Maybe the existing ClientStateError or roll your own video-specific exception type?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ClientStateError is definitely wrong choice, it is used in the server to denote that there is something wrong with the client. And, what’s the point in using another exception type if no exceptions are caught up the call stack anyway?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And, what’s the point in using another exception type if no exceptions are caught up the call stack anyway?

As I said, left uncaught this exception can result in a core file, which while useful for debugging in general, doesn't have much use here. It's just nicer to not litter the user's machine with such files for lack of a more appropriate exception type, but I guess there are lots of ways to disable core files at the OS level so I can deal with it. It's just a good code hygiene suggestion.

@rubenwardy rubenwardy added the Roadmap The change matches an item on the current roadmap. label May 27, 2023
@numberZero numberZero marked this pull request as ready for review June 17, 2023 12:48
@sfan5 sfan5 removed the Rebase needed The PR needs to be rebased by its author. label Jun 17, 2023
@numberZero
Copy link
Contributor Author

Not just rebased, this should work now)

@Zughy Zughy removed the WIP The PR is still being worked on by its author and not ready yet. label Jun 22, 2023
Copy link
Member

@Desour Desour left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

src/client/renderingengine.cpp Show resolved Hide resolved
@sfan5 sfan5 merged commit aada240 into minetest:master Jun 25, 2023
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@ Client / Audiovisuals Roadmap The change matches an item on the current roadmap. >= Two approvals ✅ ✅
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants