Don't consider despawns as invalidating EntityWorldMut

[ElliottjPierce]

Objective

#19451 changed how entities handle spawning and despawning.
One of those changes introduced the idea that despawning an entity from commands while holding an EntityWorldMut of that entity made that EntityWorldMut invalid and panicked when that happend.

Fixes #19828.

Solution

Handle these despawns in the same way as despawning without freeing.
This means an EntityWorldMut can no longer assume its EntityId is valid;
it can not assume that its generation is up to date.
AFAIK, this restriction doesn't introduce any new or exciting ways for this to fail.
It just delays the panics for some cases.
For example, despawning from commands and then attempting an insert will panic later (at the insert) instead of earlier (at the despawn).

Testing

• CI

[ElliottjPierce]

This might also mean we could revert #22437, but there are other reasons to keep it.

[alice-i-cecile]

This is fine to add to the 0.18.1 milestone: it's effectively a regression fix, and won't invalidate any programs that currently work.

I would definitely prefer to not panic here, and I don't even think that this is worth warning about. Can you please add the invariant from the PR description somewhere on EntityWorldMut? Maybe a comment, or a doc comment?

Or maybe incorporate your comment on Discord:

Super simple fix. Somewhat limits how we could expand the EntityWorldMut API (Ex: we couldn't safely add a respawn, is_freed, or anything like that), but given that you're the third person to run into issues here, I think its worth it.

[mgi388]

@ElliottjPierce FYI here are the unit tests I had from investigation (I think more or less the same idea as your PR). I had an observer test as well as a hook one but I am not sure if you need both so I will leave that with you:

/// Tests that inserting a component with a hook that queues a despawn
/// command doesn't panic in `update_location`. Instead, the entity should
/// be marked as despawned and subsequent operations can check
/// `is_despawned()`.
#[test]
fn insert_with_hook_that_despawns_entity_does_not_panic() {
    #[derive(Component)]
    #[component(on_add = on_add_despawn)]
    struct DespawnOnAdd;

    fn on_add_despawn(mut world: DeferredWorld, context: HookContext) {
        // Queue a despawn command that will run during flush().
        world.commands().entity(context.entity).despawn();
    }

    let mut world = World::new();

    // Spawn an entity and then insert a component that will despawn it via
    // a hook.
    let entity = world.spawn_empty().id();

    // Get an EntityWorldMut and insert the component.
    let mut entity_mut = world.entity_mut(entity);
    entity_mut.insert(DespawnOnAdd);

    // The entity should now be marked as despawned.
    assert!(entity_mut.is_despawned());

    // Verify the entity is actually gone from the world.
    assert!(world.get_entity(entity).is_err());
}

/// Tests that inserting a component with an observer that queues a despawn
/// command doesn't panic in `update_location`.
#[test]
fn insert_with_observer_that_despawns_entity_does_not_panic() {
    #[derive(Component)]
    struct TriggerDespawn;

    let mut world = World::new();

    // Add an observer that despawns the entity when TriggerDespawn is
    // added.
    world.add_observer(|add: On<Add, TriggerDespawn>, mut commands: Commands| {
        commands.entity(add.event_target()).despawn();
    });

    // Spawn an entity and then insert a component that will despawn it via
    // observer.
    let entity = world.spawn_empty().id();

    // Get an EntityWorldMut and insert the component.
    let mut entity_mut = world.entity_mut(entity);
    entity_mut.insert(TriggerDespawn);

    // The entity should now be marked as despawned.
    assert!(entity_mut.is_despawned());

    // Verify the entity is actually gone from the world.
    assert!(world.get_entity(entity).is_err());
}

And the diff for Bevy 0.18 (main is now slightly different I believe): Edit: Actually I didn't use get_spawned so mine could be wrong.

diff --git a/crates/bevy_ecs/src/world/entity_access/world_mut.rs b/crates/bevy_ecs/src/world/entity_access/world_mut.rs
index b8a816d84..dc0cd24e3 100644
--- a/crates/bevy_ecs/src/world/entity_access/world_mut.rs
+++ b/crates/bevy_ecs/src/world/entity_access/world_mut.rs
@@ -1801,9 +1801,11 @@ impl<'w> EntityWorldMut<'w> {
     ///
     /// This is *only* required when using the unsafe function [`EntityWorldMut::world_mut`],
     /// which enables the location to change.
+    ///
+    /// If the entity was despawned (e.g., by a command that ran during a flush), this will
+    /// set the location to `None`, which can be checked via [`EntityWorldMut::is_despawned`].
     pub fn update_location(&mut self) {
-        self.location = self.world.entities().get(self.entity)
-            .expect("Attempted to update the location of a despawned entity, which is impossible. This was the result of performing an operation on this EntityWorldMut that queued a despawn command");
+        self.location = self.world.entities().get(self.entity).ok().flatten();
     }

     /// Returns if the entity has been despawned.

[mgi388]

Actually I didn't use get_spawned so mine could be wrong.

[mgi388]

This fixes the issue for my recently-migrated-to-Bevy-0.18 game so I am leaving an approval based on that, not because I fully understand the implications of this change for everything/everyone else. 🙏

[grind086]

This also looks like it fixes #19828 (unless it was already fixed?)

[urben1680]

This does not look controversial at all to me. It is basically simple. EntityWorldMut has the is_despawned method. Therefore it is valid if an instance exists despite the entity being despawned. So for me ideally as few methods as possible panic on being despawned.

If I would change anything here, then add a note to the update_location docs that there might be no location anymore in the case of despawning. Still the user has to use this method in that case to fulfill world_scope's safety contract.

[ElliottjPierce]

This also looks like it fixes #19828 (unless it was already fixed?)

This just made things more exciting! Yeah, this should fix that now. I just added some checks to make sure bundle affects respect that the entity might not be spawned.

[chescock]

I agree that this restores the original behavior!

Combining this diff with the one in #19451 gives

-        self.location = self.world.entities().get(self.entity);
+        self.location = self.world.entities().get_spawned(self.entity).ok();

It looks like that PR changed all the get calls other than this one to get_spawned, so the new code here is consistent with everything else.

The only difference in this PR is the behavior if the entity has the wrong generation, and the old get method returned None there, so using None again now should restore the original behavior.

[chescock]

How can this cause a double free? This method consumes the EntityWorldMut by value, so I would think the caller would have full knowledge of the lifetime.

[ElliottjPierce]

What if this is called from a hook or something? The caller needs to know that nothing else has freed the entity. If the entity was still spawned before this call, that's trivial to satisfy. But if not, the caller needs to know that nothing in a "on remove" hook or something already despawned the entity and freed the entity. This is why we still need those extra checks in despawn.

This is confusing, and it's one of those edge cases this introduces. We might need a better way of communicating this.

[chescock]

What if this is called from a hook or something?

I think hooks only have &mut EntityWorldMut, so they can't call despawn_no_free(self). They can use reborrow_scope to get a nested EntityWorldMut... but that updates location on the original &mut EntityWorldMut when the scope returns, so it would get set to None and everything else would panic.

... Oh, wait, no, despawn_no_free_with_caller doesn't panic if the location is invalid. So if you despawn_no_free inside of a reborrow_scope and again outside of the scope, then it will return the same Entity twice.

Maybe despawn_no_free should return a Result, and not give you back the Entity if the entity had already been despawned? Anyway, that's outside of the scope of this PR.

[chescock]

Why did you move this call outside of the world_scope?

I think it's a no-op pointer cast, so it's not a big deal either way.

[ElliottjPierce]

I moved it out to make sure it was dropped even when the entity was not spawned. But I'm no expert with these pointer types. Maybe it's unnecessary?

[chescock]

I moved it out to make sure it was dropped even when the entity was not spawned. But I'm no expert with these pointer types. Maybe it's unnecessary?

I believe it's unnecessary. But it's also harmless!

The goal for deconstruct_moving_ptr! is to work exactly like ordinary destructuring. Dropping a MovingPtr will call the destructor on the owned type, so in one case we drop the whole SpawnRelatedBundle and in the other case we drop the individual fields, which amounts to the same thing.

[chescock]

Oh, nobody is ever going to remember to check this :).

I wonder whether we want a rule that we just skip bundle effects if a hook or observer despawns an entity. Then we could do the check once in the engine and it would be solved everywhere. But maybe there are use cases where we'd still want effects to run?

Or maybe we want to pass an Option<&mut EntityWorldMut> to make it more obvious that it's a case they have to handle? Oh, but then they don't have access to the world anymore, so I guess it would need to be some kind of Result<&mut EntityWorldMut, SomethingElseWithTheDataYouNeed>, which starts getting kind of ridiculous.

Or maybe it just doesn't matter, since third-party bundle effects are supposed to be pretty rare.

Either way, this seems like the right change for now!

[ElliottjPierce]

Yeah. I agree that an option or result would be nice, but then, it would be nice everywhere that uses EntityWorldMut, so...

And I don't think we can skip this when the entity is despawned because the safety requirements say this must be called.