Merge branch 'xvar/rework-char-deletes' into 'master'

Character deletes are now processed by CharacterUpdater and clear any pending...

See merge request veloren/veloren!2128

server/src/lib.rs

@@ -610,11 +610,20 @@ impl Server {
         // 7 Persistence updates
         let before_persistence_updates = Instant::now();
 
-        // Get character-related database responses and notify the requesting client
-        self.state
+        let character_loader = self
+            .state
             .ecs()
-            .read_resource::<persistence::character_loader::CharacterLoader>()
+            .read_resource::<persistence::character_loader::CharacterLoader>();
+
+        let character_updater = self
+            .state
+            .ecs()
+            .read_resource::<persistence::character_updater::CharacterUpdater>();
+
+        // Get character-related database responses and notify the requesting client
+        character_loader
             .messages()
+            .chain(character_updater.messages())
             .for_each(|query_result| match query_result.result {
                 CharacterLoaderResponseKind::CharacterList(result) => match result {
                     Ok(character_list_data) => self.notify_client(
@@ -671,6 +680,9 @@ impl Server {
                 },
             });
 
+        drop(character_loader);
+        drop(character_updater);
+
         {
             // Check for new chunks; cancel and regenerate all chunks if the asset has been
             // reloaded. Note that all of these assignments are no-ops, so the

server/src/persistence/character.rs

@@ -9,7 +9,7 @@ extern crate rusqlite;
 use super::{error::PersistenceError, models::*};
 use crate::{
     comp,
-    comp::{item::MaterialStatManifest, Inventory},
+    comp::Inventory,
     persistence::{
         character::conversions::{
             convert_body_from_database, convert_body_to_database_json,
@@ -101,7 +101,6 @@ pub fn load_character_data(
     requesting_player_uuid: String,
     char_id: CharacterId,
     connection: &mut Transaction,
-    msm: &MaterialStatManifest,
 ) -> CharacterDataResult {
     let character_containers = get_pseudo_containers(connection, char_id)?;
     let inventory_items = load_items_bfs(connection, character_containers.inventory_container_id)?;
@@ -205,7 +204,6 @@ pub fn load_character_data(
             &inventory_items,
             character_containers.loadout_container_id,
             &loadout_items,
-            msm,
         )?,
         char_waypoint,
     ))
@@ -221,7 +219,6 @@ pub fn load_character_data(
 pub fn load_character_list(
     player_uuid_: &str,
     connection: &mut Transaction,
-    msm: &MaterialStatManifest,
 ) -> CharacterListResult {
     let characters;
     {
@@ -282,7 +279,7 @@ pub fn load_character_list(
             let loadout_items = load_items_bfs(connection, loadout_container_id)?;
 
             let loadout =
-                convert_loadout_from_database_items(loadout_container_id, &loadout_items, msm)?;
+                convert_loadout_from_database_items(loadout_container_id, &loadout_items)?;
 
             Ok(CharacterItem {
                 character: char,
@@ -298,7 +295,6 @@ pub fn create_character(
     character_alias: &str,
     persisted_components: PersistedComponents,
     connection: &mut Transaction,
-    msm: &MaterialStatManifest,
 ) -> CharacterCreationResult {
     check_character_limit(uuid, connection)?;
 
@@ -445,7 +441,7 @@ pub fn create_character(
     }
     drop(stmt);
 
-    load_character_list(uuid, connection, msm).map(|list| (character_id, list))
+    load_character_list(uuid, connection).map(|list| (character_id, list))
 }
 
 /// Delete a character. Returns the updated character list.
@@ -453,7 +449,6 @@ pub fn delete_character(
     requesting_player_uuid: &str,
     char_id: CharacterId,
     connection: &mut Transaction,
-    msm: &MaterialStatManifest,
 ) -> CharacterListResult {
     #[rustfmt::skip]
     let mut stmt = connection.prepare_cached("
@@ -549,7 +544,7 @@ pub fn delete_character(
         )));
     }
 
-    load_character_list(requesting_player_uuid, connection, msm)
+    load_character_list(requesting_player_uuid, connection)
 }
 
 /// Before creating a character, we ensure that the limit on the number of

server/src/persistence/character/conversions.rs

@@ -22,6 +22,7 @@ use common::{
 };
 use core::{convert::TryFrom, num::NonZeroU64};
 use hashbrown::HashMap;
+use lazy_static::lazy_static;
 use std::{collections::VecDeque, sync::Arc};
 use tracing::trace;
 
@@ -31,6 +32,13 @@ pub struct ItemModelPair {
     pub model: Item,
 }
 
+// Decoupled from the ECS resource because the plumbing is getting complicated;
+// shouldn't matter unless someone's hot-reloading material stats on the live
+// server
+lazy_static! {
+    pub static ref MATERIAL_STATS_MANIFEST: MaterialStatManifest = MaterialStatManifest::default();
+}
+
 /// Returns a vector that contains all item rows to upsert; parent is
 /// responsible for deleting items from the same owner that aren't affirmatively
 /// kept by this.
@@ -230,7 +238,6 @@ pub fn convert_inventory_from_database_items(
     inventory_items: &[Item],
     loadout_container_id: i64,
     loadout_items: &[Item],
-    msm: &MaterialStatManifest,
 ) -> Result<Inventory, PersistenceError> {
     // Loadout items must be loaded before inventory items since loadout items
     // provide inventory slots. Since items stored inside loadout items actually
@@ -238,7 +245,7 @@ pub fn convert_inventory_from_database_items(
     // on populating the loadout items first, and then inserting the items into the
     // inventory at the correct position.
     //
-    let loadout = convert_loadout_from_database_items(loadout_container_id, loadout_items, msm)?;
+    let loadout = convert_loadout_from_database_items(loadout_container_id, loadout_items)?;
     let mut inventory = Inventory::new_with_loadout(loadout);
     let mut item_indices = HashMap::new();
 
@@ -310,7 +317,7 @@ pub fn convert_inventory_from_database_items(
             }
         } else if let Some(&j) = item_indices.get(&db_item.parent_container_item_id) {
             if let Some(Some(parent)) = inventory.slot_mut(slot(&inventory_items[j].position)?) {
-                parent.add_component(item, msm);
+                parent.add_component(item, &MATERIAL_STATS_MANIFEST);
             } else {
                 return Err(PersistenceError::ConversionError(format!(
                     "Parent slot {} for component {} was empty even though it occurred earlier in \
@@ -332,7 +339,6 @@ pub fn convert_inventory_from_database_items(
 pub fn convert_loadout_from_database_items(
     loadout_container_id: i64,
     database_items: &[Item],
-    msm: &MaterialStatManifest,
 ) -> Result<Loadout, PersistenceError> {
     let loadout_builder = LoadoutBuilder::new();
     let mut loadout = loadout_builder.build();
@@ -367,7 +373,7 @@ pub fn convert_loadout_from_database_items(
         } else if let Some(&j) = item_indices.get(&db_item.parent_container_item_id) {
             loadout
                 .update_item_at_slot_using_persistence_key(&database_items[j].position, |parent| {
-                    parent.add_component(item, msm);
+                    parent.add_component(item, &MATERIAL_STATS_MANIFEST);
                 })
                 .map_err(convert_error)?;
         } else {

server/src/persistence/character_loader.rs

@@ -3,12 +3,8 @@ use crate::persistence::{
     error::PersistenceError,
     establish_connection, DatabaseSettings, PersistedComponents,
 };
-use common::{
-    character::{CharacterId, CharacterItem},
-    comp::inventory::item::MaterialStatManifest,
-};
+use common::character::{CharacterId, CharacterItem};
 use crossbeam_channel::{self, TryIter};
-use lazy_static::lazy_static;
 use rusqlite::Transaction;
 use std::sync::{Arc, RwLock};
 use tracing::{error, trace};
@@ -83,13 +79,6 @@ pub struct CharacterLoader {
     update_tx: crossbeam_channel::Sender<CharacterLoaderRequest>,
 }
 
-// Decoupled from the ECS resource because the plumbing is getting complicated;
-// shouldn't matter unless someone's hot-reloading material stats on the live
-// server
-lazy_static! {
-    pub static ref MATERIAL_STATS_MANIFEST: MaterialStatManifest = MaterialStatManifest::default();
-}
-
 impl CharacterLoader {
     pub fn new(settings: Arc<RwLock<DatabaseSettings>>) -> Result<Self, PersistenceError> {
         let (update_tx, internal_rx) = crossbeam_channel::unbounded::<CharacterLoaderRequest>();
@@ -163,7 +152,6 @@ impl CharacterLoader {
                     &character_alias,
                     persisted_components,
                     &mut transaction,
-                    &MATERIAL_STATS_MANIFEST,
                 )),
                 CharacterLoaderRequestKind::DeleteCharacter {
                     player_uuid,
@@ -172,25 +160,18 @@ impl CharacterLoader {
                     &player_uuid,
                     character_id,
                     &mut transaction,
-                    &MATERIAL_STATS_MANIFEST,
                 )),
                 CharacterLoaderRequestKind::LoadCharacterList { player_uuid } => {
                     CharacterLoaderResponseKind::CharacterList(load_character_list(
                         &player_uuid,
                         &mut transaction,
-                        &MATERIAL_STATS_MANIFEST,
                     ))
                 },
                 CharacterLoaderRequestKind::LoadCharacterData {
                     player_uuid,
                     character_id,
                 } => {
-                    let result = load_character_data(
-                        player_uuid,
-                        character_id,
-                        &mut transaction,
-                        &MATERIAL_STATS_MANIFEST,
-                    );
+                    let result = load_character_data(player_uuid, character_id, &mut transaction);
                     if result.is_err() {
                         error!(
                             ?result,

server/src/persistence/character_updater.rs

@@ -2,9 +2,13 @@ use crate::comp;
 use common::character::CharacterId;
 
 use crate::persistence::{
-    error::PersistenceError, establish_connection, DatabaseSettings, VelorenConnection,
+    character_loader::{CharacterLoaderResponse, CharacterLoaderResponseKind},
+    error::PersistenceError,
+    establish_connection, DatabaseSettings, VelorenConnection,
 };
+use crossbeam_channel::TryIter;
 use rusqlite::DropBehavior;
+use specs::Entity;
 use std::{
     collections::HashMap,
     sync::{
@@ -18,6 +22,11 @@ pub type CharacterUpdateData = (comp::SkillSet, comp::Inventory, Option<comp::Wa
 
 pub enum CharacterUpdaterEvent {
     BatchUpdate(Vec<(CharacterId, CharacterUpdateData)>),
+    DeleteCharacter {
+        entity: Entity,
+        requesting_player_uuid: String,
+        character_id: CharacterId,
+    },
     DisconnectedSuccess,
 }
 
@@ -28,6 +37,7 @@ pub enum CharacterUpdaterEvent {
 /// such as inventory, loadout, etc...
 pub struct CharacterUpdater {
     update_tx: Option<crossbeam_channel::Sender<CharacterUpdaterEvent>>,
+    response_rx: crossbeam_channel::Receiver<CharacterLoaderResponse>,
     handle: Option<std::thread::JoinHandle<()>>,
     pending_logout_updates: HashMap<CharacterId, CharacterUpdateData>,
     /// Will disconnect all characters (without persistence) on the next tick if
@@ -38,6 +48,8 @@ pub struct CharacterUpdater {
 impl CharacterUpdater {
     pub fn new(settings: Arc<RwLock<DatabaseSettings>>) -> rusqlite::Result<Self> {
         let (update_tx, update_rx) = crossbeam_channel::unbounded::<CharacterUpdaterEvent>();
+        let (response_tx, response_rx) = crossbeam_channel::unbounded::<CharacterLoaderResponse>();
+
         let disconnect_all_clients_requested = Arc::new(AtomicBool::new(false));
         let disconnect_all_clients_requested_clone = Arc::clone(&disconnect_all_clients_requested);
 
@@ -68,6 +80,33 @@ impl CharacterUpdater {
                                     .store(true, Ordering::Relaxed);
                             };
                         },
+                        CharacterUpdaterEvent::DeleteCharacter {
+                            entity,
+                            requesting_player_uuid,
+                            character_id,
+                        } => {
+                            match execute_character_delete(
+                                entity,
+                                &requesting_player_uuid,
+                                character_id,
+                                &mut conn,
+                            ) {
+                                Ok(response) => {
+                                    if let Err(e) = response_tx.send(response) {
+                                        error!(?e, "Could not send character deletion response");
+                                    } else {
+                                        debug!(
+                                            "Processed character delete for character ID {}",
+                                            character_id
+                                        );
+                                    }
+                                },
+                                Err(e) => error!(
+                                    "Error deleting character ID {}, error: {:?}",
+                                    character_id, e
+                                ),
+                            }
+                        },
                         CharacterUpdaterEvent::DisconnectedSuccess => {
                             info!(
                                 "CharacterUpdater received DisconnectedSuccess event, resuming \
@@ -84,6 +123,7 @@ impl CharacterUpdater {
 
         Ok(Self {
             update_tx: Some(update_tx),
+            response_rx,
             handle: Some(handle),
             pending_logout_updates: HashMap::new(),
             disconnect_all_clients_requested,
@@ -125,6 +165,32 @@ impl CharacterUpdater {
             .load(Ordering::Relaxed)
     }
 
+    pub fn delete_character(
+        &mut self,
+        entity: Entity,
+        requesting_player_uuid: String,
+        character_id: CharacterId,
+    ) {
+        if let Err(e) =
+            self.update_tx
+                .as_ref()
+                .unwrap()
+                .send(CharacterUpdaterEvent::DeleteCharacter {
+                    entity,
+                    requesting_player_uuid,
+                    character_id,
+                })
+        {
+            error!(?e, "Could not send character deletion request");
+        } else {
+            // Once a delete request has been sent to the channel we must remove any pending
+            // updates for the character in the event that it has recently logged out.
+            // Since the user has actively chosen to delete the character there is no value
+            // in the pending update data anyway.
+            self.pending_logout_updates.remove(&character_id);
+        }
+    }
+
     /// Updates a collection of characters based on their id and components
     pub fn batch_update<'a>(
         &mut self,
@@ -185,6 +251,9 @@ impl CharacterUpdater {
                  future persistence batches from running",
             );
     }
+
+    /// Returns a non-blocking iterator over CharacterLoaderResponse messages
+    pub fn messages(&self) -> TryIter<CharacterLoaderResponse> { self.response_rx.try_iter() }
 }
 
 fn execute_batch_update(
@@ -205,6 +274,30 @@ fn execute_batch_update(
     Ok(())
 }
 
+fn execute_character_delete(
+    entity: Entity,
+    requesting_player_uuid: &str,
+    character_id: CharacterId,
+    connection: &mut VelorenConnection,
+) -> Result<CharacterLoaderResponse, PersistenceError> {
+    let mut transaction = connection.connection.transaction()?;
+
+    let response = CharacterLoaderResponse {
+        entity,
+        result: CharacterLoaderResponseKind::CharacterList(super::character::delete_character(
+            requesting_player_uuid,
+            character_id,
+            &mut transaction,
+        )),
+    };
+
+    if !response.is_err() {
+        transaction.commit()?;
+    };
+
+    Ok(response)
+}
+
 impl Drop for CharacterUpdater {
     fn drop(&mut self) {
         drop(self.update_tx.take());

server/src/sys/msg/character_screen.rs

@@ -13,7 +13,7 @@ use common::{
 };
 use common_ecs::{Job, Origin, Phase, System};
 use common_net::msg::{ClientGeneral, ServerGeneral};
-use specs::{Entities, Join, Read, ReadExpect, ReadStorage};
+use specs::{Entities, Join, Read, ReadExpect, ReadStorage, WriteExpect};
 use std::sync::atomic::Ordering;
 use tracing::{debug, warn};
 
@@ -24,7 +24,7 @@ impl Sys {
         entity: specs::Entity,
         client: &Client,
         character_loader: &ReadExpect<'_, CharacterLoader>,
-        character_updater: &ReadExpect<'_, CharacterUpdater>,
+        character_updater: &mut WriteExpect<'_, CharacterUpdater>,
         uids: &ReadStorage<'_, Uid>,
         players: &ReadStorage<'_, Player>,
         presences: &ReadStorage<'_, Presence>,
@@ -132,7 +132,7 @@ impl Sys {
             },
             ClientGeneral::DeleteCharacter(character_id) => {
                 if let Some(player) = players.get(entity) {
-                    character_loader.delete_character(
+                    character_updater.delete_character(
                         entity,
                         player.uuid().to_string(),
                         character_id,
@@ -154,7 +154,7 @@ impl<'a> System<'a> for Sys {
         Entities<'a>,
         Read<'a, EventBus<ServerEvent>>,
         ReadExpect<'a, CharacterLoader>,
-        ReadExpect<'a, CharacterUpdater>,
+        WriteExpect<'a, CharacterUpdater>,
         ReadStorage<'a, Uid>,
         ReadStorage<'a, Client>,
         ReadStorage<'a, Player>,
@@ -173,7 +173,7 @@ impl<'a> System<'a> for Sys {
             entities,
             server_event_bus,
             character_loader,
-            character_updater,
+            mut character_updater,
             uids,
             clients,
             players,
@@ -191,7 +191,7 @@ impl<'a> System<'a> for Sys {
                     entity,
                     client,
                     &character_loader,
-                    &character_updater,
+                    &mut character_updater,
                     &uids,
                     &players,
                     &presences,