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

Mipmaps on GL/GLES require GL_TEXTURE_MAX_LEVEL set explicitly. #911

Closed
wants to merge 2 commits into from

Conversation

ambrusc
Copy link

@ambrusc ambrusc commented Oct 4, 2023

Problem: Any texture with a mipmap sampler was black on GL/GLES but working on e.g. Metal (ARM).
Cause: GL texture was incomplete: (See: mipmap completeness, mipmap range)
Fix: set GL_TEXTURE_MAX_LEVEL based on num_mipmaps when creating the texture.
Verified: textures load with mipamp samplers on desktop GL 3.3 (Manjaro) and WASM (Emscripten, GLES3, Firefox).

Hopefully this fix is conceptually in the right place in the code. Let me know if not, and I can update.

As an aside, this approach doesn't support loading smaller LODs progressively into mip levels over several frames like many engines do. Not sure if any of the other backends support such usage, so the point may be moot.

@floooh
Copy link
Owner

floooh commented Oct 4, 2023

Hmm, this is a bit strange because mipmapping is working here in dedicated testing sample.

https://floooh.github.io/sokol-html5/mipmap-sapp.html

Can you post your texture creation code here? Because with the sokol-gfx validation layer active it should actually be impossible to create a texture with an incomplete mipmap chain. I guess there must be a bug in the validation layer, or an edge case which I didn't think of.

Thanks!

@ambrusc
Copy link
Author

ambrusc commented Oct 4, 2023

Do I need to add an include or something on Windows?

@ambrusc
Copy link
Author

ambrusc commented Oct 4, 2023

Hmm, this is a bit strange because mipmapping is working here in dedicated testing sample.

https://floooh.github.io/sokol-html5/mipmap-sapp.html

Can you post your texture creation code here? Because with the sokol-gfx validation layer active it should actually be impossible to create a texture with an incomplete mipmap chain. I guess there must be a bug in the validation layer, or an edge case which I didn't think of.

Thanks!

Oh nice, hopefully it's user error on my part. Let me take a look at the sample.

@floooh
Copy link
Owner

floooh commented Oct 4, 2023

Do I need to add an include or something on Windows?

On Windows sokol_gfx.h uses its own GL loader, you'd need to add a line here:

https://github.com/floooh/sokol/blob/master/sokol_gfx.h#L3910

The value for GL_TEXTURE_MAX_LEVEL is:

#define GL_TEXTURE_MAX_LEVEL 0x813D

@floooh
Copy link
Owner

floooh commented Oct 4, 2023

Oh nice, hopefully it's user error on my part. Let me take a look at the sample.

In any case let me know what you find out, because as I wrote above, it should actually be impossible to create an incomplete mipmap chain due to the validation layer.

@ambrusc
Copy link
Author

ambrusc commented Oct 4, 2023

Ahh! Okay I have a repro. It has to do with non-square images. My reading of the GL spec suggests they support non-power-of-two (NPOT) mipmaps (rounding sizes down) as well, but I haven't tried yet.

Here's the quick and dirty code: https://github.com/floooh/sokol-samples/compare/master...ambrusc:sokol-samples:master?diff=split

Is this user error, or a bug in the library, or a bug in the validation layer?

Happy to help in any way I can :-)

@floooh
Copy link
Owner

floooh commented Oct 4, 2023

...non-power-of-two (NPOT)...

Ok interesting! It's my impression as well that NPOT images with mipmaps were only restricted in GLES2/WebGL1, but those restrictions don't apply in desktop GL, GLES3 and WebGL2. I'll definitely need to investigate this (and I don't understand yet why setting GL_MAX_TEXTURE_LEVEL would help).

The restrictions in GLES2/WebGL were that non-power-of-two textures must have no mipmaps, and must be filtered with CLAMP_TO_EDGE. But since GLES2 support has been removed I didn't think it important to have validation layer checks for this.

Rectangular textures with power-of-2 dimensions shouldn't have any restrictions though.

I'll log off for today, but will investigate the issue over the next few days (looking at your sokol-samples changes I have a hunch that it's about the number of mipmaps, e.g. that a 256x128 texture needs 8 mipmaps, in any case - this should trigger a validation layer error).

@ambrusc
Copy link
Author

ambrusc commented Oct 4, 2023

Okay, by playing with the mipmap-sapp sample with various combination of NPOT and non-square textures, I think I have a reasonable handle what's going on.

  1. GL defines rules for mipmap completeness, which I was inadvertently breaking by not generating the smallest N mipmaps.
  2. This is okay, as long as you set GL_TEXTURE_MAX_LEVEL correctly for the number of mipmaps actually generated, which is why this fix works and is probably worth making.
  3. If you don't set GL_TEXTURE_MAX_LEVEL explicitly but generate mipmaps down to the smallest 1x1 image, GL considers the texture to be complete (I have not found the docs as to why is the case, but it appears to work empirically as the samples were depending upon it until now).

@floooh
Copy link
Owner

floooh commented Oct 5, 2023

Makes sense yeah, I'll need to add an incomplete mip chain to the mipmap sample and see what happens on the other backends, and also maybe tighten up the validation layer. I think the whole topic of complete versus incomplete mip chains was too murky so far...

@floooh
Copy link
Owner

floooh commented Oct 25, 2023

Apologies, I'll close this PR in favour of #924

(I also added more detailed error logging for the GL framebuffer completeness check there)

The PR will be merged together with new sokol-samples which cover rendering to mipmaps (and maybe texture-array-layers and 3D-texture-slices).

@floooh floooh closed this Oct 25, 2023
@ambrusc
Copy link
Author

ambrusc commented Oct 25, 2023

Awesome, thank you so much! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants