From 67231aff9096297d68e8fae42541623dc0016789 Mon Sep 17 00:00:00 2001 From: Joshua Yanovski Date: Thu, 15 Sep 2022 17:11:10 -0700 Subject: [PATCH 1/2] Remove persistence loading error from SkillSet. This is needed (for now) in order to parallelize ingame_chat, because one of the handled messages updates this value on the server. It turns out that the value is not actually used on the server, only the client, so this was mostly a matter of threading this back to the correct place. Additionally, we took the opportunity to modify the UI to not log you into the game until your character was confirmed to be loaded, which was a todo item that lets us simplify some error handling logic and remove stuff from global state. --- assets/voxygen/i18n/en/char_selection.ftl | 3 +- client/src/lib.rs | 14 ++-- common/net/src/msg/client.rs | 2 - common/net/src/msg/server.rs | 7 +- common/src/comp/skillset/mod.rs | 20 ++--- common/src/event.rs | 3 + server/src/client.rs | 4 +- server/src/events/entity_creation.rs | 5 +- server/src/events/mod.rs | 8 +- server/src/lib.rs | 5 +- server/src/persistence/character.rs | 35 +++++---- .../src/persistence/character/conversions.rs | 7 +- server/src/persistence/character_loader.rs | 8 +- server/src/sys/msg/character_screen.rs | 12 +-- server/src/sys/msg/in_game.rs | 5 -- voxygen/src/hud/mod.rs | 65 ++++++++-------- voxygen/src/lib.rs | 4 - voxygen/src/main.rs | 1 - voxygen/src/menu/char_selection/mod.rs | 53 +++++++------ voxygen/src/menu/char_selection/ui/mod.rs | 78 ++++++++++++------- voxygen/src/session/mod.rs | 19 +++-- 21 files changed, 200 insertions(+), 158 deletions(-) diff --git a/assets/voxygen/i18n/en/char_selection.ftl b/assets/voxygen/i18n/en/char_selection.ftl index ab23cff563..ba8aeb28b2 100644 --- a/assets/voxygen/i18n/en/char_selection.ftl +++ b/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... @@ -20,4 +21,4 @@ char_selection-skin = Skin char_selection-eyeshape = Eye Details char_selection-accessories = Accessories char_selection-create_info_name = Your Character needs a name! -char_selection-version_mismatch = WARNING! This server is running a different, possibly incompatible game version. Please update your game. \ No newline at end of file +char_selection-version_mismatch = WARNING! This server is running a different, possibly incompatible game version. Please update your game. diff --git a/client/src/lib.rs b/client/src/lib.rs index 2c41087d7a..472c85e196 100644 --- a/client/src/lib.rs +++ b/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, @@ -106,6 +106,7 @@ pub enum Event { Outcome(Outcome), CharacterCreated(CharacterId), CharacterEdited(CharacterId), + CharacterJoined(UpdateCharacterMetadata), CharacterError(String), MapMarker(comp::MapMarkerUpdate), StartSpectate(Vec3), @@ -846,7 +847,6 @@ impl Client { | ClientGeneral::UnlockSkillGroup(_) | ClientGeneral::RequestPlayerPhysics { .. } | ClientGeneral::RequestLossyTerrainCompression { .. } - | ClientGeneral::AcknowledgePersistenceLoadError | ClientGeneral::UpdateMapMarker(_) | ClientGeneral::SpectatePosition(_) => { #[cfg(feature = "tracy")] @@ -1663,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( @@ -2398,7 +2394,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(); diff --git a/common/net/src/msg/client.rs b/common/net/src/msg/client.rs index 998d7873d7..83953d9609 100644 --- a/common/net/src/msg/client.rs +++ b/common/net/src/msg/client.rs @@ -100,7 +100,6 @@ pub enum ClientGeneral { RequestLossyTerrainCompression { lossy_terrain_compression: bool, }, - AcknowledgePersistenceLoadError, } impl ClientMsg { @@ -141,7 +140,6 @@ impl ClientMsg { | ClientGeneral::UnlockSkillGroup(_) | ClientGeneral::RequestPlayerPhysics { .. } | ClientGeneral::RequestLossyTerrainCompression { .. } - | ClientGeneral::AcknowledgePersistenceLoadError | ClientGeneral::UpdateMapMarker(_) | ClientGeneral::SpectatePosition(_) => { c_type == ClientType::Game && presence.is_some() diff --git a/common/net/src/msg/server.rs b/common/net/src/msg/server.rs index 80d1872d30..e07e9815a6 100644 --- a/common/net/src/msg/server.rs +++ b/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 - CharacterDataLoadError(String), + /// Result of loading character data + CharacterDataLoadResult(Result), /// A list of characters belonging to the a authenticated player was sent CharacterListUpdate(Vec), /// An error occurred while creating or deleting a character @@ -293,7 +294,7 @@ impl ServerMsg { registered && match g { //Character Screen related - ServerGeneral::CharacterDataLoadError(_) + ServerGeneral::CharacterDataLoadResult(_) | ServerGeneral::CharacterListUpdate(_) | ServerGeneral::CharacterActionError(_) | ServerGeneral::CharacterEdited(_) diff --git a/common/src/comp/skillset/mod.rs b/common/src/comp/skillset/mod.rs index a5bf7e8967..be72711b69 100644 --- a/common/src/comp/skillset/mod.rs +++ b/common/src/comp/skillset/mod.rs @@ -238,10 +238,6 @@ pub struct SkillSet { skills: HashMap, 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, } impl Component for SkillSet { @@ -259,7 +255,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 +276,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, mut all_skills: HashMap, SkillsPersistenceError>>, - ) -> Self { + ) -> (Self, Option) { 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,18 +315,16 @@ 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) => { - skillset.persistence_load_error = Some(persistence_error) - }, + Err(persistence_error) => persistence_load_error = Some(persistence_error), } } } - skillset + (skillset, persistence_load_error) } /// Checks if a particular skill group is accessible for an entity diff --git a/common/src/event.rs b/common/src/event.rs index 4455484ace..251e065b52 100644 --- a/common/src/event.rs +++ b/common/src/event.rs @@ -35,6 +35,8 @@ pub enum LocalEvent { CreateOutcome(Outcome), } +pub type UpdateCharacterMetadata = Option; + #[allow(clippy::large_enum_variant)] // TODO: Pending review in #587 #[derive(strum::EnumDiscriminants)] #[strum_discriminants(repr(usize))] @@ -122,6 +124,7 @@ pub enum ServerEvent { comp::ActiveAbilities, Option, ), + metadata: UpdateCharacterMetadata, }, ExitIngame { entity: EcsEntity, diff --git a/server/src/client.rs b/server/src/client.rs index 094b4ad620..b7aba8af39 100644 --- a/server/src/client.rs +++ b/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(_) diff --git a/server/src/events/entity_creation.rs b/server/src/events/entity_creation.rs index 3e0e9f390e..a9c7ada3d1 100644 --- a/server/src/events/entity_creation.rs +++ b/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( diff --git a/server/src/events/mod.rs b/server/src/events/mod.rs index 9b496c77b0..f40834b231 100644 --- a/server/src/events/mod.rs +++ b/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); diff --git a/server/src/lib.rs b/server/src/lib.rs index a50616c3c3..0b23f51e5e 100644 --- a/server/src/lib.rs +++ b/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 diff --git a/server/src/persistence/character.rs b/server/src/persistence/character.rs index 60f79e0f02..6344d20fc8 100644 --- a/server/src/persistence/character.rs +++ b/server/src/persistence/character.rs @@ -254,21 +254,26 @@ pub fn load_character_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), - inventory: convert_inventory_from_database_items( - character_containers.inventory_container_id, - &inventory_items, - character_containers.loadout_container_id, - &loadout_items, - )?, - waypoint: char_waypoint, - pets, - active_abilities: convert_active_abilities_from_database(&ability_set_data), - map_marker: char_map_marker, - }) + 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, + inventory: convert_inventory_from_database_items( + character_containers.inventory_container_id, + &inventory_items, + character_containers.loadout_container_id, + &loadout_items, + )?, + waypoint: char_waypoint, + pets, + active_abilities: convert_active_abilities_from_database(&ability_set_data), + map_marker: char_map_marker, + }, + skill_set_persistence_load_error, + )) } /// Loads a list of characters belonging to the player. This data is a small diff --git a/server/src/persistence/character/conversions.rs b/server/src/persistence/character/conversions.rs index 2c12b05d14..922d604832 100644 --- a/server/src/persistence/character/conversions.rs +++ b/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) { let (skillless_skill_groups, deserialized_skills) = convert_skill_groups_from_database(skill_groups); SkillSet::load_from_database(skillless_skill_groups, deserialized_skills) diff --git a/server/src/persistence/character_loader.rs b/server/src/persistence/character_loader.rs index 1e496255c0..e4b98b3fc1 100644 --- a/server/src/persistence/character_loader.rs +++ b/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, PersistenceErro pub(crate) type CharacterCreationResult = Result<(CharacterId, Vec), PersistenceError>; pub(crate) type CharacterEditResult = Result<(CharacterId, Vec), PersistenceError>; -pub(crate) type CharacterDataResult = Result; +pub(crate) type CharacterDataResult = + Result<(PersistedComponents, UpdateCharacterMetadata), PersistenceError>; type CharacterLoaderRequest = (specs::Entity, CharacterLoaderRequestKind); /// Available database operations when modifying a player's character list diff --git a/server/src/sys/msg/character_screen.rs b/server/src/sys/msg/character_screen.rs index c2be1a0ffd..2511e61253 100644 --- a/server/src/sys/msg/character_screen.rs +++ b/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 => { diff --git a/server/src/sys/msg/in_game.rs b/server/src/sys/msg/in_game.rs index 95d92bee8b..4a98d5f95d 100644 --- a/server/src/sys/msg/in_game.rs +++ b/server/src/sys/msg/in_game.rs @@ -276,11 +276,6 @@ 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 }); }, diff --git a/voxygen/src/hud/mod.rs b/voxygen/src/hud/mod.rs index a6e96c96cd..b54ec6a1b7 100644 --- a/voxygen/src/hud/mod.rs +++ b/voxygen/src/hud/mod.rs @@ -92,6 +92,7 @@ use common::{ BuffData, BuffKind, Health, Item, MapMarkerChange, }, consts::MAX_PICKUP_RANGE, + event::UpdateCharacterMetadata, link::Is, mounting::Mount, outcome::Outcome, @@ -505,6 +506,7 @@ pub struct HudInfo { pub mutable_viewpoint: bool, pub target_entity: Option, pub selected_entity: Option<(specs::Entity, Instant)>, + pub persistence_load_error: UpdateCharacterMetadata, } #[derive(Clone)] @@ -1288,41 +1290,38 @@ 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) = 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 \ - you last played." - }, - SkillsPersistenceError::DeserializationFailure => { - "There was a error in loading some of your skills from the \ - database." - }, - SkillsPersistenceError::SpentExpMismatch => { - "The amount of free experience you had in one of your skill groups \ - differed from when you last played." - }, - SkillsPersistenceError::SkillsUnlockFailed => { - "Your skills were not able to be obtained in the same order you \ - acquired them. Prerequisites or costs may have changed." - }, - }; + 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 \ + last played." + }, + SkillsPersistenceError::DeserializationFailure => { + "There was a error in loading some of your skills from the database." + }, + SkillsPersistenceError::SpentExpMismatch => { + "The amount of free experience you had in one of your skill groups \ + differed from when you last played." + }, + SkillsPersistenceError::SkillsUnlockFailed => { + "Your skills were not able to be obtained in the same order you \ + acquired them. Prerequisites or costs may have changed." + }, + }; - let common_message = "Some of your skill points have been reset. You will \ - need to reassign them."; + let common_message = "Some of your skill points have been reset. You will \ + need to reassign them."; - warn!("{}\n{}", persistence_error, common_message); - let prompt_dialog = PromptDialogSettings::new( - format!("{}\n", common_message), - Event::AcknowledgePersistenceLoadError, - None, - ) - .with_no_negative_option(); - // self.set_prompt_dialog(prompt_dialog); - self.show.prompt_dialog = Some(prompt_dialog); - } + warn!("{}\n{}", persistence_error, common_message); + let prompt_dialog = PromptDialogSettings::new( + format!("{}\n", common_message), + Event::AcknowledgePersistenceLoadError, + None, + ) + .with_no_negative_option(); + // self.set_prompt_dialog(prompt_dialog); + self.show.prompt_dialog = Some(prompt_dialog); } } diff --git a/voxygen/src/lib.rs b/voxygen/src/lib.rs index d43ce0c85e..a2b9efc1c8 100644 --- a/voxygen/src/lib.rs +++ b/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, // 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 diff --git a/voxygen/src/main.rs b/voxygen/src/main.rs index 910fa897af..e96a7de641 100644 --- a/voxygen/src/main.rs +++ b/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, diff --git a/voxygen/src/menu/char_selection/mod.rs b/voxygen/src/menu/char_selection/mod.rs index 4100df4a1d..f29901aaa9 100644 --- a/voxygen/src/menu/char_selection/mod.rs +++ b/voxygen/src/menu/char_selection/mod.rs @@ -12,7 +12,7 @@ use client::{self, Client}; use common::{comp, 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) -> PlayStateResult { span!(_guard, "tick", "::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), - ))); + 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, + }); }, ui::Event::Spectate => { { @@ -159,6 +153,7 @@ impl PlayState for CharSelectionState { } return PlayStateResult::Switch(Box::new(SessionState::new( global_state, + None, 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,18 @@ 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. + 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 +254,15 @@ 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 character screen, pending, or registered state. Popping char \ + selection play state" + ); // TODO set global_state.info_message PlayStateResult::Pop } diff --git a/voxygen/src/menu/char_selection/ui/mod.rs b/voxygen/src/menu/char_selection/ui/mod.rs index 97e1b2a1e6..865e543b46 100644 --- a/voxygen/src/menu/char_selection/ui/mod.rs +++ b/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 => {}, } } diff --git a/voxygen/src/session/mod.rs b/voxygen/src/session/mod.rs index e1f4e2898b..91e36687d4 100644 --- a/voxygen/src/session/mod.rs +++ b/voxygen/src/session/mod.rs @@ -24,6 +24,7 @@ use common::{ ChatMsg, ChatType, InputKind, InventoryUpdateEvent, Pos, Stats, UtteranceKind, Vel, }, consts::MAX_MOUNT_RANGE, + event::UpdateCharacterMetadata, link::Is, mounting::Mount, outcome::Outcome, @@ -74,6 +75,7 @@ enum TickAction { pub struct SessionState { scene: Scene, client: Rc>, + metadata: UpdateCharacterMetadata, hud: Hud, key_state: KeyState, inputs: comp::ControllerInputs, @@ -97,7 +99,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>) -> Self { + pub fn new( + global_state: &mut GlobalState, + metadata: UpdateCharacterMetadata, + client: Rc>, + ) -> Self { // Create a scene for this session. The scene handles visible elements of the // game world. let mut scene = Scene::new( @@ -153,6 +159,7 @@ impl SessionState { #[cfg(not(target_os = "macos"))] mumble_link, hitboxes: HashMap::new(), + metadata, } } @@ -355,9 +362,8 @@ impl SessionState { client::Event::Outcome(outcome) => outcomes.push(outcome), client::Event::CharacterCreated(_) => {}, client::Event::CharacterEdited(_) => {}, - client::Event::CharacterError(error) => { - global_state.client_error = Some(error); - }, + client::Event::CharacterError(_) => {}, + client::Event::CharacterJoined(_) => {}, client::Event::MapMarker(event) => { self.hud.show.update_map_markers(event); }, @@ -1258,6 +1264,7 @@ impl PlayState for SessionState { mutable_viewpoint, target_entity: self.target_entity, selected_entity: self.selected_entity, + persistence_load_error: self.metadata, }, self.interactable, ); @@ -1677,9 +1684,7 @@ impl PlayState for SessionState { settings_change.process(global_state, self); }, HudEvent::AcknowledgePersistenceLoadError => { - self.client - .borrow_mut() - .acknolwedge_persistence_load_error(); + self.metadata = None; }, HudEvent::MapMarkerEvent(event) => { self.client.borrow_mut().map_marker_event(event); From 1c14ec7ee1c6e3de73539f52916d531c915f5277 Mon Sep 17 00:00:00 2001 From: Joshua Yanovski Date: Thu, 15 Sep 2022 19:38:13 -0700 Subject: [PATCH 2/2] Parallelize ingame messages. --- client/src/lib.rs | 1 - common/net/src/msg/client.rs | 10 +- common/src/comp/skillset/mod.rs | 58 ++++-- common/src/event.rs | 6 +- common/src/lib.rs | 1 + common/src/resources.rs | 2 +- server/src/persistence/character.rs | 9 +- server/src/sys/msg/in_game.rs | 263 +++++++++++++++++-------- voxygen/src/hud/mod.rs | 6 +- voxygen/src/menu/char_selection/mod.rs | 12 +- voxygen/src/session/mod.rs | 4 +- 11 files changed, 250 insertions(+), 122 deletions(-) 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);