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: Don't call glTexParameter* for invalid filter and repeat modes #79685

Merged
merged 1 commit into from
Jul 24, 2023

Conversation

LRFLEW
Copy link
Contributor

@LRFLEW LRFLEW commented Jul 20, 2023

This fixes #79315. That issue was caused by TextureStorage::_clear_render_target calling Texture::gl_set_filter and Texture::gl_set_repeat with the value *_MAX. The gl_set_filter and gl_set_repeat functions weren't designed to be called with the values (which appear to be used to force the next calls to make glTexParameter* calls), which results in whatever happens to be the currently bound OpenGL texture to be assigned unexpected parameters. This PR updates Texture::gl_set_filter and Texture::gl_set_repeat so that they only make calls to glTexParameter* when p_filter/p_repeat is a valid value.

I also made some small changes to these functions that shouldn't affect functionality, but figured would be nice to cleanup while I'm here. I removed the (IMO) unhelpful comments and updated how max_lod is assigned to be more consistent with the other local variables in gl_set_filter. I also changed the default values of Texture::state_filter and Texture::state_repeat to better represent their uninitialized state. This shouldn't have an effect on the engine, as these values get initialized in TextureStorage::_texture_set_data either way.

The only part of this I'm not sure about is the handling of *_DEFAULT as the argument. From what I can tell, they should never be used with these functions, so I didn't add a specific case for them, and chose to handle them the same a *_MAX (or any invalid enum value). If this needs to be changed, let me know what behavior you would want these to have, and I can update this PR.

This PR targets the master branch, but it should be safe to cherry-pick for a 4.1.X release. This PR may interact with #79568 and may require a rebase if that gets merged first.

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! _DEFAULT should definitely be handled like _MAX neither is technically a valid input to this function, but _MAX is handy to invalidate the cache. In either case avoid gl calls makes sense.

@LRFLEW
Copy link
Contributor Author

LRFLEW commented Jul 22, 2023

I rebased off the current master branch to resolve the merge conflict

@clayjohn clayjohn added the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Jul 23, 2023
@YuriSizov YuriSizov changed the title GLES3: Don't call glTexParameter* for invalid filter and repeat modes GLES3: Don't call glTexParameter* for invalid filter and repeat modes Jul 24, 2023
@YuriSizov YuriSizov merged commit 4ba24f6 into godotengine:master Jul 24, 2023
13 checks passed
@YuriSizov
Copy link
Contributor

Thanks!

@YuriSizov
Copy link
Contributor

Cherry-picked for 4.1.2.

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.

Texture Repeat sometimes ignored on Compatibility renderer
4 participants