Skip to content

Commit

Permalink
Clean up shader parameter remap
Browse files Browse the repository at this point in the history 
This PR is a follow up to godotengine#64092, which fixed important issues but it was implemented in an overly complex and inefficient way (because it forced the default code path to always go through string operations).
This cleans up all the shader parameter code.

This fixes godotengine#54336. Also fixes godotengine#56219 because, as the new code never queries the RenderingServer on load, potential deadlocks are avoided.

**NOTE**: materials saved between godotengine#62972 and godotengine#64092 will no longer work and will need to be resaved in an earlier version.
  • Loading branch information
@reduz
reduz committed Jan 21, 2023
1 parent 9f74f0f commit 7dbc458
Show file tree
Hide file tree
Showing 5 changed files with 63 additions and 75 deletions.
10 changes: 5 additions & 5 deletions doc/classes/Shader.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,12 +26,12 @@
Returns the shader mode for the shader, either [constant MODE_CANVAS_ITEM], [constant MODE_SPATIAL] or [constant MODE_PARTICLES].
</description>
</method>
<method name="has_parameter" qualifiers="const">
<return type="bool" />
<param index="0" name="name" type="StringName" />
<method name="get_shader_uniform_list">
<return type="Array" />
<param index="0" name="get_groups" type="bool" default="false" />
<description>
Returns [code]true[/code] if the shader has this param defined as a uniform in its code.
[b]Note:[/b] [param name] must match the name of the uniform in the code exactly.
Get the list of shader uniforms that can be assigned to a [ShaderMaterial], for use with [method ShaderMaterial.set_shader_parameter] and [method ShaderMaterial.get_shader_parameter]. The parameters returned are contained in dictionaries in a similar format to the ones returned by [method Object.get_property_list].
If argument [param get_groups] is true, parameter grouping hints will be provided.
</description>
</method>
<method name="set_default_texture_parameter">
Expand Down
57 changes: 44 additions & 13 deletions scene/resources/material.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -149,21 +149,47 @@ Material::~Material() {

bool ShaderMaterial::_set(const StringName &p_name, const Variant &p_value) {
if (shader.is_valid()) {
StringName pr = shader->remap_parameter(p_name);
if (pr) {
set_shader_parameter(pr, p_value);
const StringName *sn = remap_cache.getptr(p_name);
if (sn) {
set_shader_parameter(*sn, p_value);
return true;
}
String s = p_name;
if (s.begins_with("shader_parameter/")) {
String param = s.replace_first("shader_parameter/", "");
remap_cache[s] = param;
set_shader_parameter(param, p_value);
return true;
}
#ifndef DISABLE_DEPRECATED
// Compatibility remaps are only needed here.
if (s.begins_with("param/")) {
s = s.replace_first("param/", "shader_parameter/");
} else if (s.begins_with("shader_param/")) {
s = s.replace_first("shader_param/", "shader_parameter/");
} else if (s.begins_with("shader_uniform/")) {
s = s.replace_first("shader_uniform/", "shader_parameter/");
} else {
return false; // Not a shader parameter.
}

WARN_PRINT("This material (containing shader with path: '" + shader->get_path() + "') uses an old deprecated parameter names. Consider re-saving this resource (or scene which contains it) in order for it to continue working in future versions.");
String param = s.replace_first("shader_parameter/", "");
remap_cache[s] = param;
set_shader_parameter(param, p_value);
return true;
#endif
}

return false;
}

bool ShaderMaterial::_get(const StringName &p_name, Variant &r_ret) const {
if (shader.is_valid()) {
StringName pr = shader->remap_parameter(p_name);
if (pr) {
r_ret = get_shader_parameter(pr);
const StringName *sn = remap_cache.getptr(p_name);
if (sn) {
// Only return a parameter if it was previosly set.
r_ret = get_shader_parameter(*sn);
return true;
}
}
Expand Down Expand Up @@ -247,6 +273,12 @@ void ShaderMaterial::_get_property_list(List<PropertyInfo> *p_list) const {

PropertyInfo info = E->get();
info.name = "shader_parameter/" + info.name;
if (!param_cache.has(E->get().name)) {
// Property has never been edited, retrieve with default value.
Variant default_value = RenderingServer::get_singleton()->shader_get_parameter_default(shader->get_rid(), E->get().name);
param_cache.insert(E->get().name, default_value);
remap_cache.insert(info.name, E->get().name);
}
groups[last_group][last_subgroup].push_back(info);
}

Expand Down Expand Up @@ -275,11 +307,10 @@ void ShaderMaterial::_get_property_list(List<PropertyInfo> *p_list) const {

bool ShaderMaterial::_property_can_revert(const StringName &p_name) const {
if (shader.is_valid()) {
StringName pr = shader->remap_parameter(p_name);
const StringName *pr = remap_cache.getptr(p_name);
if (pr) {
Variant default_value = RenderingServer::get_singleton()->shader_get_parameter_default(shader->get_rid(), pr);
Variant current_value;
_get(p_name, current_value);
Variant default_value = RenderingServer::get_singleton()->shader_get_parameter_default(shader->get_rid(), *pr);
Variant current_value = get_shader_parameter(*pr);
return default_value.get_type() != Variant::NIL && default_value != current_value;
}
}
Expand All @@ -288,9 +319,9 @@ bool ShaderMaterial::_property_can_revert(const StringName &p_name) const {

bool ShaderMaterial::_property_get_revert(const StringName &p_name, Variant &r_property) const {
if (shader.is_valid()) {
StringName pr = shader->remap_parameter(p_name);
if (pr) {
r_property = RenderingServer::get_singleton()->shader_get_parameter_default(shader->get_rid(), pr);
const StringName *pr = remap_cache.getptr(p_name);
if (*pr) {
r_property = RenderingServer::get_singleton()->shader_get_parameter_default(shader->get_rid(), *pr);
return true;
}
}
Expand Down
3 changes: 2 additions & 1 deletion scene/resources/material.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,8 @@ class ShaderMaterial : public Material {
GDCLASS(ShaderMaterial, Material);
Ref<Shader> shader;

HashMap<StringName, Variant> param_cache;
mutable HashMap<StringName, StringName> remap_cache;
mutable HashMap<StringName, Variant> param_cache;

protected:
bool _set(const StringName &p_name, const Variant &p_value);
Expand Down
19 changes: 10 additions & 9 deletions scene/resources/shader.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,6 @@ Shader::Mode Shader::get_mode() const {

void Shader::_dependency_changed() {
RenderingServer::get_singleton()->shader_set_code(shader, RenderingServer::get_singleton()->shader_get_code(shader));
params_cache_dirty = true;

emit_changed();
}
Expand Down Expand Up @@ -93,7 +92,6 @@ void Shader::set_code(const String &p_code) {
}

RenderingServer::get_singleton()->shader_set_code(shader, pp_code);
params_cache_dirty = true;

emit_changed();
}
Expand All @@ -108,8 +106,6 @@ void Shader::get_shader_uniform_list(List<PropertyInfo> *p_params, bool p_get_gr

List<PropertyInfo> local;
RenderingServer::get_singleton()->get_shader_parameter_list(shader, &local);
params_cache.clear();
params_cache_dirty = false;

for (PropertyInfo &pi : local) {
bool is_group = pi.usage == PROPERTY_USAGE_GROUP || pi.usage == PROPERTY_USAGE_SUBGROUP;
Expand All @@ -120,7 +116,6 @@ void Shader::get_shader_uniform_list(List<PropertyInfo> *p_params, bool p_get_gr
if (default_textures.has(pi.name)) { //do not show default textures
continue;
}
params_cache[pi.name] = pi.name;
}
if (p_params) {
//small little hack
Expand Down Expand Up @@ -176,11 +171,17 @@ bool Shader::is_text_shader() const {
return true;
}

bool Shader::has_parameter(const StringName &p_name) const {
return params_cache.has(p_name);
void Shader::_update_shader() const {
}

void Shader::_update_shader() const {
Array Shader::_get_shader_uniform_list(bool p_get_groups) {
List<PropertyInfo> uniform_list;
get_shader_uniform_list(&uniform_list, p_get_groups);
Array ret;
for (const PropertyInfo &pi : uniform_list) {
ret.push_back(pi.operator Dictionary());
}
return ret;
}

void Shader::_bind_methods() {
Expand All @@ -192,7 +193,7 @@ void Shader::_bind_methods() {
ClassDB::bind_method(D_METHOD("set_default_texture_parameter", "name", "texture", "index"), &Shader::set_default_texture_parameter, DEFVAL(0));
ClassDB::bind_method(D_METHOD("get_default_texture_parameter", "name", "index"), &Shader::get_default_texture_parameter, DEFVAL(0));

ClassDB::bind_method(D_METHOD("has_parameter", "name"), &Shader::has_parameter);
ClassDB::bind_method(D_METHOD("get_shader_uniform_list", "get_groups"), &Shader::_get_shader_uniform_list, DEFVAL(false));

ADD_PROPERTY(PropertyInfo(Variant::STRING, "code", PROPERTY_HINT_NONE, "", PROPERTY_USAGE_NO_EDITOR), "set_code", "get_code");

Expand Down
49 changes: 2 additions & 47 deletions scene/resources/shader.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,15 +57,12 @@ class Shader : public Resource {
HashSet<Ref<ShaderInclude>> include_dependencies;
String code;

// hack the name of performance
// shaders keep a list of ShaderMaterial -> RenderingServer name translations, to make
// conversion fast and save memory.
mutable bool params_cache_dirty = true;
mutable HashMap<StringName, StringName> params_cache; //map a shader param to a material param..
HashMap<StringName, HashMap<int, Ref<Texture2D>>> default_textures;

void _dependency_changed();
virtual void _update_shader() const; //used for visual shader
Array _get_shader_uniform_list(bool p_get_groups = false);

protected:
static void _bind_methods();

Expand All @@ -79,55 +76,13 @@ class Shader : public Resource {
String get_code() const;

void get_shader_uniform_list(List<PropertyInfo> *p_params, bool p_get_groups = false) const;
bool has_parameter(const StringName &p_name) const;

void set_default_texture_parameter(const StringName &p_name, const Ref<Texture2D> &p_texture, int p_index = 0);
Ref<Texture2D> get_default_texture_parameter(const StringName &p_name, int p_index = 0) const;
void get_default_texture_parameter_list(List<StringName> *r_textures) const;

virtual bool is_text_shader() const;

// Finds the shader parameter name for the given property name, which should start with "shader_parameter/".
_FORCE_INLINE_ StringName remap_parameter(const StringName &p_property) const {
if (params_cache_dirty) {
get_shader_uniform_list(nullptr);
}

String n = p_property;

// Backwards compatibility with old shader parameter names.
// Note: The if statements are important to make sure we are only replacing text exactly at index 0.
if (n.find("param/") == 0) {
n = n.replace_first("param/", "shader_parameter/");
}
if (n.find("shader_param/") == 0) {
n = n.replace_first("shader_param/", "shader_parameter/");
}
if (n.find("shader_uniform/") == 0) {
n = n.replace_first("shader_uniform/", "shader_parameter/");
}

{
// Additional backwards compatibility for projects between #62972 and #64092 (about a month of v4.0 development).
// These projects did not have any prefix for shader uniforms due to a bug.
// This code should be removed during beta or rc of 4.0.
const HashMap<StringName, StringName>::Iterator E = params_cache.find(n);
if (E) {
return E->value;
}
}

if (n.begins_with("shader_parameter/")) {
n = n.replace_first("shader_parameter/", "");
const HashMap<StringName, StringName>::Iterator E = params_cache.find(n);
if (E) {
return E->value;
}
}

return StringName();
}

virtual RID get_rid() const override;

Shader();
Expand Down

0 comments on commit 7dbc458

Please sign in to comment.