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

Removes an internal error report if shader fails compile #51548

Merged
merged 1 commit into from
Aug 12, 2021

Conversation

Chaosus
Copy link
Member

@Chaosus Chaosus commented Aug 12, 2021

I don't think the user should need to know where in the code internals failed if the shader contains errors:

image

Note: in the 3.x it does not report it either

@Chaosus Chaosus requested a review from a team as a code owner August 12, 2021 07:13
@Chaosus Chaosus added this to the 4.0 milestone Aug 12, 2021
@akien-mga akien-mga merged commit 22ccb74 into godotengine:master Aug 12, 2021
@akien-mga
Copy link
Member

Thanks!

@akien-mga
Copy link
Member

Does it still report shader errors properly when running directly without the editor?

@Chaosus Chaosus deleted the shader_silence_error branch August 12, 2021 07:49
@Chaosus
Copy link
Member Author

Chaosus commented Aug 12, 2021

Hmm, I think not these errors, just errors made by the user in the shader itself.

@akien-mga
Copy link
Member

If we make errors in the Godot's own shader code, we still need to have a way to catch them.

@Chaosus
Copy link
Member Author

Chaosus commented Aug 12, 2021

Then I guess we should catch them using conditions like if (!editor_hint()) { ERR_FAIL_COND(x) }?

@Chaosus
Copy link
Member Author

Chaosus commented Aug 12, 2021

Or pass a boolean indicates that this shader is created by the user and not by the engine...

@akien-mga
Copy link
Member

Or pass a boolean indicates that this shader is created by the user and not by the engine...

Yeah that might be the better option. The ERR_FAIL was important here, so we need to make sure it's only hidden when we know that the users will be told about the error differently.

@Chaosus
Copy link
Member Author

Chaosus commented Aug 12, 2021

Yeah that might be the better option.

Ok, I will try to make a pull request for this

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.

2 participants