Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add helpful error message when using old type hint in shader language #82548

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Kalrog
Copy link

@Kalrog Kalrog commented Sep 29, 2023

  • Adds an error message telling users to use source_color if they used hint_color in their code

@Kalrog Kalrog requested a review from a team as a code owner September 29, 2023 19:53
@Chaosus Chaosus added this to the 4.2 milestone Sep 29, 2023
@akien-mga akien-mga requested a review from a team September 30, 2023 17:02
- Adds an error message telling users to use source_color if they
  used hint_color in their code

Update servers/rendering/shader_language.cpp

Co-authored-by: Yuri Rubinsky <[email protected]>
@akien-mga
Copy link
Member

akien-mga commented Nov 14, 2023

There are more obsolete type hints than hint_color, so while this PR is useful in this specific case, it's missing a bunch of cases:

const char *RenamesMap3To4::shaders_renames[][2] = {
        { "ALPHA_SCISSOR", "ALPHA_SCISSOR_THRESHOLD" },
        { "CAMERA_MATRIX", "INV_VIEW_MATRIX" },
        { "INV_CAMERA_MATRIX", "VIEW_MATRIX" },
        { "NORMALMAP", "NORMAL_MAP" },
        { "NORMALMAP_DEPTH", "NORMAL_MAP_DEPTH" },
        { "TRANSMISSION", "BACKLIGHT" },
        { "WORLD_MATRIX", "MODEL_MATRIX" },
        { "depth_draw_alpha_prepass", "depth_prepass_alpha" },
        { "hint_albedo", "source_color" },
        { "hint_aniso", "hint_anisotropy" },
        { "hint_black", "hint_default_black" },
        { "hint_black_albedo", "hint_default_black" },
        { "hint_color", "source_color" },
        { "hint_white", "hint_default_white" },
        { nullptr, nullptr },
};

Maybe it could use the above map directly (from editor/renames_map_3_to_4.cpp).

One issue though is architectural, shader_language.cpp is part of servers / the runtime and should normally not depend on editor code. This could still be added with proper #if defined(TOOLS_ENABLED) && !defined(DISABLE_DEPRECATED) guards, but it would be a breach of our guidelines: #53295.

GDScript does it though:

#if defined(TOOLS_ENABLED) && !defined(DISABLE_DEPRECATED)
#define SUGGEST_GODOT4_RENAMES
#include "editor/renames_map_3_to_4.h"
#endif

If we could handle this in the shader editor code directly, that would be best, but otherwise doing it like GDScript might be acceptable even if it's breaks encapsulation slightly (technically this editor/renames_map_3_to_4.h header file doesn't really depend on editor-specific stuff, so we could even move it to core if that was really a problem - but I wouldn't suggest doing that in this PR).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants