Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Multiple ReflectionProbe with UPDATE_ALWAYS crashes Godot #85829

Closed
lyuma opened this issue Dec 6, 2023 · 3 comments
Closed

Multiple ReflectionProbe with UPDATE_ALWAYS crashes Godot #85829

lyuma opened this issue Dec 6, 2023 · 3 comments

Comments

@lyuma
Copy link
Contributor

lyuma commented Dec 6, 2023

Tested versions

  • Reproducible in 4.2 stable
  • Reproducible in 4.1.3 stable
  • Reproducible in 4.0.3 stable

System information

Godot v4.2.stable - Windows 10.0.19045 - Vulkan (Forward+) - dedicated NVIDIA GeForce RTX 3090 (NVIDIA; 31.0.15.3713) - Intel(R) Core(TM) i9-9900K CPU @ 3.60GHz (16 Threads)

Issue description

If multiple reflection probes see each other, one of those probes may be added to the linked list
While the Main Thread is locked within _render_reflection_probe_step:
image
The WorkerThreadPool will render the probe, and enqueue more probes into the list, which seems to create a cycle about 6-8 probes long.
image

Steps to reproduce

  1. Open the attached project.
  2. Notice that refl_probe_update_once.tscn loads fine.
  3. Open refl_probe_crasher_simple.tscn which is the same scene but all probes are set to UPDATE_ALWAYS
  4. Godot will hang after rendering one frame.

Minimal reproduction project (MRP)

reflection_probe_update_always_crash.zip

@yosoyfreeman
Copy link
Contributor

Tested and reproducible on linux fedora too. In my case it does not render even one frame.

@daustria
Copy link
Contributor

daustria commented Jan 2, 2024

had a look at this, still not sure what is going on but i thought maybe i can share my findings anyway. first i was able to reproduce with 3 reflection probes with these steps:

  1. Add 3 reflection probes to a scene. move one of them far away from the other.
  2. Set all the probes to update always.
  3. Move the far probe towards the two other probes.

I have an MRP for this with steps 1 and 2 already done, step 3 should complete the crash.
reflection_probe_test_3.zip

With only 3 probes it seems that the code uses single threaded behaviour for rendering probes which probably makes things easier to debug. specifically in renderer_scene_cull.cpp, in the call to _render_reflection_probe_step() we call _render_scene() and this code executes

} else {
	//single threaded
	_scene_cull(cull_data, scene_cull_result, cull_from, cull_to);
}

the call to _scene_cull() here seems to be where we add the reflection probes back to the list as we are processing it, like the screenshots in the original post show. on the other hand nodes are removed in render_probes(). i observed in the debugger that once we go into render_probes() we never get out as the linked list of reflection probes never becomes empty. if A,B,C are labels for the reflection probes, the linked list mutates over time in the following way:

A -> B -> C
B -> C
B -> C -> A
C -> A
C -> A -> B

... and so on. the first node of the list is processed in render_probes(), then removed from the linked list. As the new head is processed, the call to _scene_cull above adds back the node we just removed to the end of the linked list.

@Calinou Calinou changed the title Multiple ReflectionProbe with UPDATE_ALWAYS crashes godot Multiple ReflectionProbe with UPDATE_ALWAYS crashes godot Mar 18, 2025
@Calinou Calinou changed the title Multiple ReflectionProbe with UPDATE_ALWAYS crashes godot Multiple ReflectionProbe with UPDATE_ALWAYS crashes Godot Mar 18, 2025
@Calinou
Copy link
Member

Calinou commented Mar 18, 2025

I can't reproduce this on 4.5.dev 28102e6 (Linux, GeForce RTX 4090 with NVIDIA 570.86.16), both in the editor and when running the project (I've tested both scenes). I've also tried following the steps from #85829 (comment) and couldn't get it to crash either.

This was likely fixed by #84976, closing. Please comment if you can reproduce this on the latest Godot version.

@Calinou Calinou closed this as completed Mar 18, 2025
@Calinou Calinou added this to the 4.4 milestone Mar 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants