Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add Metal support for macOS (arm64) and iOS #88199
Add Metal support for macOS (arm64) and iOS #88199
Changes from all commits
2d01655
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should consider enabling
metal
by default sooner than later IMO, as it should probably be our preferred RenderingDevice implementation for macOS and iOS.d3d12
above defaults to False because it adds some build dependencies and we also used to have to ship a proprietary DLL for it (now solved), but I guess for Metal that may not be a concern, all deps should be part of the macOS and iOS SDKs?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm given later considerations on the lack of support for
metal
on non-Apple GPUs, this should probably stay opt-in for now indeed.I wonder whether it will work for official
universal
builds witharm64
andx86_64
lipo'd together?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@akien-mga yes, it should work with lipo'd builds, as they have no dependencies on each other.
Perhaps we can enable dynamically at build time, such that ARM builds have Metal by default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO ARM builds should have metal onby default while X86 builds should have it off by default
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree – best way to start getting it tested 👍🏻
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added this to be consistent with
RENDERING_DRIVER_D3D12
, but I don't see how this is used anywhere.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These don't seem used anywhere indeed, we could flag this enum as deprecated.
It used to be used in 3.x for
OS::get_current_video_driver()
(7a79eee), and was refactored for 4.x in f82deaa, but the API that used it no longer exists. I'm not sure what replaced it in 4.x, CC @Calinou.We also have this piece of dead code to remove eventually:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We seem to have:
but not exposed to scripting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a PR to expose these: #85430
It's awaiting review (specifically about moving it from OS to Engine). It's not exposed currently, so moving it doesn't break compatibility.