godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
91.47k stars 21.26k forks source link

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

Open geekley opened 1 month ago

geekley commented 1 month ago

Tested versions

System information

Godot v4.3.stable (77dcf97d8) - 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 commented 1 month ago

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 commented 1 month ago

@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: https://github.com/godotengine/godot/issues/96253#issuecomment-2333734897

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).