From 6c7c8d1e68b4354fb4bda9a38289abe7f7003044 Mon Sep 17 00:00:00 2001 From: Jonas Date: Wed, 10 Jul 2024 09:12:52 +0300 Subject: [PATCH 1/2] Fix `remove_ancestor_marker` crash if the entity is already despawned --- src/sync/ancestor_marker.rs | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/src/sync/ancestor_marker.rs b/src/sync/ancestor_marker.rs index bf308495..2a5533ba 100644 --- a/src/sync/ancestor_marker.rs +++ b/src/sync/ancestor_marker.rs @@ -213,6 +213,13 @@ fn add_ancestor_markers( } } +/// Remove the component from entity, unless it is already despawned. +fn remove_component(commands: &mut Commands, entity: Entity) { + if let Some(mut entity_commands) = commands.get_entity(entity) { + entity_commands.remove::(); + } +} + #[allow(clippy::type_complexity)] fn remove_ancestor_markers( entity: Entity, @@ -232,11 +239,11 @@ fn remove_ancestor_markers( if keep_marker { return; } else { - commands.entity(entity).remove::>(); + remove_component::>(commands, entity); } } else { // The parent has no children, so it cannot be an ancestor. - commands.entity(entity).remove::>(); + remove_component::>(commands, entity); } } @@ -254,11 +261,11 @@ fn remove_ancestor_markers( if keep_marker { return; } else { - commands.entity(parent_entity).remove::>(); + remove_component::>(commands, parent_entity); } } else { // The parent has no children, so it cannot be an ancestor. - commands.entity(parent_entity).remove::>(); + remove_component::>(commands, entity); } previous_parent = parent_entity; From 1f1edb821f9ba3888988f9ac51fbb9022eecf576 Mon Sep 17 00:00:00 2001 From: Jonas Date: Wed, 10 Jul 2024 10:54:30 +0300 Subject: [PATCH 2/2] added tests which (re)move children and despawn the previous parents. --- src/sync/ancestor_marker.rs | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/src/sync/ancestor_marker.rs b/src/sync/ancestor_marker.rs index 2a5533ba..4d970c2f 100644 --- a/src/sync/ancestor_marker.rs +++ b/src/sync/ancestor_marker.rs @@ -421,6 +421,21 @@ mod tests { app.world_mut().run_schedule(PostUpdate); assert!(!app.world().entity(an).contains::>()); + + // Make CY a child of AN again. The `AncestorMarker` 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::>()); + + // 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] @@ -496,5 +511,11 @@ mod tests { assert!(!app.world().entity(bn).contains::>()); assert!(app.world().entity(cy).contains::>()); assert!(app.world().entity(an).contains::>()); + + // 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); } }