godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
91.3k stars 21.24k forks source link

Add lightmap bake cancelling #99483

Open badsectoracula opened 1 day ago

badsectoracula commented 1 day ago

This enables the "Cancel" button in the lightmap baking progress window and checks the cancel request whenever a progress is updated/reported during lightmap baking. Most of the functionality was already there, including a potential cancel request by the callback, but callback's return value was never checked.

This adds some checks where possible. A couple of progress updates wouldn't forward a cancellation result so i didn't add them but in practice i never saw them when testing cancelling (though i mainly tested with the GI demo). The main part that takes most of the time, indirect light integration, does get cancelled.

The patch just stops the lightmap calculation when it gets cancelled. I didn't notice this creating any leaks, but i might have missed them as this is the first time i saw the lightmapper code, so someone who is more familiar with this should review it.

This should address https://github.com/godotengine/godot-proposals/issues/8181.

clayjohn commented 19 hours ago

However, a memory leak occurs if I cancel baking while indirect light is being calculated:

I'm not surprised, these early return error conditions will need to free the GPU resources before returning as we do with the current error handling.

See for example

https://github.com/godotengine/godot/blob/9e6098432aac35bae42c9089a29ba2a80320d823/modules/lightmapper_rd/lightmapper_rd.cpp#L1453-L1465

badsectoracula commented 18 hours ago

I updated the PR, it should release memory now (thanks to @Calinou 's video i also learned the AMD equivalent to the tool he uses - amdgpu_top - so i ran a bunch of bakes and cancelled them at various points and memory returned at the pre-bake numbers).

There were already some existing early exits in the function so i thought it was ok to exit, i guess they are also leaks. There are a lot of exit points in this so it should probably be refactored to have a single exit point with some sort of "if (error) goto fail" at the end of the function that cleans up everything, so perhaps someone more familiar with the code could clean it up (i could do it in the future if nobody does but i think some stuff may need to be moved around and at the moment i do not feel confident enough to touch anything that would affect the code's logic).