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

GLES3: Ensure all ShaderData is properly initialized in set_code #84752

Merged
merged 1 commit into from
Nov 13, 2023

Conversation

akien-mga
Copy link
Member

I refactored the code to be clearer and hopefully prevent such errors in the future, but it's still quite error prone. The config booleans could maybe be moved to an internal struct that would always be zero'ed in set_code, so this can all be done in a single line instead of listing them all one by one.

I noticed that version doesn't get reset when calling set_code; I suppose this is intended.

Some ShaderData implementations initialized most of their booleans before the early return with empty code, while others did them after declaring gen_code. I unified to be like the former which seems to be the least bug prone (doesn't assume that none of these will be accessed when valid = false), but I could refactor the other way around if preferred.

@akien-mga akien-mga added this to the 4.2 milestone Nov 11, 2023
@akien-mga akien-mga requested a review from a team as a code owner November 11, 2023 14:17
@akien-mga akien-mga changed the title GLES3: Ensure all ShaderData is properly initialized in set_code GLES3: Ensure all ShaderData is properly initialized in set_code Nov 11, 2023
@akien-mga
Copy link
Member Author

I can't reliably trigger the warning locally with GCC builds, so would be good if @jsjtxietian or @chrisl8 could confirm that it fixes the issue for them.

@chrisl8
Copy link
Contributor

chrisl8 commented Nov 11, 2023

Will test later today.

@chrisl8
Copy link
Contributor

chrisl8 commented Nov 11, 2023

@akien-mga Yes, this appears to fix the issue entirely:

v4.2.beta.custom_build [e38686f] (master at the moment)
image

v4.2.beta.custom_build [0e04203]
image

Comment on lines -321 to -322
uint64_t last_pass = 0;
uint32_t index = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

I can see that they are probably unused, but are you sure?

Copy link
Member Author

@akien-mga akien-mga Nov 12, 2023

Choose a reason for hiding this comment

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

It wouldn't compile after the removal if they were used anywhere.

Copy link
Contributor

@YuriSizov YuriSizov left a comment

Choose a reason for hiding this comment

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

Superficially it looks okay, but I would wait for the rendering team to confirm. The composition and order in the header may matter, and can't judge if your changes there are safe.

@jsjtxietian
Copy link
Contributor

Yes, this appears to fix the issue entirely:

Same for me.

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.

Looks great to me! In theory it doesn't matter if we initialize before or after the early return as we set valid to false (so the shader should be ignored anyway). But I think its safer to initialize before to ensure everything is default and we aren't accidentally relying on old state.

@akien-mga akien-mga merged commit 787e98e into godotengine:master Nov 13, 2023
15 checks passed
@akien-mga akien-mga deleted the gles3-shaderdata-initialize branch November 13, 2023 11:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants