-
-
Notifications
You must be signed in to change notification settings - Fork 21.4k
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 error for undefined function in shader #79459
Add error for undefined function in shader #79459
Conversation
I just noticed a similar but different warning elsewhere:
This one should actually fire if we return false. An option would be to return false instead of setting an error, as that would trigger behavior that is already defined, but not visbily triggered at the moment. |
Indeed, there are a few places that call if (exists) {
if (last_arg_count > args.size()) {
_set_error(vformat(RTR("Too few arguments for \"%s(%s)\" call. Expected at least %d but received %d."), String(name), arg_list, last_arg_count, args.size()));
} else if (last_arg_count < args.size()) {
_set_error(vformat(RTR("Too many arguments for \"%s(%s)\" call. Expected at most %d but received %d."), String(name), arg_list, last_arg_count, args.size()));
}
} |
That seems like a better solution indeed! Seems to work alright. |
You need to fix the code formatting to pass tests, we use clang-format: https://docs.godotengine.org/en/latest/contributing/development/code_style_guidelines.html#using-clang-format-locally (in this case you might be able to fix the style by hand, but using the tool is more reliable). Also, please squash your commits into a single commit, see https://docs.godotengine.org/en/latest/contributing/workflow/pr_workflow.html#the-interactive-rebase |
60eadd5
to
16c3f4b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested locally and it works, looks good to me.
So if I understand correctly, we aren't actually adding a new error message, we already have one. And with this PR we are preventing earlier checks from triggering and showing a less useful message, right? |
Correct. The PR was created under a different assumption and although it has been updated since, the title hasn't. |
Thanks! And congrats on your first merged Godot pull request! |
Problem
Currently, as described in #72120, when an undefined function is called in a shader, an error is shown for the number of arguments, as the for-loop looking for functions did not find any but does following code to continue.
Solution
Adds an error when an undefined function is called in a shader, when the loop does not find the matching function.
Fixes #72120