Skip to content

Commit

Permalink
Fix remove ancestor markers crash (#413)
Browse files Browse the repository at this point in the history
# Objective

Fix the issue where `remove_ancestor_markers` crashes if an entity has children moved or removed, and then is despawned before `remove_ancestor_markers`  is run.

## Solution

When remove component from an entity, just check if that entity exists first.
  • Loading branch information
JonasAAA committed Jul 11, 2024
1 parent 803ec63 commit db535b2
Showing 1 changed file with 32 additions and 4 deletions.
36 changes: 32 additions & 4 deletions src/sync/ancestor_marker.rs
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,13 @@ fn add_ancestor_markers<C: Component>(
}
}

/// Remove the component from entity, unless it is already despawned.
fn remove_component<T: Bundle>(commands: &mut Commands, entity: Entity) {
if let Some(mut entity_commands) = commands.get_entity(entity) {
entity_commands.remove::<T>();
}
}

#[allow(clippy::type_complexity)]
fn remove_ancestor_markers<C: Component>(
entity: Entity,
Expand All @@ -232,11 +239,11 @@ fn remove_ancestor_markers<C: Component>(
if keep_marker {
return;
} else {
commands.entity(entity).remove::<AncestorMarker<C>>();
remove_component::<AncestorMarker<C>>(commands, entity);
}
} else {
// The parent has no children, so it cannot be an ancestor.
commands.entity(entity).remove::<AncestorMarker<C>>();
remove_component::<AncestorMarker<C>>(commands, entity);
}
}

Expand All @@ -254,11 +261,11 @@ fn remove_ancestor_markers<C: Component>(
if keep_marker {
return;
} else {
commands.entity(parent_entity).remove::<AncestorMarker<C>>();
remove_component::<AncestorMarker<C>>(commands, parent_entity);
}
} else {
// The parent has no children, so it cannot be an ancestor.
commands.entity(parent_entity).remove::<AncestorMarker<C>>();
remove_component::<AncestorMarker<C>>(commands, entity);
}

previous_parent = parent_entity;
Expand Down Expand Up @@ -414,6 +421,21 @@ mod tests {
app.world_mut().run_schedule(PostUpdate);

assert!(!app.world().entity(an).contains::<AncestorMarker<C>>());

// Make CY a child of AN again. The `AncestorMarker<C>` marker should
// now be added to AN.
let mut entity_mut = app.world_mut().entity_mut(an);
entity_mut.add_child(cy);

app.world_mut().run_schedule(PostUpdate);
assert!(app.world().entity(an).contains::<AncestorMarker<C>>());

// Make CY an orphan and delete AN. This must not crash.
let mut entity_mut = app.world_mut().entity_mut(an);
entity_mut.remove_children(&[cy]);
entity_mut.despawn();

app.world_mut().run_schedule(PostUpdate);
}

#[test]
Expand Down Expand Up @@ -489,5 +511,11 @@ mod tests {
assert!(!app.world().entity(bn).contains::<AncestorMarker<C>>());
assert!(app.world().entity(cy).contains::<AncestorMarker<C>>());
assert!(app.world().entity(an).contains::<AncestorMarker<C>>());

// Move all children from CY to BN and remove CY. This must not crash.
let mut entity_mut = app.world_mut().entity_mut(bn);
entity_mut.push_children(&[dn, en, fy, gy]);

app.world_mut().run_schedule(PostUpdate);
}
}

0 comments on commit db535b2

Please sign in to comment.