diff --git a/.gitignore b/.gitignore index 0cd452cb91..3bc13c3292 100644 --- a/.gitignore +++ b/.gitignore @@ -1,4 +1,3 @@ -common/src/uid.rs # Rust target diff --git a/client/src/lib.rs b/client/src/lib.rs index 501281709c..554af175fc 100644 --- a/client/src/lib.rs +++ b/client/src/lib.rs @@ -2221,6 +2221,7 @@ impl Client { if let Some(presence) = self.presence { self.presence = Some(match presence { PresenceKind::Spectator => PresenceKind::Spectator, + PresenceKind::LoadingCharacter(_) => PresenceKind::Possessor, PresenceKind::Character(_) => PresenceKind::Possessor, PresenceKind::Possessor => PresenceKind::Possessor, }); @@ -2748,12 +2749,12 @@ impl Client { // Recreate client entity with Uid let entity_builder = self.state.ecs_mut().create_entity(); - let uid = entity_builder + entity_builder .world .write_resource::() - .allocate(entity_builder.entity, Some(client_uid)); + .add_entity(client_uid, entity_builder.entity); - let entity = entity_builder.with(uid).build(); + let entity = entity_builder.with(client_uid).build(); self.state.ecs().write_resource::().0 = Some(entity); } diff --git a/common/net/src/sync/sync_ext.rs b/common/net/src/sync/sync_ext.rs index 12cd62b12f..7f4d8d5430 100644 --- a/common/net/src/sync/sync_ext.rs +++ b/common/net/src/sync/sync_ext.rs @@ -66,10 +66,12 @@ impl WorldSyncExt for specs::World { /// This method should be used from the client-side when processing network /// messages that delete entities. - // TODO: rename method + // 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_(uid); + let maybe_entity = self + .write_resource::() + .remove_entity(None, uid, None, None); if let Some(entity) = maybe_entity { if let Err(e) = self.delete_entity(entity) { error!(?e, "Failed to delete entity"); @@ -142,19 +144,24 @@ impl WorldSyncExt for specs::World { } // Private utilities +// +// Only used on the client. fn create_entity_with_uid(specs_world: &mut specs::World, entity_uid: u64) -> specs::Entity { let entity_uid = Uid::from(entity_uid); let existing_entity = specs_world.read_resource::().uid_entity(entity_uid); + // TODO: Are there any expected cases where there is an existing entity with + // this UID? If not, we may want to log an error. Otherwise, it may be useful to + // document these cases. match existing_entity { Some(entity) => entity, None => { let entity_builder = specs_world.create_entity(); - let uid = entity_builder + entity_builder .world .write_resource::() - .allocate(entity_builder.entity, Some(entity_uid)); - entity_builder.with(uid).build() + .add_entity(entity_uid, entity_builder.entity); + entity_builder.with(entity_uid).build() }, } } diff --git a/common/src/comp/presence.rs b/common/src/comp/presence.rs index e8e4adfbb0..ac2208736b 100644 --- a/common/src/comp/presence.rs +++ b/common/src/comp/presence.rs @@ -31,10 +31,10 @@ impl Component for Presence { #[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] pub enum PresenceKind { Spectator, - Character(CharacterId), // Note: we don't know if this character ID is valid and associated with the respective player // until it the character has loaded successfully. LoadingCharacter(CharacterId), + Character(CharacterId), Possessor, } diff --git a/common/src/rtsim.rs b/common/src/rtsim.rs index 9617aa7109..ea111de60b 100644 --- a/common/src/rtsim.rs +++ b/common/src/rtsim.rs @@ -24,7 +24,7 @@ slotmap::new_key_type! { pub struct FactionId; } slotmap::new_key_type! { pub struct ReportId; } -#[derive(Copy, Clone, Debug)] +#[derive(Copy, Clone, Debug, Hash, PartialEq, Eq)] pub struct RtSimEntity(pub NpcId); impl Component for RtSimEntity { diff --git a/common/src/uid.rs b/common/src/uid.rs index f2edcf62ff..8f3825bc17 100644 --- a/common/src/uid.rs +++ b/common/src/uid.rs @@ -62,11 +62,10 @@ mod not_wasm { // -- Fields below only used on the server -- uid_allocator: UidAllocator, - // Maps below are only used on the server. /// Character IDs. cid_mapping: HashMap, /// Rtsim Entities. - rid_mapping: HashMap, + rid_mapping: HashMap, } impl IdMaps { @@ -84,12 +83,12 @@ mod not_wasm { /// Given a `CharacterId` retrieve the corresponding `Entity`. pub fn cid_entity(&self, id: CharacterId) -> Option { - self.uid_mapping.get(&id).copied() + self.cid_mapping.get(&id).copied() } /// Given a `RtSimEntity` retrieve the corresponding `Entity`. pub fn rid_entity(&self, id: RtSimEntity) -> Option { - self.uid_mapping.get(&id).copied() + self.rid_mapping.get(&id).copied() } // TODO: I think this is suitable to use on both the client and the server. @@ -105,17 +104,19 @@ mod not_wasm { expected_entity: Option, uid: Uid, cid: Option, - rid: Option, + rid: Option, ) -> Option { #[cold] #[inline(never)] fn unexpected_entity() { - error!("Provided was {kind} mapped to an unexpected entity!"); + let kind = core::any::type_name::(); + error!("Provided {kind} was mapped to an unexpected entity!"); } #[cold] #[inline(never)] fn not_present() { - error!("Provided was {kind} not mapped to any entity!"); + let kind = core::any::type_name::(); + error!("Provided {kind} was not mapped to any entity!"); } fn remove( @@ -124,8 +125,8 @@ mod not_wasm { expected: Option, ) -> Option { if let Some(id) = id { - if let Some(e) = mapping.remove(id) { - if Some(expected) = expected && e != expected { + if let Some(e) = mapping.remove(&id) { + if expected.map_or(true, |expected| e != expected) { unexpected_entity::(); } Some(e) diff --git a/server/src/events/entity_creation.rs b/server/src/events/entity_creation.rs index 1d66598dfd..c7c0902248 100644 --- a/server/src/events/entity_creation.rs +++ b/server/src/events/entity_creation.rs @@ -83,15 +83,19 @@ pub fn handle_loaded_character_data( ))), ); } - let result_msg = if let Err(err) = server.state.update_character_data(entity, loaded_components) { - handle_exit_igname(entity, false); // remove client from in-game state + + let result_msg = if let Err(err) = server + .state + .update_character_data(entity, loaded_components) + { + handle_exit_ingame(server, entity, false); // remove client from in-game state ServerGeneral::CharacterDataLoadResult(Err(err)) } else { sys::subscription::initialize_region_subscription(server.state.ecs(), entity); // We notify the client with the metadata result from the operation. ServerGeneral::CharacterDataLoadResult(Ok(metadata)) }; - server.notify_client(entity, result_msg)); + server.notify_client(entity, result_msg); } pub fn handle_create_npc(server: &mut Server, pos: Pos, mut npc: NpcBuilder) -> EcsEntity { diff --git a/server/src/events/player.rs b/server/src/events/player.rs index 2d9c05dab5..9d7745369e 100644 --- a/server/src/events/player.rs +++ b/server/src/events/player.rs @@ -80,6 +80,8 @@ pub fn handle_exit_ingame(server: &mut Server, entity: EcsEntity, skip_persisten 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)?, )) @@ -107,6 +109,10 @@ pub fn handle_exit_ingame(server: &mut Server, entity: EcsEntity, skip_persisten .write_resource::() .allocate(entity_builder.entity, Some(uid.into())); let new_entity = entity_builder.with(uid).build(); + + // Note, since the Uid has been removed from the old entity, that prevents + // `delete_entity_recorded` from making any changes to the group (TODO double check this + // logic) if let Some(group) = maybe_group { let mut group_manager = state.ecs().write_resource::(); if group_manager diff --git a/server/src/state_ext.rs b/server/src/state_ext.rs index 9a5ad3339e..0a7b5dcfd2 100644 --- a/server/src/state_ext.rs +++ b/server/src/state_ext.rs @@ -27,7 +27,7 @@ use common::{ resources::{Secs, Time, TimeOfDay}, rtsim::{Actor, RtSimEntity}, slowjob::SlowJobPool, - uid::{Uid, IdMaps}, + uid::{IdMaps, Uid}, LoadoutBuilder, ViewDistances, }; use common_net::{ @@ -38,7 +38,7 @@ use common_state::State; use rand::prelude::*; use specs::{Builder, Entity as EcsEntity, EntityBuilder as EcsEntityBuilder, Join, WorldExt}; use std::time::{Duration, Instant}; -use tracing::{trace, warn}; +use tracing::{error, trace, warn}; use vek::*; pub trait StateExt { @@ -125,7 +125,11 @@ pub trait StateExt { fn initialize_spectator_data(&mut self, entity: EcsEntity, view_distances: ViewDistances); /// Update the components associated with the entity's current character. /// Performed after loading component data from the database - fn update_character_data(&mut self, entity: EcsEntity, components: PersistedComponents); + fn update_character_data( + &mut self, + entity: EcsEntity, + components: PersistedComponents, + ) -> Result<(), String>; /// Iterates over registered clients and send each `ServerMsg` fn validate_chat_msg( &self, @@ -1173,6 +1177,9 @@ impl StateExt for State { 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? self.ecs() .read_storage::() .get(entity) @@ -1184,16 +1191,28 @@ impl StateExt for State { self.ecs().read_storage::().get(entity).copied(), ); - if let Some(uid) = a { - if self.ecs().write_resource::() - .remove_entity().is_none() + 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.kind { + PresenceKind::Spectator + | PresenceKind::Possessed + | PresenceKind::LoadingCharacter(_) => None, + PresenceKind::Character(id) => Some(id), + }), + maybe_rtsim_entity, + ) } else { - warn!("Deleting entity without Uid component"); + error!("Deleting entity without Uid component"); } 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. if let Some(region_key) = self .ecs() .read_resource::()