From 67038471ffdaf91d8385c9324698e99443c72056 Mon Sep 17 00:00:00 2001
From: bitsawer <sawerduster@gmail.com>
Date: Mon, 29 May 2023 12:05:22 +0300
Subject: [PATCH] Fix shader preprocessor cyclic include handling

---
 scene/resources/shader.cpp                | 16 ++++++++--------
 scene/resources/shader_include.cpp        | 15 ++++++++-------
 servers/rendering/shader_preprocessor.cpp |  5 +++--
 3 files changed, 19 insertions(+), 17 deletions(-)

diff --git a/scene/resources/shader.cpp b/scene/resources/shader.cpp
index fd2be9ba22d0..62c2483a930e 100644
--- 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
index 68137cbec0a5..6e9c64d34b43 100644
--- 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
index 0644f5918cc5..f8081cda44d5 100644
--- 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);
 	}