Merge pull request #71797 from reduz/cleanup-shader-parameter-remap

Clean up shader parameter remap

4 files changed, 58 insertions, 70 deletions

diff --git a/scene/resources/material.cpp b/scene/resources/material.cpp
index 3e2a952ea7..db7385428b 100644
--- a/scene/resources/material.cpp
+++ b/scene/resources/material.cpp
@@ -149,11 +149,36 @@ 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;
@@ -161,9 +186,10 @@ bool ShaderMaterial::_set(const StringName &p_name, const Variant &p_value) {
 
 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;
 		}
 	}
@@ -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);
 		}
 
@@ -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;
 		}
 	}
@@ -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;
 		}
 	}
diff --git a/scene/resources/material.h b/scene/resources/material.h
index f1777d31f4..83c7a09cc4 100644
--- a/scene/resources/material.h
+++ b/scene/resources/material.h
@@ -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);
diff --git a/scene/resources/shader.cpp b/scene/resources/shader.cpp
index 9bb2db17ab..b3952e745f 100644
--- a/scene/resources/shader.cpp
+++ b/scene/resources/shader.cpp
@@ -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();
 }
@@ -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();
 }
@@ -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;
@@ -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
@@ -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() {
@@ -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");
 
diff --git a/scene/resources/shader.h b/scene/resources/shader.h
index e579c2fca1..75c490e912 100644
--- a/scene/resources/shader.h
+++ b/scene/resources/shader.h
@@ -57,15 +57,12 @@ private:
 	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();
 
@@ -79,7 +76,6 @@ public:
 	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;
@@ -87,47 +83,6 @@ public:
 
 	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();