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

Implement Fallback to Vulkan for MoltenVK #102341

Merged

Conversation

SheepYhangCN
Copy link
Contributor

@SheepYhangCN SheepYhangCN commented Feb 2, 2025

Implemented fallback_to_vulkan in macOS

@SheepYhangCN SheepYhangCN requested review from a team as code owners February 2, 2025 21:31
@SheepYhangCN SheepYhangCN changed the title Implemented Fallback to Vulkan for MoltenVK Implement Fallback to Vulkan for MoltenVK Feb 2, 2025
@AThousandShips AThousandShips added this to the 4.x milestone Feb 3, 2025
@AThousandShips AThousandShips requested a review from a team February 3, 2025 08:55
Copy link
Contributor

@stuartcarnie stuartcarnie left a comment

Choose a reason for hiding this comment

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

Looks good! One required change to resolve a memory leak.

rendering_context = nullptr;
bool failed = true;
#if defined(VULKAN_ENABLED)
bool fallback_to_vulkan = GLOBAL_GET("rendering/rendering_device/fallback_to_vulkan");
Copy link
Member

Choose a reason for hiding this comment

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

Are there any iOS devices that support MoltenVK, but not our Metal backend?

Copy link
Member

@bruvzg bruvzg Feb 3, 2025

Choose a reason for hiding this comment

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

Probably no, it's the opposite if I remember, A11 for Metal and A12 for Vulkan. And on macOS, every Apple Silicon device support both MoltenVK and Metal, and we do not have x86 Metal support.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's correct – I did additional work in #99820 ensures A11 Bionic support for Metal, which is better than we had for MoltenVK.

Copy link
Member

Choose a reason for hiding this comment

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

So the only use case is fallback from Vulkan to Metal on A11, assuming it will work (most likely MotenVK will crash instead).

Copy link
Contributor

Choose a reason for hiding this comment

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

@bruvzg that's a good point! We actually want the reverse; fallback to Metal from Vulkan, although I don't know if MoltenVK fails if the device is below A12?

Copy link
Member

Choose a reason for hiding this comment

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

I do not have anything to test it on, but from previous experience it's either crash on init or will start normally and fail during shader compilation. I do not think I have ever seen rendering_context creation fail in any conditions other than completely missing support for specific rendering backend (like no Vulkan on ARM/Windows).

@stuartcarnie
Copy link
Contributor

stuartcarnie commented Feb 3, 2025

Presumably we still want the feature; however, the code could be implemented a little differently to match the behaviour of our implementation.

  • Metal for iOS has a lower device requirement than MoltenVK, so if Metal fails, MoltenVK will fail too; therefore, the feature should not be necessary for iOS
  • Metal is not available or compiled into x86_64 binaries, so it wouldn't even try to initialise a Metal driver; therefore, fallback should always occur.
  • All Apple Silicon Macs support Metal, so it should never need to fallback to Vulkan, meaning the feature is not required for Apple Silicon (arm64 builds)

In other words,

  • Remove implementation for iOS
  • Change implementation for macOS to something like:
rendering_driver = p_rendering_driver;

#if defined(RD_ENABLED)
#if defined(VULKAN_ENABLED)
#if defined(__x86_64__)
	bool fallback_to_vulkan = GLOBAL_GET("rendering/rendering_device/fallback_to_vulkan");
	// Metal rendering driver not available on Intel
	if (fallback_to_vulkan && rendering_driver == "metal") {
		rendering_driver = "vulkan";
		OS::get_singleton()->set_current_rendering_driver_name(rendering_driver);
	}
#endif

	if (rendering_driver == "vulkan") {
...

@SheepYhangCN
Copy link
Contributor Author

All fixed I think

Copy link
Contributor

@stuartcarnie stuartcarnie left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Code looks good to me.

@Calinou
Copy link
Member

Calinou commented Feb 8, 2025

I'm wondering if we should get this in for 4.4, since the Metal rendering driver was made the default already. Otherwise, projects will stop working on Intel Macs by default if they use Forward+ or Mobile.

@stuartcarnie
Copy link
Contributor

@Calinou I agree – we don't want to break or alienate users on Intel hardware.

@bruvzg
Copy link
Member

bruvzg commented Feb 10, 2025

For the reference, this change should not be necessary in normal conditions:

  • If project is edited/exported from Apple Silicon editor, it won't save metal in the project.godot since it default value and default values are omitted, so x86_64 executable will use its default (vulkan) without any additional checks.
  • If you try to force Metal render on x86_64 with command line argument (--rendering-driver metal), it will fail during argument validation in the Main and never get to this code.
  • But it is possible to manually edit project.godot, and explicitly specify metal, and this code will work as expected.

@akien-mga akien-mga force-pushed the rendering-driver-fallback-moltenvk branch from e9f641b to c0eec97 Compare February 10, 2025 12:24
@akien-mga
Copy link
Member

I squashed the commits and clarified in the docs that this is only for macOS x86_64.

@akien-mga akien-mga modified the milestones: 4.x, 4.4 Feb 10, 2025
@Repiteo Repiteo merged commit 4b644ed into godotengine:master Feb 10, 2025
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Feb 10, 2025

Thanks!

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.

8 participants