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

Fix shader crash when the comma used in for loop as a trailing #95350

Merged

Conversation

Chaosus
Copy link
Member

@Chaosus Chaosus commented Aug 10, 2024

Prevents shader compiler crash (not an actual crash) when using incorrect trailing commas in for loop expression:

for(int i = 0; i < 0, ; i++, )

@Chaosus Chaosus force-pushed the shader_fix_for_loop_comma_crash branch from 7407577 to d74749f Compare August 10, 2024 08:23
@Chaosus Chaosus added this to the 4.4 milestone Aug 10, 2024
@geekley
Copy link

geekley commented Aug 12, 2024

Thanks @Chaosus . Is this only for dealing with trailing commas (to show text editor error instead of crash)?

for (int i = 0, j = 0; i < 9 && j < 9; i++, j++)
Is comma on 3rd (continuation) part still valid?

for (int i = 0, j = 0; (i = f(i)), i < 9; i++, j++)
Is comma on 2nd (condition) part now made invalid or does it have comma operator semantics (fixing to allow non-bool before the comma)? Should I still open an issue for this one? Done at #95451

@Chaosus
Copy link
Member Author

Chaosus commented Aug 13, 2024

Is comma on 3rd (continuation) part still valid?

Yes

for (int i = 0, j = 0; (i = f(i)), i < 9; i++, j++)

This is not a valid because only boolean expressions are allowed on middle place. That is allowed expression in GLSL, though.

@geekley
Copy link

geekley commented Aug 15, 2024

This is not a valid because only boolean expressions are allowed on middle place. That is allowed expression in GLSL, though.

Exactly. It's valid GLSL because only the last comma part needs to be a boolean (if you allow commas here like in GLSL).
If all parts are required to be a boolean, it makes it seem as if the comma means something else, like e.g. an AND or OR operator (which do get passed a boolean). Comma operator doesn't need compatible types between its parts.

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.

Tested locally, it works as expected.

image

In master, I get a core shader compilation error and the editor freezes instead.

@akien-mga akien-mga added the cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release label Aug 16, 2024
@akien-mga akien-mga merged commit 9c77f57 into godotengine:master Aug 16, 2024
18 checks passed
@Chaosus Chaosus deleted the shader_fix_for_loop_comma_crash branch August 16, 2024 12:42
@akien-mga
Copy link
Member

Thanks!

@akien-mga
Copy link
Member

Cherry-picked for 4.3.1.

@akien-mga akien-mga removed the cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release label Sep 16, 2024
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