Add custom shader template support to rendering server (core)#111939
Add custom shader template support to rendering server (core)#111939BastiaanOlij wants to merge 1 commit intogodotengine:masterfrom
Conversation
|
Update test project for now is in this branch: https://github.com/BastiaanOlij/test-custom-shader-templates/tree/ldotn_template |
263aa20 to
2f3998e
Compare
d7e1c31 to
2c18e66
Compare
|
@clayjohn @DarioSamo I think this is pretty much in working condition and as far as I want to take the foundation. I am tempted to add template support to the compatibility renderer as well but might do that in a follow up. A few things I'd like to discuss with you before taking this out of draft.
|
|
Can we have more uniforms than the standard include files specify? It would be common to bind some extra uniform buffers for custom shading models. |
At this point the only way to get extra uniforms in, is to add those in through the user shader that is embedded. There was someone who did a PR on my old implementation to add in the ability to define additional uniforms in the template that would be exposed on the material. I think its worth revisiting that but as a follow up to the initial implementation. One of the difficulties of getting this merged initially was that we were trying to do too much and reviewers don't see the wood for the trees anymore. |
Calinou
left a comment
There was a problem hiding this comment.
Tested locally, it works as expected.
shader_template_reload.mp4
Some feedback:
-
Class reference XML files need to be updated with
--doctool, and descriptions for new items must be filled in. -
It's possible to have multiple
shader_templatedeclarations in the same shader, without any errors:
It's not clear which one "wins" in this situation, so I'd say this should be an error, just like duplicate shader_type or render_mode declarations.
-
Shader template reload on changes only affects the editor, not the running project. That said, we don't have live reloading for
.gdshaderin the first place. -
I'm not sure if the
.gdtemplateextension is ideal; it could be referring to any kind of template such as a script or export template, not necessarily a shader template. I can't think of any better alternatives right now, still.
45e4d45 to
27f7852
Compare
This is one that could probably do with a rethink and moving the group logic into the templates at a later date. However for now this logic remains implemented on the ShaderRD level. You can see that in both the forward+ and mobile renderers, when groups are enabled this is applied to all templates. |
27f7852 to
fe5183d
Compare
|
@Calinou got all of your comments sorted. Docs should now be updated, and it should now check for double inclusion of "shader_template" in the user shader. Live updating seems like a whole different thing to tackle, especially as this would be the same workload as making this work for gdshader, I think we should keep that out of scope for this PR. Finally, agree that gdtemplate is a very ambiguous name. But I haven't heard an alternative yet that works better without it becoming very verbose. Totally up for suggestions though... |
|
Marked this as ready for review, I'm sure there will be more requests for changes. |
Some ideas that would come to mind for me would be:
I would be in favor of having the connection with the |
fe5183d to
b7b2a90
Compare
| } else { | ||
| shader_name = shader_name.replace("res://", ""); | ||
| shader_name = shader_name.replace(".gdtemplate", ""); | ||
| shader_name = shader_name.replace("/", "_"); |
There was a problem hiding this comment.
| shader_name = shader_name.replace("/", "_"); | |
| shader_name = shader_name.replace_char('/', '_'); |
After discussing during a render meeting some weeks back, we discussed reviving #94427 but breaking it up into parts.
This PR purely adds in the core of the implementation. E.G it introduces the shader template framework and allows us to set a shader template for a
.gdshaderfile. It does not touch theBaseMaterialclass nor does it add the breakup of shaders.I'm still basing this on the
.gdtemplatefile extension but the logic has been expanded that it can include files though there is still a pathing limitation here. If built-in include files are specified, the include directive remains in the source, but files on disc are loaded immediately and embedded in the imported resource.For now I've removed the ability to specify multiple renderer shader code, the files just become unwieldy long and its easy for the developer to just switch out shaders.
Currently only Mobile and Forward+ work, still contemplating to add support for compatibility but that may become a separate PR.
I'm still unsure if we've got a few things in the right place, too much of the previous system assumed there would be only one
ShaderRDinstance centralized and this is no longer the case. Definitely the grouping system and possibly the mutexes applied may need a bit more thought.Finally we discussed that instead of trying to expose more of the shader code through build-in includes, we build the shader with the complete default template code. In my sample project I've extracted what I need, but the idea was to add a feature to save the built-in shader template so the user can edit it to their hearts content. They will need to be made aware that the code will likely only work with that version of Godot.
This still needs to be added.