Skip to content

Commit

Permalink
Clean up taffy nodes when UI node entities are removed (bevyengine#5886)
Browse files Browse the repository at this point in the history
# Objective

Clean up taffy nodes when the associated UI node gets removed. The current UI code will keep the taffy nodes around forever.

## Solution

Use `RemovedComponents<Node>` to iterate over nodes that are no longer valid UI nodes or that have been despawned, and remove them from taffy and the internal hash map.

## Implementation Notes

Do note that using `despawn()` instead of `despawn_recursive()` on a UI node that has children will result in a [warnings spam](https://github.com/bevyengine/bevy/blob/main/crates/bevy_ui/src/flex/mod.rs#L120) since the children will not be part of a proper UI hierarchy anymore.

---

## Changelog

- Fixed memory leak when nodes are removed in bevy_ui
  • Loading branch information
oceantume authored and nicopap committed Sep 12, 2022
1 parent 4617dbf commit a70e090
Show file tree
Hide file tree
Showing 2 changed files with 22 additions and 2 deletions.
9 changes: 9 additions & 0 deletions crates/bevy_ecs/src/system/system_param.rs
Expand Up @@ -780,6 +780,15 @@ impl<'a, T: Component> RemovedComponents<'a, T> {
}
}

impl<'a, T: Component> IntoIterator for &'a RemovedComponents<'a, T> {
type Item = Entity;
type IntoIter = std::iter::Cloned<std::slice::Iter<'a, Entity>>;

fn into_iter(self) -> Self::IntoIter {
self.iter()
}
}

// SAFETY: Only reads World components
unsafe impl<T: Component> ReadOnlySystemParamFetch for RemovedComponentsState<T> {}

Expand Down
15 changes: 13 additions & 2 deletions crates/bevy_ui/src/flex/mod.rs
Expand Up @@ -5,7 +5,7 @@ use bevy_ecs::{
entity::Entity,
event::EventReader,
query::{Changed, With, Without, WorldQuery},
system::{Query, Res, ResMut, Resource},
system::{Query, RemovedComponents, Res, ResMut, Resource},
};
use bevy_hierarchy::{Children, Parent};
use bevy_log::warn;
Expand Down Expand Up @@ -172,6 +172,15 @@ without UI components as a child of an entity with UI components, results may be
}
}

/// Removes each entity from the internal map and then removes their associated node from taffy
pub fn remove_entities(&mut self, entities: impl IntoIterator<Item = Entity>) {
for entity in entities {
if let Some(node) = self.entity_to_taffy.remove(&entity) {
self.taffy.remove(node);
}
}
}

pub fn get_layout(&self, entity: Entity) -> Result<&taffy::layout::Layout, FlexError> {
if let Some(taffy_node) = self.entity_to_taffy.get(&entity) {
self.taffy
Expand Down Expand Up @@ -208,6 +217,7 @@ pub fn flex_node_system(
>,
children_query: Query<(Entity, &Children), (With<Node>, Changed<Children>)>,
mut node_transform_query: Query<(Entity, &mut Node, &mut Transform, Option<&Parent>)>,
removed_nodes: RemovedComponents<Node>,
) {
// update window root nodes
for window in windows.iter() {
Expand Down Expand Up @@ -244,7 +254,8 @@ pub fn flex_node_system(
flex_surface.upsert_leaf(entity, style, *calculated_size, scale_factor);
}

// TODO: handle removed nodes
// clean up removed nodes
flex_surface.remove_entities(&removed_nodes);

// update window children (for now assuming all Nodes live in the primary window)
if let Some(primary_window) = windows.get_primary() {
Expand Down

0 comments on commit a70e090

Please sign in to comment.