diff --git a/client/src/lib.rs b/client/src/lib.rs index 472c85e196..8c0e47ec20 100644 --- a/client/src/lib.rs +++ b/client/src/lib.rs @@ -844,7 +844,6 @@ impl Client { | ClientGeneral::PlayerPhysics { .. } | ClientGeneral::UnlockSkill(_) | ClientGeneral::RequestSiteInfo(_) - | ClientGeneral::UnlockSkillGroup(_) | ClientGeneral::RequestPlayerPhysics { .. } | ClientGeneral::RequestLossyTerrainCompression { .. } | ClientGeneral::UpdateMapMarker(_) diff --git a/common/net/src/msg/client.rs b/common/net/src/msg/client.rs index 83953d9609..e91f728d02 100644 --- a/common/net/src/msg/client.rs +++ b/common/net/src/msg/client.rs @@ -1,11 +1,5 @@ use super::{world_msg::SiteId, PingMsg}; -use common::{ - character::CharacterId, - comp, - comp::{Skill, SkillGroupKind}, - terrain::block::Block, - ViewDistances, -}; +use common::{character::CharacterId, comp, comp::Skill, 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), @@ -137,7 +130,6 @@ impl ClientMsg { | ClientGeneral::LodZoneRequest { .. } | ClientGeneral::UnlockSkill(_) | ClientGeneral::RequestSiteInfo(_) - | ClientGeneral::UnlockSkillGroup(_) | ClientGeneral::RequestPlayerPhysics { .. } | ClientGeneral::RequestLossyTerrainCompression { .. } | ClientGeneral::UpdateMapMarker(_) diff --git a/common/src/comp/skillset/mod.rs b/common/src/comp/skillset/mod.rs index be72711b69..4c9813d6ab 100644 --- a/common/src/comp/skillset/mod.rs +++ b/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}; @@ -327,15 +328,21 @@ impl 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)); @@ -458,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, + C: BorrowMut, + { if let Some(skill_group_kind) = skill.skill_group_kind() { - let next_level = self.next_skill_level(skill); - let prerequisites_met = self.prerequisites_met(skill); + let this = (&*this_).borrow(); + 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 let Some(mut skill_group) = self.skill_group_mut(skill_group_kind) { + if !matches!(this.skills.get(&skill), Some(level) if *level == skill.max_level()) { + 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"); @@ -511,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)| { diff --git a/common/src/event.rs b/common/src/event.rs index 251e065b52..cc76203441 100644 --- a/common/src/event.rs +++ b/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,7 +36,10 @@ pub enum LocalEvent { CreateOutcome(Outcome), } -pub type UpdateCharacterMetadata = Option; +#[derive(Clone, Debug, Default, Deserialize, Serialize)] +pub struct UpdateCharacterMetadata { + pub skill_set_persistence_load_error: Option, +} #[allow(clippy::large_enum_variant)] // TODO: Pending review in #587 #[derive(strum::EnumDiscriminants)] diff --git a/common/src/lib.rs b/common/src/lib.rs index 6cf1b69302..e10df670f3 100644 --- a/common/src/lib.rs +++ b/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, diff --git a/common/src/resources.rs b/common/src/resources.rs index 5b0061aa8b..7e984ed569 100644 --- a/common/src/resources.rs +++ b/common/src/resources.rs @@ -41,7 +41,7 @@ pub enum GameMode { #[derive(Copy, Clone, Default, Debug)] pub struct PlayerEntity(pub Option); -#[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) diff --git a/server/src/persistence/character.rs b/server/src/persistence/character.rs index 6344d20fc8..508f8bed60 100644 --- a/server/src/persistence/character.rs +++ b/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}; @@ -272,7 +275,9 @@ pub fn load_character_data( active_abilities: convert_active_abilities_from_database(&ability_set_data), map_marker: char_map_marker, }, - skill_set_persistence_load_error, + UpdateCharacterMetadata { + skill_set_persistence_load_error, + }, )) } diff --git a/server/src/sys/msg/in_game.rs b/server/src/sys/msg/in_game.rs index 4a98d5f95d..6c50879800 100644 --- a/server/src/sys/msg/in_game.rs +++ b/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>; #[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>, force_updates: &ReadStorage<'_, ForceUpdate>, - skill_sets: &mut WriteStorage<'_, SkillSet>, + skill_set: &mut Option>, healths: &ReadStorage<'_, Health>, - block_changes: &mut Write<'_, BlockChange>, - positions: &mut WriteStorage<'_, Pos>, - velocities: &mut WriteStorage<'_, Vel>, - orientations: &mut WriteStorage<'_, Ori>, - controllers: &mut WriteStorage<'_, Controller>, + rare_writes: &parking_lot::Mutex>, + position: Option<&mut Pos>, + velocity: Option<&mut Vel>, + orientation: Option<&mut Ori>, + controller: Option<&mut Controller>, settings: &Read<'_, Settings>, build_areas: &Read<'_, BuildAreas>, - player_physics_settings: &mut Write<'_, PlayerPhysicsSettings>, - _terrain_persistence: &mut TerrainPersistenceData<'_>, - maybe_player: &Option<&Player>, + player_physics_setting: Option<&mut PlayerPhysicsSetting>, 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); - let _ = velocities.get_mut(entity).map(|v| *v = vel); - let _ = orientations.get_mut(entity).map(|o| *o = ori); + position.map(|p| *p = pos); + velocity.map(|v| *v = vel); + 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 - .get_mut(entity) - .map(|mut skill_set| skill_set.unlock_skill(skill)); - }, - ClientGeneral::UnlockSkillGroup(skill_group_kind) => { - skill_sets - .get_mut(entity) - .map(|mut skill_set| skill_set.unlock_skill_group(skill_group_kind)); + // FIXME: How do we want to handle the error? Probably not by swallowing it. + let _ = skill_set.as_mut().map(|skill_set| { + SkillSet::unlock_skill_cow(skill_set, skill, |skill_set| skill_set.to_mut()) + }).transpose(); }, 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; } @@ -281,7 +282,7 @@ impl Sys { }, 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; } } @@ -317,6 +318,7 @@ impl<'a> System<'a> for Sys { Entities<'a>, Read<'a, EventBus>, ReadExpect<'a, TerrainGrid>, + ReadExpect<'a, SlowJobPool>, ReadStorage<'a, CanBuild>, ReadStorage<'a, ForceUpdate>, ReadStorage<'a, Is>, @@ -347,6 +349,7 @@ impl<'a> System<'a> for Sys { entities, server_event_bus, terrain, + slow_jobs, can_build, force_updates, is_rider, @@ -361,62 +364,152 @@ 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() - { - // 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 _ = super::try_recv_all(client, 2, |client, msg| { - Self::handle_client_in_game_msg( - &mut server_emitter, + // 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 clearable_maybe_presence, - &terrain, - &can_build, - &is_rider, - &force_updates, - &mut skill_sets, - &healths, - &mut block_changes, - &mut positions, - &mut velocities, - &mut orientations, - &mut controllers, - &settings, - &build_areas, - &mut player_physics_settings, - &mut terrain_persistence, - &player, - &maybe_admin, - time_for_vd_changes, - msg, - ) - }); + 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( + server_emitter, + entity, + client, + &mut clearable_maybe_presence, + &terrain, + &can_build, + &is_rider, + &force_updates, + &mut skill_set, + &healths, + &rare_writes, + pos.as_deref_mut(), + vel.as_deref_mut(), + ori.as_deref_mut(), + controller.as_deref_mut(), + &settings, + &build_areas, + new_player_physics_setting.as_mut(), + &maybe_admin, + time_for_vd_changes, + msg, + ) + }); - // Ensure deferred view distance changes are applied (if the - // requsite time has elapsed). - if let Some(presence) = maybe_presence { - presence.terrain_view_distance.update(time_for_vd_changes); - presence.entity_view_distance.update(time_for_vd_changes); - } - } + // Ensure deferred view distance changes are applied (if the + // requsite time has elapsed). + if let Some(presence) = maybe_presence { + 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::>(); + 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); + }); } } diff --git a/voxygen/src/hud/mod.rs b/voxygen/src/hud/mod.rs index b54ec6a1b7..8e777850c6 100644 --- a/voxygen/src/hud/mod.rs +++ b/voxygen/src/hud/mod.rs @@ -88,11 +88,10 @@ 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, - event::UpdateCharacterMetadata, link::Is, mounting::Mount, outcome::Outcome, @@ -506,7 +505,7 @@ pub struct HudInfo { pub mutable_viewpoint: bool, pub target_entity: Option, pub selected_entity: Option<(specs::Entity, Instant)>, - pub persistence_load_error: UpdateCharacterMetadata, + pub persistence_load_error: Option, } #[derive(Clone)] @@ -1291,7 +1290,6 @@ impl Hud { // display a dialog prompt if self.show.prompt_dialog.is_none() { if let Some(persistence_error) = info.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 you \ diff --git a/voxygen/src/menu/char_selection/mod.rs b/voxygen/src/menu/char_selection/mod.rs index f29901aaa9..24ea77dfae 100644 --- a/voxygen/src/menu/char_selection/mod.rs +++ b/voxygen/src/menu/char_selection/mod.rs @@ -9,7 +9,7 @@ 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, rc::Rc}; @@ -153,7 +153,7 @@ impl PlayState for CharSelectionState { } return PlayStateResult::Switch(Box::new(SessionState::new( global_state, - None, + UpdateCharacterMetadata::default(), Rc::clone(&self.client), ))); }, @@ -232,6 +232,9 @@ impl PlayState for CharSelectionState { 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, @@ -259,10 +262,7 @@ impl PlayState for CharSelectionState { PlayStateResult::Continue } else { - error!( - "Client not in character screen, 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 } diff --git a/voxygen/src/session/mod.rs b/voxygen/src/session/mod.rs index 91e36687d4..85afcd2791 100644 --- a/voxygen/src/session/mod.rs +++ b/voxygen/src/session/mod.rs @@ -1264,7 +1264,7 @@ impl PlayState for SessionState { mutable_viewpoint, target_entity: self.target_entity, selected_entity: self.selected_entity, - persistence_load_error: self.metadata, + persistence_load_error: self.metadata.skill_set_persistence_load_error, }, self.interactable, ); @@ -1684,7 +1684,7 @@ impl PlayState for SessionState { settings_change.process(global_state, self); }, HudEvent::AcknowledgePersistenceLoadError => { - self.metadata = None; + self.metadata.skill_set_persistence_load_error = None; }, HudEvent::MapMarkerEvent(event) => { self.client.borrow_mut().map_marker_event(event);