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

Fix memory leak using one-shot systems #235

Merged
merged 2 commits into from
Aug 6, 2024
Merged

Conversation

janhohenheim
Copy link
Member

Also implements #234

Tried adding a closure directly to fn button, but that is rather cumbersome, as ChildBuilder does not have a register_one_shot_system method directly, which means we would have to re-implement it:

let entity = self.spawn_empty().id();
self.push(RegisterSystem::new(system, entity));
SystemId::from_entity(entity)

which is very very meh

Comment on lines 57 to 58
// Add the one-shot system as a child so that will get cleaned up when the button is destroyed.
entity.add_child(on_press.entity());
Copy link
Member Author

Choose a reason for hiding this comment

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

This here is the line that fixes the leak, as otherwise we never clean up the entities holding our one-shot systems but continue to register new ones when the menu is reconstructed.

@janhohenheim janhohenheim linked an issue Aug 6, 2024 that may be closed by this pull request
Copy link
Collaborator

@benfrankel benfrankel left a comment

Choose a reason for hiding this comment

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

Wording nit

src/ui/widgets.rs Outdated Show resolved Hide resolved
Co-authored-by: Ben Frankel <[email protected]>
@janhohenheim janhohenheim enabled auto-merge (squash) August 6, 2024 18:48
@janhohenheim janhohenheim merged commit fb06a20 into main Aug 6, 2024
4 checks passed
@janhohenheim janhohenheim deleted the fix-memory-leak branch August 6, 2024 18:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add OnPress to button widget
2 participants