diff --git a/server/src/cmd.rs b/server/src/cmd.rs index bafb4ede57..96a0139fb4 100644 --- a/server/src/cmd.rs +++ b/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); } } diff --git a/server/src/events/entity_manipulation.rs b/server/src/events/entity_manipulation.rs index 2cea4f658f..61b8759fdb 100644 --- a/server/src/events/entity_manipulation.rs +++ b/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")); } diff --git a/server/src/events/inventory_manip.rs b/server/src/events/inventory_manip.rs index 063714611b..e0df6c6164 100644 --- a/server/src/events/inventory_manip.rs +++ b/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.", ); diff --git a/server/src/events/player.rs b/server/src/events/player.rs index 81e5dcc09a..dff4d34704 100644 --- a/server/src/events/player.rs +++ b/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 - // `delete_entity_recorded`. So we cancel the trade here. (maybe the trade - // key could be switched from `Uid` to `Entity`) + // Cancel trades here since we don't use `delete_entity_recorded` and we + // remove `Uid` below. super::cancel_trades_for(state, entity); - let maybe_admin = state.ecs().write_storage::().remove(entity); - let maybe_group = state - .ecs() - .write_storage::() - .get(entity) - .cloned(); + let maybe_group = state.read_component_copied::(entity); + let maybe_admin = state.delete_component::(entity); + // Not sure if we still need to actually remove the Uid or if the group + // logic below relies on this... + let maybe_uid = state.delete_component::(entity); - let uid = if let Some((client, uid, player)) = (|| { - let ecs = state.ecs(); - Some(( - ecs.write_storage::().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::().remove(entity)?, - ecs.write_storage::().remove(entity)?, - )) - })() { + if let Some(client) = state.delete_component::(entity) + && let Some(uid) = maybe_uid + && let Some(player) = state.delete_component::(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 - ecs.write_resource::().remap_entity(uid, new_entity); + // Ensure IdMaps maps this uid to the new entity. + state.mut_resource::().remap_entity(uid, new_entity); - // Note, since the Uid has been removed from the old entity and the - // Group component will be removed below, that prevents - // `delete_entity_recorded` from making any changes to the group. + let ecs = state.ecs(); + // Note, we use `delete_entity_common` directly to avoid `delete_entity_recorded` from + // making any changes to the group. if let Some(group) = maybe_group { let mut group_manager = ecs.write_resource::(); if group_manager @@ -130,17 +121,29 @@ pub fn handle_exit_ingame(server: &mut Server, entity: EcsEntity, skip_persisten ); } } - - // Erase group component to avoid group restructure when deleting the entity - ecs.write_storage::().remove(entity); - - Some(uid) + // delete_entity_recorded` is not used so we don't need to worry aobut + // group restructuring when deleting this entity. } else { - None - }; + error!("handle_exit_ingame called with entity that is missing expected components"); + } + let maybe_character = state + .read_storage::() + .get(entity) + .and_then(|p| p.kind.character_id()); + let maybe_rtsim = state.read_component_copied::(entity); + state.mut_resource::().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, diff --git a/server/src/lib.rs b/server/src/lib.rs index fe154812f4..8d8ad6219a 100644 --- a/server/src/lib.rs +++ b/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"); } } diff --git a/server/src/state_ext.rs b/server/src/state_ext.rs index ce855b4523..48986dae7f 100644 --- a/server/src/state_ext.rs +++ b/server/src/state_ext.rs @@ -149,7 +149,6 @@ pub trait StateExt { fn delete_entity_recorded( &mut self, entity: EcsEntity, - exit_ingame: Option>, ) -> Result<(), specs::error::WrongGeneration>; /// Get the given entity as an [`Actor`], if it is one. fn entity_as_actor(&self, entity: EcsEntity) -> Option; @@ -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>, ) -> 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::(); let uids = self.ecs().read_storage::(); let mut group_manager = self.ecs().write_resource::(); @@ -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); - } + events::cancel_trades_for(self, entity); - let (maybe_uid, maybe_presence, maybe_rtsim_entity, maybe_pos) = ( - self.ecs().read_storage::().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 - // check) - self.ecs() - .read_storage::() - .get(entity) - .map(|p| p.kind), - self.ecs() - .read_storage::() - .get(entity) - .copied(), - self.ecs().read_storage::().get(entity).copied(), - ); + // NOTE: We expect that these 3 components are never removed from an entity (nor + // mutated) (at least not without updating the relevant mappings)! + let maybe_uid = self.read_component_copied::(entity); + let maybe_character = self + .read_storage::() + .get(entity) + .and_then(|p| p.kind.character_id()); + let maybe_rtsim = self.read_component_copied::(entity); - if maybe_uid.or(exit_ingame.flatten()).is_none() { - // For now we expect all entities have a Uid component. - error!("Deleting entity without Uid component"); - } - - self.ecs().write_resource::().remove_entity( + self.mut_resource::().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_rtsim_entity, + maybe_character, + maybe_rtsim, ); - let res = self.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 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::() - .find_region(entity, pos.0) - { - self.ecs() - .write_resource::() - .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 + delete_entity_common(self, entity, maybe_uid) } fn entity_as_actor(&self, entity: EcsEntity) -> Option { @@ -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, +) -> 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::(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::() + .find_region(entity, pos.0); + if let Some(region_key) = region_key { + state + .mut_resource::() + .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 +}