GLES3: Change default texture repeat to disabled#79566
Closed
LRFLEW wants to merge 1 commit into
Closed
Conversation
This was referenced Jul 17, 2023
Contributor
Author
|
I finally found documentation that seems to directly address the default value for texture repeating, and found this
https://registry.khronos.org/OpenGL-Refpages/gl4/html/glTexParameter.xhtml So using |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is another attempt to fix #79315 (the previous one being #79367, which was declined for not being a direct fix for the issue).
I used RenderDoc to analyze how the texture is being initialized to see if I could get any insights into what's going on. (Side note: I was having trouble getting RenderDoc to work with Godot, and I eventually figured out that
GLManager_Windows::_nvapi_disable_threaded_optimization()was crashing when running the editor through RenderDoc. If you want to use RenderDoc with Godot, you need to stub this function if you're using Windows and an Nvidia GPU.) Looking through the "Resource Initialization Parameters" for a texture exhibiting this bug, I noticed that it was first initializing the filtering mode to one that the final texture didn't use, set the texture (and mipmap) data withglTexImage2D, and then set the actual filter mode. The repeat mode was similarly enabled before the calls toglTexImage2D, but wasn't set (either enabled or disabled) again after. This seemed to confirm the second of the theories I mentioned in the description for #79367, which is that callingglTexImage2Dwas disabling the texture repeat.I looked through the code and figured out that the initial calls to set the repeat mode before calling
glTexImage2Dwas happening here:godot/drivers/gles3/storage/texture_storage.cpp
Lines 1293 to 1300 in a33b548
The comment states that it's setting these to "default" values, which seems to indicate to me that it's here specifically to deal with
glTexImage2Dresetting parameters to default values. The problem is that the actual default appears to be to disable texture repeat, not enable it. ChangingRS::CANVAS_ITEM_TEXTURE_REPEAT_ENABLEDtoRS::CANVAS_ITEM_TEXTURE_REPEAT_DISABLEDin these defaults makes it so thatTexture::state_repeatmatches the state of the texture after callingglTexImage2D, which fixes the bug.One part of this I'm not sure of is whether this behavior of
glTexImage2Dresetting the texture repeat state is part of the OpenGL / OpenGL ES standard, or is an implementation detail that may vary between drivers. I tried looking it up online, but I couldn't find any relevant documentation about this. While I did test this on the machines I had access to, and they all pointed to a repeat-disabled default, I can't say for certain that there aren't platforms out there where the opposite is true, which will make setting the repeat state to a "default" value here a challenge, and may require a different solution to fix this issue.This PR targets the master branch, but this should be trivial to cherry-pick for a 4.1.X release.