Fix never ending loop with overlapping probes

[BastiaanOlij]

This PR changes two issues.

1. If we have overlapping probes set to update always, we render the first probe, remove it from our probe render list, then render our second probe, where our culling sees our first probe is within our AABB, which adds the first probe back into the render list, after which we remove the second probe, and return to process the first probe, which then finds the second probe and adds it to the render list, and we keep going and going and going...
   I've changed it so the probes remain in our render lists and are only removed once we're done. That seems to fix it.
2. After fixing that bug I ran into another bug because I had two render probes set to update always, and a 3rd render probe set to once. Godot handles the probes in reverse order so it renders the "update once" probe first. Then it renders the the "update always" probe, however as I didn't have my probe size set to 256, it tosses away the atlas and starts over. Our "update once" probe now panics as it tries to complete it's rendering process.

• This closes Crash when adding multiple reflection probes #88940.

[BastiaanOlij]

Still looking into an issue with the second issue...

Should work now :)

[BastiaanOlij]

With the other fixes we don't get here anymore, but I still think we should change this to return true; to indicate there is nothing more to render. Now if there is no atlas and we somehow get this far, we would just keep calling this endlessly for this probe with nothing really happening.

[clayjohn]

I'm not sure this will always continue rendering. I think that this will, in most cases, ensure that the probe gets updated the following frame (which it should). I have a feeling that setting to true will result in situations where the probe doesn't get updated at all if set to update once

[BastiaanOlij]

Indeed, if we return true here, it marks the probe as done, so if there wasn't another change that causes it to be re-queued for updating (like I've done in the situation where the atlas is rebuild) then it gets lost.

However if we return false, that just means it gets stuck at its current processing step and it will forever try and redo this processing step. As the failure isn't being resolved, it will never get past this point.

The only failure scenario that I can think of that would cause this situation, is when the atlas is full. If I follow the code correctly there is a condition where a probe that was rendered some frames ago will give up its spot when a new probe comes in. But it is unclear to me whether this can happen to a probe that is still being updated over multiple frames.

So if we manage to remove a probe where we completed the first processing step (e.g. rendered the faces) but where now updating on level of the mips each frame, than that probe will forever be stuck attempting to update that mip which it can never complete because it will never attempt to redo the first step.

It really seems to me that we're dealing with logic that was written once upon a time when certain things worked differently and it would retry rendering a probe in this condition, but it is definitely broken at this point in time.

[BastiaanOlij]

I have removed the comment, without an MRP this is all theory and I may have done enough to prevent ever getting into this situation.
Suggest we keep this in the back of our heads in case someone runs into a broken probe :P

[clayjohn]

Overall, this looks great. I replied to your comment, but I am not sure it is a good idea. I suspect that code is there for a reason and flipping the false to true will break something.

Feel free to look deeper into it if you really feel like the false should become true, but otherwise I think we can merge this once you remove the comment with the question

[akien-mga]

Thanks!