Move common code into a separate function to avoid the complexity of calling delete_entity_recorded from handle_exit_ingame

server/src/cmd.rs

@@ -2303,7 +2303,7 @@ fn handle_kill_npcs(
     let count = to_kill.len();
     for entity in to_kill {
         // Directly remove entities instead of modifying health to avoid loot drops.
-        if let Err(e) = server.state.delete_entity_recorded(entity, None) {
+        if let Err(e) = server.state.delete_entity_recorded(entity) {
             error!(?e, ?entity, "Failed to delete entity");
         }
     }
@@ -3465,7 +3465,7 @@ fn handle_remove_lights(
     let size = to_delete.len();
 
     for entity in to_delete {
-        if let Err(e) = server.state.delete_entity_recorded(entity, None) {
+        if let Err(e) = server.state.delete_entity_recorded(entity) {
             error!(?e, "Failed to delete light: {:?}", e);
         }
     }

server/src/events/entity_manipulation.rs

@@ -577,7 +577,7 @@ pub fn handle_destroy(server: &mut Server, entity: EcsEntity, last_change: Healt
     }
 
     if should_delete {
-        if let Err(e) = state.delete_entity_recorded(entity, None) {
+        if let Err(e) = state.delete_entity_recorded(entity) {
             error!(?e, ?entity, "Failed to delete destroyed entity");
         }
     }
@@ -589,7 +589,7 @@ pub fn handle_destroy(server: &mut Server, entity: EcsEntity, last_change: Healt
 pub fn handle_delete(server: &mut Server, entity: EcsEntity) {
     let _ = server
         .state_mut()
-        .delete_entity_recorded(entity, None)
+        .delete_entity_recorded(entity)
         .map_err(|e| error!(?e, ?entity, "Failed to delete destroyed entity"));
 }
 

server/src/events/inventory_manip.rs

@@ -228,7 +228,7 @@ pub fn handle_inventory(server: &mut Server, entity: EcsEntity, manip: comp::Inv
                     // entirely.
                     drop(item_storage);
                     drop(inventories);
-                    state.delete_entity_recorded(item_entity, None).expect(
+                    state.delete_entity_recorded(item_entity).expect(
                         "We knew item_entity existed since we just successfully removed its Item \
                          component.",
                     );

server/src/events/player.rs

@@ -56,7 +56,7 @@ pub fn handle_exit_ingame(server: &mut Server, entity: EcsEntity, skip_persisten
         entity
     };
 
-    // Create new entity with just `Client`, `Uid`, `Player`, and `...Stream`
+    // Create new entity with just `Client`, `Uid`, `Player`, `Admin`, `Group`
     // components.
     //
     // Easier than checking and removing all other known components.
@@ -68,28 +68,20 @@ pub fn handle_exit_ingame(server: &mut Server, entity: EcsEntity, skip_persisten
     // Note: If other `ServerEvent`s are referring to this entity they will be
     // disrupted.
 
-    // Since we remove `Uid` below, any trades won't be cancelled by
+    // Cancel trades here since we don't use `delete_entity_recorded` and we
-    // `delete_entity_recorded`. So we cancel the trade here. (maybe the trade
+    // remove `Uid` below.
-    // key could be switched from `Uid` to `Entity`)
     super::cancel_trades_for(state, entity);
 
-    let maybe_admin = state.ecs().write_storage::<comp::Admin>().remove(entity);
+    let maybe_group = state.read_component_copied::<group::Group>(entity);
-    let maybe_group = state
+    let maybe_admin = state.delete_component::<comp::Admin>(entity);
-        .ecs()
+    // Not sure if we still need to actually remove the Uid or if the group
-        .write_storage::<group::Group>()
+    // logic below relies on this...
-        .get(entity)
+    let maybe_uid = state.delete_component::<Uid>(entity);
-        .cloned();
 
-    let uid = if let Some((client, uid, player)) = (|| {
+    if let Some(client) = state.delete_component::<Client>(entity)
-        let ecs = state.ecs();
+        && let Some(uid) = maybe_uid
-        Some((
+        && let Some(player) = state.delete_component::<comp::Player>(entity)
-            ecs.write_storage::<Client>().remove(entity)?,
+    {
-            // TODO: we need to handle the case where the UID component is removed but there may be
-            // a character ID mapping that also needs to be removed!
-            ecs.write_storage::<Uid>().remove(entity)?,
-            ecs.write_storage::<comp::Player>().remove(entity)?,
-        ))
-    })() {
         // Tell client its request was successful
         client.send_fallible(ServerGeneral::ExitInGameSuccess);
 
@@ -105,13 +97,12 @@ pub fn handle_exit_ingame(server: &mut Server, entity: EcsEntity, skip_persisten
             .with(uid)
             .build();
 
-        let ecs = state.ecs();
+        // Ensure IdMaps maps this uid to the new entity.
-        // Ensure IdMaps maps this uid to the new entity
+        state.mut_resource::<IdMaps>().remap_entity(uid, new_entity);
-        ecs.write_resource::<IdMaps>().remap_entity(uid, new_entity);
 
-        // Note, since the Uid has been removed from the old entity and the
+        let ecs = state.ecs();
-        // Group component will be removed below, that prevents
+        // Note, we use `delete_entity_common` directly to avoid `delete_entity_recorded` from
-        // `delete_entity_recorded` from making any changes to the group.
+        // making any changes to the group.
         if let Some(group) = maybe_group {
             let mut group_manager = ecs.write_resource::<group::GroupManager>();
             if group_manager
@@ -130,17 +121,29 @@ pub fn handle_exit_ingame(server: &mut Server, entity: EcsEntity, skip_persisten
                 );
             }
         }
-
+        // delete_entity_recorded` is not used so we don't need to worry aobut
-        // Erase group component to avoid group restructure when deleting the entity
+        // group restructuring when deleting this entity.
-        ecs.write_storage::<group::Group>().remove(entity);
-
-        Some(uid)
     } else {
-        None
+        error!("handle_exit_ingame called with entity that is missing expected components");
-    };
+    }
 
+    let maybe_character = state
+        .read_storage::<Presence>()
+        .get(entity)
+        .and_then(|p| p.kind.character_id());
+    let maybe_rtsim = state.read_component_copied::<common::rtsim::RtSimEntity>(entity);
+    state.mut_resource::<IdMaps>().remove_entity(
+        Some(entity),
+        None, // Uid re-mapped, we don't want to remove the mapping
+        maybe_character,
+        maybe_rtsim,
+    );
+
+    // We don't want to use delete_entity_recorded since we are transfering the
+    // Uid to a new entity (and e.g. don't want it to be unmapped).
+    //
     // Delete old entity
-    if let Err(e) = state.delete_entity_recorded(entity, Some(uid)) {
+    if let Err(e) = crate::state_ext::delete_entity_common(state, entity, maybe_uid) {
         error!(
             ?e,
             ?entity,
@@ -235,7 +238,7 @@ pub fn handle_client_disconnect(
     }
 
     // Delete client entity
-    if let Err(e) = server.state.delete_entity_recorded(entity, None) {
+    if let Err(e) = server.state.delete_entity_recorded(entity) {
         error!(?e, ?entity, "Failed to delete disconnected client");
     }
 
@@ -584,7 +587,7 @@ pub fn handle_possess(server: &mut Server, possessor_uid: Uid, possessee_uid: Ui
     // this). See note on `persist_entity` call above for why we do this.
     if let Some(entity) = delete_entity {
         // Delete old entity
-        if let Err(e) = state.delete_entity_recorded(entity, None) {
+        if let Err(e) = state.delete_entity_recorded(entity) {
             error!(
                 ?e,
                 ?entity,

server/src/lib.rs

@@ -855,7 +855,7 @@ impl Server {
 
         // Actually perform entity deletion
         for entity in to_delete {
-            if let Err(e) = self.state.delete_entity_recorded(entity, None) {
+            if let Err(e) = self.state.delete_entity_recorded(entity) {
                 error!(?e, "Failed to delete agent outside the terrain");
             }
         }

server/src/state_ext.rs

@@ -149,7 +149,6 @@ pub trait StateExt {
     fn delete_entity_recorded(
         &mut self,
         entity: EcsEntity,
-        exit_ingame: Option<Option<Uid>>,
     ) -> Result<(), specs::error::WrongGeneration>;
     /// Get the given entity as an [`Actor`], if it is one.
     fn entity_as_actor(&self, entity: EcsEntity) -> Option<Actor>;
@@ -1152,19 +1151,13 @@ impl StateExt for State {
     fn delete_entity_recorded(
         &mut self,
         entity: EcsEntity,
-        // Indicates this is being called from handle_exit_ingame, where the `Uid` is removed and
-        // transferred to a new entity.
-        //
-        // Inner option is dependent on if the entity had a `Uid` component (which all clients
-        // should...).
-        exit_ingame: Option<Option<Uid>>,
     ) -> Result<(), specs::error::WrongGeneration> {
+        // NOTE: both this and handle_exit_ingame call delete_entity_common, so cleanup
+        // added here may need to be duplicated in handle_exit_ingame (depending
+        // on its nature).
+
         // Remove entity from a group if they are in one.
-        //
+        {
-        // If called from exit ingame, all group related things are already handled
-        // (this code wouldn't do anything anyway since the Group component was
-        // removed).
-        if exit_ingame.is_none() {
             let clients = self.ecs().read_storage::<Client>();
             let uids = self.ecs().read_storage::<Uid>();
             let mut group_manager = self.ecs().write_resource::<comp::group::GroupManager>();
@@ -1192,77 +1185,25 @@ impl StateExt for State {
         }
 
         // Cancel extant trades
-        //
-        // handle_exit_ingame already calls this since cancelling trades retrieves the
-        // Uid internally.
-        if exit_ingame.is_none() {
         events::cancel_trades_for(self, entity);
-        }
 
-        let (maybe_uid, maybe_presence, maybe_rtsim_entity, maybe_pos) = (
+        // NOTE: We expect that these 3 components are never removed from an entity (nor
-            self.ecs().read_storage::<Uid>().get(entity).copied(),
-            // NOTE: We expect that these 2 components are never removed from an entity (nor
         // mutated) (at least not without updating the relevant mappings)!
-            // TODO: I think Presence may have some other locations where kind is modified (double
+        let maybe_uid = self.read_component_copied::<Uid>(entity);
-            // check)
+        let maybe_character = self
-            self.ecs()
             .read_storage::<Presence>()
             .get(entity)
-                .map(|p| p.kind),
+            .and_then(|p| p.kind.character_id());
-            self.ecs()
+        let maybe_rtsim = self.read_component_copied::<RtSimEntity>(entity);
-                .read_storage::<RtSimEntity>()
-                .get(entity)
-                .copied(),
-            self.ecs().read_storage::<comp::Pos>().get(entity).copied(),
-        );
 
-        if maybe_uid.or(exit_ingame.flatten()).is_none() {
+        self.mut_resource::<IdMaps>().remove_entity(
-            // For now we expect all entities have a Uid component.
-            error!("Deleting entity without Uid component");
-        }
-
-        self.ecs().write_resource::<IdMaps>().remove_entity(
             Some(entity),
-            // We don't want to pass in the Uid from exit_ingame since it has already
-            // been mapped to another entity.
             maybe_uid,
-            maybe_presence.and_then(|p| p.character_id()),
+            maybe_character,
-            maybe_rtsim_entity,
+            maybe_rtsim,
         );
 
-        let res = self.ecs_mut().delete_entity(entity);
+        delete_entity_common(self, entity, maybe_uid)
-        if res.is_ok() {
-            // Note: Adding the `Uid` to the deleted list when exiting "in-game" relies on
-            // the client not being able to immediately re-enter the game in the
-            // same tick (since we could then mix up the ordering of things and
-            // tell other clients to forget the new entity).
-            //
-            // The client will ignore requests to delete its own entity that are trigger by
-            // this. (TODO: implement this)
-            if let (Some(uid), Some(pos)) = (maybe_uid.or(exit_ingame.flatten()), maybe_pos) {
-                if let Some(region_key) = self
-                    .ecs()
-                    .read_resource::<common::region::RegionMap>()
-                    .find_region(entity, pos.0)
-                {
-                    self.ecs()
-                        .write_resource::<DeletedEntities>()
-                        .record_deleted_entity(uid, region_key);
-                } else {
-                    // Don't panic if the entity wasn't found in a region maybe it was just created
-                    // and then deleted before the region manager had a chance to assign it a
-                    // region
-                    warn!(
-                        ?uid,
-                        ?pos,
-                        "Failed to find region containing entity during entity deletion, assuming \
-                         it wasn't sent to any clients and so deletion doesn't need to be \
-                         recorded for sync purposes"
-                    );
-                }
-            }
-        }
-        res
     }
 
     fn entity_as_actor(&self, entity: EcsEntity) -> Option<Actor> {
@@ -1293,3 +1234,50 @@ fn send_to_group(g: &Group, ecs: &specs::World, msg: &comp::ChatMsg) {
         }
     }
 }
+
+/// This should only be called from `handle_exit_ingame` and
+/// `delete_entity_recorded`!!!!!!!
+pub(crate) fn delete_entity_common(
+    state: &mut State,
+    entity: EcsEntity,
+    maybe_uid: Option<Uid>,
+) -> Result<(), specs::error::WrongGeneration> {
+    if maybe_uid.is_none() {
+        // For now we expect all entities have a Uid component.
+        error!("Deleting entity without Uid component");
+    }
+    let maybe_pos = state.read_component_copied::<comp::Pos>(entity);
+
+    let res = state.ecs_mut().delete_entity(entity);
+    if res.is_ok() {
+        // Note: Adding the `Uid` to the deleted list when exiting "in-game" relies on
+        // the client not being able to immediately re-enter the game in the
+        // same tick (since we could then mix up the ordering of things and
+        // tell other clients to forget the new entity).
+        //
+        // The client will ignore requests to delete its own entity that are triggered
+        // by this.
+        if let Some((uid, pos)) = maybe_uid.zip(maybe_pos) {
+            let region_key = state
+                .ecs()
+                .read_resource::<common::region::RegionMap>()
+                .find_region(entity, pos.0);
+            if let Some(region_key) = region_key {
+                state
+                    .mut_resource::<DeletedEntities>()
+                    .record_deleted_entity(uid, region_key);
+            } else {
+                // Don't panic if the entity wasn't found in a region, maybe it was just created
+                // and then deleted before the region manager had a chance to assign it a region
+                warn!(
+                    ?uid,
+                    ?pos,
+                    "Failed to find region containing entity during entity deletion, assuming it \
+                     wasn't sent to any clients and so deletion doesn't need to be recorded for \
+                     sync purposes"
+                );
+            }
+        }
+    }
+    res
+}