Allow default case at the top of a switch scope in shaders
#103177
Conversation
|
This is changing code that hasn't changed in 3 years. And the error has been around since switch statements were added 6 years ago (in #31596). Therefore, I don't think it is responsible for the change in behaviour between 4.3 and 4.4. I definitely wouldn't remove an error check that has been around for 3 years at this point in the 4.x dev cycle without a very compelling reason. |
The code for the error has been there for a long time, but for some unknown reason the error didn't hit in previous versions. And default case is allowed at the top of switch scopes in GLSL. Right now it is a breaking change in behavior between 4.3 and 4.4 to the user, which I'm trying to fix. But yeah, I get your concern for potential side effects. |
AThousandShips
changed the title
|
Also, one good reason why this change is needed is that people who rely on large external GLSL libraries might have to make a lot of changes to the libraries since the compiler is not fully compatible with GLSL. This is the problem I have with my own project. |
The fix should target the code that changed instead of deleting the error check altogether. |
|
I found that with this fix, if you have: void test() {
switch (0) {
default:
break;
default:
break;
case 0:
break;
};
}We get an error as expected, but if we have: void test() {
switch (0) {
default:
break;
case 0:
break;
default:
break;
};
}We don't get an error and the editor hangs, |
Now this is fixed. |
editor/translations/editor/ar.po
Outdated
| msgid "Cases must be defined before default case." | ||
| msgstr "يجب تحديد الحالات قبل الحالة الافتراضية." | ||
|
|
There was a problem hiding this comment.
It's correct that those translations aren't needed anymore, but we don't modify translations directly in this repo, as those are just copies of what's being translated on Weblate: https://hosted.weblate.org/projects/godot-engine/godot/
They will be removed automatically next time I sync the translations, so for this commit you can undo the .po changes to reduce the diff.
|
Looks good to me! Could you squash the commits? See PR workflow for instructions. If you're not familiar with that workflow or find it overwhelming, just tell me and I can squash the commits myself. |
akien-mga
changed the title
default case at the top of a switch scope in shaders
Revert "Removed translations of unused error message" This reverts commit 6dbc75e. Variable name change Detecting multiple default cases in shaders Removed translations of unused error message Allowing default case at top of scope in switch statement in shaders
Murrent
force-pushed
the
shader_default_at_top
branch
from
41c3393 to
4f46ecc
Compare
|
I think I got it, never squashed before 🤞 |
There was a problem hiding this comment.
This looks good to me to merge for 4.5, as I agree it makes sense to reduce differences with GLSL for portability, and the fix seems simple - actually @Chaosus independently came up with the same change in #103288.
It's not critical for 4.4 though as pointed out in #103174 (comment), since in this release candidate phase we only merge fixes to showstopping regressions, and this one is more a bug that morphed into expected behavior.
|
Thanks! Congratulations on your first merged contribution! 🎉 |
Fixes #103174