godot icon indicating copy to clipboard operation
godot copied to clipboard

Fix cross-platform configuration of rendering driver settings (narrower approach)

Open akien-mga opened this issue 10 months ago • 5 comments

Simpler alternative to #103026 which avoids breaking compatibility.

Instead of introducing a new auto default value, we ensure that all supported drivers are registered regardless of the editor's host platform, and that the defaults are the intended ones.

This solves the following issues:

  • macOS exports are meant to default to Metal in 4.4, but they would default to Vulkan if exported from Linux, Windows, or Android editors.
  • Windows exports couldn't be made with Direct3D 12 from Linux, macOS, or Android editors, as the option couldn't be selected outside Windows.

Unlike #103026, it doesn't solve the issue of not always saving the rendering drivers to project.godot, but now the defaults are at least consistent between editor platforms. So that issue is a lot less problematic, or maybe not even an issue anymore, but may be considered working as intended (so we can keep changing defaults if we want and have projects auto-upgrade).

  • Reverts #103026.
  • Fixes #103156.
  • Partially supersedes #103144. Some of the changes there are still worth doing, I can add them to this PR too, or #103144 could be rebased after this is merged.

Before (Linux editor):

image

After (Linux editor):

image

akien-mga avatar Feb 22 '25 23:02 akien-mga

There are issues with this approach:

  • Intel Mac users will have almost no indication they aren't running with Metal renderer (since Vulkan will be selected via fallback), which might be confusing.
  • Custom builds without Metal support on macOS or without Vulkan support on Windows, or projects with rendering/rendering_device/fallback_to_vulkan/rendering/rendering_device/fallback_to_d3d12 disabled will crash on start, current fallback system works only in specific conditions.

bruvzg avatar Feb 23 '25 08:02 bruvzg

So I do not see how it's safer, making project crash by unchecking one setting vs incorrect user code might no longer work in some cases doesn't feel safer.

Most of https://github.com/godotengine/godot/pull/103144 is still relevant (everything except || x == "auto" parts).

bruvzg avatar Feb 23 '25 08:02 bruvzg

Or we should at least relax fallback requirements (ignore fallback setting on Intel mac, and on Windows build with only one rendering driver):

diff --git a/platform/macos/display_server_macos.mm b/platform/macos/display_server_macos.mm
index 346faef842..556c4c966e 100644
--- a/platform/macos/display_server_macos.mm
+++ b/platform/macos/display_server_macos.mm
@@ -3791,8 +3791,11 @@ DisplayServerMacOS::DisplayServerMacOS(const String &p_rendering_driver, WindowM
 #if defined(VULKAN_ENABLED)
 #if defined(__x86_64__)
 	bool fallback_to_vulkan = GLOBAL_GET("rendering/rendering_device/fallback_to_vulkan");
+	if (!fallback_to_vulkan) {
+		WARN_PRINT("Metal is not supported on Intel Macs, switching to Vulkan.");
+	}
 	// Metal rendering driver not available on Intel.
-	if (fallback_to_vulkan && rendering_driver == "metal") {
+	if (rendering_driver == "metal") {
 		rendering_driver = "vulkan";
 		OS::get_singleton()->set_current_rendering_driver_name(rendering_driver);
 	}
diff --git a/platform/windows/display_server_windows.cpp b/platform/windows/display_server_windows.cpp
index fff237f10c..b3ce38b038 100644
--- a/platform/windows/display_server_windows.cpp
+++ b/platform/windows/display_server_windows.cpp
@@ -6763,24 +6763,30 @@ DisplayServerWindows::DisplayServerWindows(const String &p_rendering_driver, Win
 	_register_raw_input_devices(INVALID_WINDOW_ID);
 
 #if defined(RD_ENABLED)
+	bool fallback_to_vulkan = GLOBAL_GET("rendering/rendering_device/fallback_to_vulkan");
+	bool fallback_to_d3d12 = GLOBAL_GET("rendering/rendering_device/fallback_to_d3d12");
+
 #if defined(VULKAN_ENABLED)
 	if (rendering_driver == "vulkan") {
 		rendering_context = memnew(RenderingContextDriverVulkanWindows);
 		tested_drivers.set_flag(DRIVER_ID_RD_VULKAN);
 	}
+#else
+	fallback_to_d3d12 = true; // Always enable fallback if engine was built w/o other driver support.
 #endif
 #if defined(D3D12_ENABLED)
 	if (rendering_driver == "d3d12") {
 		rendering_context = memnew(RenderingContextDriverD3D12);
 		tested_drivers.set_flag(DRIVER_ID_RD_D3D12);
 	}
+#else
+	fallback_to_vulkan = true; // Always enable fallback if engine was built w/o other driver support.
 #endif
 
 	if (rendering_context) {
 		if (rendering_context->initialize() != OK) {
 			bool failed = true;
 #if defined(VULKAN_ENABLED)
-			bool fallback_to_vulkan = GLOBAL_GET("rendering/rendering_device/fallback_to_vulkan");
 			if (failed && fallback_to_vulkan && rendering_driver != "vulkan") {
 				memdelete(rendering_context);
 				rendering_context = memnew(RenderingContextDriverVulkanWindows);
@@ -6794,7 +6800,6 @@ DisplayServerWindows::DisplayServerWindows(const String &p_rendering_driver, Win
 			}
 #endif
 #if defined(D3D12_ENABLED)
-			bool fallback_to_d3d12 = GLOBAL_GET("rendering/rendering_device/fallback_to_d3d12");
 			if (failed && fallback_to_d3d12 && rendering_driver != "d3d12") {
 				memdelete(rendering_context);
 				rendering_context = memnew(RenderingContextDriverD3D12);

bruvzg avatar Feb 23 '25 09:02 bruvzg

So I do not see how it's safer, making project crash by unchecking one setting vs incorrect user code might no longer work in some cases doesn't feel safer.

To clarify, by "safer" I don't mean that it's more bullet-proof for edge case situations. I meant "safer" in the context of the 4.4 release cycle, by making changes which have minimal impact compared to the 4.4-beta4 state, and won't break potential user code that relies on querying the driver settings.

Adding auto helps cover more of the edge cases with smooth fallbacks but exposes uncertainty by not being able to really know which driver is going to be used.

So I want to go back to the previous state, make minimal improvements that help fix bugs without creating new ones, and then we can rework things further for 4.5, earlier on in that dev cycle.

So for example the fact that setting an unsupported driver leads to a crash is not something added by this PR. Before this and #103026, you could also use the official Windows editor builds on Windows, set d3d12 as driver, and then use custom export templates without d3d12 support, leading to a broken export.

This PR doesn't aim to address this which is IMO an edge case (doesn't happen with official builds), but if we can address it without risk then I'm not against it.

Intel Mac users will have almost no indication they aren't running with Metal renderer (since Vulkan will be selected via fallback), which might be confusing.

That's true, but I don't think it's a deal breaker for 4.4. The startup message in the output dock when they run the game should show that it's running Vulkan over MoltenVK I believe. The documentation I added also makes it clear that metal for macOS actually falls back to vulkan for Intel Macs. #103042 should help address that further once there's a consensus on the design.

Custom builds without Metal support on macOS or without Vulkan support on Windows, or projects with rendering/rendering_device/fallback_to_vulkan/rendering/rendering_device/fallback_to_d3d12 disabled will crash on start, current fallback system works only in specific conditions.

I'll add your patch which should address this.

akien-mga avatar Feb 23 '25 10:02 akien-mga

Custom builds without Metal support on macOS or without Vulkan support on Windows, or projects with rendering/rendering_device/fallback_to_vulkan/rendering/rendering_device/fallback_to_d3d12 disabled will crash on start, current fallback system works only in specific conditions.

I'll add your patch which should address this.

Done.

Most of #103144 is still relevant (everything except || x == "auto" parts).

Yes, I've included it here (CC @syntaxerror247) so we can review/test everything together.

akien-mga avatar Feb 23 '25 11:02 akien-mga

Thanks!

Repiteo avatar Feb 24 '25 15:02 Repiteo