-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Configurable World Shader Commands System #1738
base: master
Are you sure you want to change the base?
Configurable World Shader Commands System #1738
Conversation
Hey thanks a lot and cool that it already works :) I think we need to design this a bit more to make it more in line in the way that it is supposed to be used, i.e. by putting these snippets as files somewhere and merging the shader code better. I'll think about it and will come back to you with more ideas. |
Okay, I finally have time to respond here :D As said before, I like the direction where this is going, but I have a few requests for how commands should be integrated.
|
29cafd1
to
e259788
Compare
e259788
to
64620ae
Compare
Here are some updates:
But the current state is a very basic version, here are some current limitations
|
Because right now I didn't implement alpha setting function, so I hardcoded different alpha code into fragment shader to test. demo_7_world.frag.glsl
world_commands.config
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we could still make this much simpler :D
I'm not sure if we even need a custom config format that needs parsing. If we do placeholder replacement, we can skip most of the parsing logic while supporting even more use cases.
Example
Template: shader.template
#version 330
uniform float template_local;
// PLACEHOLDER: unifs_code.snippet
void main() {
// PLACEHOLDER: x_code.snippet
// PLACEHOLDER: y_code.snippet
gl_Position = move * vec4(x, y, 0.0, 1.0);
}
Snippet: unifs_code.snippet
uniform float value;
Snippet: x_code.snippet
float x = value + 1.0
Snippet: y_code.snippet
float y = value - 1.0
End result:
#version 330
uniform float template_local;
uniform float value;
void main() {
float x = value + 1.0
float y = value - 1.0
gl_Position = move * vec4(x, y, 0.0, 1.0);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think using a JSON config file makes sense? The structure is clean and we only need to load the JSON file instead of scanning for snippets:
{
"snippets": {
"lighting": "snippets/lighting.glsl",
"color_processing": "snippets/color.glsl",
"effects": "snippets/effects.glsl"
}
}
File structure would be:
shaders/
├── world2d.frag.template # Main shader template
├── world2d.vert.glsl # Vertex shader
├── shader_config.json # Snippet configuration
└── snippets/ # User-modifiable snippets
├── lighting.glsl
├── color.glsl
└── effects.glsl
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or maybe we could simply read all the snippets directly from the snippets/
folder without setting a config file.
I've simplified the shader template system to focus on its core functionality.
Fragment Shader Template: #version 330
in vec2 tex_coord;
out vec4 frag_color;
uniform float time;
void main() {
// PLACEHOLDER: color
// PLACEHOLDER: alpha
frag_color = vec4(r, g, b, alpha);
} Shader Command Snippets: float alpha = (sin(time) + 1.0) * 0.5; color.snippet float r = 0.5 + 0.5 * sin(time);
float g = 0.5 + 0.5 * sin(time + 2.0);
float b = 0.5 + 0.5 * sin(time + 4.0); Result: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the long wait on the review 😿 I was unfortunately very busy at work which made the open source reviews a bit difficult. But now that I have time I can, I must say I like your solution very much! I have a few small suggestions, but that is only really minor stuff. I would be happy to merge this soon :)
* @return Complete shader code. | ||
* @throws Error if any required placeholders are missing snippets. | ||
*/ | ||
std::string generate_source() const; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could generate renderer::resources::ShaderSource
directly.
// Original template code with placeholders | ||
std::string template_code; | ||
// Mapping of placeholder IDs to their code snippets | ||
std::map<std::string, std::string> snippets; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need three slashes, so that doxygen detects it as a docstring.
// Original template code with placeholders | |
std::string template_code; | |
// Mapping of placeholder IDs to their code snippets | |
std::map<std::string, std::string> snippets; | |
/// Original template code with placeholders | |
std::string template_code; | |
/// Mapping of placeholder IDs to their code snippets | |
std::map<std::string, std::string> snippets; |
std::string file_name = snippet_path.get_name(); | ||
std::string name = file_name.substr(0, file_name.find_last_of('.')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
util::Path
has a method implemented for what you are doing here (get_stem()
).
std::string placeholder = "// PLACEHOLDER: " + name; | ||
size_t pos = result.find(placeholder); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Idea for a faster approach: Instead of searching the whole source string for each snippet, we could search for the placeholder positions during initialization of the class and save the positions. Then when we want to insert the snippets later, we just have to look up the positions again.
Probably not that big of a deal right now, but it might be more performant on large template files.
|
||
auto shaderdir = path / "assets" / "test" / "shaders"; | ||
|
||
// Initialize shader templlalte |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Initialize shader templlalte | |
// Initialize shader template |
Resolve #1541