Merge pull request #109999 from aaronfranke/shader-cleanup

Clean up some things in shader editor code

Author: Repiteo

editor/shader/shader_create_dialog.cpp

@@ -67,36 +67,45 @@ void ShaderCreateDialog::_notification(int p_what) {
 		} break;
 
 		case NOTIFICATION_THEME_CHANGED: {
-			static const char *shader_types[3] = { "Shader", "VisualShader", "TextFile" };
-			for (int i = 0; i < 3; i++) {
-				Ref<Texture2D> icon = get_editor_theme_icon(shader_types[i]);
-				if (icon.is_valid()) {
-					type_menu->set_item_icon(i, icon);
-				}
-			}
-
+			_refresh_type_icons();
 			path_button->set_button_icon(get_editor_theme_icon(SNAME("Folder")));
 		} break;
 	}
 }
 
+void ShaderCreateDialog::_refresh_type_icons() {
+	for (int i = 0; i < type_menu->get_item_count(); i++) {
+		const String item_name = type_menu->get_item_text(i);
+		Ref<Texture2D> icon = get_editor_theme_icon(item_name);
+		if (icon.is_valid()) {
+			type_menu->set_item_icon(i, icon);
+		} else {
+			icon = get_editor_theme_icon("TextFile");
+			if (icon.is_valid()) {
+				type_menu->set_item_icon(i, icon);
+			}
+		}
+	}
+}
+
 void ShaderCreateDialog::_update_language_info() {
 	type_data.clear();
 
 	for (int i = 0; i < SHADER_TYPE_MAX; i++) {
 		ShaderTypeData shader_type_data;
 		if (i == int(SHADER_TYPE_TEXT)) {
 			shader_type_data.use_templates = true;
-			shader_type_data.extensions.push_back("gdshader");
 			shader_type_data.default_extension = "gdshader";
 		} else if (i == int(SHADER_TYPE_INC)) {
-			shader_type_data.extensions.push_back("gdshaderinc");
 			shader_type_data.default_extension = "gdshaderinc";
 		} else {
 			shader_type_data.default_extension = "tres";
 		}
+		shader_type_data.extensions.push_back(shader_type_data.default_extension);
+		if (shader_type_data.default_extension != "tres") {
+			shader_type_data.extensions.push_back("tres");
+		}
 		shader_type_data.extensions.push_back("res");
-		shader_type_data.extensions.push_back("tres");
 		type_data.push_back(shader_type_data);
 	}
 }
@@ -432,31 +441,33 @@ void ShaderCreateDialog::config(const String &p_base_path, bool p_built_in_enabl
 }
 
 String ShaderCreateDialog::_validate_path(const String &p_path) {
-	String p = p_path.strip_edges();
+	ERR_FAIL_COND_V(current_type >= type_data.size(), TTR("Invalid shader type selected."));
+	String stripped_file_path = p_path.strip_edges();
 
-	if (p.is_empty()) {
+	if (stripped_file_path.is_empty()) {
 		return TTR("Path is empty.");
 	}
-	if (p.get_file().get_basename().is_empty()) {
+	if (stripped_file_path.get_file().get_basename().is_empty()) {
 		return TTR("Filename is empty.");
 	}
 
-	p = ProjectSettings::get_singleton()->localize_path(p);
-	if (!p.begins_with("res://")) {
+	stripped_file_path = ProjectSettings::get_singleton()->localize_path(stripped_file_path);
+	if (!stripped_file_path.begins_with("res://")) {
 		return TTR("Path is not local.");
 	}
 
 	Ref<DirAccess> d = DirAccess::create(DirAccess::ACCESS_RESOURCES);
-	if (d->change_dir(p.get_base_dir()) != OK) {
+	if (d->change_dir(stripped_file_path.get_base_dir()) != OK) {
 		return TTR("Invalid base path.");
 	}
 
 	Ref<DirAccess> f = DirAccess::create(DirAccess::ACCESS_RESOURCES);
-	if (f->dir_exists(p)) {
+	if (f->dir_exists(stripped_file_path)) {
 		return TTR("A directory with the same name exists.");
 	}
 
-	String extension = p.get_extension();
+	const ShaderCreateDialog::ShaderTypeData &current_type_data = type_data.get(current_type);
+	const String file_extension = stripped_file_path.get_extension();
 	HashSet<String> extensions;
 
 	List<ShaderCreateDialog::ShaderTypeData>::ConstIterator itr = type_data.begin();
@@ -472,10 +483,10 @@ String ShaderCreateDialog::_validate_path(const String &p_path) {
 	bool match = false;
 
 	for (const String &ext : extensions) {
-		if (ext.nocasecmp_to(extension) == 0) {
+		if (ext.nocasecmp_to(file_extension) == 0) {
 			found = true;
-			for (const String &type_ext : type_data.get(current_type).extensions) {
-				if (type_ext.nocasecmp_to(extension) == 0) {
+			for (const String &type_ext : current_type_data.extensions) {
+				if (type_ext.nocasecmp_to(file_extension) == 0) {
 					match = true;
 					break;
 				}

editor/shader/shader_create_dialog.h

@@ -93,6 +93,7 @@ class ShaderCreateDialog : public ConfirmationDialog {
 	void _mode_changed(int p_mode = 0);
 	void _browse_path();
 	void _file_selected(const String &p_file);
+	void _refresh_type_icons();
 	String _validate_path(const String &p_path);
 	virtual void ok_pressed() override;
 	void _create_new();

editor/shader/shader_editor_plugin.cpp

@@ -137,33 +137,37 @@ void ShaderEditorPlugin::edit(Object *p_object) {
 	if (!p_object) {
 		return;
 	}
-
 	EditedShader es;
-
-	ShaderInclude *si = Object::cast_to<ShaderInclude>(p_object);
-	if (si != nullptr) {
+	// First, check for ShaderInclude.
+	ShaderInclude *shader_include = Object::cast_to<ShaderInclude>(p_object);
+	if (shader_include != nullptr) {
+		// Check if this shader include is already being edited.
 		for (uint32_t i = 0; i < edited_shaders.size(); i++) {
-			if (edited_shaders[i].shader_inc.ptr() == si) {
+			if (edited_shaders[i].shader_inc.ptr() == shader_include) {
 				shader_tabs->set_current_tab(i);
 				shader_list->select(i);
 				_switch_to_editor(edited_shaders[i].shader_editor);
 				return;
 			}
 		}
-		es.shader_inc = Ref<ShaderInclude>(si);
+		es.shader_inc = Ref<ShaderInclude>(shader_include);
 		es.shader_editor = memnew(TextShaderEditor);
-		es.shader_editor->edit_shader_include(si);
+		es.shader_editor->edit_shader_include(shader_include);
 	} else {
-		Shader *s = Object::cast_to<Shader>(p_object);
+		// If it's not a ShaderInclude, check for Shader.
+		Shader *shader = Object::cast_to<Shader>(p_object);
+		ERR_FAIL_NULL_MSG(shader, "ShaderEditorPlugin: Unable to edit object " + p_object->to_string() + " because it is not a Shader or ShaderInclude.");
+		// Check if this shader is already being edited.
 		for (uint32_t i = 0; i < edited_shaders.size(); i++) {
-			if (edited_shaders[i].shader.ptr() == s) {
+			if (edited_shaders[i].shader.ptr() == shader) {
 				shader_tabs->set_current_tab(i);
 				shader_list->select(i);
 				_switch_to_editor(edited_shaders[i].shader_editor);
 				return;
 			}
 		}
-		es.shader = Ref<Shader>(s);
+		// If we did not return, the shader needs to be opened in a new shader editor.
+		es.shader = Ref<Shader>(shader);
 		Ref<VisualShader> vs = es.shader;
 		if (vs.is_valid()) {
 			es.shader_editor = memnew(VisualShaderEditor);
@@ -173,6 +177,7 @@ void ShaderEditorPlugin::edit(Object *p_object) {
 		es.shader_editor->edit_shader(es.shader);
 	}
 
+	// TextShaderEditor-specific setup code.
 	TextShaderEditor *text_shader_editor = Object::cast_to<TextShaderEditor>(es.shader_editor);
 	if (text_shader_editor) {
 		text_shader_editor->connect("validation_changed", callable_mp(this, &ShaderEditorPlugin::_update_shader_list));
@@ -434,7 +439,7 @@ void ShaderEditorPlugin::_make_script_list_context_menu() {
 	}
 
 	Control *control = shader_tabs->get_tab_control(selected);
-	bool is_valid_editor_control = Object::cast_to<TextShaderEditor>(control) || Object::cast_to<VisualShaderEditor>(control);
+	bool is_valid_editor_control = Object::cast_to<ShaderEditor>(control) != nullptr;
 
 	_setup_popup_menu(is_valid_editor_control ? CONTEXT_VALID_ITEM : CONTEXT, context_menu);
 
@@ -774,6 +779,7 @@ void ShaderEditorPlugin::_update_shader_editor_zoom_factor(CodeTextEditor *p_sha
 }
 
 void ShaderEditorPlugin::_switch_to_editor(ShaderEditor *p_editor) {
+	ERR_FAIL_NULL(p_editor);
 	if (file_menu->get_parent() != nullptr) {
 		file_menu->get_parent()->remove_child(file_menu);
 	}

scene/resources/shader.h

@@ -41,6 +41,7 @@ class Shader : public Resource {
 	OBJ_SAVE_TYPE(Shader);
 
 public:
+	// Must be kept in sync with the List<String> of shader types in `servers/rendering/shader_types.cpp`.
 	enum Mode {
 		MODE_SPATIAL,
 		MODE_CANVAS_ITEM,

servers/rendering/shader_types.cpp

@@ -30,6 +30,8 @@
 
 #include "shader_types.h"
 
+#include "scene/resources/shader.h"
+
 const HashMap<StringName, ShaderLanguage::FunctionInfo> &ShaderTypes::get_functions(RS::ShaderMode p_mode) const {
 	return shader_modes[p_mode].functions;
 }
@@ -516,11 +518,13 @@ ShaderTypes::ShaderTypes() {
 	shader_modes[RS::SHADER_FOG].functions["fog"].built_ins["EMISSION"] = ShaderLanguage::TYPE_VEC3;
 	shader_modes[RS::SHADER_FOG].functions["fog"].main_function = true;
 
+	// Must be kept in sync with the Shader::Mode enum.
 	shader_types_list.push_back("spatial");
 	shader_types_list.push_back("canvas_item");
 	shader_types_list.push_back("particles");
 	shader_types_list.push_back("sky");
 	shader_types_list.push_back("fog");
+	DEV_ASSERT(shader_types_list.size() == Shader::MODE_MAX);
 
 	for (const String &type : shader_types_list) {
 		shader_types.insert(type);