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

Add lightmap bake canceling #99483

Merged
merged 1 commit into from
Nov 22, 2024

Conversation

badsectoracula
Copy link
Contributor

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 godotengine/godot-proposals#8181.

@badsectoracula badsectoracula requested review from a team as code owners November 21, 2024 01:09
@AThousandShips AThousandShips added this to the 4.x milestone Nov 21, 2024
@fire fire requested a review from a team November 21, 2024 17:38
Copy link
Member

@fire fire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The proposal enhancement makes sense to me.

I have yet to review the pull request, but this is good from a usability point of view. Cancelling a potentially 30-minute plus bake is essential rather than forcing quitting.

@fire fire requested a review from a team November 21, 2024 17:41
Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested locally, it works as expected.

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

lightmap_bake_leak.mp4

Every time I cancel baking, VRAM utilization grows by 600 MB without shrinking after.

No leak occurs if I cancel baking just after it starts (by pressing Space immediately after the progress dialog pops up, which presses the autofocused Cancel button).

@clayjohn
Copy link
Member

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

if (err != OK) {
FREE_TEXTURES
FREE_BUFFERS
FREE_RASTER_RESOURCES
memdelete(rd);
if (rcd != nullptr) {
memdelete(rcd);
}
compute_shader->print_errors("compute_shader");
}
ERR_FAIL_COND_V(err != OK, BAKE_ERROR_LIGHTMAP_CANT_PRE_BAKE_MESHES);

@badsectoracula
Copy link
Contributor Author

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).

Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

@clayjohn clayjohn modified the milestones: 4.x, 4.4 Nov 22, 2024
Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested locally, there are no more memory leaks now, regardless of how many times I cancel the bake (at different stages).

@Repiteo Repiteo merged commit f2763b2 into godotengine:master Nov 22, 2024
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Nov 22, 2024

Thanks! Congratulations on your first merged contribution! 🎉

@badsectoracula badsectoracula deleted the cancel_lightmap_bake branch November 22, 2024 21:09
@akien-mga akien-mga changed the title Add lightmap bake cancelling Add lightmap bake canceling Mar 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants