Steps towards properly synchronizing when a client exits in-game to

return to the character screeen. And other related improvements.

* Uid now optional when removing an entity from IdMaps since that Uid
  may have been transferred to the new entity created when the client
  exits "in-game".
* Added notes about making sure to update the CharacterId mapping when
  changing the `kind` field of `Presence`.
* Add a parameter to `delete_entity_recorded` that indicates whether it
  is being called from `handle_exit_ingame` and that provides the `Uid`
  that was removed from the entity so that we can more explicitly and
  correctly handle that case.
* We now add the Uid of the client to the deleted entities list when it
  exits "ingame". (still need to rework code so that the client doesn't
  delete its own entity).
* Improved integration of possession code with ForceUpdate logic.
* Don't remove `Presence` component from old entity in possession so
  that deleting that entity will properly handle updating the
  CharacterId unmapping (if the old entity isn't going to be deleted we
  still remove the `Presence` component so that it doesn't keep terrain
  loaded).
* Added a couple TODOs on existing tangential things I noticed.

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::<IdMaps>()
-            .remove_entity(None, uid, None, None);
+        let maybe_entity =
+            self.write_resource::<IdMaps>()
+                .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");

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,

common/src/uid.rs

@@ -102,7 +102,7 @@ mod not_wasm {
         pub fn remove_entity(
             &mut self,
             expected_entity: Option<Entity>,
-            uid: Uid,
+            uid: Option<Uid>,
             cid: Option<CharacterId>,
             rid: Option<RtSimEntity>,
         ) -> Option<Entity> {
@@ -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);

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);
         }
     }

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);
     }
 }

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"));
 }
 

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.",
                     );

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::<Client>().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::<group::Group>().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::<BattleModeBuffer>(),
     ) {
         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::<Client>();
             let players = ecs.read_storage::<comp::Player>();
+            let presences = ecs.read_storage::<comp::Presence>();
 
             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::<comp::Player>();
-        let mut presence = ecs.write_storage::<Presence>();
         let mut subscriptions = ecs.write_storage::<RegionSubscription>();
         let mut admins = ecs.write_storage::<comp::Admin>();
         let mut waypoints = ecs.write_storage::<comp::Waypoint>();
+        let mut force_updates = ecs.write_storage::<comp::ForceUpdate>();
 
         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::<Presence>();
+        // 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,

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");
             }
         }

server/src/state_ext.rs

@@ -149,6 +149,7 @@ pub trait StateExt {
     fn delete_entity_recorded(
         &mut self,
         entity: EcsEntity,
+        exit_ingame: Option<Option<Uid>>,
     ) -> Result<(), specs::error::WrongGeneration>;
     /// Get the given entity as an [`Actor`], if it is one.
     fn entity_as_actor(&self, entity: EcsEntity) -> Option<Actor>;
@@ -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::<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);
+                        self.ecs()
+                            .write_resource::<IdMaps>()
+                            .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<Option<Uid>>,
     ) -> 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::<Client>();
             let uids = self.ecs().read_storage::<Uid>();
             let mut group_manager = self.ecs().write_resource::<comp::group::GroupManager>();
@@ -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::<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?
+            // 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::<Presence>()
                 .get(entity)
@@ -1191,32 +1215,35 @@ impl StateExt for State {
             self.ecs().read_storage::<comp::Pos>().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::<IdMaps>().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::<IdMaps>().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::<common::region::RegionMap>()

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)