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

GDShader "invalid pragma directive" error on unrecognized pragma should be dropped #97738

Open
geekley opened this issue Oct 2, 2024 · 2 comments

Comments

@geekley
Copy link

geekley commented Oct 2, 2024

Tested versions

  • Reproducible in: v4.3.stable.flathub [77dcf97]

System information

Godot v4.3.stable (77dcf97) - Freedesktop SDK 23.08 (Flatpak runtime) - X11 - Vulkan (Forward+) - integrated Intel(R) HD Graphics 5500 (BDW GT2) - Intel(R) Core(TM) i5-5300U CPU @ 2.30GHz (4 Threads)

Issue description

The GLSL ES 3.00 spec says on page 15:

#pragma allows implementation dependent compiler control. Tokens following #pragma are not subject to preprocessor macro expansion. If an implementation does not recognize the tokens following #pragma, then it will ignore that pragma.

I believe the whole point of #pragma is to allow forward-compatibility, by letting any unrecognized instructions be ignored. So I believe showing an error like "Invalid pragma directive" defeats the whole purpose, no? Invalid pragma lines should be ignored like how it is in the spec.

In fact, both C and GLSL preprocessors don't complain at all if you add a custom pragma.

The spec also says:

The following pragmas are defined as part of the language.
#pragma STDGL
The STDGL pragma is used to reserve pragmas for use by this and future revisions of the language. No
implementation may use a pragma whose first token is STDGL.

So, I think it would also be beneficial to add something similar for mandatory directives that should raise error if unrecognized. For example, you could exceptionally raise the error if it starts with #pragma required:

shader_type spatial;
#pragma required command_that_must_be_supported
#pragma optional_command

Note that both C and GLSL preprocessors leave the entire #pragma lines (after joining line continuations) in their output as well (unlike other directives such as #define, etc. which are stripped out). Presumably, this could be to allow the compilation after the preprocessor to be affected by pragmas as well. So the ignoring behavior should be implemented in both the preprocessor and the lexer/parser after it, to account for potential usages that may extend beyond the preprocessor.

Steps to reproduce

shader_type spatial;
#pragma some_custom "etc" 3.14 // unrecognized pragma should be ignored instead of raising error
cd /tmp
echo '#pragma some_custom "etc" 3.14' | tee a.c > a.frag
echo '=== GLSL ==='; glslang -E a.frag; echo '=== C ==='; gcc -E a.c

Of course, the error should still show for recognized pragma lines that are malformed:

#pragma disable_preprocessor blah blah // should still raise the error

Minimal reproduction project (MRP)

N/A

@dustdfg
Copy link
Contributor

dustdfg commented Oct 2, 2024

It is mostly just a game of words but godot doesn't use GLSL ES 3.0. It uses it's own language: Godot shading language... Which is yeah very similar to glsl

Anyway effectively godot doesn't have any "useful" pragmas and uses it only for disabling preprocessor so I agree that throwing error isn't a good approach.

@geekley
Copy link
Author

geekley commented Oct 2, 2024

@dustdfg Yes, but GDShader is based on GLSL ES 3.0, and it's been decided that the preprocessor behavior should aim to be GLSL-compatible where possible: #96253 (comment)

I brought up this Issue in the 2024-09-03 Rendering Meeting, these are the key points:

  • Aim for glsl-like behavior, so that shaders can be ported over very easily [...]

And this is also the behavior in C preprocessor (GCC).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants