Merge pull request #77608 from bitsawer/fix_cyclic_includes

Fix shader preprocessor cyclic include handling
godotengine · May 30, 2023 · faa73c9 · faa73c9
2 parents 3a895ea + 6703847
commit faa73c9
Show file tree

Hide file tree

Showing 3 changed files with 19 additions and 17 deletions.
diff --git a/scene/resources/shader.cpp b/scene/resources/shader.cpp
@@ -62,7 +62,7 @@ void Shader::set_include_path(const String &p_path) {
 }
 
 void Shader::set_code(const String &p_code) {
-	for (Ref<ShaderInclude> E : include_dependencies) {
+	for (const Ref<ShaderInclude> &E : include_dependencies) {
 		E->disconnect(SNAME("changed"), callable_mp(this, &Shader::_dependency_changed));
 	}
 
@@ -83,8 +83,6 @@ void Shader::set_code(const String &p_code) {
 	code = p_code;
 	String pp_code = p_code;
 
-	HashSet<Ref<ShaderInclude>> new_include_dependencies;
-
 	{
 		String path = get_path();
 		if (path.is_empty()) {
@@ -93,14 +91,16 @@ void Shader::set_code(const String &p_code) {
 		// Preprocessor must run here and not in the server because:
 		// 1) Need to keep track of include dependencies at resource level
 		// 2) Server does not do interaction with Resource filetypes, this is a scene level feature.
+		HashSet<Ref<ShaderInclude>> new_include_dependencies;
 		ShaderPreprocessor preprocessor;
-		preprocessor.preprocess(p_code, path, pp_code, nullptr, nullptr, nullptr, &new_include_dependencies);
+		Error result = preprocessor.preprocess(p_code, path, pp_code, nullptr, nullptr, nullptr, &new_include_dependencies);
+		if (result == OK) {
+			// This ensures previous include resources are not freed and then re-loaded during parse (which would make compiling slower)
+			include_dependencies = new_include_dependencies;
+		}
 	}
 
-	// This ensures previous include resources are not freed and then re-loaded during parse (which would make compiling slower)
-	include_dependencies = new_include_dependencies;
-
-	for (Ref<ShaderInclude> E : include_dependencies) {
+	for (const Ref<ShaderInclude> &E : include_dependencies) {
 		E->connect(SNAME("changed"), callable_mp(this, &Shader::_dependency_changed));
 	}
 

diff --git a/scene/resources/shader_include.cpp b/scene/resources/shader_include.cpp
@@ -37,10 +37,9 @@ void ShaderInclude::_dependency_changed() {
 }
 
 void ShaderInclude::set_code(const String &p_code) {
-	HashSet<Ref<ShaderInclude>> new_dependencies;
 	code = p_code;
 
-	for (Ref<ShaderInclude> E : dependencies) {
+	for (const Ref<ShaderInclude> &E : dependencies) {
 		E->disconnect(SNAME("changed"), callable_mp(this, &ShaderInclude::_dependency_changed));
 	}
 
@@ -51,14 +50,16 @@ void ShaderInclude::set_code(const String &p_code) {
 		}
 
 		String pp_code;
+		HashSet<Ref<ShaderInclude>> new_dependencies;
 		ShaderPreprocessor preprocessor;
-		preprocessor.preprocess(p_code, path, pp_code, nullptr, nullptr, nullptr, &new_dependencies);
+		Error result = preprocessor.preprocess(p_code, path, pp_code, nullptr, nullptr, nullptr, &new_dependencies);
+		if (result == OK) {
+			// This ensures previous include resources are not freed and then re-loaded during parse (which would make compiling slower)
+			dependencies = new_dependencies;
+		}
 	}
 
-	// This ensures previous include resources are not freed and then re-loaded during parse (which would make compiling slower)
-	dependencies = new_dependencies;
-
-	for (Ref<ShaderInclude> E : dependencies) {
+	for (const Ref<ShaderInclude> &E : dependencies) {
 		E->connect(SNAME("changed"), callable_mp(this, &ShaderInclude::_dependency_changed));
 	}
 

diff --git a/servers/rendering/shader_preprocessor.cpp b/servers/rendering/shader_preprocessor.cpp
@@ -700,7 +700,7 @@ void ShaderPreprocessor::process_include(Tokenizer *p_tokenizer) {
 	if (!included.is_empty()) {
 		uint64_t code_hash = included.hash64();
 		if (state->cyclic_include_hashes.find(code_hash)) {
-			set_error(RTR("Cyclic include found."), line);
+			set_error(RTR("Cyclic include found") + ": " + path, line);
 			return;
 		}
 	}
@@ -733,7 +733,7 @@ void ShaderPreprocessor::process_include(Tokenizer *p_tokenizer) {
 
 	FilePosition fp;
 	fp.file = state->current_filename;
-	fp.line = line;
+	fp.line = line + 1;
 	state->include_positions.push_back(fp);
 
 	String result;
@@ -1157,6 +1157,7 @@ void ShaderPreprocessor::set_error(const String &p_error, int p_line) {
 	if (state->error.is_empty()) {
 		state->error = p_error;
 		FilePosition fp;
+		fp.file = state->current_filename;
 		fp.line = p_line + 1;
 		state->include_positions.push_back(fp);
 	}