Start refactoring UidAllocator to contain mappings for different Id

types.

* Add new `IdMaps` type that contains mappings to Entity from Uid, CharacterId, and
  RtsimEntity.
* Add PresenceKind::LoadingCharacter variant for when the CharacterId
  has not actually been confirmed to exist and belong to the logged in
  client. Switches to the regular PresenceKind::Character once the
  character is loaded from the database and that is used to update the
  entity.
* Start refactoring `delete_entity_recorded` to check for CharacterId
  and RtsimEntity values that may need to be removed from the map (just
  very start, not near complete).
* Other misc tweaks.

common/net/src/sync/sync_ext.rs

@@ -53,16 +53,21 @@ impl WorldSyncExt for specs::World {
 
     fn create_entity_synced(&mut self) -> specs::EntityBuilder {
         let builder = self.create_entity();
+        // TODO: if we split the UidAllocator and the IdMaps into two things then we
+        // need to fetch 2 resources when creating entities.
         let uid = builder
             .world
             .write_resource::<UidAllocator>()
-            .allocate(builder.entity, None);
+            .allocate(builder.entity);
         builder.with(uid)
     }
 
+    /// This method should be used from the client-side when processing network
+    /// messages that delete entities.
+    // TODO: rename method
     fn delete_entity_and_clear_from_uid_allocator(&mut self, uid: Uid) {
         // Clear from uid allocator
-        let maybe_entity = self.write_resource::<UidAllocator>().remove_entity(uid);
+        let maybe_entity = self.write_resource::<UidAllocator>().remove_entity_(uid);
         if let Some(entity) = maybe_entity {
             if let Err(e) = self.delete_entity(entity) {
                 error!(?e, "Failed to delete entity");

common/src/comp/presence.rs

@@ -32,6 +32,9 @@ impl Component for Presence {
 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),
     Possessor,
 }
 

common/src/uid.rs

@@ -1,10 +1,16 @@
-#[cfg(not(target_arch = "wasm32"))]
-use hashbrown::HashMap;
+// TODO: rename this module or create new one for ID maps
 use serde::{Deserialize, Serialize};
-#[cfg(not(target_arch = "wasm32"))]
-use specs::{Component, Entity, FlaggedStorage, VecStorage};
 use std::{fmt, u64};
 
+#[cfg(not(target_arch = "wasm32"))]
+use {
+    crate::character::CharacterId,
+    crate::rtsim::RtSimEntity,
+    hashbrown::HashMap,
+    specs::{Component, Entity, FlaggedStorage, VecStorage},
+};
+
+// TODO: could we switch this to `NonZeroU64`?
 #[derive(Copy, Clone, Debug, Eq, Hash, PartialEq, Serialize, Deserialize)]
 pub struct Uid(pub u64);
 
@@ -20,45 +26,175 @@ impl fmt::Display for Uid {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { write!(f, "{}", self.0) }
 }
 
+pub use not_wasm::*;
 #[cfg(not(target_arch = "wasm32"))]
+mod not_wasm {
+    use super::*;
+
     impl Component for Uid {
         type Storage = FlaggedStorage<Self, VecStorage<Self>>;
     }
 
-#[cfg(not(target_arch = "wasm32"))]
+    // NOTE: This is technically only needed by the server code server. Keeping here
+    // for now since this is related to the other code here.
     #[derive(Debug)]
     pub struct UidAllocator {
-    next_id: u64,
-    mapping: HashMap<Uid, Entity>,
+        /// Next Uid.
+        next_uid: u64,
     }
 
-#[cfg(not(target_arch = "wasm32"))]
     impl UidAllocator {
-    pub fn new() -> Self {
-        Self {
-            next_id: 0,
-            mapping: HashMap::new(),
-        }
-    }
-
-    // Useful for when a single entity is deleted because it doesn't reconstruct the
-    // entire hashmap
-    pub fn remove_entity(&mut self, id: Uid) -> Option<Entity> { self.mapping.remove(&id) }
+        pub fn new() -> Self { Self { next_uid: 0 } }
 
         pub fn allocate(&mut self, entity: Entity, id: Option<Uid>) -> Uid {
             let id = id.unwrap_or_else(|| {
-            let id = self.next_id;
-            self.next_id += 1;
+                let id = self.next_uid;
+                self.next_uid += 1;
                 Uid(id)
             });
-        self.mapping.insert(id, entity);
+            self.uid_mapping.insert(id, entity);
+            id
+        }
+    }
+
+    #[derive(Debug)]
+    pub struct IdMaps {
+        /// "Universal" IDs (used to communicate entity identity over the
+        /// network).
+        uid_mapping: HashMap<Uid, Entity>,
+
+        // Maps below are only used on the server.
+        /// Character IDs.
+        cid_mapping: HashMap<CharacterId, Entity>,
+        /// Rtsim Entities.
+        rid_mapping: HashMap<RtsimEntity, Entity>,
+    }
+
+    impl IdManager {
+        pub fn new() -> Self {
+            Self {
+                uid_mapping: HashMap::new(),
+                cid_mapping: HashMap::new(),
+                rid_mapping: HashMap::new(),
+            }
+        }
+
+        /// Given a `Uid` retrieve the corresponding `Entity`
+        pub fn uid_entity(&self, id: Uid) -> Option<Entity> { self.uid_mapping.get(&id).copied() }
+
+        /// Given a `CharacterId` retrieve the corresponding `Entity`
+        pub fn cid_entity(&self, id: CharacterId) -> Option<Entity> {
+            self.uid_mapping.get(&id).copied()
+        }
+
+        /// Given a `Uid` retrieve the corresponding `Entity`
+        pub fn rid_entity(&self, id: RtSimEntity) -> Option<Entity> {
+            self.uid_mapping.get(&id).copied()
+        }
+
+        // NOTE: This is only used on the client? Do we not remove on the server?
+        // NOTE: We need UID mapping also on the client but we don't need the other
+        // mappings on the client!
+        //
+        // Useful for when a single entity is deleted because it doesn't reconstruct the
+        // entire hashmap
+        pub fn remove_entity(
+            &mut self,
+            expected_entity: Option<Entity>,
+            uid: Uid,
+            cid: Option<CharacterId>,
+            rid: Option<RtsimEntity>,
+        ) -> Option<Entity> {
+            #[cold]
+            #[inline(never)]
+            fn unexpected_entity<ID>() {
+                error!("{kind} mapped to an unexpected entity!");
+            }
+            #[cold]
+            #[inline(never)]
+            fn not_present<ID>() {
+                error!("{kind} not mapped to any entity!");
+            }
+
+            fn remove<ID: Hash + Eq>(
+                mapping: &mut HashMap<ID, Entity>,
+                id: Option<ID>,
+                expected: Option<Entity>,
+            ) -> Option<Entity> {
+                if let Some(id) = id {
+                    if let Some(e) = mapping.remove(id) {
+                        if Some(expected) = expected && e != expected {
+                            unexpected_entity::<ID>();
+                        }
+                        Some(e)
+                    } else {
+                        not_present::<ID>();
+                        None
+                    }
+                } else {
+                    None
+                }
+            }
+
+            let maybe_entity = remove(&mut self.uid_mapping, Some(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);
+            maybe_entity
+        }
+
+        // TODO: we probably need separate methods here
+        // TODO: document what methods are called only on the client or the server
+        pub fn add_entity(
+            &mut self,
+            entity: Entity,
+            uid: Uid,
+            cid: Option<CharacterId>,
+            rid: Option<RtSimEntity>,
+        ) {
+            #[cold]
+            #[inline(never)]
+            fn already_present<ID>() {
+                let kind = core::any::type_name::<ID>();
+                error!("{kind} already mapped to an entity!!!");
+            }
+
+            fn insert<ID: Hash + Eq>(
+                mapping: &mut HashMap<ID, Entity>,
+                new_id: ID,
+                entity: Entity,
+            ) {
+                if let Some(_previous_entity) = mapping.insert(new_id, entity) {
+                    already_present::<ID>();
+                }
+            }
+
+            insert(&mut self.uid_mapping, uid, entity);
+            if let Some(cid) = cid {
+                insert(&mut self.cid_mapping, cid, entity);
+            }
+            if let Some(rid) = rid {
+                insert(&mut self.rid_mapping, rid, entity);
+            }
+        }
+
+        pub fn allocate(&mut self, entity: Entity) -> Uid {
+            let id = id.unwrap_or_else(|| {
+                let id = self.next_uid;
+                self.next_uid += 1;
+                Uid(id)
+            });
+            // TODO: not sure we want to insert here?
+            self.uid_mapping.insert(id, entity);
             id
         }
-
-    pub fn lookup_entity(&self, id: Uid) -> Option<Entity> { self.mapping.get(&id).copied() }
     }
 
-#[cfg(not(target_arch = "wasm32"))]
     impl Default for UidAllocator {
         fn default() -> Self { Self::new() }
     }
+
+    impl Default for IdMaps {
+        fn default() -> Self { Self::new() }
+    }
+}

server/src/events/entity_creation.rs

@@ -83,12 +83,15 @@ pub fn handle_loaded_character_data(
             ))),
         );
     }
-    server
-        .state
-        .update_character_data(entity, loaded_components);
+    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
+        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.
-    server.notify_client(entity, ServerGeneral::CharacterDataLoadResult(Ok(metadata)));
+        ServerGeneral::CharacterDataLoadResult(Ok(metadata))
+    };
+    server.notify_client(entity, result_msg));
 }
 
 pub fn handle_create_npc(server: &mut Server, pos: Pos, mut npc: NpcBuilder) -> EcsEntity {
@@ -132,6 +135,7 @@ pub fn handle_create_npc(server: &mut Server, pos: Pos, mut npc: NpcBuilder) ->
         entity
     };
 
+    // TODO: rtsim entity added here
     let entity = if let Some(rtsim_entity) = npc.rtsim_entity {
         entity.with(rtsim_entity).with(RepositionOnChunkLoad {
             needs_ground: false,

server/src/events/player.rs

@@ -439,7 +439,7 @@ pub fn handle_possess(server: &mut Server, possessor_uid: Uid, possessee_uid: Ui
                 PresenceKind::Spectator => PresenceKind::Spectator,
                 // This prevents persistence from overwriting original character info with stuff
                 // from the new character.
-                PresenceKind::Character(_) => {
+                PresenceKind::Character(_) | PresenceKind::LoadingCharacter(_) => {
                     delete_entity = Some(possessor);
                     PresenceKind::Possessor
                 },

server/src/lib.rs

@@ -954,6 +954,8 @@ impl Server {
                                         active_abilities,
                                         map_marker,
                                     );
+                                    // TODO: Does this need to be a server event? E.g. we could
+                                    // just handle it here.
                                     ServerEvent::UpdateCharacterData {
                                         entity: response.target_entity,
                                         components: character_data,
@@ -962,9 +964,8 @@ impl Server {
                                 },
                                 Err(error) => {
                                     // We failed to load data for the character from the DB. Notify
-                                    // the client to push the
-                                    // state back to character selection, with the error
-                                    // to display
+                                    // the client to push the state back to character selection,
+                                    // with the error to display
                                     self.notify_client(
                                         response.target_entity,
                                         ServerGeneral::CharacterDataLoadResult(Err(

server/src/state_ext.rs

@@ -640,7 +640,7 @@ impl StateExt for State {
 
             self.write_component_ignore_entity_dead(
                 entity,
-                Presence::new(view_distances, PresenceKind::Character(character_id)),
+                Presence::new(view_distances, PresenceKind::LoadingCharacter(character_id)),
             );
 
             // Tell the client its request was successful.
@@ -675,7 +675,12 @@ impl StateExt for State {
         }
     }
 
-    fn update_character_data(&mut self, entity: EcsEntity, components: PersistedComponents) {
+    /// Returned error intended to be sent to the client.
+    fn update_character_data(
+        &mut self,
+        entity: EcsEntity,
+        components: PersistedComponents,
+    ) -> Result<(), String> {
         let PersistedComponents {
             body,
             stats,
@@ -687,7 +692,26 @@ impl StateExt for State {
             map_marker,
         } = components;
 
+        // TODO: CharacterId established here (update id map)
+
         if let Some(player_uid) = self.read_component_copied::<Uid>(entity) {
+            let result =
+                if let Some(presence) = self.ecs().write_storage::<Presence>().get_mut(entity) {
+                    if let PresenceKind::LoadingCharacter(id) = presence.kind {
+                        *presence.kind = PresenceKind::Character(id);
+                        Ok(())
+                    } else {
+                        Err("PresenceKind is not LoadingCharacter")
+                    }
+                } else {
+                    Err("Presence component missing")
+                };
+            if let Err(err) = result {
+                let err = format!("Unexpected state when applying loaded character info: {err}");
+                error!("{err}");
+                return Err(err);
+            }
+
             // Notify clients of a player list update
             self.notify_players(ServerGeneral::PlayerListUpdate(
                 PlayerListUpdate::SelectedCharacter(player_uid, CharacterInfo {
@@ -768,7 +792,7 @@ impl StateExt for State {
                     restore_pet(self.ecs(), pet_entity, entity, pet);
                 }
             } else {
-                warn!("Player has no pos, cannot load {} pets", pets.len());
+                error!("Player has no pos, cannot load {} pets", pets.len());
             }
 
             let presences = self.ecs().read_storage::<Presence>();
@@ -801,6 +825,8 @@ impl StateExt for State {
                 }
             }
         }
+
+        Ok(())
     }
 
     fn validate_chat_msg(
@@ -1145,11 +1171,26 @@ impl StateExt for State {
         // Cancel extant trades
         events::cancel_trades_for(self, entity);
 
-        let (maybe_uid, maybe_pos) = (
+        let (maybe_uid, maybe_presence, maybe_rtsim_entity, maybe_pos) = (
             self.ecs().read_storage::<Uid>().get(entity).copied(),
+            self.ecs()
+                .read_storage::<Presence>()
+                .get(entity)
+                .map(|p| p.kind),
+            self.ecs()
+                .read_storage::<RtSimEntity>()
+                .get(entity)
+                .copied(),
             self.ecs().read_storage::<comp::Pos>().get(entity).copied(),
         );
 
+        if let Some(uid) = a {
+            if self.ecs().write_resource::<IdMaps>() 
+                .remove_entity().is_none()
+        } else {
+            warn!("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) {