Works as intended. This make super easy to work with reflection probes and to have nice results <3

Added a check in the for loop where reflection_process is called to break it if both reflection_accum.a and ambient_accum.a are >= 1.

Thanks for the review @BastiaanOlij!

in reality it's unlikely that more than 2, maybe 3, probes overlap I think it's fine.

This probably isn't entirely true, it is likely there are more overlapping probes especially when using nested probes. It's common to add a central probe for a particular space, and add smaller probes to resolve accuracy mismatches. However, it is true that it's unlikely that more than 2-3 probes will need to be computed for a given pixel, and just 1 in the vast majority. The advantage to presorting them is that we can do an early out and just skip all the irrelevant reflection_processes: The highest priority will be computed, and unless the pixel calculated is on a spot where there's blending it won't run any of the lower priority probes, so even 16 overlapping probes with this pre-sorting step should be cheaper than 16 overlapping probes on master (16 overlapping probes is a bit wild though, just an example). Only on pixels where there are multiple probes not at full strength, which realistically is only a small amount of pixels in standard situations, we need run reflection process multiple times.

My only question would be whether this should be an option, not always applied. Personally I've never had very good results with the current approach...

I think the old approach makes the probes practically unusuable, which is an experience I've heard echoed by colleagues using Godot. Personally I don't think the old approach is something we should keep support for, unless there's actual vocal demand for allowing probes to fully blend for whatever reason. In the end fully blending reflections is always undesirable since it introduces pretty bad artifacts.
Additionally, with #99958 those who want could just increase the blend-distance to far enough so you do pretty much get full blending over the entirety of the probe.

It could use a video showing it in action...

4.3

This PR (Again with temporary blend adjustment, to ensure the center of the probes have full blending strength.)

This does show an issue where the sky gets blended every time there's blending between probes. You can see it here too, where in the transition zones the colors get darker:

This is caused by:

if (reflections.data[ref_index].exterior) {
	reflection.rgb = mix(specular_light, reflection.rgb, reflection_blend);
}

To fix this this blending step needs to run after the reflection_process for-loop is finished: We should only blend in the environment contributions after accumulating all probe contributions when accum.a < 1.0.

I've moved the environment blending to happen outside of reflection_process, after the reflection_process for-loop. There is no darkening in the blends anymore. The weird blending between green-yellow. This is a result of the current blending logic on master, hence you see the blue probe seeping through. This is why I adjusted that in previous screenshots/showcases :)

With temporary blend adjustment (Notice the blue not leaking through anymore between the yellow and green zones):

And no sky leakage anymore:

I have removed the weight float division where we are setting the specular_light and ambient_light, since we aren't accumulating all reflection contributions indefinitely, only until accum.a is 1, so dividing by this weight factor is not needed anymore.

@lander-vr Nice catch

@lander-vr does the interior boolean property still works?

Edit: I think there is a problem with the blending distance on this PR. I have to make my probes really huge to not blend with the sky.

Also not sure if interior is working

@jcostello Interior should still work:

The issues you're seeing with blending distance aren't caused by this PR, though this PR makes them more obvious, and is exactly the reason why I have added blend = clamp(blend * 2.0, 0.0, 1.0); at the end of the blend calculation for all of my screenshots and showcases (including the gif above): The blending on master never reaches full strength, which means your probe never has full contribution over what's inside. For a probe with interior toggled on, this means that the sky will still seep through. Especially now that I've moved reflection.rgb = mix(specular_light, reflection.rgb, reflection_blend); to be after all reflection processes have run, if the shader sees that accum.a isn't fully saturated to 1.0, it will fill the remainder with sky reflections.

So in short: probe doesn't have full strength -> blend doesn't ever reach 1.0 -> accum.a doesn't reach 1.0 -> skylight gets blended in until accum.a reaches 1.0.

Moving that "blending in the sky" line out of reflection_process also actually allows for proper blending from interior to exterior probes, instead of the hard cutoff we've had in the past.

Here is me toggling the interior bool without that blend adjustment (So the PR as it has been submitted):

This is why it's really important to merge #99958 before this PR, they go hand in hand.

If you want to test this PR, I'd advice to make the same blend adjustment blend = clamp(blend * 2.0, 0.0, 1.0); on line 880 in scene_forward_lights_inc.glsl, and comment out line 877.

Thanks for the explanation. IDK if it has to do with what you mention but and issue I see in this PR compared to master is this one

Al probes have 1.0 of intensity and set to internal

Edit: Before your last change this wasn't hapenning. I think this in notorious because the roughness of floor that is .4

This PR:

Master:

Is it possible for you to share that project? Then I can have a closer look and see where exactly things are going wrong in that scene. I've tested my PR in several scenes so far and haven't come across anything like this.

Is it possible for you to share that project? Then I can have a closer look and see where exactly things are going wrong in that scene. I've tested my PR in several scenes so far and haven't come across anything like this.

I uploaded it to drive. Its 2.6GB project that I use to test things. I didnt remove any part of the project, sorry

https://drive.google.com/file/d/1EaRDUhnuV0YVxy5EPNf4RD2nXz3Oalan/view?usp=sharing

Thanks for taking the time for the upload, and no worries about the size!

I've opened your project with my PR and it seems completely fine on my end, regardless of whether interior is toggled or not:

I also checked in @Calinou's Sponza demoscene, turned down the roughness on some materials, also seems to work fine:

Could you try toggling the Enable Shadow checkbox, to force them to update? If that doesn't fix it, double check that those meshes didn't lose their lightbake with the lighting debug viewmode. Either way, I'm not convinced that that issue is caused by my PR. It seems specific to a select few objects, and I don't really see how any of the changes that I've made would be able to cause that.

Rebased due to #99958 getting merged.

Bistro looks fine:

And blending looks fine here too:

Seems to be working now. Maybe I did something wrong. Also I have to play with the blending distance to get this right. Im courious why with high blending distance the sky gets reflected even in inside probes

Also I have to play with the blending distance to get this right.

Yes this is normal, and part of the reason why it was important to have that as an exposed setting.

Im courious why with high blending distance the sky gets reflected even in inside probes.

Because you're blending in surrounding reflections/probes. Inside probes now get blended appropriately with surrounding probes instead of causing the harsh cutoff we had in the past (Or bleeding due to overlap with another probe).

With other probe (This PR)	With other probe (4.3)

With sky (This PR)	With sky (4.3)

@Calinou I'm wondering if it could make sense to completely get rid of the green solid box for reflection probes. Despite the opacity adjustment, it still seems super intense in specific scenes and still requires you to move your view to the inside of the probe which is really cumbersome, it's extremely intrusive. You can't get a nice view of how the probe is blending with the surroundings because you're either only seeing whatever is outside the probe or whatever is inside.

Outside	Inside

I would remove it. It is annoying. I have to deactivate gizmos when I'm working with probes to fully see the results.

A outline of the bounds its just enough IMO

Rebased and resolved conflict with #100344. Tested in Bistro, Sponza, and some quick diy test-scenes with no issues.

@Calinou I'm wondering if it could make sense to completely get rid of the green solid box for reflection probes. Despite the opacity adjustment, it still seems super intense in specific scenes and still requires you to move your view to the inside of the probe which is really cumbersome, it's extremely intrusive. You can't get a nice view of how the probe is blending with the surroundings because you're either only seeing whatever is outside the probe or whatever is inside.

That makes sense. We might want to do the same for VoxelGI as well, keeping the lines only.

Edit: Pull request opened: #100370

Rebased and resolved conflict with #100344. Tested in Bistro, Sponza, and some quick diy test-scenes with no issues.

Awesome, I think it works as expected

I've changed the entire pr to a much more sensible and logical (and faster) approach.

I was worried about the cost of sorting in GLSL and found with some quick and dirty performance tests that with several overlapping probes (e.g. the Bistro scene that I set up with probes) there was a not insignificant difference in frame times between master and this PR.

I've gotten rid of the sorting step in the shader, reverted the reflection_process() to be executed in the main reflection loop, and removed that secondary for-loop that I added, so most of that is back to as it is on master. There was already a sorting step in light_storage.cpp based on the depth of the probe to the camera, so I've changed it to sort based on the extents instead. The probe indexes are now sorted before they enter the shader and reflection process executes the smaller probes first, then the larger ones.

From my very rudimentary (and unscientific, though the frametime had clear differences) performance tests, due to the early out (break loop if both reflection_accum.a and ambient_accum.a are >= 1.0) we now get a small performance increase when there are several probes overlapping compared to master.

*So far these changes only apply to forward+, not to mobile

Preliminary implementation done for mobile, adding a sorting function in light_storage.cpp. It works, but sorting only happens when the probes are paired to the geometry, so changing a probes size doesn't update the sorting until pair_reflection_probe_instances() is called.

@Calinou Thanks for the tests!

The main reason for those visual inconsistencies is due to the unorthodox setup. If you have a bunch of overlapping probes with the same size, whichever one is loaded first will be given priority since there is not a single one that takes priority by being smaller and getting sorted to the front. In reality this isn't an issue, because there is no reason to have (or for us to properly support) a probe setup as you have here. (Some more info on probe setups here #89408 (comment))
It's also worth noting probes don't deal the best with capturing metallic surfaces, I think doing tests in a demo environment like Sponza, changing some materials to be reflective, will yield much more representative results.

If this gets merged I'll invest some time into updating the documentation around ReflectionProbes, how they work, how to use them efficiently, and how to get good result. :)

If you increase the size of that central probe you should get a much more similar result in foward+.

I'm a bit confused by your frame times, I see you have higher frame times using the PR compared to master, which shouldn't be the case anymore since I switched the sorting to happen on CPU. If I compare your test scene in foward+ I get:
PR: 1.37ms
Master: 1.65ms
on a 5600x and rtx2060

Moved the sorting function to be called in fill_push_constant_instance_indices(), which resolves the sorting not updating when changing the probes sizes, and I changed GeometryInstanceForwardMobile to store the RID of the probe instances instead of the forward ID, because in order to sort the probes we need access to the size property (for which we need the RID).

Sorting now behaves as expected on mobile. :)

Edit:
Did two test runs on mobile, one with three probes with overlap on a geometry instance, and another one with 8.

No change on CPU.

I'm a bit confused by your frame times, I see you have higher frame times using the PR compared to master, which shouldn't be the case anymore since I switched the sorting to happen on CPU. If I compare your test scene in foward+ I get:

I had the Before and After tables swapped, sorry. I fixed the post accordingly.