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

[WIP] Add basic shader preprocessing support #17797

Closed
wants to merge 1 commit into from

Conversation

bitsawer
Copy link
Member

This PR adds basic preprocessing functionality to the Godot Shader Language. See issues #17303 and #11691. This still needs some testing and feedback especially as a bug in the preprocessor can break shaders, but I think at least the #include functionality is important to many users (including myself). This is my first PR to this project, so any feedback is welcome.

First, the shader code is run through a filter that strips away all comments from the code. After that the real preprocessor runs and it handles all directives and includes. After that the processed code is passed to the actual Godot shader compiler.

Some notes:

-The actual Godot shader compiler will not see any comments in the code anymore as they are removed by the preprocessor, so that functionality would become reduntant (and untested).

-No support for macros spanning multiple lines yet.

-Line numbers are preserved, so that editor error messages etc. match the preprocessed output. This added some minor complexity to the preprocessor but it is still simpler than creating support for source code maps etc.

-Duplicate #includes are processed once and only the first one is processed. In C-terms you could say that every shader file has include guards (or #pragma once) automatically inserted. The "shader_type XYZ;" part is also stripped away from each included file.

-During interactive shader editing all currently cached shaders that use includes are reloaded, so if you edit some file that is included into some other file you can see the changes without first modifying the "topmost" shaders. This logic could be improved, it's very rough at the moment.

Currently implemented:

  • #define: Defines a macro, optionally with argument(s).

  • #if: Can be used to evaluate expressions and conditionally remove code from the shader. It uses the Godot script evaluator, so you can use any expressions that are valid GDScript. This might cause some surprises to people who are used to C-style operators like || and && instead of "and", "or". Maybe just convert those C-operators into a GScript versions behind the scenes?

  • #ifdef: Tests if a macro is defined.

  • #else and #endif: Used for conditional branching with #if and #endif.

  • #undef: Undefines a macro.

  • #include: Include other shader files.

#define BACKGROUND_COLOR vec3(1.0, 1.0, 0.0)
#define PI 3.14159
#define PIXEL_TEST(value, cmp) if (value < cmp) discard

#define CUTOFF 5.0
#if (3 * 4 > CUTOFF) or EDITOR
#define ALIVE 1.0
#else
#define ALIVE 0.0
#endif

#ifdef DEBUG
ALBEDO = vec3(1.0, 0.0, 0.0);
#else:
ALBEDO = color;
#endif

#define VALUE 5
#undef VALUE

#include "res://Common/ShaderUtils.shader"
#include "Nature.shader"

@akien-mga akien-mga added this to the 3.1 milestone Apr 3, 2018
@reduz
Copy link
Member

reduz commented Apr 4, 2018

My main problem with this is that, as shaders in Godot are compiled in real time (and the shader compiler is meant to be fast), this will just add to the compilation time as it needs an extra pass and, honestly, most of what was shown as example could be done without it.

@bitsawer
Copy link
Member Author

bitsawer commented Apr 6, 2018

@reduz Worrying about the performance impact is understandable, I haven't done any measurements about it yet. Still, I don't think it will be too bad unless very complex include trees etc. are used. I'll try to do some performance measurements about this.

As for the examples, they were intentionally very simple to demonstrate the basic use cases. In my use cases they are more complex and painful to manage without some kind of compile time code modification. The native Godot GLSL shaders (SSAO, blur, DOF etc.) are one example, it would be pretty messy to implement them without some form of preprocessesing. Same kind functionality should be available to normal users, too.

@reduz
Copy link
Member

reduz commented Apr 7, 2018

I think we should probably implement constants before implementing this and see if most common problem cases are solved. I am still unconvinced about adding this functionality, mainly for the sake of performance and simplicity.

@Zylann
Copy link
Contributor

Zylann commented Apr 18, 2018

That PR would help me a lot for my terrain system, because my shader is becoming complex. I need to be able to re-use code rather than copy/pasting the entire shader, and I need users to be able to toggle features on and off (aka toggling macros like Unity's SetShaderKeyword())), because of problems like graphics card texture samplers limits for example. Also, using ifs all over the place is not nice, and so far users had to manually change the shader to make it work, at the risk of breaking the tool, which is not very welcoming if you aren't experienced in the area.
I also considered generating my shaders, but the code I ended up with was a mess so I would rather prefer a proper preprocessor system.

I stress the word users because when you make plugins for others it's like you are writing engine tools: it's not just for your game and you are not working exclusively for programmers, hence modifying the internals of a plugin is the last thing I want to happen for users because it kinda defeats the point of making a tool. It's still a possibility, but it doesn't have to.
I agree about performance issues, but I'm fine with it as long as you don't pay for it when you don't use it (and to be fair, almost all other engines use preprocessor in their shaders, which should be cached anyways).
I don't agree about simplicity problems for users because it shifts those problems back to people who asked for it in the first place, as I explained above.

@Chaosus
Copy link
Member

Chaosus commented May 7, 2018

@Zylann there could be some render tag such as "enable_preprocessor" and either @reduz and you will be satisfy

@neikeq
Copy link
Contributor

neikeq commented May 18, 2018

My main problem with this is that, as shaders in Godot are compiled in real time (and the shader compiler is meant to be fast), this will just add to the compilation time as it needs an extra pass and, honestly, most of what was shown as example could be done without it.

Can't we run the preprocessor at export time?

@Zylann
Copy link
Contributor

Zylann commented May 22, 2018

Can't we run the preprocessor at export time?

Maybe, it covers switching shaders but being able to turn them on/off at runtime would be my main use case for it, considering that designers may switch features in editor in realtime (just like how Godot shaders appear to work), or even gamers when they change game graphics options.

includes and defines are also complementary, for example if defines aren't doable, it's still possible to write as many shader files as combinations (still quite bad) but at least devs can import the common code in them.

@reduz
Copy link
Member

reduz commented Jul 25, 2018

This is still pending someone implementing constness, which I believe is more prioritary

@akien-mga akien-mga modified the milestones: 3.1, 3.2 Dec 9, 2018
@akien-mga
Copy link
Member

Moving to 3.2 milestone as we are reaching beta stage for 3.1 and no longer merging new features.

@akien-mga
Copy link
Member

As this is still a WIP and we're about to reach feature freeze for 3.2, I'll move this PR to the next milestone.

@Chaosus has been doing a lot of work on the shader language lately, including implementing constness as required by @reduz, so this might be worth revisiting.

@akien-mga akien-mga modified the milestones: 3.2, 4.0 Aug 28, 2019
@bitsawer
Copy link
Member Author

I'm currently pretty busy with other things, but if anyone wants to move this PR forward or even do their own implementation from scratch I'm all for it. I might return to this eventually if there is interest.

Few notes in case anyone is interested:

  • Update Vector access to use write/read accessors and fix other conflicts.

  • include support might need a dependency graph (or something similar) to know which shaders should be recompiled if one changes. The current implementation just reloads all cached shaders that use includes which is slow. Alternatively include depth could be limited to one?

  • Some optimizations could be done and maybe replace some quick-and-dirty regular expressions with a proper parser.

In general this preprocessor was very useful in some of my experimental projects even in this limited state and I still think a feature like this should be in Godot too.

@aaronfranke
Copy link
Member

@bitsawer Is this still desired? If so, it needs to be rebased on the latest master branch. I see above that you say you've been busy, if you don't want to rebase this yourself, you can just close this.

Otherwise, abandoned pull requests will be closed in the future as announced here.

@Zylann
Copy link
Contributor

Zylann commented May 13, 2020

#17797 (comment) is still a thing

@aaronfranke
Copy link
Member

This PR has not received any new commits for over 2 years and is abandoned, closing.

The "salvageable" tag still applies, anyone is welcome to take this PR and update/finish it.

@jamie-pate
Copy link
Contributor

jamie-pate commented Mar 18, 2024

Can't we run the preprocessor at export time?

Maybe, it covers switching shaders but being able to turn them on/off at runtime would be my main use case for it, considering that designers may switch features in editor in realtime (just like how Godot shaders appear to work), or even gamers when they change game graphics options.

includes and defines are also complementary, for example if defines aren't doable, it's still possible to write as many shader files as combinations (still quite bad) but at least devs can import the common code in them.

You could write this by adding a script that does the preprocessing, but it'd be cumbersome to apply to all ShaderMaterial nodes

e.g. jamie-pate/MagicaVoxel-Importer@deae37e

@AThousandShips AThousandShips removed this from the 4.0 milestone Mar 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants