godot icon indicating copy to clipboard operation
godot copied to clipboard

Add macro override API to `ShaderMaterial`

Open Chaosus opened this issue 2 years ago • 2 comments

This will allow users to set #define expressions at runtime, by allocating the internal shader inside the shader material if any macro override is declared. This is a first step to solve https://github.com/godotengine/godot-proposals/issues/5062.

shader_material_macro

Test Project: Test.zip

Chaosus avatar Dec 30 '22 18:12 Chaosus

This looks really nice and surprisingly simple. Let's plan on discussing this in detail after 4.0 is out.

I think @Zylann will be able to provide some good, user-centered feedback

clayjohn avatar Jan 05 '23 23:01 clayjohn

Looks great, although I'm suggesting a slightly more versatile version. See my suggestion in section 2.

  1. Like Zylann, I wonder if there are any performance pitfalls where the user manages to creates multiple shaders with code and macros that resolve to the exact same shader textual source code but the engine still considers them different shaders and has to do state switching, uniform update etc. as if they are completely different shaders. To put it simply, are there any shader sharing/caching/duplication issues users might run into? I don't have much Godot expertise about this, just something that I have encountered when implementing things like this in other engines. I also wondered about this when I wrote the original preprocessor PR, but never investigated it that much.

  2. I would suggest a slightly different API and functionality. It looks like you can only override macros, not define new ones with this PR. Instead of adding an override API, I would simply add a call ShaderMaterial.set_preprocessor_macro(name: String, content: String) together with get_preprocessor_macro(name: String), remove_preprocessor_macro(name: String) and clear_preprocessor_macros(). set_preprocessor_macro() would store these macros in a map and simply insert them into the preprocessor defines when the preprocessor is run. This makes it possible to define new macros programmatically and the whole code path and behavior is natural as both shader defined and programmatically defined macros behave the same way. Shader code can even #undef and/or re-define those programmatically added macros if they need to which adds to the flexibility.

For example, with my solution you could do shader code like this:

    void vertex() {
    #ifdef MY_DEBUG_MACRO
        ...
    #endif
    }

Then user could then simply call ShaderMaterial.set_preprocessor_macro("MY_DEBUG_MACRO", "") and things will work even if the MY_DEBUG_MACRO is not defined.

With this PR's current solution, to get the same effect (like in a library shader include which doesn't know if certain preprocessor macros will be defined by the user including the file) it would have to be written something like this (because you must also test if the value of a macro is true, not just if it exists):

    #define MY_DEBUG_MACRO false

    void vertex() {
    #ifdef MY_DEBUG_MACRO
    #if MY_DEBUG_MACRO
        ...
    #endif
    #endif
    }

User will then have to call ShaderMaterial.set_shader_macro_override("MY_DEBUG_MACRO", "true"). The user will also always have to define the possible macros in the shader code because it's not possible to define new ones using the API, only override existing macro values.

My API suggestion is a bit more versatile. And with this PR functionality, it's not possible to temporary alter macros, for example debugging purposes (especially when also using tool scripts that would use macro overrides both in editor and during game). Let's imagine you use a debug visualization macro DEBUG_VISUALIZATION_LEVEL and you want to change it temporary for a part of some code to debug it.

    #undef DEBUG_VISUALIZATION_LEVEL
    #define DEBUG_VISUALIZATION_LEVEL 2 //Force debug visualization for following code

    #include "HelperFunctions.gdshaderinc"

The user always calls set_shader_macro_override("DEBUG_VISUALIZATION_LEVEL", "0") and alters it when needed. Unfortunately, this would make that kind of temporary replacement hack impossible because the override API always forces the macro DEBUG_VISUALIZATION_LEVEL to the same value no matter what. There's are workarounds for this (like creating two layers of macros), but changing the API into more flexible might be better solution. And of course it's possible to keep the override functionality and add my suggested API too, although that might be a bit confusing to the users.

Still, this will be a really useful feature either way! Thanks!

bitsawer avatar Jan 08 '23 09:01 bitsawer

Would it be possible to add custom defines to the ShaderMaterial UI, or is that better handled in a separate PR?

Was thinking it could be convenient to create multiple ShaderMatierals for different quality/LOD levels, using the same Shader code

JohanAR avatar Mar 12 '23 09:03 JohanAR

Is this pr still active?

ODtian avatar Apr 29 '24 16:04 ODtian

Currently, I lost my vision about it, too much I need to call to mind, maybe others may make it.

Chaosus avatar Apr 29 '24 18:04 Chaosus