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

Fallback to OpenGL 3 if other rendering drivers are not supported #97142

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

SheepYhangCN
Copy link

@SheepYhangCN SheepYhangCN commented Sep 18, 2024

Add an option to allow renderer fall back to OpenGL 3 if Vulkan (and D3D12 on Windows, Metal on Apple devices) are not supported
Free players from using --rendering-driver opengl3 manually
Free developers from recieving "Why my game crashed with a message that I can't understand"

@SheepYhangCN SheepYhangCN requested review from a team as code owners September 18, 2024 11:34
@akien-mga
Copy link
Member

akien-mga commented Sep 18, 2024

This might solve godotengine/godot-proposals#8006, but it should support all platforms, not just Windows.

I support the idea in general, I think this is indeed needed to better handle users who don't support modern graphics APIs.

@SheepYhangCN
Copy link
Author

SheepYhangCN commented Sep 18, 2024

but it should support all platforms, not just Windows.

I will try to do this for other platforms later
Since Metal is native now so MacOS/iOS shouldn't need for OpenGL fallback?

@SheepYhangCN SheepYhangCN changed the title Fallback to OpenGL 3 if both D3D12 and Vulkan are not supported [Windows] Fallback to OpenGL 3 if both D3D12 and Vulkan are not supported Sep 18, 2024
@SheepYhangCN SheepYhangCN changed the title [Windows] Fallback to OpenGL 3 if both D3D12 and Vulkan are not supported Fallback to OpenGL 3 if both D3D12 and Vulkan are not supported Sep 18, 2024
@SheepYhangCN SheepYhangCN requested review from a team as code owners September 18, 2024 12:22
@SheepYhangCN SheepYhangCN changed the title Fallback to OpenGL 3 if both D3D12 and Vulkan are not supported Fallback to OpenGL 3 if Vulkan (and D3D12 on Windows) are not supported Sep 18, 2024
@Calinou
Copy link
Member

Calinou commented Sep 18, 2024

Since Metal is native now so MacOS/iOS shouldn't need for OpenGL fallback?

Metal support is only available on Apple Silicon hardware, but most Intel Macs have Vulkan support through MoltenVK. That said, old Intel Macs can't run Vulkan through MoltenVK due to missing hardware features, so the Vulkan -> OpenGL fallback would still be needed on macOS.

The same goes for iOS - old devices can't run Vulkan through MoltenVK either: #74227

@SheepYhangCN
Copy link
Author

Since Metal is native now so MacOS/iOS shouldn't need for OpenGL fallback?

Metal support is only available on Apple Silicon hardware, but most Intel Macs have Vulkan support through MoltenVK. That said, old Intel Macs can't run Vulkan through MoltenVK due to missing hardware features, so the Vulkan -> OpenGL fallback would still be needed on macOS.

Understood

@SheepYhangCN SheepYhangCN changed the title Fallback to OpenGL 3 if Vulkan (and D3D12 on Windows) are not supported Fallback to OpenGL 3 if other rendering drivers are not supported Sep 18, 2024
@SheepYhangCN SheepYhangCN requested review from a team as code owners September 18, 2024 13:01
@clayjohn
Copy link
Member

The code looks good, but when I test locally (with modifications to make rendering_context->initialize() fail), I get a deadlock. But only when opening the project through the project manager. If the project is opened on its own, it falls back correctly.

I'm investigating further now.

@clayjohn
Copy link
Member

I figured out the issue, the fallback code wasn't setting the rendering_method, therefore, any calls to OS::get_singleton()->get_current_rendering_method() returned the wrong value.

We don't use the current rendering method often, but we do check it when rendering to ensure that the opengl context is bound to the correct window:

if (OS::get_singleton()->get_current_rendering_method() == "gl_compatibility") {
// This is currently needed for GLES to keep the current window being rendered to up to date
DisplayServer::get_singleton()->gl_window_make_current(p_viewport->viewport_to_screen);
}

In the editor, this code wouldn't get called. So the "loading" popup would take the openGL context and then not return it leading to the main screen never updating. So it wasn't a deadlock after all!

The solution is to set the rendering_method at the same time that we set the driver:


OS::get_singleton()->set_current_rendering_driver_name(rendering_driver);
OS::get_singleton()->set_current_rendering_method("gl_compatibility");

@SheepYhangCN
Copy link
Author

Fixed

Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

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

Looks great! The final step before merging is to squash all the commits together so that the whole PR only contains 1 big commit with all your changes. We like to merge one commit at a time to keep the git history clean and navigable.

If you don't know how to do that, we have a helpful tutorial in the official documentation https://docs.godotengine.org/en/latest/community/contributing/pr_workflow.html#the-interactive-rebase

@SheepYhangCN
Copy link
Author

Everything should be okay now I think

Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

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

Yep, looks great! Thank you

@clayjohn clayjohn modified the milestones: 4.x, 4.4 Sep 19, 2024
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