Fix RaycastOcclusionCull World3D scenario memory leak #82291
Merged
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Related issue #71884, I don't think this PR fixes that one but in theory its possible due to the changes in thread and other resource lifetime management in this PR. I tried to check the tagged Liblast commit, but it's too old to work with 4.2 without a lot of modifications, including some GDScript changes.
The problem is that
RaycastOcclusionCull
tracks it's scenarios (what World3D basically does behind the scenes) in a pretty unusual matter. When creating and deleting them usingadd_scenario()
andremove_scenario()
it tries to do so lazily. The problem is that when a scenario is deleted, it just marks it as removed but doesn't actually delete it yet. This becomes problem because the only place it actually does the deletion is inRaycastOcclusionCull::buffer_update()
, but this is only called if the scenario is active.The result is that if a World3D / scenario (or a node like Viewport using them) is removed, in many cases the scenario culling data never actually gets freed because it no longer gets updated, so it can't check the removed flag. In the issue MRP
RaycastOcclusionCull::scenarios
HashMap keeps growing forever as the scenarios are never released as the Viewports are no longer in the scene tree and thus are not updated and cleaned up.I fixed this by making the
add_scenario()
andremove_scenario()
work in a more traditional way, which makes the lifetime management much easier. I also fixed another leak as scenariocommit_thread
Thread objects were not always freed properly.However, as the existing solution was a pretty non-standard compared to other RID allocate / delete API's, there might be a reason why it is done in such a strange way. I tested this PR with a lot of projects including the occlusion culling demo and I could not find any issues, but it's always possible it's done that way because of some obscure corner-case. Or maybe it's just a workaround for some old issue that has already been fixed as I don't see how the existing RID check in add_scenario() can trigger in any normal use, it didn't happen even a single time during my testing.