Fix cross-platform configuration of rendering driver settings (narrower approach) #103197
Conversation
|
There are issues with this approach:
|
|
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 #103144 is still relevant (everything except |
|
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); |
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 Adding 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 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.
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
I'll add your patch which should address this. |
akien-mga
changed the title
…add "auto" option." This reverts commit dea20c4. This had unforeseen consequences for editor code that relies on querying these settings, and possibly thirdparty code that would do the same. In hindsight, it's a bit too late in the release cycle to make such a compatibility breaking change.
Simpler alternative to godotengine#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 godotengine#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. Co-authored-by: Pāvels Nadtočajevs <[email protected]>
Also fix iOS export logic that would force a min target of iOS 14.0 (for Metal) even when targeting the Compatibility renderer. Co-authored-by: Pāvels Nadtočajevs <[email protected]>
akien-mga
force-pushed
the
safer-rendering-driver-selection
branch
from
2dd2d67 to
28d1dcc
Compare
Done.
Yes, I've included it here (CC @syntaxerror247) so we can review/test everything together. |
| String current_renderer = GLOBAL_GET("rendering/renderer/rendering_method.mobile"); | ||
| bool uses_vulkan = (current_renderer == "forward_plus" || current_renderer == "mobile") && GLOBAL_GET("rendering/rendering_device/driver.android") == "vulkan"; | ||
| return uses_vulkan; | ||
| String rendering_method = GLOBAL_GET("rendering/renderer/rendering_method.mobile"); | ||
| String rendering_driver = GLOBAL_GET("rendering/rendering_device/driver.android"); | ||
| return (rendering_method == "forward_plus" || rendering_method == "mobile") && rendering_driver == "vulkan"; |
There was a problem hiding this comment.
@akien-mga Now that auto has been removed, this change only affects the style, while the logic remains the same as before. It would be better to revert it.
Simpler alternative to #103026 which avoids breaking compatibility.
Instead of introducing a new
autodefault value, we ensure that allsupported drivers are registered regardless of the editor's host platform,
and that the defaults are the intended ones.
This solves the following issues:
default to Vulkan if exported from Linux, Windows, or Android editors.
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 leastconsistent 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).
Before (Linux editor):
After (Linux editor):