Add error check for reflection probe invalid atlas index.

[brennennen]

Potential fix for issue #104291

If there is more reflection probes in a scene than allowed in configurations (default is 64), then in reflection_probe_instance_get_framebuffer there is a case where we try to index an array with an atlas index of -1, which crashes the editor. This pull request fixes the crash and displays an error message instead.

[akien-mga]

I'm not a rendering expert but adding another error check to prevent a crash seems good.

I suggest you amend the commit with the typo fix you identified in the issue:

"lightmap will nod display properly", i imagine this was supposed to be "not".

[Calinou]

Tested locally, it works as expected.

Code looks good to me.

Note that error spam will occur while reflection probes attempt to re-render:

ERROR: Reflection probe atlas index invalid. Maximum amount of reflection probes in use (114) may have been exceeded, reflections will not display properly.
   at: reflection_probe_instance_get_framebuffer (./servers/rendering/renderer_rd/storage_rd/light_storage.cpp:1668)
ERROR: Parameter "framebuffer" is null.
   at: framebuffer_get_format (./servers/rendering/rendering_device.cpp:2921)
ERROR: Parameter "framebuffer" is null.
   at: draw_list_begin (./servers/rendering/rendering_device.cpp:4275)
ERROR: Condition "!draw_list.active" is true.
   at: draw_list_bind_uniform_set (./servers/rendering/rendering_device.cpp:4491)
ERROR: Condition "!draw_list.active" is true.
   at: draw_list_bind_uniform_set (./servers/rendering/rendering_device.cpp:4491)
ERROR: Condition "!draw_list.active" is true.
   at: draw_list_bind_uniform_set (./servers/rendering/rendering_device.cpp:4491)
ERROR: Immediate draw list is already inactive.
   at: draw_list_end (./servers/rendering/rendering_device.cpp:4954)
ERROR: Parameter "framebuffer" is null.
   at: draw_list_begin (./servers/rendering/rendering_device.cpp:4275)
ERROR: Parameter "framebuffer" is null.
   at: framebuffer_get_format (./servers/rendering/rendering_device.cpp:2921)
ERROR: Condition "!E" is true. Returning: TEXTURE_SAMPLES_1
   at: framebuffer_format_get_texture_samples (./servers/rendering/rendering_device.cpp:2768)
ERROR: Mismatch fragment shader output mask (1) and framebuffer color output mask (0) when binding both in render pipeline.
   at: render_pipeline_create (./servers/rendering/rendering_device.cpp:3873)

This can slow down the editor quite a bit with hundreds of probes in the scene.

[brennennen]

I decided to test compatibility too and it has the same issue in a copy of a function with the same name. I fixed the issue in the same way as forward+, but it's still got issues and should be reviewed/discussed.

For compatibility, this fix removes a crash, so I figured it was worth still committing, but it hangs all ui rendering. It looks like maybe compatibility has less error handling down stream for a bad frame buffer handles than forward+ has and should probably be investigated further, either in this ticket or a spin off one.

EDIT: I've found a way to fix the crash and not cause a hang in compatibility, but it is a bit hacky. I've re-requested a review so this doesn't get merged without this new change being reviewed.

[brennennen]

Alrighty, In finding a fix for compatibility (that wasn't a hack), I ended up going a route that used "reflection_probe_has_atlas_index" to bypass the branch that caused the crash and also not spam the error logs.

I tried to do this same thing for Forward+, but it didn't work, so I'm keeping the error spammy solution. I'm not sure how to finagle my way through the render_scene function to avoid the error spam log, but also not crash.

[brennennen]

Current behavioral changes from this pr as of the latest commit:

• fixes a crash in forward/mobile/compatibility
• for forward/mobile: when exceeding the maximum number of reflection probes configured, instead of crashing, logs many errors messages that will impact performance.
• for compatibility: when exceeding the maximum number of reflection probes configured, silently ignores reflection probes past the maximum, no logs at all (which is also not ideal, probably want at least a warning).