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

Add shader preprocessor singleton access #71919

Closed

Conversation

bitsawer
Copy link
Member

@bitsawer bitsawer commented Jan 23, 2023

Implements part 6 of godotengine/godot-proposals#5062, but in a bit more powerful and flexible way.

This PR exposes ClassDB singletons (like OS, Engine, ProjectSettings etc.) for preprocessor condition evaluation. This makes it possible to use more complex logic to control how the shaders are preprocessed and compiled. For example, it is now possible to use Engine.is_editor_hint() to draw extra debug stuff while in editor, checking OS.get_environment() or OS.has_feature() for special flags or doing special processing if some ProjectSettings.get_setting() is set. Possibilities are almost endless.

And in case you didn't know yet, the preprocessor #if and #elif code evaluation has always been done using the GDScript expression parser, although many have not noticed it because usually those conditions are pretty simple and &&, || etc. operators work in GDScript, too. You can replace && with "and" and things will work just the same. If this PR is accepted, I can update the preprocessor docs to be explicit about this and give some examples.

Naturally, it is possible to use these together to make more complex conditions and it is still possible to mix functions and macros in expressions. Here is a code example to demonstrate the syntax:

#define ENABLED true
#define DEBUG_MODE OS.has_feature("debug") || "force_debug" in OS.get_cmdline_args()
#define GET_SETTING(name) ProjectSettings.get_setting(name)

#if (OS.has_feature("release") && Engine.is_editor_hint()) || OS.get_environment("FORCE_DEBUG") == "1"
    ALBEDO *= 1.0;
#elif (ENABLED && OS.get_name() == "Android") || (defined(TEST_MODE) || DEBUG_MODE)
    ALBEDO *= 2.0;
#endif

#if GET_SETTING("rendering/anti_aliasing/quality/use_taa")
    ALBEDO *= 3.0;
#else
    ALBEDO *= 4.0;
#endif

@bitsawer bitsawer requested a review from a team as a code owner January 23, 2023 15:47
@clayjohn clayjohn added this to the 4.1 milestone Jan 23, 2023
@clayjohn
Copy link
Member

This looks amazing! I have tagged as 4.1 as this is a bit late to make it in for 4.0, but definitely seems very interesting and something that we should discuss shortly after we release 4.0

@celyk
Copy link

celyk commented Mar 12, 2023

I don't like it. GDScript doesn't belong in shader source code, and this will be very confusing for beginners. #70749 does more to address the main problem and without any bloat.

I'm all for extending the preprocessor though, this would make for a great addon if that were possible. Exposing a custom preprocessor API would be ideal

@bitsawer
Copy link
Member Author

@celyk The main point of this feature is to make it possible to write fully self-configuring shaders. You can query information about the runtime environment including current renderer and adjust the shader to that. It just works. Otherwise, you will need to instruct the user to manually edit the shader to add or remove macros or even write GDScript/C# code to do that so it will work in your project configuration, inluding writing @tool scripts to make sure the shader gets the runtime config it needs in every situation including editor mode and material previews. I would say that manual editing scenario is much worse for beginners compared to just dropping an example or addon shader file to their project which will then automatically detect the renderer etc. without needing any external user work. Writing portable shader code might not be a big issue if you are the lone coder+artist targeting known specs and a renderer, but for bigger teams, tutorials, demos, collaborations and addon-developers this is a one more pain point when sharing shader code.

And as for GDScript in shader preprocessor: it already works there, we just haven't really documented it. So this doesn't really bloat things. Unless you mean code size, in which case this PR bloats the engine by a grand total of 14 new code lines.

And if you don't like GDScript in there you don't have to use it, normal macros will continue to work as always. But as an example, this shader preprocessor GDScript code already works (and evaluates to true):

#if 5 in [1, 2, 5] and abs(-1) == 1 and "test".find("te") != -1
    ALBEDO = vec3(0, 1, 0);
#endif

I mean, is #if Engine.is_editor_hint() so much more harder for a beginner to understand than something like #if defined(EDITOR)?

Godot has often gone its own way against the norm if there is benefit to it. We even have our custom shader language instead of using GLSL or HLSL and most users including beginners don't seem too confused about it.

@bitsawer bitsawer force-pushed the preprocessor_expose_singletons branch from fa30128 to cfbd4b7 Compare April 12, 2023 11:00
@bitsawer bitsawer requested review from a team as code owners April 12, 2023 11:00
@bitsawer bitsawer force-pushed the preprocessor_expose_singletons branch from cfbd4b7 to 975a9d7 Compare April 12, 2023 11:30
@bitsawer bitsawer force-pushed the preprocessor_expose_singletons branch from 975a9d7 to bcb42a7 Compare May 25, 2023 07:21
@BastiaanOlij
Copy link
Contributor

This PR hasn't gotten on my radar yet, that looks like an elegant and simple solution that brings a lot of power to shader code.

Seeing Godot users will have learned GDScript before they even get to dive into writing shader code, this seems a natural extension.

@gamma-delta
Copy link

Seconding this; I have this pretty wild post-processing setup that requires the world to look like this:

image

This is impossible to see any of the editor widgets over (right now I'm messing with NavMeshes and I can't visually debug).

Being able to flip the shader to just display plain-ol shaded white would be invaluable.

@clayjohn clayjohn removed this from the 4.2 milestone Sep 19, 2023
@clayjohn
Copy link
Member

We discussed this in the September 6 rendering meeting (notes here) and those present weren't sure if this is a direction we want to go with for GDShaders.

First of all, this is a super cool PR and it is obviously extremely powerful, great work!

That being said, I'm not sure that this is something that can be merged. In the meeting we discussed two issues:

  1. adding GDScript into GDShaders may be bad for GDShaders (just because we can doesn't mean we should)
  2. This may not be the best way to achieve what users need.

Personally, to me this seems like a way to give users a ton of power that requires minimal code changes. But I don't think this is exposed in the most intuitive way for users, and I think this creates a lot of footguns for users.

For example, users might tie a shader path into a project setting, change the project setting, and then end up with a difficult to debug visual error that is difficult to trace. Even scarier, users might try to do things like

#if Engine.get_process_frames > 100
//do something
#endif

While this seems kind of cool. It is an extremely dangerous thing to do. As you are essentially bundling game logic into your shader code.

Similarly, I find this using engine singletons quite awkward for doing the thing that users request most: controlling defines from script.

Ultimately, what we need is a way for users to add #defines to shaders dynamically in such a way that they can manage their own shader variants using Godot Shaders. This PR is one way of doing that as users can just piggyback on the project settings to control the flow of preprocessor directives. However, I think this is not the optimal way of doing that from a usability standpoint. I think we should design a system more catered to shaders.

I think we should take a step back and see if we can design a system that is catered towards users needs, even if it requires more work from us to implement than what you have already done here.

@YuriSizov YuriSizov modified the milestones: 4.3, 4.x Sep 26, 2023
@bitsawer
Copy link
Member Author

Closing this considering the consensus.

I'm probably going to implement part 4 of godotengine/godot-proposals#5062 at some point during the Godot 4.3 cycle. I'll possibly create a more detailed proposal first and gather more feedback first. The feature should have an UI for adding global macros and probably also an API for programmatically adding / removing them during runtime (for platforms that support them), so it might combine part 3 and 4 in a more traditional way compared to this PR. So, feel free to continue discussion in the proposals.

Adding some builtin macros like EDITOR is also planned and trivial to implement, I'll probably make proposal and PR at some point in the future.

Also, this PR should still work pretty well and everyone can still continue to use it in their custom builds until a better solution is available if they have need for an feature like this.

@bitsawer bitsawer closed this Oct 17, 2023
@bitsawer bitsawer removed this from the 4.x milestone Oct 17, 2023
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.

6 participants