-
-
Notifications
You must be signed in to change notification settings - Fork 98
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
Make Godot GLSL shader files interoperable with standard GLSL tools #11274
Comments
From RocketChat, I learned that render pipeline shaders include several other godot-specific preprocessor-ish statements that aren't valid GLSL, used for injecting code from user visual shaders in the right places. Looking through the Godot source tree, I found the following additional statements:
Their purpose is pretty self-explanatory. They are processed separately from the other macros, in These have the same issue as the macros in the original proposal: GLSL preprocessor macros are a closed set, and compliant parsers will reject files containing unknown macros. So, these also break tooling like We can fix these in the same way, by turning them into compiler pragmas. Unlike macros names, pragmas are an open set and compilers ignore pragmas they don't understand. A possible syntax: We can also break up this proposal into two smaller pieces, for ease of reviewing the code: first, focus only on compute shader ergonomics, which means only addressing the original |
I did notice is that an empty uniform #ifdef MATERIAL_UNIFORMS_USED
/* clang-format off */
layout(set = 1, binding = 0, std140) uniform MaterialUniforms {
#pragma godot_user_material_uniforms
} material;
/* clang-format on */
#endif For example, in CLion, with the glsl plugin: I think that is a lint, vs the other being invalid code. |
Hmm, indeed, empty structs are forbidden in GLSL. That's annoying :) That pattern also breaks with the current file format, right? That inner section would be Do you happen to know what compiler/linter CLion is using? I wonder if it has clang-format-style markers to disable a lint check. Aside from silencing the linter in this instance, the only thing I can think of is either horrible or very clever 😂 . We could teach the preprocessor an additional "placeholder struct member" syntax, with the same effect as the pragma (replace entire line with appropriate value), but in a syntax that's also valid GLSL in the template. Something like: layout(set=1, binding=0, std140) uniform MaterialUniforms {
int __GODOT_MATERIAL_UNIFORMS___;
} material; Maybe that's taking things too far, I don't know 😅 |
We discussed this in the rendering meeting. TL;DR 👍 to implement the pragma/ifdef part of the proposal; drop the file extension stuff for now. I'll tidy up commits and send a PR as my free time permits. More detailed notes follow. Pragmas/ifdef instead of custom macrosNo objections heard, a couple of echoes of "oh yeah that annoys me in vscode too" 🎉 . I heard reinforcement that we must not break compatibility for existing code. I agree and confirmed that my implementation conforms with this. Shader stages based on file extensionThere is disagreement on user ergonomics: this lets you share shader code from other tools/engines... But it also means that if you miss "oh btw save this code as .compute.glsl or it won't work" in the instructions, the shader mysteriously doesn't work and you end up with sad users. It might be a nice workflow for advanced users, but a lot of people just paste a shader they like from godotshaders.com, and that should "just work", always. For that workflow, the shader type(s) needs to stay inside the source code. We also can't take shortcuts like "assume a file with no stage declaration is a compute shader". Right now compute is the only "accessible" GLSL stage, but the WIP shader templates means that very soon, it will be more common to also paste vertex/fragment GLSL shaders into projects, and that also needs to just work. There might be a way to do this that balances the different UX constraints, but it should be a dedicated proposal because it's not trivial or obvious what to do. Question for future proposal: if we support per-type filenames, does Khronos Group define a convention for this? If yes, a godot proposal to do this should account for that convention. Code injection macros in render shadersWe didn't talk about this in the meeting. The general guidance I got: break things into smaller standalone parts if possible, to make it easier to review. So, I'm going to defer dealing with the code injection macros, and focus on the ergonomics of compute shaders for now. Changing the code injection logic might also clash with the WIP templates changes, so I have to go learn about that before I can safely propose a change there. |
Can we replace "version(s)" with "variation(s)" (just on the new format)? I feel like if we're defining a macro like I'd expect that to be used like this, as Unity does something similar in C#: #ifdef GODOT_VERSION_4_3_OR_LATER
// and/or even better:
#if GODOT_VERSION_MAJOR == 4 && GODOT_VERSION_MINOR >= 3 IMO it would be better to change the pragma to #version 450
#pragma godot_stages(vertex, fragment)
#pragma godot_shader_variations(STANDARD, RAY_MARCHING)
#if GODOT_STAGE_VERTEX
void main() {
do_common();
#if GODOT_SHADER_VARIATION_STANDARD
fast_vertex_process();
#elif GODOT_SHADER_VARIATION_RAY_MARCHING
cast_lots_of_rays();
make_super_pretty();
#endif
cleanup_common();
}
#endif // GODOT_STAGE_VERTEX |
Is it possible to disallow the typo only on the new parser? Compatibility is needed on the old parser of course, but on the new one this would just cause inconsistency between shader files IMHO. I think it's not really necessary to allow it, unless there's a reason to? |
It's also worth considering how those macros should be defined. Instead of defining them or not, it's better to always define the ones declared in the pragmas as either
|
If you're going that route, would this make more sense, since the macro can expand to arbitrary fields of arbitrary types? layout(set=1, binding=0, std140) uniform MaterialUniforms {
__GODOT_MATERIAL_UNIFORMS___
} material; IDE support could be configured the same way as the stages then, by manually defining it to anything so the linter doesnt complain, e.g.: (Sorry if I'm saying something stupid, as I don't understand anything about this) |
I tried the new format of this proposal, but it still couldn't make the VSCode GLSL lint extension work out of the box, because macros such as GODOT_STAGR_VERTEX are not defined, so I couldn't get compilation hints. Or I can only define one of them, and can't get the compilation hints for multiple stages simultaneously. To truly get IDE linters support for the existing godot GLSL as well as the format of this proposal, we can achieve it in https://github.com/godotengine/godot-vscode-plugin or separate extension. If compatibility is not taken into account, one good thing about the new format is that we can skip parsing the GLSL and define the macro and directly compile the same piece of GLSL for different stages. However, when considering compatibility, this proposal increases the complexity of parsing. I'm afraid it's kind of getting more trouble than it's worth. |
The problem with that approach is that there are more IDEs than vscode, and teaching all of them about a bespoke file format is a tall order. On the other hand, giving them standard GLSL and only requiring symbol definition through a standard GLSL compiler mechanism should be a baseline feature for GLSL support (or if it isn't, it would be a useful thing to support for all GLSL, not just Godot). I think realistically, telling every IDE "please spend time to support custom Godot formats/languages" will result in "no thanks", whereas "please improve your GLSL support" is less effort, and has more universal benefit. |
Another alternative is layout(set=1, binding=0, std140) uniform MaterialUniforms {
#ifdef GODOT_COMPILER
__GODOT_MATERIAL_UNIFORMS___
#else
int __tmp;
#endif
} material; And when compiling in Godot, the The original would work, but would fail to parse in IDEs, whereas this would only enable the block if Yet another alternative is at the top of your file #ifndef __GODOT_MATERIAL_UNIFORMS___
#define __GODOT_MATERIAL_UNIFORMS___ int __tmp;
#endif Note For this approach to work, it assumes that the code replacement happens before the preprocessor for Godot's GLSL compiler is run. |
Perhaps I didn't express myself clearly. I mean I cannot get diagnosis within IDE. Many linters use To get the diagnostic, I need to run this from command line: glslangValidator -S vert -DGODOT_STAGE_VERTEX -l ./my_shader.glsl
glslangValidator -S frag -DGODOT_STAGE_FRAGMENT -l ./my_shader.glsl It still requires further support from IDE plugins to generate diagnosis. For syntax highlighting or code formatting, some plugins already ignore preprocessor directives and work well. |
@beicause I would suggest you open an issue in their extension repo asking to allow configuring "glsllint.glslangValidatorArgs": {
"*.vert.glsl": ["-DGODOT_STAGE_VERTEX"]`,
"*.frag.glsl": ["-DGODOT_STAGE_FRAGMENT"], // etc.
} That's a simple change to the implementation that would solve the issue without being specific to Godot at all. And what I think he meant by:
|
Note: I am volunteering to ship this myself, if this proposal is acceptable to Godot devs. I have a working prototype already, and I volunteer to get it merged and update documentation.
Describe the project you are working on
I'm experimenting with procedural generation algorithms. For many of them I use compute shaders to speed up the generation process.
Describe the problem or limitation you are having in your project
Godot uses a non-standard GLSL format. Conceptually, the format stores several independent shaders in a single file, separated by
#[shader_stage_name]
markers (one ofvertex
,fragment
,tesselation_control
,tesselation_evaluation
,compute
). A special#[versions]
section also exists, which allows the user to define "variants" of shader with C preprocessor directives. This format is used internally by Godot renderers for the shader code they need, and it's also the user facing format for compute shaders. In the case of compute shaders, the format must be obeyed but only#[compute]
and#[versions]
sections can appear in the file.This format is specific to Godot, which means that standard GLSL tools don't function well on Godot shader files. Take for example the trivial compute shader from the manual:
If you look at the GLSL specification, this file is invalid in 2 ways:
#[compute]
is not a valid GLSL preprocessor directive. Section 3.3 of the spec lists the valid directives, and explicitly says that directives not listed result in a compiler error. Empirically, all GLSL tools (linters, IDE extensions, compilers) I've tried do indeed choke on this directive.#[compute]
must come before#version
, because the Godot format requires all code to be inside a section. However, section 3.3 of the spec says that the#version
directive must appear before anything else, except comments and empty lines.This only gets worse if you add a
#[versions]
section, because the contents of the versions section is not valid GLSL syntax at all.This makes it hard to work with compute shaders in Godot (and, I presume, also rendering shaders, but I don't know). Basic syntax highlighting works, but other IDE functionality is degraded: there is no intelligent complete/navigation because GLSL language servers can't parse the file, there is a constant sea of red underlines from the IDE trying and failing to parse things, and so on. It's not a fatal problem, but it makes shader hacking in Godot quite unpleasant.
This also causes an adoption problem: you can't just grab some shader code off the internet, or from a GLSL shader library, and drop it in your Godot project. Again this is not a huge barrier since you usually just have to add
#[compute]
at the top, but it's a less smooth experience than in other engines. It also forces new developers to learn GLSL without good IDE support, if they want to use Godot.Describe the feature / enhancement and how it helps to overcome the problem or limitation
The discussion in #10880 sketches a solution to this (which is also detailed below) : make "Godot-style GLSL" compatible with regular GLSL. This means replacing the
#[section]
marker syntax with a combination of compiler#pragma
directives and preprocessor#ifdef
s. The result is a shader file that is standard GLSL, with additional Godot-specific hints to support multi-stage and multi-version shaders.In addition (although this can be separated into a different proposal), add support for specifying the type of simple shaders in the filename, e.g.
myshader.compute.glsl
gets compiled as a compute shader with no extra work needed.Obviously, we cannot break compatibility with existing Godot GLSL shaders. Fortunately, this proposal is completely backwards compatible: all existing shader code keeps working exactly the same, the shader loading code can trivially detect the format per-file and adapt.
Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams
I got carried away a little bit while prototyping, and implemented this proposal already 😅 , if you prefer to just read code: godotengine/godot@master...danderson:godot:push-xvurqvqkplyr . I broke down the change into small incremental commits, so I recommend reading one commit at a time, for smaller diffs. Please consider this just a prototype to show that it can be done with low pain, and to try the proposed format interactively. I'm happy to adjust the implementation based on feedback, of course!
The new logic is entirely in RDShaderFile, which implements the current Godot-style GLSL format. Conceptually, RDShaderFile acts as a pre-preprocessor stage that figures out what shader stages and variants are present, assembles the correct source code for each one, and invokes the RD's SPIR-V compiler for each.
I added a second parsing codepath in RDShaderFile, which processes the new format (described below). The top-level
parse_versions_from_text
method uses a quick heuristic to figure out which of the 2 formats it's looking at, and delegates to either the existing format parser, or the new one. Additionally I factored out a few common helpers that both parsers use, to reduce duplication and improve readability.Everything else stays the same. Aside from a tiny API expansion to support filename-based stage identification in the editor, the rest of Godot is unaware of the new format. The editor resource for GLSL shaders just hands the source text to RDShaderFile, and gets back a pile of compiled SPIR-V and possibly compiler errors. Similarly the GLSL compiler itself doesn't need any changes, it just receives slightly different source text from RDShaderFile.
New file format
The new format is standard, spec-compliant GLSL. Two compiler pragmas tell Godot the desired shader stages and variants. For each {stage, variant}, the entire file is sent to the GLSL compiler, with two additional preprocessor #defines that specify the stage and variant. The shader code can then use standard #ifdefs to specialize the code as it likes.
Here is the compute shader from the Godot manual in this new format (compare to old one shown earlier):
This shader is trivial since it only has one stage and no variants, so all that's required is to declare the desired stage with the
godot_shader_stages
pragma. Alternatively, if the file is saved asblahblah.compute.glsl
, the stage is deduced from the file extension, and the#pragma
is not required.Here's a skeleton of a more complex shader file, which has both vertex and fragment shaders as well as 2 versions:
Of course this is more complex, and standard GLSL tools will need some manual configuration to set the right GODOT_{STAGE,VERSION}_* macros. However, this is fairly easy to do, and is only necessary for these more complex "multi-shaders". The one-stage, one-version compute shader above can be processed by standard tools out of the box. This also leaves open the possibility of tools adding explicit Godot support: the GLSL spec says that unknown #pragmas must be ignored, but in future third party tools could learn the Godot pragmas, and do the right thing even for complex shaders. This is not required, just a possibility for the future.
For a more realistic example, I picked a random complex shader from the Godot source tree (
lm_blendseams.glsl
, a vertex+fragment shader with 2 variants) and ported it to this new format. You can see the result in https://gist.github.com/danderson/f5b518b66c1b7670016b3a817cbefbd5. (Note this was done on an earlier prototype with different pragma names, but the semantics are the same).Detailed semantics
These details are easy to change, as long as we stick to some requirements:
The parser detects the format by looking for a line that starts with
#[
(not standard GLSL, use old parser) or#version
(this cannot appear before#[
in the old format, use new parser).The old parser is unchanged, except for some refactoring to extract helper functions.
The new parser scans the source file and handles the following specially:
#version
: remember for use later when compiling.#include
: handled identically to current parser.#pragma
: ignore non-Godot pragmas. For Godot pragmas:#pragma godot_shader_stages(...)
per file.godot_shader_stages
andgodot_shader_versions
. Any othergodot_*
pragma is a compile error (to reserve the namespace for future additions).godot_shader_stages
arevertex
,fragment
,tessellation_control
,tessellation_evaluation
andcompute
. For backwards compatibility, we also allowtesselation
(typo, with one L). Unknown values, or duplicates, are a compile error.godot_shader_versions
are ASCII identifiers. No duplicates allowed, and duplicate checking is case insensitive: you can declare a versiontRiAnGlEs
if you like pain, but you can't have another versiontriangles
as well.Rules for a structurally valid shader file:
#pragma godot_shader_stages
or derived from the filename.""
version just like the old parser.If the file is structurally valid, then for each stage+version:
blah
, we add#define GODOT_STAGE_BLAH
.foo
, we add#define GODOT_VERSION_FOO
. If the user didn't ask for custom versions, we don't defineGODOT_VERSION_...
.Implementation roadmap
Assuming this proposal is acceptable and we resolve any open questions, I volunteer to ship this feature. This means:
Open questions/syntax war 😂
(assuming the proposal in general sounds good)
godot_shader_stages
andgodot_shader_versions
to reserve a Godot-specific prefix, and match the names from the existing format. This also matches the prefix for the macro namespace in C#, for consistency.GODOT_STAGE_<uppercase stage name>
andGODOT_VERSION_<uppercase version name>
. Again reserving a prefix for Godot macros, and and matching the names to the names of the pragmas..vertex.glsl
,.fragment.glsl
,.tessellation_control.glsl
,.tessellation_evaluation.glsl
,.compute.glsl
. I decided to keep the.glsl
suffix so that the shader language is immediately obvious, and so that in future if Godot adds HLSL/WGSL/SPIR-V/whatever support, RDShaderFile doesn't have to start using horrible heuristics to guess which compiler to use. I used the same stage names that RDShaderFile uses internally, just to avoid defining yet another set of names for everything, but that means they are quite verbose, especially the tessellation shaders..glsl
file with no declared stages is "valid", we just don't invoke the SPIR-V compiler and the editor says there are no shader versions/stages available (or maybe it has UI to say "this looks like an include file, click one of these buttons to turn it into a top-level shader of type X" ?).If this enhancement will not be used often, can it be worked around with a few lines of script?
I expect this to be used by anyone writing compute shaders for Godot projects, as well as people writing custom render shaders for advanced uses (or Godot devs hacking on the renderers).
The only workaround is what some people do today: put the actual shader code in a separate include file, and include it from the non-standard top level GLSL file. This works, but isn't very ergonomic because every time you edit the included shader the editor throws compile errors complaining that there is no
#[blah]
in the include file, and you end up with 2 GLSL files for even a trivial shader.The other "status quo" workaround of course is to ignore the IDE sadness when editing your shaders, and live without autocomplete/linting/etc. Obviously this "works", but it's not a good user experience IMHO.
Is there a reason why this should be core and not an add-on in the asset library?
Using compute shaders in Godot should be a pleasant default experience without requiring external plugins/add-ons, IMHO. Enabling the use of standard GLSL source code and tools is a big enough usability/quality of life improvement to justify being in core.
The code burden is also quite low: +255 lines of code net for my prototype, and I aimed for readability/maintainability rather than "clever" compact code. The change is isolated to the internals of a single class, and the +255LoC includes full backwards compatibility with existing shaders. This seems like a small price to pay in exchange for the ability to use standard compliant GLSL source code.
Proposal changelog
gd_
togodot_
, per Calinou's suggestion to make the prefixes match the reserved namespace for C# macros, and geekley's suggestion to avoid "GD" causing confusion with GDScript and GDShader.The text was updated successfully, but these errors were encountered: