Add reflection probe support to compatibility renderer

[BastiaanOlij]

This PR adds reflection probe support to the compatibility renderer.

It currently uses the approach similar to the GLES2 renderer in Godot 3 which only supports up to 2 probes per surface.
We can't use the approach in Vulkan as we do not have consistent cubemap array support.
We can't use the approach in GLES3 as this introduces seam issues and has a noticeable performance impact in the scenario that is sketched as to why people want this. e.g. a large(ish) level with multiple probes. This means all probes hitting a surface are processed even if most results are discarded, introducing a lot of overhead.

Notes:

• Production edit: This closes OpenGL: ReflectionProbe is not reimplemented yet #86318.
• Production edit: This closes godotengine/internal-team-priorities#16

[BastiaanOlij]

Currently not sure yet how I'm going to approach actually applying the reflection probe data.

First attempt is going down the same path as the mobile renderer however I am worried about the performance impact of handling reflection probes this way. I also ran into a snag with using a samplerCubeArray:

ERROR: SceneShaderGLES3: Fragment shader compilation failed:
0(320) : error C7532: global type samplerCubeArray requires "#version 400" or later
0(320) : error C0000: ... or #extension GL_ARB_texture_cube_map_array : enable
0(320) : error C0000: ... or #extension GL_EXT_gpu_shader4_1 : enable

So if we continue down this road we'll be dependent on extensions that may, or may not, be supported.

edit the CI already is complaining about GL_TEXTURE_CUBE_MAP_ARRAY not being available in GLES platforms, only in OpenGL. So I think that's that.

I have two alternative options we could pursue, both will change the atlas logic so we have individual cubemaps instead of an array:

1. Only allow one reflection probe to effect geometry, this allows us to properly mix in the reflection color in the main scene shader and we can use a define to remove the code if no reflection probe applies.

2. Render reflection probes additive in a separate pass, similar to what we do lights that cast shadows. This will mean colors will look "off" but we can handle multiple probes.

[Lasuch69]

This is a lot more complicated than I thought.

Second option would be a lot better approach as you can just avoid probes overlapping when creating a scene. First approach feels terrible as it would make probes nightmare to work with in complex scenes (splitting big meshes would be necessary just like with the forward rendering light object limit).

I would vote for approach number two any day of the week.

[Calinou]

Second option would be a lot better approach as you can just avoid probes overlapping when creating a scene.

The issue is that any complex scene requires probe blending to look good. You really don't want probe thresholds to be apparent. If you recall Source 1 games, this is an issue that plagued most maps that had reflective floors in them (along with lacking seamless cubemaps).

You don't need to be able to blend a ton of probes together, but being able to blend 2 at any given pixel is absolutely essential in 2024, even for a low end-oriented renderer. The blending doesn't need to be super accurate (it's encouraged to do it over the shortest possible distance for performance reasons), but it needs to be there somehow.

I believe multipass rendering is also slow when you use greater amounts of reflection probes, so that's something to keep in mind as well.

[BastiaanOlij]

OK, figured out why the sky looks weird. It's rendered upside down when rendering the sides... oops

[BastiaanOlij]

Ok, the sky is now reflected properly, but in two sides the ground isn't being reflected... might be a culling issue...

[BastiaanOlij]

Ok, the problem turns out to be that when the maximum distance of the probe is set to 0, it's really limiting things too much and it looks weird.

Setting the distance to something sensible we get a good result:

[Calinou]

Ok, the problem turns out to be that when the maximum distance of the probe is set to 0, it's really limiting things too much and it looks weird.

Max Distance set to 0 is supposed to set the probe's far clip to the longest axis of the ReflectionProbe's size. This works correctly in Forward+ and Mobile from my testing.

This also occurs if you set Max Distance to a value lower than one of the aforementioned axes: it's always clamped so that everything in the probe's bounds is within the probe's far clip. The only case where Max Distance makes a visual difference is when it's greater than the length of the longest axis, so the probe can reflect something located outside its bounds.

[BastiaanOlij]

Max Distance set to 0 is supposed to set the probe's far clip to the longest axis of the ReflectionProbe's size. This works correctly in Forward+ and Mobile from my testing.

This also occurs if you set Max Distance to a value lower than one of the aforementioned axes: it's always clamped so that everything in the probe's bounds is within the probe's far clip. The only case where Max Distance makes a visual difference is when it's greater than the length of the longest axis, so the probe can reflect something located outside its bounds.

Indeed, and this logic is controlled by the core culling logic, so it is using the same code for all renderers. I have yet to figure out why this is going wrong, I just now know that the max distance is what triggers the problem :)

[BastiaanOlij]

Ok, so the issue with adding a 3rd reflection probe turns out to be a never ending loop in RendererSceneCull::render_probes that also happens with the Vulkan renderer.

[BastiaanOlij]

Ok, so the max distance issue is really simple and dumb :P Turns out that if max distance is 0, we use the size of the reflection probe. And as I have relatively small reflection probes with different size edges, it comes out looking weird.
Not a bug.

[BastiaanOlij]

As far as I can tell cleaning up reflection probes now works, at least I can't reproduce the issue I had before.

[BastiaanOlij]

Radiance map is now calculated the same way for reflection probes, as for sky. I've moved all the logic for this into a new CubemapFilter class (@clayjohn would appreciate you taking a look at this and see if I've missed anything).

With Metallic = 1.0 and Roughness = 0.0 we can see that the existing sky reflection and new reflection probe reflection nice matches:

With Metallic = 1.0 and Roughness = 1.0 we can see that there is a little more reflection from the probe:

I was unable to determine what causes this.

There may be a clue in that ambient mode doesn't seem to have an effect even though I did add the check in:

which suggests there is an error somewhere. There are differences in how ambient light is calculated in the compatibility renderer compared to the two Vulkan renderers, this may hold a clue.

[BastiaanOlij]

After #89134 is merged this needs to be rebase and then reflection_probe_has_atlas_index implemented. That solves the crash issue with multiple probes.

[BastiaanOlij]

Rebased this on top of the reflection probe recurring error fix, that PR will need to be merged first.

This is pretty much all working now:

Only thing is that ambient isn't behaving as expected so here I need some feedback from people reviewing the code, hopefully spot what I'm doing wrong.

[Calinou]

Tested locally on desktop (Linux + RTX 4090) and mobile (Samsung Galaxy Z Fold4), it mostly works as expected. The Always update mode functions correctly as well.

In the 3D platformer demo, probes flicker back and forth when reloading the scene in the editor:

simplescreenrecorder-2024-03-05_15.23.22.mp4

This doesn't occur in the running project where probes display correctly on the GridMap's surfaces. This seems to be related to the limit of displaying 2 reflection probes per mesh (even on different pixels):

Forward+	Compatibility

Testing project: test_reflection_probes_mirage_palace.zip

Also, exiting the editor on the 3D platformer demo while stage.tscn is open prints this:

ERROR: Texture with GL ID of 1330: leaked 349524 bytes.
   at: ~Utilities (drivers/gles3/storage/utilities.cpp:79)
ERROR: Texture with GL ID of 1333: leaked 349524 bytes.
   at: ~Utilities (drivers/gles3/storage/utilities.cpp:79)
ERROR: Texture with GL ID of 1337: leaked 349524 bytes.
   at: ~Utilities (drivers/gles3/storage/utilities.cpp:79)
ERROR: Texture with GL ID of 1338: leaked 349524 bytes.
   at: ~Utilities (drivers/gles3/storage/utilities.cpp:79)
ERROR: Texture with GL ID of 1103: leaked 349524 bytes.
   at: ~Utilities (drivers/gles3/storage/utilities.cpp:79)
ERROR: Texture with GL ID of 1334: leaked 349524 bytes.
   at: ~Utilities (drivers/gles3/storage/utilities.cpp:79)
ERROR: Texture with GL ID of 1313: leaked 349524 bytes.
   at: ~Utilities (drivers/gles3/storage/utilities.cpp:79)
ERROR: Texture with GL ID of 1314: leaked 349524 bytes.
   at: ~Utilities (drivers/gles3/storage/utilities.cpp:79)

I haven't been able to reproduce this consistently though.

[BastiaanOlij]

@Calinou indeed, the two reflection probe limit is per draw call, that's the price we pay on low end hardware. This will require breaking up once meshes instead of having large scene filling meshes.

I'll have a look if I can figure out where we're leaking textures. Interesting it's not printing the texture description, I might improve that in utilities

[BastiaanOlij]

@Calinou note that if you run a dev build it will output which textures leaked by name.

[BastiaanOlij]

@clayjohn as discussed during the meeting, flipped the order around on the ambient color. The color setting does come through now. I'm not sure if it's what is expected, but as far as I can tell its working.

[timothyqiu]

Web exports look dark and no reflection is available. Browser's console outputs lots of invalid texture type errors for framebufferTextureLayer.

It turns out that glFramebufferTextureLayer doesn't like cubemap textures. Changing this line

https://github.com/godotengine/godot/blob/da7bb612b48dbc32105e38d3d0aa9a8b60482e2b/drivers/gles3/storage/light_storage.cpp#L895

to use glFramebufferTexture2D solves the issue:

glFramebufferTexture2D(GL_FRAMEBUFFER, GL_COLOR_ATTACHMENT0, GL_TEXTURE_CUBE_MAP_POSITIVE_X + side, color, 0);

[BastiaanOlij]

Web exports look dark and no reflection is available. Browser's console outputs lots of invalid texture type errors for framebufferTextureLayer.

It turns out that glFramebufferTextureLayer doesn't like cubemap textures. Changing this line

@dsnopek @clayjohn what do you think of this? Should we add an extra #ifdef to use this notation for for web only, or would it be safe to just switch over for all platforms?

[dsnopek]

what do you think of this? Should we add an extra #ifdef to use this notation for for web only, or would it be safe to just switch over for all platforms?

I can confirm via independent Googling that glFramebufferTexture2D() needs to be used with WebGL. :-)

Assuming that will work fine on other platforms, my personal preference would be to avoid #ifdef's and just use that always (with a comment explaining why we're doing it this way). If there's some downside to using that on other platforms, then I guess an #ifdef would be better, but I personally wouldn't expect there to a downside (caveat being that I'm a novice with regard to rendering :-))

[BastiaanOlij]

@dsnopek @timothyqiu done, if you don't mind testing if it all works as advertised :)

[timothyqiu]

Tested on Linux, Windows (on Wine), and Web exports. Works as expected 🎉

[clayjohn]

Looks great! I'm not going to do an in depth code review. I tested locally and it seems mostly fine.

Two small issues, the roughness mapping of the cubemap filter seems off. Values are way less rough when using reflection probes than they should be

Forward+: Sky

Forward+: Reflection Probe

GLES3: Sky

GLES3: Reflection Probe

It seems the difference comes from ReflectionProbes using a mipmap count of Image::get_image_required_mipmaps() + 1 while the sky uses Image::get_image_required_mipmaps() - 1 Nevermind, this comes from the shader. Sky used RADIANCE_MAX_LOD which is always 5.0, while reflection probes use MAX_ROUGHNESS_LOD which defaults to 7 nevermind, not that either. it seems we never use the radiance texture

I also noticed that the reflection_mask seems to not be implemented.

[BastiaanOlij]

Thanks @clayjohn , should all be corrected now!

I think most of these snuck in when I changed from having everything in a single buffer, to having color and radiance separated to do the blur correctly.

Also missing the radiance mask was an oversight seeing that didn't exist before. Adding that functionality went in parallel to developing this :P Turned out to be an easy fix though

[clayjohn]

Looks good to me! Let's merge this for dev 6 :)

[akien-mga]

Thanks! Amazing work 🎉