-
-
Notifications
You must be signed in to change notification settings - Fork 21.1k
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
Use raw string literals for BaseMaterial3D shader code generation #89267
Use raw string literals for BaseMaterial3D shader code generation #89267
Conversation
scene/resources/material.cpp
Outdated
code += R"( | ||
uniform float roughness : hint_range(0.0, 1.0); | ||
uniform sampler2D texture_metallic : hint_default_white, )" + | ||
texfilter_str + R"(; | ||
uniform vec4 metallic_texture_channel; | ||
)"; |
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.
Likewise, if this can work with vformat
, I think it would be more readable. The closing parenthesis of the raw-string format in the middle of the string really hurts readability IMO, in something that can typically be generated code with parentheses.
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.
For me, for raw strings to be usable, there needs to be a clear separation between the R"(
starting and closing )"
tokens, and the actual generated code, so never use R"(;
to start with a closing semicolon, and avoid concatenation and re-starting a raw-string mid statement.
code += R"( | |
uniform float roughness : hint_range(0.0, 1.0); | |
uniform sampler2D texture_metallic : hint_default_white, )" + | |
texfilter_str + R"(; | |
uniform vec4 metallic_texture_channel; | |
)"; | |
code += vformat(R"( | |
uniform float roughness : hint_range(0.0, 1.0); | |
uniform sampler2D texture_metallic : hint_default_white, %s; | |
uniform vec4 metallic_texture_channel; | |
)", | |
texfilter_str); |
In other words, IMO, raw strings should only be used for full lines / paragraphs.
The best would be something like Python f-strings where we could use something like {texfilter_str}
in the raw string to inject it, but I don't think that's supported.
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 missed changing this one it seems.
scene/resources/material.cpp
Outdated
if (shading_mode == SHADING_MODE_PER_VERTEX) { | ||
code += " ROUGHNESS=roughness;\n"; | ||
code += " ROUGHNESS = roughness;\n"; | ||
} | ||
|
||
if (!flags[FLAG_UV1_USE_TRIPLANAR]) { | ||
code += " UV=UV*uv1_scale.xy+uv1_offset.xy;\n"; | ||
code += " UV = UV * uv1_scale.xy + uv1_offset.xy;\n"; | ||
} |
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'm wondering if we should systematically use R-strings (with a line break after R"(
and before ");
) even for single line additions like this. This would add two extra lines of code each time, but the advantage would be that all generated code would start at column 0 in the file, and have its indentation convey the actual generated code indentation. This should make it easier to ensure that indentation is consistent.
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'm wondering if we should systematically use R-strings (with a line break after R"( and before ");) even for single line additions like this.
If we do this, then every line of code will be separated by two blank lines in the generated shader since there are two newlines embedded in the string:
R"(
something
)"
We could do this to avoid always adding 2 newlines where relevant, but then the newline will be at the beginning of the string instead of the end:
R"(
something)"
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.
Hm yeah that's subpar either way. Amazing that even with a new feature like R-strings they couldn't figure out an actually user-friendly syntax :D
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.
To be fair, most languages with multiline strings have this issue. You typically need to call a method to trim leading/trailing whitespace if you don't want these to be part of the final string (which means it's not 100% compile-time anymore).
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.
Looks great overall! Some stylistic decisions to make, see my comments, but this is already a very nice improvement.
93e3716
to
a5be464
Compare
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.
Looks really good! Final nitpicks, but then I think we should merge. It likely won't be perfect as there are too many conditionals to really ensure especially that vertical spacing will always be consistent. But it should be pretty close to perfect :)
scene/resources/material.cpp
Outdated
code += R"( | ||
uniform float roughness : hint_range(0.0, 1.0); | ||
uniform sampler2D texture_metallic : hint_default_white, )" + | ||
texfilter_str + R"(; | ||
uniform vec4 metallic_texture_channel; | ||
)"; |
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 missed changing this one it seems.
- Add range hints to all uniforms that match the BaseMaterial3D property hints, so that ranges in the inspector remain identical after converting to a shader. - Add comments to describe selected options within the shader. This makes it easier to remember what each block of code does. - Format code to follow the Godot shader language style guide.
a5be464
to
1e2b899
Compare
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.
Haven't re-reviewed in depth, but I believe this is ready to go now.
Thanks! |
particle_process_material
instead of adding statements line by line #85595.Testing material (before conversion): max_material.tres.zip
Preview
Minimal material
Create a BaseMaterial3D then convert it to a ShaderMaterial.
Before
After
Full-blown material
Create a BaseMaterial3D, enable all features except triplanar mapping (which is incompatible with height mapping) then convert it to a ShaderMaterial.
Before
After