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 preprocessor pass on visual shader when showing generated code #82570

Merged

Conversation

100gold
Copy link
Contributor

@100gold 100gold commented Sep 30, 2023

Fixes #82555
Add preprocessor pass and check for errors in visual shader editor.

@100gold 100gold requested a review from a team as a code owner September 30, 2023 02:35
@100gold 100gold force-pushed the preprocessor_in_vshader_editor branch from 7d5c126 to 60d095b Compare September 30, 2023 02:45
@Chaosus Chaosus added this to the 4.2 milestone Sep 30, 2023
@bitsawer
Copy link
Member

bitsawer commented Oct 2, 2023

Looks pretty good. I would also add the preprocessor error file name, which makes it easier to find the actual issue if you have complex or nested includes.

Also, looks like the error line number highlight is off by one, needs - 1. Updated section of the code would be this:

if (err != OK) {
    ERR_FAIL_COND(err_positions.size() == 0);

    String file = err_positions.front()->get().file;
    int err_line = err_positions.front()->get().line;
    Color error_line_color = EDITOR_GET("text_editor/theme/highlighting/mark_color");
    preview_text->set_line_background_color(err_line - 1, error_line_color);
    error_panel->show();

    error_label->set_text("error(" + file + ":" + itos(err_line) + "): " + error_pp);
    shader_error = true;
    return;
}

@100gold 100gold force-pushed the preprocessor_in_vshader_editor branch from 60d095b to 7ce0e74 Compare October 2, 2023 15:25
@100gold
Copy link
Contributor Author

100gold commented Oct 2, 2023

Looks pretty good. I would also add the preprocessor error file name, which makes it easier to find the actual issue if you have complex or nested includes.

Also, looks like the error line number highlight is off by one, needs - 1. Updated section of the code would be this:

...

Thanks! There was another issue with the error line number when there was an error in the included file. Fixed both issues.

Copy link
Member

@bitsawer bitsawer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested locally and works as expected. One comment should be updated to the new format (capitalized and ends with a dot), but other than that looks good.

As a note to myself and future maintainers, this code is now pretty similar to ShaderTextEditor version with some small differences, so it might be possible to merge them in the future. No need to do that in this PR, however.

editor/plugins/visual_shader_editor_plugin.cpp Outdated Show resolved Hide resolved
@100gold 100gold force-pushed the preprocessor_in_vshader_editor branch from 6ab10f7 to 0486d40 Compare October 29, 2023 16:12
@AThousandShips
Copy link
Member

Your commit seems not to be linked to your GitHub account. See: Why are my commits linked to the wrong user? for more info.

@100gold 100gold requested a review from bitsawer October 29, 2023 16:38
Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems fine to me. I am not familiar with this code, but I trust the other reviewers

@akien-mga akien-mga merged commit 55fca13 into godotengine:master Oct 30, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

@100gold 100gold deleted the preprocessor_in_vshader_editor branch January 7, 2024 03:09
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.

Using preprocessor in visual shader global expression produces error "Tokenizer: unknown character #35"
6 participants