Fix PanoramaSky artifacts on Android/GLES2

[ForestKatsch]

The root cause of the issue is that OpenGL ES 2 does not support the textureCubeLod function.
There are (optional) extensions to support this, but they don't appear to be exposed with the ES2 renderer (even though the hardware needed to support LOD features are certainly available.)
The existing shim in drivers/gles2/shaders/cubemap_filter.glsl just creates a macro:

#define textureCubeLod(img, coord, lod) textureCube(img, coord)

But the third parameter of textureCube is actually a mip bias, not an absolute mip level.
(And it doesn't seem to work regardless.)
In this specific case, the cubemap_filter should only sample from the first level of the "source" panorama cubemap.
In lieu of a method to force a lod level of zero, I've chosen to comment out the switchover from a 2D equirectangular panorama to the cubemap version of the same image, therefore always sampling roughness values from the 2D equirectangular panorama.
This may cause additional artifacts or issues across the seam, but at least it prevents the glaringly obvious black areas.

---

This same issue (no fragment texture LOD support) has rather large repercussions elsewhere too; it means materials with larger cubemap density (i.e. planar or distant objects) will be far rougher than expected.
Since GLES 3 appears to properly support fragment texture*Lod functions, switching to the GLES 3 backend would solve this problem.

---

Root cause discovered with help from @kaadmy.

Bugsquad edit: Fix #43667

[clayjohn]

Thanks for making a pull request! This certainly is a troubling bug.

As you'll see my comments, I'm a little skeptical that these are the proper fixes. I will take a closer look tonight and try running this on an android device. In the meantime, do you mind uploading an image of a radiance cubemap generated from this PR. you can capture it in RenderDoc and save to a png fairly easily.

[ForestKatsch]

A better fix would be to check for the existence of the GL_ARB_shader_texture_lod or GL_EXT_shader_texture_lod extensions, and only enable USE_SOURCE_PANORAMA (i.e. read from the cubemap instead of the source equirectangular map) only if textureCubeLod is available. I'm not sure how Godot handles OpenGL extension detection though, and I'd like to reuse that existing code if possible. Any pointers are welcome!

[clayjohn]

@ForestKatsch GLSL extension checking is handled within each shader with preprocessor directives as below:

godot/drivers/gles2/shaders/cubemap_filter.glsl

Lines 30 to 45 in 68013d2

	#ifndef USE_GLES_OVER_GL
	#ifdef GL_EXT_shader_texture_lod
	#extension GL_EXT_shader_texture_lod : enable
	#define texture2DLod(img, coord, lod) texture2DLodEXT(img, coord, lod)
	#define textureCubeLod(img, coord, lod) textureCubeLodEXT(img, coord, lod)
	#endif
	#endif // !USE_GLES_OVER_GL
	#ifdef GL_ARB_shader_texture_lod
	#extension GL_ARB_shader_texture_lod : enable
	#endif
	#if !defined(GL_EXT_shader_texture_lod) && !defined(GL_ARB_shader_texture_lod)
	#define texture2DLod(img, coord, lod) texture2D(img, coord, lod)
	#define textureCubeLod(img, coord, lod) textureCube(img, coord, lod)
	#endif

[ForestKatsch]

This is definitely not a long-term proper fix. Without support for fragment shader texture LOD sampling, I don't know if there can be a proper fix for this bug. But an incorrect result is arguably an improvement over the black artifacts.

I don't have a native Android device, only an Oculus Quest 2. I'll look into getting RenderDoc captures.

GLSL extension checking is handled within each shader with preprocessor directives

Without checking for the extension on the C++ side, I don't think there's a clean workaround for devices that don't support the textureCubeLod function, because the C++ side would need to know to not set USE_SOURCE_PANORAMA and the source cubemap texture.

[clayjohn]

@ForestKatsch Well, we can just do a different codepath for all android devices. While it won't be as smooth, we could just force android devices to read from the source panorama. You would do so by putting an #ifdef GLES_OVER_GL check around the if (lod == 1) block

[akien-mga]

@ForestKatsch Well, we can just do a different codepath for all android devices. While it won't be as smooth, we could just force android devices to read from the source panorama. You would do so by putting an #ifdef GLES_OVER_GL check around the if (lod == 1) block

Note that this would apply to iOS too (and WebGL I think).

[ForestKatsch]

I don't know all of the tradeoffs between sampling from the source equirectangular panorama (with USE_SOURCE_PANORAMA set) and from the transformed cubemap. If they're functionally identical, I don't think there would be an issue with always sampling from the source equirectangular panorama on all platforms.

[ForestKatsch]

Here is a comparison between the two methods on a desktop with the GLES2 backend:

#if 1
      glActiveTexture(GL_TEXTURE0);
      glBindTexture(GL_TEXTURE_CUBE_MAP, sky->radiance);
      shaders.cubemap_filter.set_conditional(CubemapFilterShaderGLES2::USE_SOURCE_PANORAMA, false);
#endif

Comparing these two images with a diff tool, I don't see any difference between the two at all.

[clayjohn]

@ForestKatsch The difference is quality. A huge difference in quality when using high frequency environment maps. Try the same comparison with the environment map from #33616.

Although, thinking about it, GLES2 may not use an HDR panorama, so the impact may not be so severe. Please test with the environment map from #33616 and if it looks fine in both cases I am happy to remove the hack altogether.

_Edit: _ to be clear, in your above example, you tested once with #if 1 and once with if 0 right?

[ForestKatsch]

Try the same comparison with the environment map from #33616.

---

#if 1: sampling from cubemap

---

#if 0: sampling from panorama

---

Although, thinking about it, GLES2 may not use an HDR panorama, so the impact may not be so severe.

I think you're correct:

//make framebuffer size the texture size, need to use a separate texture for compatibility
glActiveTexture(GL_TEXTURE3);
glBindTexture(GL_TEXTURE_2D, resources.mipmap_blur_color);
glTexImage2D(GL_TEXTURE_2D, 0, GL_RGB, size, size, 0, GL_RGB, GL_UNSIGNED_BYTE, NULL);
glFramebufferTexture2D(GL_FRAMEBUFFER, GL_COLOR_ATTACHMENT0, GL_TEXTURE_2D, resources.mipmap_blur_color, 0);

Edit: to be clear, in your above example, you tested once with #if 1 and once with if 0 right?

Yes.

[clayjohn]

Well that settles it. Looks like our little hack isn't needed in GLES2.

I'll test tonight when I get home from work before approving.

In the meantime, you can remove the #if 0 block. We prefer to remove all dead code and just use the git history to keep things for posterity.

[ForestKatsch]

It's worth noting that this exact problem (no texture LOD support in GLES2) will cause issues elsewhere; notably, the wrong roughness will be used on planar or distant metallic surfaces (which should normally have a uniform roughness). This is visually very apparent.

As this issue probably isn't easily solved (given the strict GLES2/GLES3 separation), isn't immediately apparent as a visual defect, and with Godot moving to Vulkan in the future, I don't plan on opening an issue for that. But it is a problem on any device that doesn't support texture LOD extensions.

[clayjohn]

It's worth noting that this exact problem (no texture LOD support in GLES2) will cause issues elsewhere; notably, the wrong roughness will be used on planar or distant metallic surfaces (which should normally have a uniform roughness). This is visually very apparent.

Yep. Could be useful to document in https://docs.godotengine.org/en/stable/tutorials/misc/gles2_gles3_differences.html which is becoming a catch all for GLES2 specific limitations.

[clayjohn]

Tested out on my hardware and removing the cubemap hack works great. Since GLES2 always uses LDR, we don't need the extra smoothness that comes from resampling from lower radiance levels.

I also tested out without your changes to the texture2DLod, and I was correct before that those changes are unnecessary. So just remove those changes and this PR will be good to go!

Thanks for your hard work and your great sleuthing. It will be really nice to have this fixed.

[akien-mga]

Could you squash the commits into one? See PR workflow for instructions.

[ForestKatsch]

Could you squash the commits into one? See PR workflow for instructions.

Done!

[akien-mga]

Thanks!