Some random changes and a fmt

client/src/lib.rs

@@ -47,7 +47,7 @@ use common::{
         TerrainGrid,
     },
     trade::{PendingTrade, SitePrices, TradeAction, TradeId, TradeResult},
-    uid::{Uid, IdMaps},
+    uid::{IdMaps, Uid},
     vol::RectVolSize,
     weather::{Weather, WeatherGrid},
 };

common/net/src/sync/mod.rs

@@ -7,7 +7,7 @@ mod sync_ext;
 mod track;
 
 // Reexports
-pub use common::uid::{Uid, IdMaps};
+pub use common::uid::{IdMaps, Uid};
 pub use net_sync::{NetSync, SyncFrom};
 pub use packet::{
     handle_insert, handle_interp_insert, handle_interp_modify, handle_interp_remove, handle_modify,

common/src/combat.rs

@@ -19,7 +19,7 @@ use crate::{
     outcome::Outcome,
     resources::Secs,
     states::utils::StageSection,
-    uid::{Uid, IdMaps},
+    uid::{IdMaps, Uid},
     util::Dir,
 };
 

common/src/uid.rs

@@ -147,7 +147,8 @@ mod not_wasm {
         }
 
         /// Only used on the client (server solely uses `Self::allocate` to
-        /// allocate and add Uid mappings).
+        /// allocate and add Uid mappings and `Self::remap` to move the `Uid` to
+        /// a different entity).
         pub fn add_entity(&mut self, uid: Uid, entity: Entity) {
             Self::insert(&mut self.uid_mapping, uid, entity);
         }
@@ -171,6 +172,18 @@ mod not_wasm {
             uid
         }
 
+        /// Links an existing `Uid` to a new entity.
+        ///
+        /// Only used on the server.
+        ///
+        /// Used for `handle_exit_ingame` which moves the same `Uid` to a new
+        /// entity.
+        pub fn remap_entity(&mut self, uid: Uid, new_entity: Entity) {
+            if self.uid_mapping.insert(uid, new_entity).is_none() {
+                error!("Uid {uid:?} remaped but there was no existing entry for it!");
+            }
+        }
+
         #[cold]
         #[inline(never)]
         fn already_present<ID>() {

common/state/src/plugin/memory_manager.rs

@@ -8,7 +8,7 @@ use wasmer::{Function, Memory, Value};
 
 use common::{
     comp::{Health, Player},
-    uid::{Uid, IdMaps},
+    uid::{IdMaps, Uid},
 };
 
 use super::errors::{MemoryAllocationError, PluginModuleError};

common/state/src/plugin/module.rs

@@ -238,13 +238,12 @@ fn retrieve_action(
                     EcsAccessError::EcsPointerNotAvailable,
                 ))?
             };
-            let player =
-                world
-                    .id_maps
-                    .uid_entity(e)
-                    .ok_or(RetrieveError::EcsAccessError(
-                        EcsAccessError::EcsEntityNotFound(e),
-                    ))?;
+            let player = world
+                .id_maps
+                .uid_entity(e)
+                .ok_or(RetrieveError::EcsAccessError(
+                    EcsAccessError::EcsEntityNotFound(e),
+                ))?;
 
             Ok(RetrieveResult::GetPlayerName(
                 world
@@ -267,13 +266,12 @@ fn retrieve_action(
                     EcsAccessError::EcsPointerNotAvailable,
                 ))?
             };
-            let player =
-                world
-                    .id_maps
-                    .uid_entity(e)
-                    .ok_or(RetrieveError::EcsAccessError(
-                        EcsAccessError::EcsEntityNotFound(e),
-                    ))?;
+            let player = world
+                .id_maps
+                .uid_entity(e)
+                .ok_or(RetrieveError::EcsAccessError(
+                    EcsAccessError::EcsEntityNotFound(e),
+                ))?;
             Ok(RetrieveResult::GetEntityHealth(
                 world
                     .health

common/systems/src/aura.rs

@@ -8,7 +8,7 @@ use common::{
     },
     event::{Emitter, EventBus, ServerEvent},
     resources::Time,
-    uid::{Uid, IdMaps},
+    uid::{IdMaps, Uid},
 };
 use common_ecs::{Job, Origin, Phase, System};
 use specs::{

common/systems/src/beam.rs

@@ -9,7 +9,7 @@ use common::{
     outcome::Outcome,
     resources::{DeltaTime, Time},
     terrain::TerrainGrid,
-    uid::{Uid, IdMaps},
+    uid::{IdMaps, Uid},
     vol::ReadVol,
     GroupTarget,
 };

common/systems/src/buff.rs

@@ -15,7 +15,7 @@ use common::{
     event::{Emitter, EventBus, ServerEvent},
     resources::{DeltaTime, Secs, Time},
     terrain::SpriteKind,
-    uid::{Uid, IdMaps},
+    uid::{IdMaps, Uid},
     Damage, DamageSource,
 };
 use common_base::prof_span;
@@ -275,27 +275,26 @@ impl<'a> System<'a> for Sys {
                     }
                 })
                 .for_each(|(buff_id, buff, uid, aura_key)| {
-                    let replace =
-                        if let Some(aura_entity) = read_data.id_maps.uid_entity(*uid) {
-                            if let Some(aura) = read_data
-                                .auras
-                                .get(aura_entity)
-                                .and_then(|auras| auras.auras.get(*aura_key))
-                            {
-                                if let (Some(pos), Some(aura_pos)) = (
-                                    read_data.positions.get(entity),
-                                    read_data.positions.get(aura_entity),
-                                ) {
-                                    pos.0.distance_squared(aura_pos.0) > aura.radius.powi(2)
-                                } else {
-                                    true
-                                }
+                    let replace = if let Some(aura_entity) = read_data.id_maps.uid_entity(*uid) {
+                        if let Some(aura) = read_data
+                            .auras
+                            .get(aura_entity)
+                            .and_then(|auras| auras.auras.get(*aura_key))
+                        {
+                            if let (Some(pos), Some(aura_pos)) = (
+                                read_data.positions.get(entity),
+                                read_data.positions.get(aura_entity),
+                            ) {
+                                pos.0.distance_squared(aura_pos.0) > aura.radius.powi(2)
                             } else {
                                 true
                             }
                         } else {
                             true
-                        };
+                        }
+                    } else {
+                        true
+                    };
                     if replace {
                         expired_buffs.push(*buff_id);
                         server_emitter.emit(ServerEvent::Buff {

common/systems/src/melee.rs

@@ -10,7 +10,7 @@ use common::{
     outcome::Outcome,
     resources::Time,
     terrain::TerrainGrid,
-    uid::{Uid, IdMaps},
+    uid::{IdMaps, Uid},
     util::{find_dist::Cylinder, Dir},
     vol::ReadVol,
     GroupTarget,

common/systems/src/projectile.rs

@@ -8,7 +8,7 @@ use common::{
     event::{Emitter, EventBus, ServerEvent},
     outcome::Outcome,
     resources::{DeltaTime, Time},
-    uid::{Uid, IdMaps},
+    uid::{IdMaps, Uid},
     util::Dir,
     GroupTarget,
 };

common/systems/src/shockwave.rs

@@ -8,7 +8,7 @@ use common::{
     event::{EventBus, ServerEvent},
     outcome::Outcome,
     resources::{DeltaTime, Time},
-    uid::{Uid, IdMaps},
+    uid::{IdMaps, Uid},
     util::Dir,
     GroupTarget,
 };

server/agent/src/data.rs

@@ -21,7 +21,7 @@ use common::{
     rtsim::{Actor, RtSimEntity},
     states::utils::{ForcedMovement, StageSection},
     terrain::TerrainGrid,
-    uid::{Uid, IdMaps},
+    uid::{IdMaps, Uid},
 };
 use specs::{
     shred::ResourceId, Entities, Entity as EcsEntity, Join, Read, ReadExpect, ReadStorage,

server/src/cmd.rs

@@ -47,7 +47,7 @@ use common::{
     resources::{BattleMode, PlayerPhysicsSettings, Secs, Time, TimeOfDay, TimeScale},
     rtsim::{Actor, Role},
     terrain::{Block, BlockKind, CoordinateConversions, SpriteKind, TerrainChunkSize},
-    uid::{Uid, IdMaps},
+    uid::{IdMaps, Uid},
     vol::ReadVol,
     weather, Damage, DamageKind, DamageSource, Explosion, LoadoutBuilder, RadiusEffect,
 };

server/src/events/entity_manipulation.rs

@@ -32,7 +32,7 @@ use common::{
     states::utils::StageSection,
     terrain::{Block, BlockKind, TerrainGrid},
     trade::{TradeResult, Trades},
-    uid::{Uid, IdMaps},
+    uid::{IdMaps, Uid},
     util::Dir,
     vol::ReadVol,
     Damage, DamageKind, DamageSource, Explosion, GroupTarget, RadiusEffect,
@@ -1040,13 +1040,8 @@ pub fn handle_explosion(server: &Server, pos: Vec3<f32>, explosion: Explosion, o
                             .and_then(|cs| cs.attack_immunities())
                             .map_or(false, |i| i.explosions);
                         // PvP check
-                        let may_harm = combat::may_harm(
-                            alignments,
-                            players,
-                            id_maps,
-                            owner_entity,
-                            entity_b,
-                        );
+                        let may_harm =
+                            combat::may_harm(alignments, players, id_maps, owner_entity, entity_b);
                         let attack_options = combat::AttackOptions {
                             target_dodging,
                             may_harm,

server/src/events/group_manip.rs

@@ -305,7 +305,7 @@ pub fn handle_group(server: &mut Server, entity: Entity, manip: GroupManip) {
                         ));
                     }
                     // Tell the old leader that the transfer was succesful
-                    if let Some(client) = clients.get(target) {
+                    if let Some(client) = clients.get(entity) {
                         client.send_fallible(ServerGeneral::server_msg(
                             ChatType::Meta,
                             "You are no longer the group leader.",

server/src/events/player.rs

@@ -8,7 +8,7 @@ use common::{
     comp,
     comp::{group, pet::is_tameable, Presence, PresenceKind},
     resources::Time,
-    uid::{Uid, IdMaps},
+    uid::{IdMaps, Uid},
 };
 use common_base::span;
 use common_net::msg::{PlayerListUpdate, ServerGeneral};
@@ -61,6 +61,10 @@ pub fn handle_exit_ingame(server: &mut Server, entity: EcsEntity, skip_persisten
     //
     // Easier than checking and removing all other known components.
     //
+    // Also, allows clients to not update their Uid based references to this
+    // client (e.g. for this specific client's knowledge of its own Uid and for
+    // groups since exiting in-game does not affect group membership)
+    //
     // Note: If other `ServerEvent`s are referring to this entity they will be
     // disrupted.
 
@@ -89,30 +93,24 @@ pub fn handle_exit_ingame(server: &mut Server, entity: EcsEntity, skip_persisten
         // Tell client its request was successful
         client.send_fallible(ServerGeneral::ExitInGameSuccess);
 
-        let entity_builder = state.ecs_mut().create_entity().with(client).with(player);
-
-        // Preserve group component if present
-        let entity_builder = match maybe_group {
-            Some(group) => entity_builder.with(group),
-            None => entity_builder,
-        };
-
-        // Preserve admin component if present
-        let entity_builder = match maybe_admin {
-            Some(admin) => entity_builder.with(admin),
-            None => entity_builder,
-        };
+        let new_entity = state
+            .ecs_mut()
+            .create_entity()
+            .with(client)
+            .with(player)
+            // Preserve group component if present
+            .maybe_with(maybe_group)
+            // Preserve admin component if present
+            .maybe_with(maybe_admin)
+            .with(uid)
+            .build();
 
         // Ensure IdMaps maps this uid to the new entity
-        let uid = entity_builder
-            .world
-            .write_resource::<IdMaps>()
-            .allocate(entity_builder.entity, Some(uid.into()));
-        let new_entity = entity_builder.with(uid).build();
+        ecs.write_resource::<IdMaps>().remap_entity(uid, new_entity);
 
-        // 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) 
+        // Note, since the Uid has been removed from the old entity and the
+        // Group component will be removed below, that prevents
+        // `delete_entity_recorded` from making any changes to the group.
         if let Some(group) = maybe_group {
             let mut group_manager = state.ecs().write_resource::<group::GroupManager>();
             if group_manager
@@ -131,10 +129,10 @@ pub fn handle_exit_ingame(server: &mut Server, entity: EcsEntity, skip_persisten
                 );
             }
         }
-    }
-    // Erase group component to avoid group restructure when deleting the entity
-    state.ecs().write_storage::<group::Group>().remove(entity);
 
+        // Erase group component to avoid group restructure when deleting the entity
+        state.ecs().write_storage::<group::Group>().remove(entity);
+    }
     // Delete old entity
     if let Err(e) = state.delete_entity_recorded(entity) {
         error!(

server/src/state_ext.rs

@@ -1212,7 +1212,11 @@ impl StateExt for State {
         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.
+                // 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?
                 if let Some(region_key) = self
                     .ecs()
                     .read_resource::<common::region::RegionMap>()

server/src/sys/entity_sync.rs

@@ -191,6 +191,8 @@ impl<'a> System<'a> for Sys {
                                         .map(|key| !regions.contains(key))
                                         .unwrap_or(true)
                                     {
+                                        // TODO: I suspect it would be more efficient (in terms of
+                                        // bandwidth) to batch messages like this.
                                         client.send_fallible(ServerGeneral::DeleteEntity(uid));
                                     }
                                 }

server/src/sys/msg/register.rs

@@ -11,7 +11,7 @@ use common::{
     recipe::{default_component_recipe_book, default_recipe_book, default_repair_recipe_book},
     resources::TimeOfDay,
     shared_server_config::ServerConstants,
-    uid::{Uid, IdMaps},
+    uid::{IdMaps, Uid},
 };
 use common_base::prof_span;
 use common_ecs::{Job, Origin, Phase, System};
@@ -54,7 +54,7 @@ pub struct ReadData<'a> {
     trackers: TrackedStorages<'a>,
     _healths: ReadStorage<'a, Health>, // used by plugin feature
     _plugin_mgr: ReadPlugin<'a>,       // used by plugin feature
-    _id_maps: Read<'a, IdMaps>, // used by plugin feature
+    _id_maps: Read<'a, IdMaps>,        // used by plugin feature
 }
 
 /// This system will handle new messages from clients

server/src/sys/pets.rs

@@ -38,16 +38,14 @@ impl<'a> System<'a> for Sys {
                 _ => None,
             })
             .filter_map(|(pet_entity, pet_pos, owner_uid)| {
-                id_maps
-                    .uid_entity(owner_uid)
-                    .and_then(|owner_entity| {
-                        match (positions.get(owner_entity), physics.get(owner_entity)) {
-                            (Some(position), Some(physics)) => {
-                                Some((pet_entity, position, physics, pet_pos))
-                            },
-                            _ => None,
-                        }
-                    })
+                id_maps.uid_entity(owner_uid).and_then(|owner_entity| {
+                    match (positions.get(owner_entity), physics.get(owner_entity)) {
+                        (Some(position), Some(physics)) => {
+                            Some((pet_entity, position, physics, pet_pos))
+                        },
+                        _ => None,
+                    }
+                })
             })
             .filter(|(_, owner_pos, owner_physics, pet_pos)| {
                 // Don't teleport pets to the player if they're in the air, nobody wants

voxygen/src/hud/group.rs

@@ -18,7 +18,7 @@ use common::{
     combat,
     comp::{group::Role, inventory::item::MaterialStatManifest, invite::InviteKind, Stats},
     resources::Time,
-    uid::{Uid, IdMaps},
+    uid::{IdMaps, Uid},
 };
 use common_net::sync::WorldSyncExt;
 use conrod_core::{

voxygen/src/scene/particle.rs

@@ -287,9 +287,7 @@ impl ParticleMgr {
                 if target.is_some() {
                     let ecs = scene_data.state.ecs();
                     if target
-                        .and_then(|target| {
-                            ecs.read_resource::<IdMaps>().uid_entity(target)
-                        })
+                        .and_then(|target| ecs.read_resource::<IdMaps>().uid_entity(target))
                         .and_then(|entity| {
                             ecs.read_storage::<Body>()
                                 .get(entity)