Adapting various things to the new interface, fixing compilation errors,

and progressing on half done things. Also, added a few TODO comments.

.gitignore

@@ -1,4 +1,3 @@
-common/src/uid.rs
 # Rust
 target
 

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::<IdMaps>()
-            .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::<PlayerEntity>().0 = Some(entity);
     }
 

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::<IdMaps>().remove_entity_(uid);
+        let maybe_entity = self
+            .write_resource::<IdMaps>()
+            .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::<IdMaps>().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::<IdMaps>()
-                .allocate(entity_builder.entity, Some(entity_uid));
+                .add_entity(entity_uid, entity_builder.entity);
-            entity_builder.with(uid).build()
+            entity_builder.with(entity_uid).build()
         },
     }
 }

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,
 }
 

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 {

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<CharacterId, Entity>,
         /// Rtsim Entities.
-        rid_mapping: HashMap<RtsimEntity, Entity>,
+        rid_mapping: HashMap<RtSimEntity, Entity>,
     }
 
     impl IdMaps {
@@ -84,12 +83,12 @@ mod not_wasm {
 
         /// Given a `CharacterId` retrieve the corresponding `Entity`.
         pub fn cid_entity(&self, id: CharacterId) -> Option<Entity> {
-            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<Entity> {
-            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<Entity>,
             uid: Uid,
             cid: Option<CharacterId>,
-            rid: Option<RtsimEntity>,
+            rid: Option<RtSimEntity>,
         ) -> Option<Entity> {
             #[cold]
             #[inline(never)]
             fn unexpected_entity<ID>() {
-                error!("Provided was {kind} mapped to an unexpected entity!");
+                let kind = core::any::type_name::<ID>();
+                error!("Provided {kind} was mapped to an unexpected entity!");
             }
             #[cold]
             #[inline(never)]
             fn not_present<ID>() {
-                error!("Provided was {kind} not mapped to any entity!");
+                let kind = core::any::type_name::<ID>();
+                error!("Provided {kind} was not mapped to any entity!");
             }
 
             fn remove<ID: Hash + Eq>(
@@ -124,8 +125,8 @@ mod not_wasm {
                 expected: Option<Entity>,
             ) -> Option<Entity> {
                 if let Some(id) = id {
-                    if let Some(e) = mapping.remove(id) {
+                    if let Some(e) = mapping.remove(&id) {
-                        if Some(expected) = expected && e != expected {
+                        if expected.map_or(true, |expected| e != expected) {
                             unexpected_entity::<ID>();
                         }
                         Some(e)

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 {

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::<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)?,
         ))
@@ -107,6 +109,10 @@ pub fn handle_exit_ingame(server: &mut Server, entity: EcsEntity, skip_persisten
             .write_resource::<IdMaps>()
             .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::<group::GroupManager>();
             if group_manager

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::<Uid>().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::<Presence>()
                 .get(entity)
@@ -1184,16 +1191,28 @@ impl StateExt for State {
             self.ecs().read_storage::<comp::Pos>().get(entity).copied(),
         );
 
-        if let Some(uid) = a {
+        if let Some(uid) = maybe_uid {
-            if self.ecs().write_resource::<IdMaps>() 
+            // TODO: exit_ingame for player doesn't hit this path since Uid is removed
-                .remove_entity().is_none()
+            self.ecs().write_resource::<IdMaps>().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::<common::region::RegionMap>()