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

Disable Metal and Vulkan renderers in simulator builds. Remove simulator support from editor/exporter. #102179

Merged
merged 1 commit into from
Feb 3, 2025

Conversation

bruvzg
Copy link
Member

@bruvzg bruvzg commented Jan 30, 2025

Cleaner alternative to #102178

  • Fixes simulator build by disabling unsupported Vulkan and Metal renderers.
  • Removes explicit support for iOS simulator from editor/exporter (one click deploy, library conversion).
  • Bumps default export iOS version to 14.0 (required for Metal, which is default).

TODO:

  • Update build docs to remove simulator from generic build instructions and add note how to use it and it's limitations.

@bruvzg bruvzg requested a review from a team as a code owner January 30, 2025 09:32
Comment on lines -297 to +296
// TODO(sgc): set to iOS 14.0 for Metal
r_options->push_back(ExportOption(PropertyInfo(Variant::STRING, "application/min_ios_version"), "12.0"));
r_options->push_back(ExportOption(PropertyInfo(Variant::STRING, "application/min_ios_version"), "14.0"));
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to give the option to still compile iOS templates without Metal and thus target down to 12.0? Or do we consider that 14.0 is a fine min version either way?

Technically I guess it's still possible as 14.0 is just a default here so users could change it manually to 12.0 in that (advanced) use case?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's only default value, you can switch it to 12.0, and it will work with the same template (or custom template) if renderer is set to OpenGL in project settings. But since default is Metal, there's no reason to keep it at 12.0, it's just an annoyance since you have to switch it to 14.0 for each new project.

Copy link
Member Author

Choose a reason for hiding this comment

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

Library itself is still built with 12.0 as min. version. We do not need to bump it as long as Xcode/SDK have support for it.

Copy link
Member

@akien-mga akien-mga 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 to me.

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. As noted by others, the simulator is not generally useful for testing games, as it has a lot of limitations.

@Repiteo Repiteo merged commit dfedfe7 into godotengine:master Feb 3, 2025
19 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Feb 3, 2025

Thanks!

akien-mga added a commit to akien-mga/godot that referenced this pull request Feb 7, 2025
Not my finest work, but without that code removed in godotengine#102179, mingw-gcc 14.2.1 on Fedora 41
(but also confirmed with versions on macOS and WSL) crashes when linking with LTO.

We need to dig deeper to understand the bug, report it upstream and work it around in a
cleaner way. But for now this unblocks building Windows binaries with LTO, and should be
harmless.
akien-mga added a commit to akien-mga/godot that referenced this pull request Feb 7, 2025
Not my finest work, but without that code removed in godotengine#102179, mingw-gcc 14.2.1 on Fedora 41
(but also confirmed with versions on macOS and WSL) crashes when linking with LTO.

We need to dig deeper to understand the bug, report it upstream and work it around in a
cleaner way. But for now this unblocks building Windows binaries with LTO, and should be
harmless.
akien-mga added a commit to akien-mga/godot that referenced this pull request Feb 7, 2025
Not my finest work, but without that code removed in godotengine#102179, mingw-gcc 14.2.1 on Fedora 41
(but also confirmed with versions on macOS and WSL) crashes when linking with LTO.

We need to dig deeper to understand the bug, report it upstream and work it around in a
cleaner way. But for now this unblocks building Windows binaries with LTO, and should be
harmless.
adamscott pushed a commit to adamscott/godot that referenced this pull request Feb 8, 2025
Not my finest work, but without that code removed in godotengine#102179, mingw-gcc 14.2.1 on Fedora 41
(but also confirmed with versions on macOS and WSL) crashes when linking with LTO.

We need to dig deeper to understand the bug, report it upstream and work it around in a
cleaner way. But for now this unblocks building Windows binaries with LTO, and should be
harmless.
dugramen pushed a commit to dugramen/godot that referenced this pull request Feb 8, 2025
Not my finest work, but without that code removed in godotengine#102179, mingw-gcc 14.2.1 on Fedora 41
(but also confirmed with versions on macOS and WSL) crashes when linking with LTO.

We need to dig deeper to understand the bug, report it upstream and work it around in a
cleaner way. But for now this unblocks building Windows binaries with LTO, and should be
harmless.
rddi-8 pushed a commit to rddi-8/godot-custom-features that referenced this pull request Mar 2, 2025
Not my finest work, but without that code removed in godotengine#102179, mingw-gcc 14.2.1 on Fedora 41
(but also confirmed with versions on macOS and WSL) crashes when linking with LTO.

We need to dig deeper to understand the bug, report it upstream and work it around in a
cleaner way. But for now this unblocks building Windows binaries with LTO, and should be
harmless.
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.

4 participants