diff --git a/common/net/src/sync/sync_ext.rs b/common/net/src/sync/sync_ext.rs index d0f842c224..f46a2e239b 100644 --- a/common/net/src/sync/sync_ext.rs +++ b/common/net/src/sync/sync_ext.rs @@ -69,9 +69,9 @@ impl WorldSyncExt for specs::World { // TODO: rename method, document called from client only fn delete_entity_and_clear_from_id_maps(&mut self, uid: Uid) { // Clear from uid allocator - let maybe_entity = self - .write_resource::() - .remove_entity(None, uid, None, None); + let maybe_entity = + self.write_resource::() + .remove_entity(None, Some(uid), None, None); if let Some(entity) = maybe_entity { if let Err(e) = self.delete_entity(entity) { error!(?e, "Failed to delete entity"); diff --git a/common/src/comp/presence.rs b/common/src/comp/presence.rs index ac2208736b..a57620e3cc 100644 --- a/common/src/comp/presence.rs +++ b/common/src/comp/presence.rs @@ -8,6 +8,9 @@ use vek::*; pub struct Presence { pub terrain_view_distance: ViewDistance, pub entity_view_distance: ViewDistance, + /// If mutating this (or the adding/replacing the Presence component as a + /// whole), make sure the mapping of `CharacterId` in `IdMaps` is + /// updated! pub kind: PresenceKind, pub lossy_terrain_compression: bool, } @@ -32,7 +35,8 @@ impl Component for Presence { pub enum PresenceKind { Spectator, // Note: we don't know if this character ID is valid and associated with the respective player - // until it the character has loaded successfully. + // until it the character has loaded successfully. The ID should only be trusted and included + // in the mapping when the variant changed to `Character`. LoadingCharacter(CharacterId), Character(CharacterId), Possessor, @@ -66,7 +70,7 @@ enum Direction { /// (e.g. shifting from increasing the value to decreasing it). This is useful /// since we want to avoid rapid cycles of shrinking and expanding of the view /// distance. -#[derive(Debug)] +#[derive(Debug, Clone, Copy)] pub struct ViewDistance { direction: Direction, last_direction_change_time: Instant, diff --git a/common/src/uid.rs b/common/src/uid.rs index f0847e7f56..0b1185a47e 100644 --- a/common/src/uid.rs +++ b/common/src/uid.rs @@ -102,7 +102,7 @@ mod not_wasm { pub fn remove_entity( &mut self, expected_entity: Option, - uid: Uid, + uid: Option, cid: Option, rid: Option, ) -> Option { @@ -139,7 +139,7 @@ mod not_wasm { } } - let maybe_entity = remove(&mut self.uid_mapping, Some(uid), expected_entity); + let maybe_entity = remove(&mut self.uid_mapping, uid, expected_entity); let expected_entity = expected_entity.or(maybe_entity); remove(&mut self.cid_mapping, cid, expected_entity); remove(&mut self.rid_mapping, rid, expected_entity); diff --git a/server/src/cmd.rs b/server/src/cmd.rs index f12d0c71df..bafb4ede57 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) { + if let Err(e) = server.state.delete_entity_recorded(entity, None) { error!(?e, ?entity, "Failed to delete entity"); } } @@ -2560,6 +2560,7 @@ fn handle_light( .ecs_mut() .create_entity_synced() .with(pos) + // TODO: I don't think we intend to add this component to non-client entities? .with(comp::ForceUpdate::forced()) .with(light_emitter); if let Some(light_offset) = light_offset_opt { @@ -3464,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) { + if let Err(e) = server.state.delete_entity_recorded(entity, None) { error!(?e, "Failed to delete light: {:?}", e); } } diff --git a/server/src/events/entity_creation.rs b/server/src/events/entity_creation.rs index c7c0902248..3654097a05 100644 --- a/server/src/events/entity_creation.rs +++ b/server/src/events/entity_creation.rs @@ -50,7 +50,8 @@ pub fn handle_initialize_character( } } else { // A character delete or update was somehow initiated after the login commenced, - // so disconnect the client without saving any data and abort the login process. + // so kick the client out of "ingame" without saving any data and abort + // the character loading process. handle_exit_ingame(server, entity, true); } } diff --git a/server/src/events/entity_manipulation.rs b/server/src/events/entity_manipulation.rs index 61b8759fdb..2cea4f658f 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) { + if let Err(e) = state.delete_entity_recorded(entity, None) { 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) + .delete_entity_recorded(entity, None) .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 e0df6c6164..063714611b 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).expect( + state.delete_entity_recorded(item_entity, None).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 a92a2fcdca..81e5dcc09a 100644 --- a/server/src/events/player.rs +++ b/server/src/events/player.rs @@ -80,7 +80,7 @@ pub fn handle_exit_ingame(server: &mut Server, entity: EcsEntity, skip_persisten .get(entity) .cloned(); - if let Some((client, uid, player)) = (|| { + let uid = if let Some((client, uid, player)) = (|| { let ecs = state.ecs(); Some(( ecs.write_storage::().remove(entity)?, @@ -133,9 +133,14 @@ 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) + } else { + None + }; + // Delete old entity - if let Err(e) = state.delete_entity_recorded(entity) { + if let Err(e) = state.delete_entity_recorded(entity, Some(uid)) { error!( ?e, ?entity, @@ -230,7 +235,7 @@ pub fn handle_client_disconnect( } // Delete client entity - if let Err(e) = server.state.delete_entity_recorded(entity) { + if let Err(e) = server.state.delete_entity_recorded(entity, None) { error!(?e, ?entity, "Failed to delete disconnected client"); } @@ -265,8 +270,11 @@ fn persist_entity(state: &mut State, entity: EcsEntity) -> EcsEntity { state.ecs().fetch_mut::(), ) { match presence.kind { - PresenceKind::LoadingCharacter(_char_id) => { - error!("Unexpected state when persist_entity is called! Some of the components required above should only be present after a character is loaded!"); + PresenceKind::LoadingCharacter(_char_id) => { + error!( + "Unexpected state when persist_entity is called! Some of the components \ + required above should only be present after a character is loaded!" + ); }, PresenceKind::Character(char_id) => { let waypoint = state @@ -344,7 +352,7 @@ pub fn handle_possess(server: &mut Server, possessor_uid: Uid, possessee_uid: Ui ) { // In this section we check various invariants and can return early if any of // them are not met. - { + let new_presence = { let ecs = state.ecs(); // Check that entities still exist if !possessor.gen().is_alive() @@ -361,6 +369,7 @@ pub fn handle_possess(server: &mut Server, possessor_uid: Uid, possessee_uid: Ui let clients = ecs.read_storage::(); let players = ecs.read_storage::(); + let presences = ecs.read_storage::(); if clients.contains(possessee) || players.contains(possessee) { error!("Can't possess other players!"); @@ -387,8 +396,32 @@ pub fn handle_possess(server: &mut Server, possessor_uid: Uid, possessee_uid: Ui return; } + if let Some(presence) = presences.get(possessor) { + delete_entity = match presence.kind { + k @ (PresenceKind::LoadingCharacter(_) | PresenceKind::Spectator) => { + error!(?k, "Unexpected presence kind for a possessor."); + return; + }, + PresenceKind::Possessor => None, + // Since we call `persist_entity` below we will want to delete the entity (to + // avoid item duplication). + PresenceKind::Character(_) => Some(possessor), + }; + + Some(Presence { + terrain_view_distance: presence.terrain_view_distance, + entity_view_distance: presence.entity_view_distance, + // This kind (rather than copying Character presence) prevents persistence + // from overwriting original character info with stuff from the new character. + kind: PresenceKind::Possessor, + lossy_terrain_compression: presence.lossy_terrain_compression, + }) + } else { + None + } + // No early returns allowed after this. - } + }; // Sync the player's character data to the database. This must be done before // moving any components from the entity. @@ -436,31 +469,38 @@ pub fn handle_possess(server: &mut Server, possessor_uid: Uid, possessee_uid: Ui } let mut players = ecs.write_storage::(); - let mut presence = ecs.write_storage::(); let mut subscriptions = ecs.write_storage::(); let mut admins = ecs.write_storage::(); let mut waypoints = ecs.write_storage::(); + let mut force_updates = ecs.write_storage::(); transfer_component(&mut players, possessor, possessee, |x| x); - transfer_component(&mut presence, possessor, possessee, |mut presence| { - presence.kind = match presence.kind { - PresenceKind::Spectator => PresenceKind::Spectator, - // This prevents persistence from overwriting original character info with stuff - // from the new character. - PresenceKind::Character(_) | PresenceKind::LoadingCharacter(_) => { - delete_entity = Some(possessor); - PresenceKind::Possessor - }, - PresenceKind::Possessor => PresenceKind::Possessor, - }; - - presence - }); transfer_component(&mut subscriptions, possessor, possessee, |x| x); transfer_component(&mut admins, possessor, possessee, |x| x); transfer_component(&mut waypoints, possessor, possessee, |x| x); + let mut update_counter = 0; + transfer_component(&mut force_updates, possessor, possessee, |mut x| { + x.update(); + update_counter = x.counter(); + x + }); - // If a player is posessing, add possessee to playerlist as player and remove + let mut presences = ecs.write_storage::(); + // We leave Presence on the old entity for character IDs to be properly removed + // from the ID mapping if deleting the previous entity. + // + // If the entity is not going to be deleted, we remove it so that the entity + // doesn't keep an area loaded. + if delete_entity.is_none() { + presences.remove(possessor); + } + if let Some(p) = new_presence { + presences + .insert(possessee, p) + .expect("Checked entity was alive!"); + } + + // If a player is possessing, add possessee to playerlist as player and remove // old player. // Fetches from possessee entity here since we have transferred over the // `Player` component. @@ -535,7 +575,7 @@ pub fn handle_possess(server: &mut Server, possessor_uid: Uid, possessee_uid: Ui possessee, ); if !comp_sync_package.is_empty() { - client.send_fallible(ServerGeneral::CompSync(comp_sync_package, 0)); // TODO: Check if this should be zero + client.send_fallible(ServerGeneral::CompSync(comp_sync_package, update_counter)); } } @@ -544,7 +584,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) { + if let Err(e) = state.delete_entity_recorded(entity, None) { error!( ?e, ?entity, diff --git a/server/src/lib.rs b/server/src/lib.rs index 8d8ad6219a..fe154812f4 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) { + if let Err(e) = self.state.delete_entity_recorded(entity, None) { error!(?e, "Failed to delete agent outside the terrain"); } } diff --git a/server/src/state_ext.rs b/server/src/state_ext.rs index fd42498489..323d813d9c 100644 --- a/server/src/state_ext.rs +++ b/server/src/state_ext.rs @@ -149,6 +149,7 @@ 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; @@ -696,13 +697,14 @@ impl StateExt for State { map_marker, } = components; - // TODO: CharacterId established here (update id map) - if let Some(player_uid) = self.read_component_copied::(entity) { let result = if let Some(presence) = self.ecs().write_storage::().get_mut(entity) { if let PresenceKind::LoadingCharacter(id) = presence.kind { presence.kind = PresenceKind::Character(id); + self.ecs() + .write_resource::() + .add_character(id, entity); Ok(()) } else { Err("PresenceKind is not LoadingCharacter") @@ -758,6 +760,9 @@ impl StateExt for State { self.write_component_ignore_entity_dead(entity, waypoint); self.write_component_ignore_entity_dead(entity, comp::Pos(waypoint.get_pos())); self.write_component_ignore_entity_dead(entity, comp::Vel(Vec3::zero())); + // TODO: We probably want to increment the existing force update counter since + // it is added in initialized_character (to be robust we can also insert it if + // it doesn't exist) self.write_component_ignore_entity_dead(entity, comp::ForceUpdate::forced()); } @@ -814,6 +819,9 @@ impl StateExt for State { player_info.last_battlemode_change = Some(*change); } } else { + // TODO: this sounds related to handle_exit_ingame? Actually, sounds like + // trying to place character specific info on the `Player` component. TODO + // document component better. // FIXME: // ??? // @@ -1143,9 +1151,19 @@ 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> { - // Remove entity from a group if they are in one - { + // 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::(); @@ -1173,13 +1191,19 @@ impl StateExt for State { } // Cancel extant trades - events::cancel_trades_for(self, entity); + // + // 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) = ( self.ecs().read_storage::().get(entity).copied(), - // TODO: what if one of these 2 components was removed from the entity? - // We could simulate rlinear types at runtime with a `dev_panic!` in the drop for these - // components? + // 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) @@ -1191,32 +1215,35 @@ impl StateExt for State { self.ecs().read_storage::().get(entity).copied(), ); - if let Some(uid) = maybe_uid { - // TODO: exit_ingame for player doesn't hit this path since Uid is removed - self.ecs().write_resource::().remove_entity( - Some(entity), - uid, - maybe_presence.and_then(|p| match p { - PresenceKind::Spectator - | PresenceKind::Possessor - | PresenceKind::LoadingCharacter(_) => None, - PresenceKind::Character(id) => Some(id), - }), - maybe_rtsim_entity, - ); - } else { + if maybe_uid.or(exit_ingame.flatten()).is_some() { + // For now we expect all entities have a Uid component. error!("Deleting entity without Uid component"); } + self.ecs().write_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| match p { + PresenceKind::Spectator + | PresenceKind::Possessor + | PresenceKind::LoadingCharacter(_) => None, + PresenceKind::Character(id) => Some(id), + }), + maybe_rtsim_entity, + ); + let res = self.ecs_mut().delete_entity(entity); if res.is_ok() { - if let (Some(uid), Some(pos)) = (maybe_uid, maybe_pos) { - // TODO: exit_ingame for player doesn't hit this path since Uid is removed, not - // sure if that is correct. However, we don't want recording the UID in deleted - // entities to end up overwriting the rejoined client with that UID... - // - // What is the difference between leaving a region and the deleted entities - // list? + // 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::() diff --git a/server/src/sys/msg/character_screen.rs b/server/src/sys/msg/character_screen.rs index 2e2677a704..c6d68a434d 100644 --- a/server/src/sys/msg/character_screen.rs +++ b/server/src/sys/msg/character_screen.rs @@ -85,6 +85,12 @@ impl Sys { }, ClientGeneral::Character(character_id, requested_view_distances) => { if let Some(player) = players.get(entity) { + // NOTE: Because clients retain their Uid when exiting to the character + // selection screen, we rely on this check to prevent them from immediately + // re-entering in-game in the same tick so that we can synchronize their + // removal without this being mixed up (and e.g. telling other clients to + // delete the re-joined version) or requiring more complex handling to avoid + // this. if presences.contains(entity) { debug!("player already ingame, aborting"); } else if character_updater.has_pending_database_action(character_id)