From 2f83b400a881a3b7e78a2bd088e184752940309e Mon Sep 17 00:00:00 2001 From: clayjohn Date: Fri, 12 Apr 2024 10:31:36 -0700 Subject: [PATCH] Warn users when assigning VERTEX directly to POSITION due to compatibility breakage from reverse z changes --- doc/classes/ProjectSettings.xml | 3 +++ servers/rendering/shader_language.cpp | 22 ++++++++++++++++++++++ servers/rendering/shader_warnings.cpp | 4 ++++ servers/rendering/shader_warnings.h | 2 ++ 4 files changed, 31 insertions(+) diff --git a/doc/classes/ProjectSettings.xml b/doc/classes/ProjectSettings.xml index a5aeee5bc4a..172559f5f60 100644 --- a/doc/classes/ProjectSettings.xml +++ b/doc/classes/ProjectSettings.xml @@ -637,6 +637,9 @@ When set to [code]true[/code], produces a warning upon encountering certain formatting errors. Currently this only checks for empty statements. More formatting errors may be added over time. + + When set to [code]true[/code], produces a warning when the shader contains [code]POSITION = vec4(vertex,[/code] as this was very common code written in Godot 4.2 and earlier that was paired with a QuadMesh to produce a full screen post processes pass. With the switch to reversed z in 4.3, this trick no longer works, as it implicitly relied on the [code]VERTEX.z[/code] being 0. + When set to [code]true[/code], warnings are treated as errors. diff --git a/servers/rendering/shader_language.cpp b/servers/rendering/shader_language.cpp index fef1a205d61..96dd81e2402 100644 --- a/servers/rendering/shader_language.cpp +++ b/servers/rendering/shader_language.cpp @@ -5029,6 +5029,10 @@ ShaderLanguage::Node *ShaderLanguage::_parse_expression(BlockNode *p_block, cons Vector expression; //Vector operators; +#ifdef DEBUG_ENABLED + bool check_position_write = check_warnings && HAS_WARNING(ShaderWarning::MAGIC_POSITION_WRITE_FLAG); + check_position_write = check_position_write && String(shader_type_identifier) == "spatial" && current_function == "vertex"; +#endif while (true) { Node *expr = nullptr; @@ -5589,6 +5593,24 @@ ShaderLanguage::Node *ShaderLanguage::_parse_expression(BlockNode *p_block, cons _set_error(vformat(RTR("Can't use function as identifier: '%s'."), String(identifier))); return nullptr; } +#ifdef DEBUG_ENABLED + if (check_position_write && ident_type == IDENTIFIER_BUILTIN_VAR) { + if (String(identifier) == "POSITION") { + // Check if the user wrote "POSITION = vec4(VERTEX," and warn if they did. + TkPos prev_pos = _get_tkpos(); + if (_get_token().type == TK_OP_ASSIGN && + _get_token().type == TK_TYPE_VEC4 && + _get_token().type == TK_PARENTHESIS_OPEN && + _get_token().text == "VERTEX" && + _get_token().type == TK_COMMA) { + _add_line_warning(ShaderWarning::MAGIC_POSITION_WRITE); + } + + // Reset the position so compiling can continue as normal. + _set_tkpos(prev_pos); + } + } +#endif if (is_const) { last_type = IDENTIFIER_CONSTANT; } else { diff --git a/servers/rendering/shader_warnings.cpp b/servers/rendering/shader_warnings.cpp index dce8f6cff11..3b99f6c2bfc 100644 --- a/servers/rendering/shader_warnings.cpp +++ b/servers/rendering/shader_warnings.cpp @@ -65,6 +65,8 @@ String ShaderWarning::get_message() const { return subject; case DEVICE_LIMIT_EXCEEDED: return vformat(RTR("The total size of the %s for this shader on this device has been exceeded (%d/%d). The shader may not work correctly."), subject, (int)extra_args[0], (int)extra_args[1]); + case MAGIC_POSITION_WRITE: + return vformat(RTR("You are attempting to assign the VERTEX position in model space to the vertex POSITION in clip space. The definition of clip space changed in version 4.3, so if this code was written prior to 4.3, it will not continue to work. Consider specifying the clip space z-component directly i.e. use `vec4(VERTEX.xy, 1.0, 1.0)`.")); default: break; } @@ -92,6 +94,7 @@ String ShaderWarning::get_name_from_code(Code p_code) { "UNUSED_LOCAL_VARIABLE", "FORMATTING_ERROR", "DEVICE_LIMIT_EXCEEDED", + "MAGIC_POSITION_WRITE", }; static_assert((sizeof(names) / sizeof(*names)) == WARNING_MAX, "Amount of warning types don't match the amount of warning names."); @@ -122,6 +125,7 @@ static void init_code_to_flags_map() { code_to_flags_map->insert(ShaderWarning::UNUSED_LOCAL_VARIABLE, ShaderWarning::UNUSED_LOCAL_VARIABLE_FLAG); code_to_flags_map->insert(ShaderWarning::FORMATTING_ERROR, ShaderWarning::FORMATTING_ERROR_FLAG); code_to_flags_map->insert(ShaderWarning::DEVICE_LIMIT_EXCEEDED, ShaderWarning::DEVICE_LIMIT_EXCEEDED_FLAG); + code_to_flags_map->insert(ShaderWarning::MAGIC_POSITION_WRITE, ShaderWarning::MAGIC_POSITION_WRITE_FLAG); } ShaderWarning::CodeFlags ShaderWarning::get_flags_from_codemap(const HashMap &p_map) { diff --git a/servers/rendering/shader_warnings.h b/servers/rendering/shader_warnings.h index ed28ebdd2b9..69d684850f7 100644 --- a/servers/rendering/shader_warnings.h +++ b/servers/rendering/shader_warnings.h @@ -51,6 +51,7 @@ public: UNUSED_LOCAL_VARIABLE, FORMATTING_ERROR, DEVICE_LIMIT_EXCEEDED, + MAGIC_POSITION_WRITE, WARNING_MAX, }; @@ -65,6 +66,7 @@ public: UNUSED_LOCAL_VARIABLE_FLAG = 64U, FORMATTING_ERROR_FLAG = 128U, DEVICE_LIMIT_EXCEEDED_FLAG = 256U, + MAGIC_POSITION_WRITE_FLAG = 512U, }; private: