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

Custom shader templates #94427

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

BastiaanOlij
Copy link
Contributor

@BastiaanOlij BastiaanOlij commented Jul 16, 2024

Custom shader templates is a new feature in the rendering engine that allows you to override the built-in shaders into which user shaders are injected. While the inputs and outputs are fixed, this gives full control of the executed shader code.

NOTE This is an advanced feature that requires in-depth knowledge of the rendering engine to use. We should consider hiding this feature by hiding the property on the Shader resource unless a project setting is enabled. It should still load/save the property so advanced users can make plugins that make materials available that use a custom shader template. Also note that this feature is likely very version sensitive, while the include system (see #94193 which this PR is dependent on) will likely soften this, we can't completely remove this.
Also custom shader templates are renderer dependent, you can likely not use a template created for the Forward+ renderer on the mobile renderer or compatibility renderer.

Todos:

  • Add shader template to material storage.
  • Link shader to shader template in material storage.
  • Implement new material storage implementation for RD renderer.
  • Modify forward+ renderer scene shader to use shader template and create our default shader template.
  • Modify mobile renderer scene shader to use shader template and create our default shader template.
  • Create Shader Template resource
  • Modify Shader resource to assign a shader template (see Note above)
  • Modify Material resource classes so we can nominate a shader template that will be used to generate built-in shaders

Add into separate PR for enabling this on compatibility renderer:

  • Implement new material storage implementation for compatibility renderer.
  • Modify compatibility renderer scene shader to use shader template and create our default shader template.

Add into additional follow up PR(s):

  • Modify canvas shaders to use shader template and create our default shader template
  • Modify sky shaders to use shader template and create our default shader template
  • Modify fog shaders to use shader template and create our default shader template
  • Rejig built-in include shader files for ease of creating a custom shader template (follow up PR)

Known issues:

  • If you have shader templates for a different renderer than selected in your renderer, and there are shaders using those even if not currently loaded, Godot creating previews will result in errors as those templates can't be compiled.

Very simple test project can be found here:
https://github.com/BastiaanOlij/test-custom-shader-templates
The readme on this project contains extra information about writing custom shader templates.

Live streams about this feature:

@BastiaanOlij BastiaanOlij added this to the 4.4 milestone Jul 16, 2024
@BastiaanOlij BastiaanOlij self-assigned this Jul 16, 2024
@BastiaanOlij BastiaanOlij force-pushed the custom_shader_templates branch 2 times, most recently from ca97f22 to b251e10 Compare July 16, 2024 09:07
@RedMser
Copy link
Contributor

RedMser commented Jul 16, 2024

Should this feature really be called shader templates? It might cause confusion with "script templates" which work completely differently (I assumed this PR would allow a newly created shader to start with a different code preset).
Maybe since it's not (yet?) exposed in the UI, it doesn't matter as much, but could be worth thinking about alternatives while the PR is in development.

@JoNax97
Copy link
Contributor

JoNax97 commented Jul 16, 2024

I agree with the comment above. I feel something like "scaffolding" could communicate the idea.

@Calinou
Copy link
Member

Calinou commented Jul 16, 2024

I agree with the comment above. I feel something like "scaffolding" could communicate the idea.

Scaffolding generally refers to a template system that creates files/folders for you (like good old Yeoman), so I'd try to find a different term for that instead.

The concept of overriding is more accurate here, although calling it a shader override may be mistaken for a material override (which is a different concept).

I think it's important to highlight the fact that you are replacing core shaders here, not user-provided shaders. The core shaders are always written in GLSL, not the Godot shader language. Maybe core shader override?

@JoNax97
Copy link
Contributor

JoNax97 commented Jul 16, 2024

Your last paragraph made me think, if this is for overriding core shaders, why is it set per-shader instead of globally in project settings?

@BastiaanOlij BastiaanOlij force-pushed the custom_shader_templates branch from b251e10 to b02c977 Compare July 17, 2024 10:07
@BastiaanOlij
Copy link
Contributor Author

BastiaanOlij commented Jul 17, 2024

Your last paragraph made me think, if this is for overriding core shaders, why is it set per-shader instead of globally in project settings?

Because you may want to combine different shaders. For instance you may want to only use this for your terrain shader.

(that said, having a project settings that sets a default could be an interesting option)

@mrjustaguy
Copy link
Contributor

something that's Kind of like the Default Environment and Default Environment Background Color you can set in the project settings would be great..

@BastiaanOlij
Copy link
Contributor Author

something that's Kind of like the Default Environment and Default Environment Background Color you can set in the project settings would be great..

Hmm, we could add it to the compositor, that might be a good place for this.

@BastiaanOlij
Copy link
Contributor Author

BastiaanOlij commented Jul 19, 2024

something that's Kind of like the Default Environment and Default Environment Background Color you can set in the project settings would be great..

Hmm, we could add it to the compositor, that might be a good place for this.

Actually, thinking about this more, this is not possible because we could have the same shader compiled with multiple templates and we have no support for that. So we can do an all or nothing project setting, or keep it as is right now where we identify the template to use for each user shader.

@BastiaanOlij BastiaanOlij force-pushed the custom_shader_templates branch 2 times, most recently from bd50859 to 75f60ec Compare July 22, 2024 07:52
@BastiaanOlij
Copy link
Contributor Author

BastiaanOlij commented Jul 22, 2024

Basics are starting to work, we can now build ShaderMaterial based shaders using a custom template. I've linked a very simple demo up in the OP.

The user shader has a new optional keyword shader_template that allows you to specify the custom template to use:

shader_type spatial;
shader_template "res://test.gdtemplate";
render_mode blend_mix, depth_draw_opaque, cull_back, diffuse_burley, specular_schlick_ggx;

...

I've also done a basic rejig of the include files for the built-in template for the forward+ renderer. This allows us to include all the core inputs and outputs. See the test.gdtemplate in the example linked in the OP to see how this is used.
At this moment this test project simply creates an unshaded templated that only supports basic objects and shaders (misses support for motion vectors, multimesh, bones and other features), for that we need to expose much more of the built in logic. In discussions with Clay we figured it may be smarter to do this rejig after the main logic is merged.

image
(left cube is built-in template, right cube is custom template)

@BastiaanOlij BastiaanOlij force-pushed the custom_shader_templates branch 3 times, most recently from 918a194 to c3df623 Compare July 23, 2024 07:55
@BastiaanOlij
Copy link
Contributor Author

Fixed the issues around reloading the shader template, now when you modify the shader template, all shaders that use the template get recompiled automatically.

@BastiaanOlij BastiaanOlij force-pushed the custom_shader_templates branch from c3df623 to 59f2d39 Compare July 24, 2024 03:32
@BastiaanOlij
Copy link
Contributor Author

Cleaned up some more code and added support for setting the shader material on StandardMaterial3D resources:

image

@BastiaanOlij BastiaanOlij force-pushed the custom_shader_templates branch from 59f2d39 to 4f04ee3 Compare July 25, 2024 05:01
@@ -339,6 +340,9 @@ class BaseMaterial3D : public Material {
uint32_t feature_mask;
uint32_t flags;

// shader template
uint64_t shader_template_id;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may be a breaking change, I am not sure what the implications of changing the material key are. However seeing shader templates effect the compiled shader, it has to be part of the key.

}

void _unpack_vertex_attributes(vec4 p_vertex_in, vec3 p_compressed_aabb_position, vec3 p_compressed_aabb_size,
#if defined(NORMAL_USED) || defined(TANGENT_USED) || defined(NORMAL_MAP_USED) || defined(LIGHT_ANISOTROPY_USED)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@clayjohn this is an interesting difference between the mobile and clustered implementation. We check more defines in the mobile renderer, but we're not consistent with it. I wonder if this is correct or left over from an older implementation?

@BastiaanOlij BastiaanOlij marked this pull request as ready for review July 25, 2024 05:05
@BastiaanOlij BastiaanOlij requested a review from a team as a code owner July 25, 2024 05:05
@BastiaanOlij BastiaanOlij requested a review from a team as a code owner July 25, 2024 05:05
@BastiaanOlij BastiaanOlij force-pushed the custom_shader_templates branch 5 times, most recently from 759b521 to 78016e8 Compare July 26, 2024 06:33
@BastiaanOlij BastiaanOlij requested a review from a team as a code owner July 26, 2024 06:33
@BastiaanOlij
Copy link
Contributor Author

Added some extra logic so you can optionally specific templates for multiple renderers. A template in the format of:

shader_type spatial;

#[vertex]

// vertex shader code goes here

#[fragment]

// fragment shader code goes here

Will be used regardless of the renderer selected and thus will fail when used with the wrong renderer.

Alternatively you can create blocks for each renderer:

shader_type spatial;

#[forward_plus]

#[vertex]

// vertex shader code for the forward+ renderer goes here

#[fragment]

// fragment shader code for the forward+ renderer goes here

#[mobile]

#[vertex]

// vertex shader code for the mobile renderer goes here

#[fragment]

// fragment shader code for the mobile renderer goes here

In this scenario, if the current renderer does not have any code in the template, a missing code error will be given.

Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested locally (rebased on top of master 20f4d26), it works as expected.

Some feedback:

  • There is no shader compilation error when the shader template file doesn't exist:

image

You do get this message in the editor Output panel Resource file not found: res://notfound.gdtemplate (expected type: ), but no error highlighting in the shader editor due to this.

  • The .gdtemplate file extension is pretty generic. It could be a script template, a project template, anything – it's not very descriptive. I suggest using .gdshadertpl which would make it more consistent with the existing .gdshaderinc extension.

As a reminder, we'll need to contact KDE/Dolphin developers to add an icon for the new file extension we choose to use:

image

  • I've gotten this error message from making a change in a .gdtemplate file then reloading the project:
  Unknown shader type in shader template.

Could more context be added to this message (such as the type that was specified, and the line of the error)? The change was on https://github.com/BastiaanOlij/test-custom-shader-templates/blob/master/test.gdtemplate#L323, where I replaced the line with:

	albedo_output_buffer = vec4(albedo * vec3(1.0, 0.0, 0.0), alpha);

(I'm not sure why that caused an error too, either way. It seems to be valid GLSL.)

I also tried reverting my change and reloading the project again, but I still get the error message and the material is no longer using the custom shader template:

image

The cube on the right should appear unshaded, but it appears shaded.

@BastiaanOlij
Copy link
Contributor Author

@Calinou agree with all you said there and I'll look into the error you reported, though I'm not planning on spending a lot of time on this until after 4.3 stable is out, I want to just gather feedback and then do a whole bunch of changes at once.

When the shader template can't be loaded, it just defaults back to the built in template, hence your right cube showing up correctly. I think it would be better if the compilation just fails and it just defaults back to our white shader.

I need to figure out how to make that happen because if the fault lies in the template, it finds it out too late, but when the fault lies in the template not being found we can piggy back on the existing logic for this.

@BastiaanOlij BastiaanOlij force-pushed the custom_shader_templates branch from 78016e8 to 25a6152 Compare September 7, 2024 12:46
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.

5 participants