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

Move material attributes from shader code to material properties #933

Open
endragor opened this issue May 28, 2020 · 6 comments
Open

Move material attributes from shader code to material properties #933

endragor opened this issue May 28, 2020 · 6 comments

Comments

@endragor
Copy link

Describe the project you are working on:
Summon Age (https://www.summonage.com) and other mobile 3D games in RPG/MOBA genre.

Describe the problem or limitation you are having in your project:
We have to create many shaders that are basically the same, but only differ in their render_mode declarations (culling, blending, depth testing). Godot then also wastes time compiling these shaders, even though these are not shader differences and the resulting (GLES) shader code is identical.

Describe the feature / enhancement and how it helps to overcome the problem or limitation:
Move the render_mode settings that are not really part of shader but rather a material parameter to be a material property.

Describe how your proposal will work, with code, pseudocode, mockups, and/or diagrams:
The user will no longer have to create redundant shaders.

If this enhancement will not be used often, can it be worked around with a few lines of script?:
No, and it will be used often.

Is there a reason why this should be core and not an add-on in the asset library?:
Can't be implemented as an addon.


See godotengine/godot#21105 for some earlier discussion of this feature. It also correctly notices that if stencil support is added to Godot in the future, it's better be as material parameters rather than within shader code.

@lyuma
Copy link

lyuma commented Jun 21, 2021

Underrated proposal: See the hack I had to do with MToon basically copying and pasting the same shader code 7 times, and even then I don't support all combinations, but I was forced to guess what users would probably want and provide those variants:
https://github.com/V-Sekai/godot-vrm/tree/master/addons/Godot-MToon-Shader

One issue this proposal doesn't address is Transparent vs. Opaque shaders. Transparency is determined by whether the code contains "ALPHA = " anywhere in the code, and that cannot be changed with a render_mode, so unless this is addressed, you will still need at least two versions of each shader, unfortunately.

I would like there to be a render_mode blend_disabled for 3D shaders, to allow shaders with ALPHA= to be treated as opaque. If that happened, then the above proposal could be used to toggle the blend_disabled state on an otherwise transparent-compatible shader to make it opaque vs transparent.

We don't have a proposal for stencils yet it seems, but that needs to happen, too.

@Calinou
Copy link
Member

Calinou commented Jun 21, 2021

We don't have a proposal for stencils yet it seems, but that needs to happen, too.

Stencil support was discussed in an issue a while ago, but no proposal has been opened yet.

@Calinou Calinou changed the title Move material attributes from shader code Move material attributes from shader code to material properties Jun 21, 2021
@clayjohn
Copy link
Member

Godot then also wastes time compiling these shaders, even though these are not shader differences and the resulting (GLES) shader code is identical.

Note, this isn't quite true. Render modes change flags in Godot's main scene shader which can result in very different shader code being generated.

For example, render mode unshaded compiles out all lighting calculations resulting in a much smaller, simpler shader.

@lyuma
Copy link

lyuma commented Jun 21, 2021

Ok so I found the list of render_mode_defines in servers/rendering/renderer_rd/renderer_canvas_render_rd.cpp and ./servers/rendering/renderer_rd/forward_clustered/scene_shader_forward_clustered.cpp

I would argue that an exception ought to be made for DO_SIDE_CHECK:

actions.render_mode_defines["cull_front"] = "#define DO_SIDE_CHECK\n";
actions.render_mode_defines["cull_disabled"] = "#define DO_SIDE_CHECK\n";

(it should be simply made togglable for performance if necessary)

Then that leaves the following render_modes which do not change generated shader code (scene)

blend_mix
blend_add
blend_sub
blend_mul
depth_draw_opaque
depth_draw_always
depth_draw_never
depth_draw_alpha_prepass
depth_test_disable
cull_front
cull_back
cull_disabled

and this list (canvas_item)

blend_mix
blend_add
blend_sub
blend_mul
blend_premul_alpha
blend_disabled

So, this proposal should really be: move render_modes for blend_, depth_ and cull_ outside of the material, which was hinted at in the OP:

only differ in their render_mode declarations (culling, blending, depth testing)

@QbieShay
Copy link

QbieShay commented Jul 6, 2022

Up for this proposal. I find it very annoying to duplicate a shader just to change cull front/back (i often use double render pass)

@Calinou
Copy link
Member

Calinou commented Jul 6, 2022

For toggling between a transparent and opaque shader with a render_mode, there is godotengine/godot#51711. It was originally designed for a different use case, but I suppose it'd work here too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants