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

Default to non-threaded export setting for the web #91623

Merged
merged 1 commit into from
Jun 19, 2024

Conversation

adamscott
Copy link
Member

@adamscott adamscott commented May 6, 2024

As #91382 will fix most of the audio issues when exporting single-threaded, I think we should default to this setting for maximum compatibility.

It would mean, though, that existing exports would go from threaded to non-threaded without changing the default value. See #91623 (comment)

@adamscott adamscott added this to the 4.3 milestone May 6, 2024
@adamscott adamscott marked this pull request as ready for review May 6, 2024 16:23
@adamscott adamscott requested a review from a team as a code owner May 6, 2024 16:23
@Calinou
Copy link
Member

Calinou commented May 6, 2024

This makes sense, considering most web projects won't benefit that much from having threads enabled once #91382 is merged. Threads enabled usually makes the most sense for projects that need to offload their own computation to threads (which is a relatively low number of web projects I can think of).

It would mean, though, that existing exports would go from threaded to non-threaded without changing the default value.

Default values are always stored in export presets, so this won't break existing export presets.

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.

Makes sense to me. But should we wait for #91382 to be merged first before merging this too?

@adamscott
Copy link
Member Author

Makes sense to me. But should we wait for #91382 to be merged first before merging this too?

Yes, we can.

@akien-mga akien-mga merged commit 34b9eef into godotengine:master Jun 19, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@Faless
Copy link
Collaborator

Faless commented Jun 19, 2024

Note: We need to also change the default for godot-cpp, which requires some work because we don't yet support platform defaults there.

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