From 599e25c9c6db8033fd5c35970dc8699ba50182dd Mon Sep 17 00:00:00 2001
From: Imbris <imbrisf@gmail.com>
Date: Sun, 4 Jun 2023 18:10:20 -0400
Subject: [PATCH] Address review on !3971

---
 common/src/uid.rs        | 36 ++++++++++++------------------------
 server/agent/src/data.rs |  4 ++--
 server/src/state_ext.rs  |  8 ++------
 3 files changed, 16 insertions(+), 32 deletions(-)

diff --git a/common/src/uid.rs b/common/src/uid.rs
index 8ca167b116..5d89475bde 100644
--- a/common/src/uid.rs
+++ b/common/src/uid.rs
@@ -1,4 +1,3 @@
-// TODO: rename this module or create new one for ID maps
 use serde::{Deserialize, Serialize};
 use std::{fmt, u64};
 
@@ -54,7 +53,7 @@ mod not_wasm {
     }
 
     /// Mappings from various Id types to `Entity`s.
-    #[derive(Debug)]
+    #[derive(Default, Debug)]
     pub struct IdMaps {
         /// "Universal" IDs (used to communicate entity identity over the
         /// network).
@@ -64,32 +63,25 @@ mod not_wasm {
         uid_allocator: UidAllocator,
 
         /// Character IDs.
-        cid_mapping: HashMap<CharacterId, Entity>,
+        character_to_ecs: HashMap<CharacterId, Entity>,
         /// Rtsim Entities.
-        rid_mapping: HashMap<RtSimEntity, Entity>,
+        rtsim_to_ecs: HashMap<RtSimEntity, Entity>,
     }
 
     impl IdMaps {
-        pub fn new() -> Self {
-            Self {
-                uid_mapping: HashMap::new(),
-                uid_allocator: UidAllocator::new(),
-                cid_mapping: HashMap::new(),
-                rid_mapping: HashMap::new(),
-            }
-        }
+        pub fn new() -> Self { Default::default() }
 
         /// 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.cid_mapping.get(&id).copied()
+        pub fn character_entity(&self, id: CharacterId) -> Option<Entity> {
+            self.character_to_ecs.get(&id).copied()
         }
 
         /// Given a `RtSimEntity` retrieve the corresponding `Entity`.
-        pub fn rid_entity(&self, id: RtSimEntity) -> Option<Entity> {
-            self.rid_mapping.get(&id).copied()
+        pub fn rtsim_entity(&self, id: RtSimEntity) -> Option<Entity> {
+            self.rtsim_to_ecs.get(&id).copied()
         }
 
         /// Removes mappings for the provided Id(s).
@@ -142,8 +134,8 @@ mod not_wasm {
 
             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);
+            remove(&mut self.character_to_ecs, cid, expected_entity);
+            remove(&mut self.rtsim_to_ecs, rid, expected_entity);
             maybe_entity
         }
 
@@ -156,12 +148,12 @@ mod not_wasm {
 
         /// Only used on the server.
         pub fn add_character(&mut self, cid: CharacterId, entity: Entity) {
-            Self::insert(&mut self.cid_mapping, cid, entity);
+            Self::insert(&mut self.character_to_ecs, cid, entity);
         }
 
         /// Only used on the server.
         pub fn add_rtsim(&mut self, rid: RtSimEntity, entity: Entity) {
-            Self::insert(&mut self.rid_mapping, rid, entity);
+            Self::insert(&mut self.rtsim_to_ecs, rid, entity);
         }
 
         /// Allocates a new `Uid` and links it to the provided entity.
@@ -202,8 +194,4 @@ mod not_wasm {
     impl Default for UidAllocator {
         fn default() -> Self { Self::new() }
     }
-
-    impl Default for IdMaps {
-        fn default() -> Self { Self::new() }
-    }
 }
diff --git a/server/agent/src/data.rs b/server/agent/src/data.rs
index 0bf4a5cb67..1c548ac08d 100644
--- a/server/agent/src/data.rs
+++ b/server/agent/src/data.rs
@@ -326,8 +326,8 @@ pub struct ReadData<'a> {
 impl<'a> ReadData<'a> {
     pub fn lookup_actor(&self, actor: Actor) -> Option<EcsEntity> {
         match actor {
-            Actor::Character(character_id) => self.id_maps.cid_entity(character_id),
-            Actor::Npc(npc_id) => self.id_maps.rid_entity(RtSimEntity(npc_id)),
+            Actor::Character(character_id) => self.id_maps.character_entity(character_id),
+            Actor::Npc(npc_id) => self.id_maps.rtsim_entity(RtSimEntity(npc_id)),
         }
     }
 }
diff --git a/server/src/state_ext.rs b/server/src/state_ext.rs
index 5bbe68ec24..ce855b4523 100644
--- a/server/src/state_ext.rs
+++ b/server/src/state_ext.rs
@@ -715,6 +715,7 @@ impl StateExt for State {
             if let Err(err) = result {
                 let err = format!("Unexpected state when applying loaded character info: {err}");
                 error!("{err}");
+                // TODO: we could produce a `comp::Content` for this to allow localization.
                 return Err(err);
             }
 
@@ -1225,12 +1226,7 @@ impl StateExt for State {
             // 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_presence.and_then(|p| p.character_id()),
             maybe_rtsim_entity,
         );