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

GLES2: Fix VersionKey comparison in ShaderGLES2::bind() #58855

Merged

Conversation

akien-mga
Copy link
Member

@akien-mga akien-mga commented Mar 7, 2022

This was comparing arrays, GCC 12 raises a warning for it:

drivers/gles2/shader_gles2.cpp: In member function 'bool ShaderGLES2::bind()':
drivers/gles2/shader_gles2.cpp:80:71: error: comparison between two arrays [-Werror=array-compare]
   80 |         if (active != this || !version || new_conditional_version.key != conditional_version.key) {
      |                                           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/gles2/shader_gles2.cpp:80:71: note: use unary '+' which decays operands to pointers or '&'component_ref' not supported by dump_decl<declaration error>[0] != &'component_ref' not supported by dump_decl<declaration error>[0]' to compare the addresses

For context:

union VersionKey {
struct {
uint64_t version;
uint32_t code_version;
};
unsigned char key[12];
bool operator==(const VersionKey &p_key) const { return version == p_key.version && code_version == p_key.code_version; }
bool operator<(const VersionKey &p_key) const { return version < p_key.version || (version == p_key.version && code_version < p_key.code_version); }
};

GLES3 is somewhat different and spells things out more in detail:

bool ShaderGLES3::_bind(bool p_binding_fallback) {
// Same base shader and version valid version?
if (active == this && version) {
if (new_conditional_version.code_version == conditional_version.code_version) {
if (new_conditional_version.version == conditional_version.version) {
return false;
}
// From ubershader to ubershader of the same code?
if ((conditional_version.version & VersionKey::UBERSHADER_FLAG) && (new_conditional_version.version & VersionKey::UBERSHADER_FLAG)) {
conditional_version.version = new_conditional_version.version;
return false;
}
}
}

Ubershader logic aside, it should amount to the same as what my GLES2 fix does.

Tested on Jetpaca, didn't spot any obvious issue.

Would need review from rendering team, I just fixed the C++ bug, and that might fix some actual GLES2 shader issues or introduce some.

This was comparing arrays, GCC 12 raises a warning for it:

```
drivers/gles2/shader_gles2.cpp: In member function 'bool ShaderGLES2::bind()':
drivers/gles2/shader_gles2.cpp:80:71: error: comparison between two arrays [-Werror=array-compare]
   80 |         if (active != this || !version || new_conditional_version.key != conditional_version.key) {
      |                                           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/gles2/shader_gles2.cpp:80:71: note: use unary '+' which decays operands to pointers or '&'component_ref' not supported by dump_decl<declaration error>[0] != &'component_ref' not supported by dump_decl<declaration error>[0]' to compare the addresses
```
Copy link
Member

@RandomShaper RandomShaper left a comment

Choose a reason for hiding this comment

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

Looks good, and to me it looks as it can only do good.

@akien-mga akien-mga merged commit b6a6c90 into godotengine:3.x Mar 7, 2022
@akien-mga akien-mga deleted the 3.x-gles2-fix-VersionKey-comparison branch March 7, 2022 13:31
@akien-mga
Copy link
Member Author

Cherry-picked for 3.4.4.

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