Fix _shader_compile_spirv_from_source#52278
Fix _shader_compile_spirv_from_source#52278KPGabor1999 wants to merge 1 commit intogodotengine:masterfrom
Conversation
There was a problem hiding this comment.
Please amend your pull request to follow the code style guidelines and C++ usage guidelines. We don't use auto in Godot's codebase.
Also, is the lambda really necessary here? We generally keep our use of lambdas to a minimum.
|
Okay, fixed both issues. |
|
Okay, fixed all the spacing issues. |
|
Hi @Calinou, do you need anything else to be changed in the PR? Thank you! |
|
Hi @Calinou, why do the static checks fail? I'm spacing that if statement exactly as requested. |
|
Okay, now everything should be fine. |
|
After doing style fixups, please squash the commits together as for such a bugfix PR a single commit should be good once merged: PR workflow. |
KPGabor1999
force-pushed
the
fix_shader_compile_spirv_from_source
branch
from
1964736 to
42885c4
Compare
|
It seems like you squashed your work together with other unrelated commits (see merge conflicts and +709 line additions). You can get back to the previous state using Once fixed up, the resulting commit should also be given a meaningful description (currently it's "Squashing all the previous commits together.", which doesn't describe what the commit does, only what you did :)). The above can be a bit tricky if you're not familiar with these Git commands, so if preferred, it's also fine to redo your changes in a clean new branch to make a new PR. You can also set up our pre-commit hooks to ensure that you get style issues fixed right away: https://docs.godotengine.org/en/latest/community/contributing/code_style_guidelines.html |
Now it handles the compute pipeline and the render pipeline separately.
KPGabor1999
force-pushed
the
fix_shader_compile_spirv_from_source
branch
from
42885c4 to
5dda9a5
Compare
|
Hi @akien-mga, we updated the PR as requested. Could you please take a look? Thank you! |
|
This looks good style and Git wise 👍 I'll try to get the @godotengine/rendering team to review and approve the code changes. |
| bytecode.instantiate(); | ||
| for (int i = 0; i < RD::SHADER_STAGE_MAX; i++) { | ||
| String error; | ||
| String error; |
There was a problem hiding this comment.
It is mandated in the newer GLSL that you specify #version so shaders will probably never be empty if they exist. Seems like its easier to just check for empty shader and continue if nothing found.
There was a problem hiding this comment.
But we already check for whether the current stage source code != String() before we do anything. Why is that wrong?
|
Sorry for the huge delay. Is this still relevant in the current master branch? There's no linked bug report / steps to reproduce so it's difficult to assess what this aimed to fix exactly. |
6 participants
This pull request fixes _shader_compile_spirv_from_source. Compute shaders are now being compiled properly. Previously when trying to compile compute shaders, other stages were also being compiled with it causing errors.