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

Avoid switch statement in particles shader to workaround shader compiler crash on Apple silicon devices #91436

Merged
merged 1 commit into from
May 2, 2024

Conversation

clayjohn
Copy link
Member

@clayjohn clayjohn commented May 2, 2024

Partial cherrypick of #88816 for 4.2 and 4.1. This is the minimum needed to avoid the crash on Apple silicon devices (#72469)

I tested and confirmed that the issue is specifically using a switch statement where the first condition does nothing ¯\_(ツ)_/¯

@clayjohn clayjohn added bug topic:rendering crash cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release labels May 2, 2024
@clayjohn clayjohn added this to the 4.x milestone May 2, 2024
@clayjohn clayjohn requested a review from a team as a code owner May 2, 2024 02:29
@YeldhamDev
Copy link
Member

Shouldn't there be a comment here (and on master as well) informing why it needs to be done this way?

@clayjohn
Copy link
Member Author

clayjohn commented May 2, 2024

Shouldn't there be a comment here (and on master as well) informing why it needs to be done this way?

Sure!

In practice I'm not sure it matters since we would reject any PR that just changes an if statement to a switch

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! I was wondering if we should backport those PRs to 4.2 and 4.1 indeed, a minimal pick like this for Apple devices only sounds great.

@akien-mga akien-mga modified the milestones: 4.x, 4.2 May 2, 2024
@akien-mga akien-mga merged commit 604ad4e into godotengine:4.2 May 2, 2024
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@Zireael07
Copy link
Contributor

"We would reject any PR that contain a switch" only works as long as you know/remember what and why. A comment is a much more futureproof solution

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release crash topic:rendering
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants