Merge branch 'sharp/parallel-ingame' into 'master'

Parallelize ingame messages.

See merge request veloren/veloren!3627

assets/voxygen/i18n/en/char_selection.ftl

@@ -4,6 +4,7 @@ char_selection-deleting_character = Deleting Character...
 char_selection-change_server = Change Server
 char_selection-enter_world = Enter World
 char_selection-spectate = Spectate World
+char_selection-joining_character = Joining world...
 char_selection-logout = Logout
 char_selection-create_new_character = Create New Character
 char_selection-creating_character = Creating Character...

client/src/lib.rs

@@ -32,7 +32,7 @@ use common::{
         GroupManip, InputKind, InventoryAction, InventoryEvent, InventoryUpdateEvent,
         MapMarkerChange, UtteranceKind,
     },
-    event::{EventBus, LocalEvent},
+    event::{EventBus, LocalEvent, UpdateCharacterMetadata},
     grid::Grid,
     link::Is,
     lod,
@@ -107,6 +107,7 @@ pub enum Event {
     Outcome(Outcome),
     CharacterCreated(CharacterId),
     CharacterEdited(CharacterId),
+    CharacterJoined(UpdateCharacterMetadata),
     CharacterError(String),
     MapMarker(comp::MapMarkerUpdate),
     StartSpectate(Vec3<f32>),
@@ -844,10 +845,8 @@ impl Client {
                     | ClientGeneral::PlayerPhysics { .. }
                     | ClientGeneral::UnlockSkill(_)
                     | ClientGeneral::RequestSiteInfo(_)
-                    | ClientGeneral::UnlockSkillGroup(_)
                     | ClientGeneral::RequestPlayerPhysics { .. }
                     | ClientGeneral::RequestLossyTerrainCompression { .. }
-                    | ClientGeneral::AcknowledgePersistenceLoadError
                     | ClientGeneral::UpdateMapMarker(_)
                     | ClientGeneral::SpectatePosition(_) => {
                         #[cfg(feature = "tracy")]
@@ -1664,10 +1663,6 @@ impl Client {
         }))
     }
 
-    pub fn acknolwedge_persistence_load_error(&mut self) {
-        self.send_msg(ClientGeneral::AcknowledgePersistenceLoadError)
-    }
-
     /// Execute a single client tick, handle input and update the game state by
     /// the given duration.
     pub fn tick(
@@ -2402,7 +2397,11 @@ impl Client {
                 warn!("CharacterActionError: {:?}.", error);
                 events.push(Event::CharacterError(error));
             },
-            ServerGeneral::CharacterDataLoadError(error) => {
+            ServerGeneral::CharacterDataLoadResult(Ok(metadata)) => {
+                trace!("Handling join result by server");
+                events.push(Event::CharacterJoined(metadata));
+            },
+            ServerGeneral::CharacterDataLoadResult(Err(error)) => {
                 trace!("Handling join error by server");
                 self.presence = None;
                 self.clean_state();

common/net/src/msg/client.rs

@@ -1,11 +1,5 @@
 use super::{world_msg::SiteId, PingMsg};
-use common::{
+use common::{character::CharacterId, comp, comp::Skill, terrain::block::Block, ViewDistances};
-    character::CharacterId,
-    comp,
-    comp::{Skill, SkillGroupKind},
-    terrain::block::Block,
-    ViewDistances,
-};
 use serde::{Deserialize, Serialize};
 use vek::*;
 
@@ -78,7 +72,6 @@ pub enum ClientGeneral {
         force_counter: u64,
     },
     UnlockSkill(Skill),
-    UnlockSkillGroup(SkillGroupKind),
     RequestSiteInfo(SiteId),
     UpdateMapMarker(comp::MapMarkerChange),
 
@@ -100,7 +93,6 @@ pub enum ClientGeneral {
     RequestLossyTerrainCompression {
         lossy_terrain_compression: bool,
     },
-    AcknowledgePersistenceLoadError,
 }
 
 impl ClientMsg {
@@ -138,10 +130,8 @@ impl ClientMsg {
                         | ClientGeneral::LodZoneRequest { .. }
                         | ClientGeneral::UnlockSkill(_)
                         | ClientGeneral::RequestSiteInfo(_)
-                        | ClientGeneral::UnlockSkillGroup(_)
                         | ClientGeneral::RequestPlayerPhysics { .. }
                         | ClientGeneral::RequestLossyTerrainCompression { .. }
-                        | ClientGeneral::AcknowledgePersistenceLoadError
                         | ClientGeneral::UpdateMapMarker(_)
                         | ClientGeneral::SpectatePosition(_) => {
                             c_type == ClientType::Game && presence.is_some()

common/net/src/msg/server.rs

@@ -7,6 +7,7 @@ use common::{
     calendar::Calendar,
     character::{self, CharacterItem},
     comp::{self, invite::InviteKind, item::MaterialStatManifest},
+    event::UpdateCharacterMetadata,
     lod,
     outcome::Outcome,
     recipe::{ComponentRecipeBook, RecipeBook},
@@ -129,8 +130,8 @@ impl SerializedTerrainChunk {
 #[derive(Debug, Clone, Serialize, Deserialize)]
 pub enum ServerGeneral {
     //Character Screen related
-    /// An error occurred while loading character data
+    /// Result of loading character data
-    CharacterDataLoadError(String),
+    CharacterDataLoadResult(Result<UpdateCharacterMetadata, String>),
     /// A list of characters belonging to the a authenticated player was sent
     CharacterListUpdate(Vec<CharacterItem>),
     /// An error occurred while creating or deleting a character
@@ -295,7 +296,7 @@ impl ServerMsg {
                 registered
                     && match g {
                         //Character Screen related
-                        ServerGeneral::CharacterDataLoadError(_)
+                        ServerGeneral::CharacterDataLoadResult(_)
                         | ServerGeneral::CharacterListUpdate(_)
                         | ServerGeneral::CharacterActionError(_)
                         | ServerGeneral::CharacterEdited(_)

common/src/comp/skillset/mod.rs

@@ -5,6 +5,7 @@ use crate::{
         skills::{GeneralSkill, Skill},
     },
 };
+use core::borrow::{Borrow, BorrowMut};
 use hashbrown::HashMap;
 use lazy_static::lazy_static;
 use serde::{Deserialize, Serialize};
@@ -238,10 +239,6 @@ pub struct SkillSet {
     skills: HashMap<Skill, u16>,
     pub modify_health: bool,
     pub modify_energy: bool,
-    // TODO: why is this part of the component?
-    /// Used to indicate to the frontend that there was an error in loading the
-    /// skillset from the database
-    pub persistence_load_error: Option<SkillsPersistenceError>,
 }
 
 impl Component for SkillSet {
@@ -259,7 +256,6 @@ impl Default for SkillSet {
             skills: SkillSet::initial_skills(),
             modify_health: false,
             modify_energy: false,
-            persistence_load_error: None,
         };
 
         // Insert default skill groups
@@ -281,17 +277,20 @@ impl SkillSet {
         skills
     }
 
+    /// NOTE: This does *not* return an error on failure, since we can partially
+    /// recover from some failures.  Instead, it returns the error in the
+    /// second return value; make sure to handle it if present!
     pub fn load_from_database(
         skill_groups: HashMap<SkillGroupKind, SkillGroup>,
         mut all_skills: HashMap<SkillGroupKind, Result<Vec<Skill>, SkillsPersistenceError>>,
-    ) -> Self {
+    ) -> (Self, Option<SkillsPersistenceError>) {
         let mut skillset = SkillSet {
             skill_groups,
             skills: SkillSet::initial_skills(),
             modify_health: true,
             modify_energy: true,
-            persistence_load_error: None,
         };
+        let mut persistence_load_error = None;
 
         // Loops while checking the all_skills hashmap. For as long as it can find an
         // entry where the skill group kind is unlocked, insert the skills corresponding
@@ -317,29 +316,33 @@ impl SkillSet {
                         {
                             skillset = backup_skillset;
                             // If unlocking failed, set persistence_load_error
-                            skillset.persistence_load_error =
+                            persistence_load_error =
                                 Some(SkillsPersistenceError::SkillsUnlockFailed)
                         }
                     },
-                    Err(persistence_error) => {
+                    Err(persistence_error) => persistence_load_error = Some(persistence_error),
-                        skillset.persistence_load_error = Some(persistence_error)
-                    },
                 }
             }
         }
 
-        skillset
+        (skillset, persistence_load_error)
+    }
+
+    /// Check if a particular skill group is accessible for an entity, *if* it
+    /// exists.
+    fn skill_group_accessible_if_exists(&self, skill_group_kind: SkillGroupKind) -> bool {
+        self.has_skill(Skill::UnlockGroup(skill_group_kind))
     }
 
     /// Checks if a particular skill group is accessible for an entity
     pub fn skill_group_accessible(&self, skill_group_kind: SkillGroupKind) -> bool {
         self.skill_groups.contains_key(&skill_group_kind)
-            && self.has_skill(Skill::UnlockGroup(skill_group_kind))
+            && self.skill_group_accessible_if_exists(skill_group_kind)
     }
 
     ///  Unlocks a skill group for a player. It starts with 0 exp and 0 skill
     ///  points.
-    pub fn unlock_skill_group(&mut self, skill_group_kind: SkillGroupKind) {
+    fn unlock_skill_group(&mut self, skill_group_kind: SkillGroupKind) {
         if !self.skill_groups.contains_key(&skill_group_kind) {
             self.skill_groups
                 .insert(skill_group_kind, SkillGroup::new(skill_group_kind));
@@ -462,33 +465,56 @@ impl SkillSet {
 
     /// Unlocks a skill for a player, assuming they have the relevant skill
     /// group unlocked and available SP in that skill group.
-    pub fn unlock_skill(&mut self, skill: Skill) -> Result<(), SkillUnlockError> {
+    ///
+    /// NOTE: Please don't use pathological or clever implementations of to_mut
+    /// here.
+    pub fn unlock_skill_cow<'a, B, C: 'a>(
+        this_: &'a mut B,
+        skill: Skill,
+        to_mut: impl FnOnce(&'a mut B) -> &'a mut C,
+    ) -> Result<(), SkillUnlockError>
+    where
+        B: Borrow<SkillSet>,
+        C: BorrowMut<SkillSet>,
+    {
         if let Some(skill_group_kind) = skill.skill_group_kind() {
-            let next_level = self.next_skill_level(skill);
+            let this = (&*this_).borrow();
-            let prerequisites_met = self.prerequisites_met(skill);
+            let next_level = this.next_skill_level(skill);
+            let prerequisites_met = this.prerequisites_met(skill);
             // Check that skill is not yet at max level
-            if !matches!(self.skills.get(&skill), Some(level) if *level == skill.max_level()) {
+            if !matches!(this.skills.get(&skill), Some(level) if *level == skill.max_level()) {
-                if let Some(mut skill_group) = self.skill_group_mut(skill_group_kind) {
+                if let Some(skill_group) = this.skill_groups.get(&skill_group_kind) &&
+                    this.skill_group_accessible_if_exists(skill_group_kind)
+                {
                     if prerequisites_met {
                         if let Some(new_available_sp) = skill_group
                             .available_sp
                             .checked_sub(skill.skill_cost(next_level))
                         {
+                            // Perform all mutation inside this branch, to avoid triggering a copy
+                            // on write or flagged storage in cases where this matters.
+                            let this_ = to_mut(this_);
+                            let mut this = this_.borrow_mut();
+                            // NOTE: Verified to exist previously when we accessed
+                            // this.skill_groups (assuming a non-pathological implementation of
+                            // ToOwned).
+                            let skill_group = this.skill_groups.get_mut(&skill_group_kind)
+                                .expect("Verified to exist when we previously accessed this.skill_groups");
                             skill_group.available_sp = new_available_sp;
                             skill_group.ordered_skills.push(skill);
                             match skill {
                                 Skill::UnlockGroup(group) => {
-                                    self.unlock_skill_group(group);
+                                    this.unlock_skill_group(group);
                                 },
                                 Skill::General(GeneralSkill::HealthIncrease) => {
-                                    self.modify_health = true;
+                                    this.modify_health = true;
                                 },
                                 Skill::General(GeneralSkill::EnergyIncrease) => {
-                                    self.modify_energy = true;
+                                    this.modify_energy = true;
                                 },
                                 _ => {},
                             }
-                            self.skills.insert(skill, next_level);
+                            this.skills.insert(skill, next_level);
                             Ok(())
                         } else {
                             trace!("Tried to unlock skill for skill group with insufficient SP");
@@ -515,6 +541,12 @@ impl SkillSet {
         }
     }
 
+    /// Convenience function for the case where you have mutable access to the
+    /// skill.
+    pub fn unlock_skill(&mut self, skill: Skill) -> Result<(), SkillUnlockError> {
+        Self::unlock_skill_cow(self, skill, |x| x)
+    }
+
     /// Checks if the player has available SP to spend
     pub fn has_available_sp(&self) -> bool {
         self.skill_groups.iter().any(|(kind, sg)| {

common/src/event.rs

@@ -15,6 +15,7 @@ use crate::{
     util::Dir,
     Explosion,
 };
+use serde::{Deserialize, Serialize};
 use specs::Entity as EcsEntity;
 use std::{collections::VecDeque, ops::DerefMut, sync::Mutex};
 use vek::*;
@@ -35,6 +36,11 @@ pub enum LocalEvent {
     CreateOutcome(Outcome),
 }
 
+#[derive(Clone, Debug, Default, Deserialize, Serialize)]
+pub struct UpdateCharacterMetadata {
+    pub skill_set_persistence_load_error: Option<comp::skillset::SkillsPersistenceError>,
+}
+
 #[allow(clippy::large_enum_variant)] // TODO: Pending review in #587
 #[derive(strum::EnumDiscriminants)]
 #[strum_discriminants(repr(usize))]
@@ -122,6 +128,7 @@ pub enum ServerEvent {
             comp::ActiveAbilities,
             Option<comp::MapMarker>,
         ),
+        metadata: UpdateCharacterMetadata,
     },
     ExitIngame {
         entity: EcsEntity,

common/src/lib.rs

@@ -8,6 +8,7 @@
     bool_to_option,
     fundamental,
     label_break_value,
+    let_chains,
     option_zip,
     trait_alias,
     type_alias_impl_trait,

common/src/resources.rs

@@ -41,7 +41,7 @@ pub enum GameMode {
 #[derive(Copy, Clone, Default, Debug)]
 pub struct PlayerEntity(pub Option<Entity>);
 
-#[derive(Copy, Clone, Debug, Default)]
+#[derive(Copy, Clone, Debug, Default, Eq, PartialEq)]
 pub struct PlayerPhysicsSetting {
     /// true if the client wants server-authoratative physics (e.g. to use
     /// airships properly)

server/src/client.rs

@@ -93,7 +93,7 @@ impl Client {
             ServerMsg::General(g) => {
                 match g {
                     //Character Screen related
-                    ServerGeneral::CharacterDataLoadError(_)
+                    ServerGeneral::CharacterDataLoadResult(_)
                     | ServerGeneral::CharacterListUpdate(_)
                     | ServerGeneral::CharacterActionError(_)
                     | ServerGeneral::CharacterCreated(_)
@@ -164,7 +164,7 @@ impl Client {
             ServerMsg::General(g) => {
                 match g {
                     //Character Screen related
-                    ServerGeneral::CharacterDataLoadError(_)
+                    ServerGeneral::CharacterDataLoadResult(_)
                     | ServerGeneral::CharacterListUpdate(_)
                     | ServerGeneral::CharacterActionError(_)
                     | ServerGeneral::CharacterCreated(_)

server/src/events/entity_creation.rs

@@ -11,7 +11,7 @@ use common::{
         Object, Ori, PidController, Poise, Pos, Projectile, Scale, SkillSet, Stats, Vel,
         WaypointArea,
     },
-    event::EventBus,
+    event::{EventBus, UpdateCharacterMetadata},
     lottery::LootSpec,
     outcome::Outcome,
     rtsim::RtSimEntity,
@@ -60,6 +60,7 @@ pub fn handle_loaded_character_data(
     server: &mut Server,
     entity: EcsEntity,
     loaded_components: PersistedComponents,
+    metadata: UpdateCharacterMetadata,
 ) {
     if let Some(marker) = loaded_components.map_marker {
         server.notify_client(
@@ -73,6 +74,8 @@ pub fn handle_loaded_character_data(
         .state
         .update_character_data(entity, loaded_components);
     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)));
 }
 
 pub fn handle_create_npc(

server/src/events/mod.rs

@@ -150,7 +150,11 @@ impl Server {
                 ServerEvent::InitSpectator(entity, requested_view_distances) => {
                     handle_initialize_spectator(self, entity, requested_view_distances)
                 },
-                ServerEvent::UpdateCharacterData { entity, components } => {
+                ServerEvent::UpdateCharacterData {
+                    entity,
+                    components,
+                    metadata,
+                } => {
                     let (
                         body,
                         stats,
@@ -171,7 +175,7 @@ impl Server {
                         active_abilities,
                         map_marker,
                     };
-                    handle_loaded_character_data(self, entity, components);
+                    handle_loaded_character_data(self, entity, components, metadata);
                 },
                 ServerEvent::ExitIngame { entity } => {
                     handle_exit_ingame(self, entity);

server/src/lib.rs

@@ -888,7 +888,7 @@ impl Server {
                 },
                 CharacterLoaderResponseKind::CharacterData(result) => {
                     let message = match *result {
-                        Ok(character_data) => {
+                        Ok((character_data, skill_set_persistence_load_error)) => {
                             let PersistedComponents {
                                 body,
                                 stats,
@@ -912,6 +912,7 @@ impl Server {
                             ServerEvent::UpdateCharacterData {
                                 entity: query_result.entity,
                                 components: character_data,
+                                metadata: skill_set_persistence_load_error,
                             }
                         },
                         Err(error) => {
@@ -920,7 +921,7 @@ impl Server {
                             // to display
                             self.notify_client(
                                 query_result.entity,
-                                ServerGeneral::CharacterDataLoadError(error.to_string()),
+                                ServerGeneral::CharacterDataLoadResult(Err(error.to_string())),
                             );
 
                             // Clean up the entity data on the server

server/src/persistence/character.rs

@@ -25,7 +25,10 @@ use crate::{
         EditableComponents, PersistedComponents,
     },
 };
-use common::character::{CharacterId, CharacterItem, MAX_CHARACTERS_PER_PLAYER};
+use common::{
+    character::{CharacterId, CharacterItem, MAX_CHARACTERS_PER_PLAYER},
+    event::UpdateCharacterMetadata,
+};
 use core::ops::Range;
 use rusqlite::{types::Value, Connection, ToSql, Transaction, NO_PARAMS};
 use std::{num::NonZeroU64, rc::Rc};
@@ -254,10 +257,13 @@ pub fn load_character_data(
         })
     })?;
 
-    Ok(PersistedComponents {
+    let (skill_set, skill_set_persistence_load_error) =
+        convert_skill_set_from_database(&skill_group_data);
+    Ok((
+        PersistedComponents {
             body: convert_body_from_database(&body_data.variant, &body_data.body_data)?,
             stats: convert_stats_from_database(character_data.alias),
-        skill_set: convert_skill_set_from_database(&skill_group_data),
+            skill_set,
             inventory: convert_inventory_from_database_items(
                 character_containers.inventory_container_id,
                 &inventory_items,
@@ -268,7 +274,11 @@ pub fn load_character_data(
             pets,
             active_abilities: convert_active_abilities_from_database(&ability_set_data),
             map_marker: char_map_marker,
-    })
+        },
+        UpdateCharacterMetadata {
+            skill_set_persistence_load_error,
+        },
+    ))
 }
 
 /// Loads a list of characters belonging to the player. This data is a small

server/src/persistence/character/conversions.rs

@@ -606,7 +606,12 @@ pub fn convert_stats_from_database(alias: String) -> Stats {
     new_stats
 }
 
-pub fn convert_skill_set_from_database(skill_groups: &[SkillGroup]) -> SkillSet {
+/// NOTE: This does *not* return an error on failure, since we can partially
+/// recover from some failures.  Instead, it returns the error in the second
+/// return value; make sure to handle it if present!
+pub fn convert_skill_set_from_database(
+    skill_groups: &[SkillGroup],
+) -> (SkillSet, Option<skillset::SkillsPersistenceError>) {
     let (skillless_skill_groups, deserialized_skills) =
         convert_skill_groups_from_database(skill_groups);
     SkillSet::load_from_database(skillless_skill_groups, deserialized_skills)

server/src/persistence/character_loader.rs

@@ -3,7 +3,10 @@ use crate::persistence::{
     error::PersistenceError,
     establish_connection, ConnectionMode, DatabaseSettings, PersistedComponents,
 };
-use common::character::{CharacterId, CharacterItem};
+use common::{
+    character::{CharacterId, CharacterItem},
+    event::UpdateCharacterMetadata,
+};
 use crossbeam_channel::{self, TryIter};
 use rusqlite::Connection;
 use std::sync::{Arc, RwLock};
@@ -13,7 +16,8 @@ pub(crate) type CharacterListResult = Result<Vec<CharacterItem>, PersistenceErro
 pub(crate) type CharacterCreationResult =
     Result<(CharacterId, Vec<CharacterItem>), PersistenceError>;
 pub(crate) type CharacterEditResult = Result<(CharacterId, Vec<CharacterItem>), PersistenceError>;
-pub(crate) type CharacterDataResult = Result<PersistedComponents, PersistenceError>;
+pub(crate) type CharacterDataResult =
+    Result<(PersistedComponents, UpdateCharacterMetadata), PersistenceError>;
 type CharacterLoaderRequest = (specs::Entity, CharacterLoaderRequestKind);
 
 /// Available database operations when modifying a player's character list

server/src/sys/msg/character_screen.rs

@@ -83,10 +83,10 @@ impl Sys {
                         .any(|x| x == character_id)
                     {
                         debug!("player recently logged out pending persistence, aborting");
-                        client.send(ServerGeneral::CharacterDataLoadError(
+                        client.send(ServerGeneral::CharacterDataLoadResult(Err(
                             "You have recently logged out, please wait a few seconds and try again"
                                 .to_string(),
-                        ))?;
+                        )))?;
                     } else if character_updater.disconnect_all_clients_requested() {
                         // If we're in the middle of disconnecting all clients due to a persistence
                         // transaction failure, prevent new logins
@@ -95,11 +95,11 @@ impl Sys {
                             "Rejecting player login while pending disconnection of all players is \
                              in progress"
                         );
-                        client.send(ServerGeneral::CharacterDataLoadError(
+                        client.send(ServerGeneral::CharacterDataLoadResult(Err(
                             "The server is currently recovering from an error, please wait a few \
                              seconds and try again"
                                 .to_string(),
-                        ))?;
+                        )))?;
                     } else {
                         // Send a request to load the character's component data from the
                         // DB. Once loaded, persisted components such as stats and inventory
@@ -122,9 +122,9 @@ impl Sys {
                     }
                 } else {
                     debug!("Client is not yet registered");
-                    client.send(ServerGeneral::CharacterDataLoadError(String::from(
+                    client.send(ServerGeneral::CharacterDataLoadResult(Err(String::from(
                         "Failed to fetch player entity",
-                    )))?
+                    ))))?
                 }
             },
             ClientGeneral::RequestCharacterList => {

server/src/sys/msg/in_game.rs

@@ -9,22 +9,38 @@ use common::{
     event::{EventBus, ServerEvent},
     link::Is,
     mounting::Rider,
-    resources::PlayerPhysicsSettings,
+    resources::{PlayerPhysicsSetting, PlayerPhysicsSettings},
+    slowjob::SlowJobPool,
     terrain::TerrainGrid,
     vol::ReadVol,
 };
 use common_ecs::{Job, Origin, Phase, System};
 use common_net::msg::{ClientGeneral, PresenceKind, ServerGeneral};
 use common_state::{BlockChange, BuildAreas};
+use core::mem;
+use rayon::prelude::*;
 use specs::{Entities, Join, Read, ReadExpect, ReadStorage, Write, WriteStorage};
-use std::time::Instant;
+use std::{borrow::Cow, time::Instant};
 use tracing::{debug, trace, warn};
 use vek::*;
 
 #[cfg(feature = "persistent_world")]
 pub type TerrainPersistenceData<'a> = Option<Write<'a, TerrainPersistence>>;
 #[cfg(not(feature = "persistent_world"))]
-pub type TerrainPersistenceData<'a> = ();
+pub type TerrainPersistenceData<'a> = core::marker::PhantomData<&'a mut ()>;
+
+// NOTE: These writes are considered "rare", meaning (currently) that they are
+// admin-gated features that players shouldn't normally access, and which we're
+// not that concerned about the performance of when two players try to use them
+// at once.
+//
+// In such cases, we're okay putting them behind a mutex and penalizing the
+// system if they're actually used concurrently by lots of users.  Please do not
+// put less rare writes here, unless you want to serialize the system!
+struct RareWrites<'a, 'b> {
+    block_changes: &'b mut BlockChange,
+    _terrain_persistence: &'b mut TerrainPersistenceData<'a>,
+}
 
 impl Sys {
     #[allow(clippy::too_many_arguments)]
@@ -37,18 +53,16 @@ impl Sys {
         can_build: &ReadStorage<'_, CanBuild>,
         is_rider: &ReadStorage<'_, Is<Rider>>,
         force_updates: &ReadStorage<'_, ForceUpdate>,
-        skill_sets: &mut WriteStorage<'_, SkillSet>,
+        skill_set: &mut Option<Cow<'_, SkillSet>>,
         healths: &ReadStorage<'_, Health>,
-        block_changes: &mut Write<'_, BlockChange>,
+        rare_writes: &parking_lot::Mutex<RareWrites<'_, '_>>,
-        positions: &mut WriteStorage<'_, Pos>,
+        position: Option<&mut Pos>,
-        velocities: &mut WriteStorage<'_, Vel>,
+        velocity: Option<&mut Vel>,
-        orientations: &mut WriteStorage<'_, Ori>,
+        orientation: Option<&mut Ori>,
-        controllers: &mut WriteStorage<'_, Controller>,
+        controller: Option<&mut Controller>,
         settings: &Read<'_, Settings>,
         build_areas: &Read<'_, BuildAreas>,
-        player_physics_settings: &mut Write<'_, PlayerPhysicsSettings>,
+        player_physics_setting: Option<&mut PlayerPhysicsSetting>,
-        _terrain_persistence: &mut TerrainPersistenceData<'_>,
-        maybe_player: &Option<&Player>,
         maybe_admin: &Option<&Admin>,
         time_for_vd_changes: Instant,
         msg: ClientGeneral,
@@ -81,7 +95,7 @@ impl Sys {
             },
             ClientGeneral::ControllerInputs(inputs) => {
                 if presence.kind.controlling_char() {
-                    if let Some(controller) = controllers.get_mut(entity) {
+                    if let Some(controller) = controller {
                         controller.inputs.update_with_new(*inputs);
                     }
                 }
@@ -95,26 +109,19 @@ impl Sys {
                             return Ok(());
                         }
                     }
-                    if let Some(controller) = controllers.get_mut(entity) {
+                    if let Some(controller) = controller {
                         controller.push_event(event);
                     }
                 }
             },
             ClientGeneral::ControlAction(event) => {
                 if presence.kind.controlling_char() {
-                    if let Some(controller) = controllers.get_mut(entity) {
+                    if let Some(controller) = controller {
                         controller.push_action(event);
                     }
                 }
             },
             ClientGeneral::PlayerPhysics { pos, vel, ori, force_counter } => {
-                let player_physics_setting = maybe_player.map(|p| {
-                    player_physics_settings
-                        .settings
-                        .entry(p.uuid())
-                        .or_default()
-                });
-
                 if presence.kind.controlling_char()
                     && force_updates.get(entity).map_or(true, |force_update| force_update.counter() == force_counter)
                     && healths.get(entity).map_or(true, |h| !h.is_dead)
@@ -138,7 +145,7 @@ impl Sys {
                         let rejection = None
                             // Check position
                             .or_else(|| {
-                                if let Some(prev_pos) = positions.get(entity) {
+                                if let Some(prev_pos) = &position {
                                     if prev_pos.0.distance_squared(pos.0) > (500.0f32).powf(2.0) {
                                         Some(Rejection::TooFar { old: prev_pos.0, new: pos.0 })
                                     } else {
@@ -187,9 +194,9 @@ impl Sys {
                         ),
                         None => {
                             // Don't insert unless the component already exists
-                            let _ = positions.get_mut(entity).map(|p| *p = pos);
+                            position.map(|p| *p = pos);
-                            let _ = velocities.get_mut(entity).map(|v| *v = vel);
+                            velocity.map(|v| *v = vel);
-                            let _ = orientations.get_mut(entity).map(|o| *o = ori);
+                            orientation.map(|o| *o = ori);
                         },
                     }
                 }
@@ -207,10 +214,12 @@ impl Sys {
                                 .and_then(|_| terrain.get(pos).ok())
                             {
                                 let new_block = old_block.into_vacant();
-                                let _was_set = block_changes.try_set(pos, new_block).is_some();
+                                // Take the rare writes lock as briefly as possible.
+                                let mut guard = rare_writes.lock();
+                                let _was_set = guard.block_changes.try_set(pos, new_block).is_some();
                                 #[cfg(feature = "persistent_world")]
                                 if _was_set {
-                                    if let Some(terrain_persistence) = _terrain_persistence.as_mut()
+                                    if let Some(terrain_persistence) = guard._terrain_persistence.as_mut()
                                     {
                                         terrain_persistence.set_block(pos, new_block);
                                     }
@@ -232,10 +241,12 @@ impl Sys {
                                 .filter(|aabb| aabb.contains_point(pos))
                                 .is_some()
                             {
-                                let _was_set = block_changes.try_set(pos, new_block).is_some();
+                                // Take the rare writes lock as briefly as possible.
+                                let mut guard = rare_writes.lock();
+                                let _was_set = guard.block_changes.try_set(pos, new_block).is_some();
                                 #[cfg(feature = "persistent_world")]
                                 if _was_set {
-                                    if let Some(terrain_persistence) = _terrain_persistence.as_mut()
+                                    if let Some(terrain_persistence) = guard._terrain_persistence.as_mut()
                                     {
                                         terrain_persistence.set_block(pos, new_block);
                                     }
@@ -246,14 +257,10 @@ impl Sys {
                 }
             },
             ClientGeneral::UnlockSkill(skill) => {
-                skill_sets
+                // FIXME: How do we want to handle the error?  Probably not by swallowing it.
-                    .get_mut(entity)
+                let _ = skill_set.as_mut().map(|skill_set| {
-                    .map(|mut skill_set| skill_set.unlock_skill(skill));
+                    SkillSet::unlock_skill_cow(skill_set, skill, |skill_set| skill_set.to_mut())
-            },
+                }).transpose();
-            ClientGeneral::UnlockSkillGroup(skill_group_kind) => {
-                skill_sets
-                    .get_mut(entity)
-                    .map(|mut skill_set| skill_set.unlock_skill_group(skill_group_kind));
             },
             ClientGeneral::RequestSiteInfo(id) => {
                 server_emitter.emit(ServerEvent::RequestSiteInfo { entity, id });
@@ -261,12 +268,6 @@ impl Sys {
             ClientGeneral::RequestPlayerPhysics {
                 server_authoritative,
             } => {
-                let player_physics_setting = maybe_player.map(|p| {
-                    player_physics_settings
-                        .settings
-                        .entry(p.uuid())
-                        .or_default()
-                });
                 if let Some(setting) = player_physics_setting {
                     setting.client_optin = server_authoritative;
                 }
@@ -276,17 +277,12 @@ impl Sys {
             } => {
                 presence.lossy_terrain_compression = lossy_terrain_compression;
             },
-            ClientGeneral::AcknowledgePersistenceLoadError => {
-                skill_sets
-                    .get_mut(entity)
-                    .map(|mut skill_set| skill_set.persistence_load_error = None);
-            },
             ClientGeneral::UpdateMapMarker(update) => {
                 server_emitter.emit(ServerEvent::UpdateMapMarker { entity, update });
             },
             ClientGeneral::SpectatePosition(pos) => {
                 if let Some(admin) = maybe_admin && admin.0 >= AdminRole::Moderator && presence.kind == PresenceKind::Spectator {
-                    if let Some(position) = positions.get_mut(entity) {
+                    if let Some(position) = position {
                         position.0 = pos;
                     }
                 }
@@ -322,6 +318,7 @@ impl<'a> System<'a> for Sys {
         Entities<'a>,
         Read<'a, EventBus<ServerEvent>>,
         ReadExpect<'a, TerrainGrid>,
+        ReadExpect<'a, SlowJobPool>,
         ReadStorage<'a, CanBuild>,
         ReadStorage<'a, ForceUpdate>,
         ReadStorage<'a, Is<Rider>>,
@@ -352,6 +349,7 @@ impl<'a> System<'a> for Sys {
             entities,
             server_event_bus,
             terrain,
+            slow_jobs,
             can_build,
             force_updates,
             is_rider,
@@ -366,31 +364,66 @@ impl<'a> System<'a> for Sys {
             mut controllers,
             settings,
             build_areas,
-            mut player_physics_settings,
+            mut player_physics_settings_,
             mut terrain_persistence,
             players,
             admins,
         ): Self::SystemData,
     ) {
-        let mut server_emitter = server_event_bus.emitter();
-
         let time_for_vd_changes = Instant::now();
 
-        for (entity, client, mut maybe_presence, player, maybe_admin) in (
+        // NOTE: stdlib mutex is more than good enough on Linux and (probably) Windows,
+        // but not Mac.
+        let rare_writes = parking_lot::Mutex::new(RareWrites {
+            block_changes: &mut block_changes,
+            _terrain_persistence: &mut terrain_persistence,
+        });
+
+        let player_physics_settings = &*player_physics_settings_;
+        let mut deferred_updates = (
             &entities,
             &mut clients,
             (&mut presences).maybe(),
             players.maybe(),
             admins.maybe(),
+            (&skill_sets).maybe(),
+            (&mut positions).maybe(),
+            (&mut velocities).maybe(),
+            (&mut orientations).maybe(),
+            (&mut controllers).maybe(),
         )
             .join()
-        {
+            // NOTE: Required because Specs has very poor work splitting for sparse joins.
+            .par_bridge()
+            .map_init(
+                || server_event_bus.emitter(),
+                |server_emitter, (
+                    entity,
+                    client,
+                    mut maybe_presence,
+                    maybe_player,
+                    maybe_admin,
+                    skill_set,
+                    ref mut pos,
+                    ref mut vel,
+                    ref mut ori,
+                    ref mut controller,
+                )| {
+                    let old_player_physics_setting = maybe_player.map(|p| {
+                        player_physics_settings
+                            .settings
+                            .get(&p.uuid())
+                            .copied()
+                            .unwrap_or_default()
+                    });
+                    let mut new_player_physics_setting = old_player_physics_setting;
                     // If an `ExitInGame` message is received this is set to `None` allowing further
                     // ingame messages to be ignored.
                     let mut clearable_maybe_presence = maybe_presence.as_deref_mut();
+                    let mut skill_set = skill_set.map(Cow::Borrowed);
                     let _ = super::try_recv_all(client, 2, |client, msg| {
                         Self::handle_client_in_game_msg(
-                    &mut server_emitter,
+                            server_emitter,
                             entity,
                             client,
                             &mut clearable_maybe_presence,
@@ -398,18 +431,16 @@ impl<'a> System<'a> for Sys {
                             &can_build,
                             &is_rider,
                             &force_updates,
-                    &mut skill_sets,
+                            &mut skill_set,
                             &healths,
-                    &mut block_changes,
+                            &rare_writes,
-                    &mut positions,
+                            pos.as_deref_mut(),
-                    &mut velocities,
+                            vel.as_deref_mut(),
-                    &mut orientations,
+                            ori.as_deref_mut(),
-                    &mut controllers,
+                            controller.as_deref_mut(),
                             &settings,
                             &build_areas,
-                    &mut player_physics_settings,
+                            new_player_physics_setting.as_mut(),
-                    &mut terrain_persistence,
-                    &player,
                             &maybe_admin,
                             time_for_vd_changes,
                             msg,
@@ -422,6 +453,63 @@ impl<'a> System<'a> for Sys {
                         presence.terrain_view_distance.update(time_for_vd_changes);
                         presence.entity_view_distance.update(time_for_vd_changes);
                     }
-        }
+
+                    // Return the possibly modified skill set, and possibly modified server physics
+                    // settings.
+                    let skill_set_update = skill_set.and_then(|skill_set| match skill_set {
+                        Cow::Borrowed(_) => None,
+                        Cow::Owned(skill_set) => Some((entity, skill_set)),
+                    });
+                    // NOTE: Since we pass Option<&mut _> rather than &mut Option<_> to
+                    // handle_client_in_game_msg, and the new player was initialized to the same
+                    // value as the old setting , we know that either both the new and old setting
+                    // are Some, or they are both None.
+                    let physics_update = maybe_player.map(|p| p.uuid())
+                        .zip(new_player_physics_setting
+                             .filter(|_| old_player_physics_setting != new_player_physics_setting));
+                    (skill_set_update, physics_update)
+                },
+            )
+            // NOTE: Would be nice to combine this with the map_init somehow, but I'm not sure if
+            // that's possible.
+            .filter(|(x, y)| x.is_some() || y.is_some())
+            // NOTE: I feel like we shouldn't actually need to allocate here, but hopefully this
+            // doesn't turn out to be important as there shouldn't be that many connected clients.
+            // The reason we can't just use unzip is that the two sides might be different lengths.
+            .collect::<Vec<_>>();
+        let player_physics_settings = &mut *player_physics_settings_;
+        // Deferred updates to skillsets and player physics.
+        //
+        // NOTE: It is an invariant that there is at most one client entry per player
+        // uuid; since we joined on clients, it follows that there's just one update
+        // per uuid, so the physics update is sound and doesn't depend on evaluation
+        // order, even though we're not updating directly by entity or uid (note that
+        // for a given entity, we process messages serially).
+        deferred_updates
+            .iter_mut()
+            .for_each(|(skill_set_update, physics_update)| {
+                if let Some((entity, new_skill_set)) = skill_set_update {
+                    // We know this exists, because we already iterated over it with the skillset
+                    // lock taken, so we can ignore the error.
+                    //
+                    // Note that we replace rather than just updating.  This is in order to avoid
+                    // dropping here; we'll drop later on a background thread, in case skillsets are
+                    // slow to drop.
+                    skill_sets
+                        .get_mut(*entity)
+                        .map(|mut old_skill_set| mem::swap(&mut *old_skill_set, new_skill_set));
+                }
+                if let &mut Some((uuid, player_physics_setting)) = physics_update {
+                    // We don't necessarily know this exists, but that's fine, because dropping
+                    // player physics is a no op.
+                    player_physics_settings
+                        .settings
+                        .insert(uuid, player_physics_setting);
+                }
+            });
+        // Finally, drop the deferred updates in another thread.
+        slow_jobs.spawn("CHUNK_DROP", move || {
+            drop(deferred_updates);
+        });
     }
 }

voxygen/src/hud/mod.rs

@@ -90,7 +90,7 @@ use common::{
         item::{tool::ToolKind, ItemDesc, MaterialStatManifest, Quality},
         loot_owner::LootOwnerKind,
         pet::is_mountable,
-        skillset::{skills::Skill, SkillGroupKind},
+        skillset::{skills::Skill, SkillGroupKind, SkillsPersistenceError},
         BuffData, BuffKind, Health, Item, MapMarkerChange,
     },
     consts::MAX_PICKUP_RANGE,
@@ -507,6 +507,7 @@ pub struct HudInfo {
     pub mutable_viewpoint: bool,
     pub target_entity: Option<specs::Entity>,
     pub selected_entity: Option<(specs::Entity, Instant)>,
+    pub persistence_load_error: Option<SkillsPersistenceError>,
 }
 
 #[derive(Clone)]
@@ -1290,17 +1291,14 @@ impl Hud {
             // Check if there was a persistence load error of the skillset, and if so
             // display a dialog prompt
             if self.show.prompt_dialog.is_none() {
-                if let Some(skill_set) = skill_sets.get(me) {
+                if let Some(persistence_error) = info.persistence_load_error {
-                    if let Some(persistence_error) = skill_set.persistence_load_error {
-                        use comp::skillset::SkillsPersistenceError;
                     let persistence_error = match persistence_error {
                         SkillsPersistenceError::HashMismatch => {
-                                "There was a difference detected in one of your skill groups since \
+                            "There was a difference detected in one of your skill groups since you \
-                                 you last played."
+                             last played."
                         },
                         SkillsPersistenceError::DeserializationFailure => {
-                                "There was a error in loading some of your skills from the \
+                            "There was a error in loading some of your skills from the database."
-                                 database."
                         },
                         SkillsPersistenceError::SpentExpMismatch => {
                             "The amount of free experience you had in one of your skill groups \
@@ -1326,7 +1324,6 @@ impl Hud {
                     self.show.prompt_dialog = Some(prompt_dialog);
                 }
             }
-            }
 
             if (client.pending_trade().is_some() && !self.show.trade)
                 || (client.pending_trade().is_none() && self.show.trade)

voxygen/src/lib.rs

@@ -80,10 +80,6 @@ pub struct GlobalState {
     // TODO: redo this so that the watcher doesn't have to exist for reloading to occur
     pub i18n: LocalizationHandle,
     pub clipboard: iced_winit::Clipboard,
-    // NOTE: This can be removed from GlobalState if client state behavior is refactored to not
-    // enter the game before confirmation of successful character load
-    /// An error returned by Client that needs to be displayed by the UI
-    pub client_error: Option<String>,
     // Used to clear the shadow textures when entering a PlayState that doesn't utilise shadows
     pub clear_shadows_next_frame: bool,
     /// A channel that sends Discord activity updates to a background task

voxygen/src/main.rs

@@ -223,7 +223,6 @@ fn main() {
         singleplayer: None,
         i18n,
         clipboard,
-        client_error: None,
         clear_shadows_next_frame: false,
         #[cfg(feature = "discord")]
         discord,

voxygen/src/menu/char_selection/mod.rs

@@ -9,10 +9,10 @@ use crate::{
     Direction, GlobalState, PlayState, PlayStateResult,
 };
 use client::{self, Client};
-use common::{comp, resources::DeltaTime};
+use common::{comp, event::UpdateCharacterMetadata, resources::DeltaTime};
 use common_base::span;
 use specs::WorldExt;
-use std::{cell::RefCell, mem, rc::Rc};
+use std::{cell::RefCell, rc::Rc};
 use tracing::error;
 use ui::CharSelectionUi;
 
@@ -81,11 +81,11 @@ impl PlayState for CharSelectionState {
 
     fn tick(&mut self, global_state: &mut GlobalState, events: Vec<WinEvent>) -> PlayStateResult {
         span!(_guard, "tick", "<CharSelectionState as PlayState>::tick");
-        let (client_presence, client_registered) = {
+        let client_registered = {
             let client = self.client.borrow();
-            (client.presence(), client.registered())
+            client.registered()
         };
-        if client_presence.is_none() && client_registered {
+        if client_registered {
             // Handle window events
             for event in events {
                 if self.char_selection_ui.handle_event(event.clone()) {
@@ -135,18 +135,12 @@ impl PlayState for CharSelectionState {
                         self.client.borrow_mut().delete_character(character_id);
                     },
                     ui::Event::Play(character_id) => {
-                        {
                         let mut c = self.client.borrow_mut();
                         let graphics = &global_state.settings.graphics;
                         c.request_character(character_id, common::ViewDistances {
                             terrain: graphics.terrain_view_distance,
                             entity: graphics.entity_view_distance,
                         });
-                        }
-                        return PlayStateResult::Switch(Box::new(SessionState::new(
-                            global_state,
-                            Rc::clone(&self.client),
-                        )));
                     },
                     ui::Event::Spectate => {
                         {
@@ -159,6 +153,7 @@ impl PlayState for CharSelectionState {
                         }
                         return PlayStateResult::Switch(Box::new(SessionState::new(
                             global_state,
+                            UpdateCharacterMetadata::default(),
                             Rc::clone(&self.client),
                         )));
                     },
@@ -210,11 +205,12 @@ impl PlayState for CharSelectionState {
             // Tick the client (currently only to keep the connection alive).
             let localized_strings = &global_state.i18n.read();
 
-            match self.client.borrow_mut().tick(
+            let res = self.client.borrow_mut().tick(
                 comp::ControllerInputs::default(),
                 global_state.clock.dt(),
                 |_| {},
-            ) {
+            );
+            match res {
                 Ok(events) => {
                     for event in events {
                         match event {
@@ -231,8 +227,21 @@ impl PlayState for CharSelectionState {
                                 self.char_selection_ui.select_character(character_id);
                             },
                             client::Event::CharacterError(error) => {
-                                global_state.client_error = Some(error);
+                                self.char_selection_ui.display_error(error);
                             },
+                            client::Event::CharacterJoined(metadata) => {
+                                // NOTE: It's possible we'll lose disconnect messages this way,
+                                // among other things, but oh well.
+                                //
+                                // TODO: Process all messages before returning (from any branch) in
+                                // order to catch disconnect messages.
+                                return PlayStateResult::Switch(Box::new(SessionState::new(
+                                    global_state,
+                                    metadata,
+                                    Rc::clone(&self.client),
+                                )));
+                            },
+                            // TODO: See if we should handle StartSpectate here instead.
                             _ => {},
                         }
                     }
@@ -248,16 +257,12 @@ impl PlayState for CharSelectionState {
                 },
             }
 
-            if let Some(error) = mem::take(&mut global_state.client_error) {
-                self.char_selection_ui.display_error(error);
-            }
-
             // TODO: make sure rendering is not relying on cleaned up stuff
             self.client.borrow_mut().cleanup();
 
             PlayStateResult::Continue
         } else {
-            error!("Client not in pending or registered state. Popping char selection play state");
+            error!("Client not in pending, or registered state. Popping char selection play state");
             // TODO set global_state.info_message
             PlayStateResult::Pop
         }

voxygen/src/menu/char_selection/ui/mod.rs

@@ -261,6 +261,7 @@ enum InfoContent {
     CreatingCharacter,
     EditingCharacter,
     DeletingCharacter,
+    JoiningCharacter,
     CharacterError(String),
 }
 
@@ -783,6 +784,11 @@ impl Controls {
                                 .size(fonts.cyri.scale(24))
                                 .into()
                         },
+                        InfoContent::JoiningCharacter => {
+                            Text::new(i18n.get_msg("char_selection-joining_character"))
+                                .size(fonts.cyri.scale(24))
+                                .into()
+                        },
                         InfoContent::CharacterError(error) => Column::with_children(vec![
                             Text::new(error).size(fonts.cyri.scale(24)).into(),
                             Row::with_children(vec![neat_button(
@@ -1426,14 +1432,57 @@ impl Controls {
             Message::Logout => {
                 events.push(Event::Logout);
             },
+            Message::ConfirmDeletion => {
+                if let Mode::Select { info_content, .. } = &mut self.mode {
+                    if let Some(InfoContent::Deletion(idx)) = info_content {
+                        if let Some(id) = characters.get(*idx).and_then(|i| i.character.id) {
+                            events.push(Event::DeleteCharacter(id));
+                            // Deselect if the selected character was deleted
+                            if Some(id) == self.selected {
+                                self.selected = None;
+                                events.push(Event::SelectCharacter(None));
+                            }
+                        }
+                        *info_content = Some(InfoContent::DeletingCharacter);
+                    }
+                }
+            },
+            Message::CancelDeletion => {
+                if let Mode::Select { info_content, .. } = &mut self.mode {
+                    if let Some(InfoContent::Deletion(_)) = info_content {
+                        *info_content = None;
+                    }
+                }
+            },
+            Message::ClearCharacterListError => {
+                events.push(Event::ClearCharacterListError);
+            },
+            Message::DoNothing => {},
+            _ if matches!(self.mode, Mode::Select {
+                info_content: Some(_),
+                ..
+            }) =>
+            {
+                // Don't allow use of the UI on the select screen to deal with
+                // things other than the event currently being
+                // procesed; all the select screen events after this
+                // modify the info content or selection, except for Spectate
+                // which currently causes us to exit the
+                // character select state.
+            },
             Message::EnterWorld => {
-                if let (Mode::Select { .. }, Some(selected)) = (&self.mode, self.selected) {
+                if let (Mode::Select { info_content, .. }, Some(selected)) =
+                    (&mut self.mode, self.selected)
+                {
                     events.push(Event::Play(selected));
+                    *info_content = Some(InfoContent::JoiningCharacter);
                 }
             },
             Message::Spectate => {
                 if matches!(self.mode, Mode::Select { .. }) {
                     events.push(Event::Spectate);
+                    // FIXME: Enter JoiningCharacter when we have a proper error
+                    // event for spectating.
                 }
             },
             Message::Select(id) => {
@@ -1559,32 +1608,6 @@ impl Controls {
                     );
                 }
             },
-
-            Message::ConfirmDeletion => {
-                if let Mode::Select { info_content, .. } = &mut self.mode {
-                    if let Some(InfoContent::Deletion(idx)) = info_content {
-                        if let Some(id) = characters.get(*idx).and_then(|i| i.character.id) {
-                            events.push(Event::DeleteCharacter(id));
-                            // Deselect if the selected character was deleted
-                            if Some(id) == self.selected {
-                                self.selected = None;
-                                events.push(Event::SelectCharacter(None));
-                            }
-                        }
-                        *info_content = Some(InfoContent::DeletingCharacter);
-                    }
-                }
-            },
-            Message::CancelDeletion => {
-                if let Mode::Select { info_content, .. } = &mut self.mode {
-                    if let Some(InfoContent::Deletion(_)) = info_content {
-                        *info_content = None;
-                    }
-                }
-            },
-            Message::ClearCharacterListError => {
-                events.push(Event::ClearCharacterListError);
-            },
             Message::HairStyle(value) => {
                 if let Mode::CreateOrEdit { body, .. } = &mut self.mode {
                     body.hair_style = value;
@@ -1627,7 +1650,6 @@ impl Controls {
                     body.validate();
                 }
             },
-            Message::DoNothing => {},
         }
     }
 

voxygen/src/session/mod.rs

@@ -21,6 +21,7 @@ use common::{
         ChatMsg, ChatType, InputKind, InventoryUpdateEvent, Pos, Stats, UtteranceKind, Vel,
     },
     consts::MAX_MOUNT_RANGE,
+    event::UpdateCharacterMetadata,
     link::Is,
     mounting::Mount,
     outcome::Outcome,
@@ -71,6 +72,7 @@ enum TickAction {
 pub struct SessionState {
     scene: Scene,
     client: Rc<RefCell<Client>>,
+    metadata: UpdateCharacterMetadata,
     hud: Hud,
     key_state: KeyState,
     inputs: comp::ControllerInputs,
@@ -94,7 +96,11 @@ pub struct SessionState {
 /// Represents an active game session (i.e., the one being played).
 impl SessionState {
     /// Create a new `SessionState`.
-    pub fn new(global_state: &mut GlobalState, client: Rc<RefCell<Client>>) -> Self {
+    pub fn new(
+        global_state: &mut GlobalState,
+        metadata: UpdateCharacterMetadata,
+        client: Rc<RefCell<Client>>,
+    ) -> Self {
         // Create a scene for this session. The scene handles visible elements of the
         // game world.
         let mut scene = Scene::new(
@@ -150,6 +156,7 @@ impl SessionState {
             #[cfg(not(target_os = "macos"))]
             mumble_link,
             hitboxes: HashMap::new(),
+            metadata,
         }
     }
 
@@ -358,9 +365,8 @@ impl SessionState {
                 client::Event::Outcome(outcome) => outcomes.push(outcome),
                 client::Event::CharacterCreated(_) => {},
                 client::Event::CharacterEdited(_) => {},
-                client::Event::CharacterError(error) => {
+                client::Event::CharacterError(_) => {},
-                    global_state.client_error = Some(error);
+                client::Event::CharacterJoined(_) => {},
-                },
                 client::Event::MapMarker(event) => {
                     self.hud.show.update_map_markers(event);
                 },
@@ -1261,6 +1267,7 @@ impl PlayState for SessionState {
                     mutable_viewpoint,
                     target_entity: self.target_entity,
                     selected_entity: self.selected_entity,
+                    persistence_load_error: self.metadata.skill_set_persistence_load_error,
                 },
                 self.interactable,
             );
@@ -1680,9 +1687,7 @@ impl PlayState for SessionState {
                         settings_change.process(global_state, self);
                     },
                     HudEvent::AcknowledgePersistenceLoadError => {
-                        self.client
+                        self.metadata.skill_set_persistence_load_error = None;
-                            .borrow_mut()
-                            .acknolwedge_persistence_load_error();
                     },
                     HudEvent::MapMarkerEvent(event) => {
                         self.client.borrow_mut().map_marker_event(event);