- Update the stats of characters individually, reverting the change with

big combined updates.
- Add a timer to the stats persistence system and change the frequency
that it runs to 10s
- Seperate the loading of character data for the character list during
selection, and the full data we will grab during state creation. Ideally
additional persisted bits can get returned at the same point and added
to the ecs within the same block.

assets/voxygen/i18n/en.ron

@@ -136,6 +136,7 @@ https://account.veloren.net."#,
         "main.login.already_logged_in": "You are already logged into the server.",
         "main.login.network_error": "Network error",
         "main.login.failed_sending_request": "Request to Auth server failed",
+        "main.login.invalid_character": "The selected character is invalid",
         "main.login.client_crashed": "Client crashed",
 
         /// End Main screen section

client/src/error.rs

@@ -12,6 +12,8 @@ pub enum Error {
     AuthErr(String),
     AuthClientError(AuthClientError),
     AuthServerNotTrusted,
+    /// Persisted character data is invalid or missing
+    InvalidCharacter,
     //TODO: InvalidAlias,
     Other(String),
 }

client/src/lib.rs

@@ -225,6 +225,7 @@ impl Client {
                     break Err(match err {
                         RegisterError::AlreadyLoggedIn => Error::AlreadyLoggedIn,
                         RegisterError::AuthError(err) => Error::AuthErr(err),
+                        RegisterError::InvalidCharacter => Error::InvalidCharacter,
                     });
                 },
                 ServerMsg::StateAnswer(Ok(ClientState::Registered)) => break Ok(()),
@@ -234,25 +235,18 @@ impl Client {
     }
 
     /// Request a state transition to `ClientState::Character`.
-    pub fn request_character(
-        &mut self,
-        character_id: i32,
-        body: comp::Body,
-        main: Option<String>,
-        stats: comp::Stats,
-    ) {
+    pub fn request_character(&mut self, character_id: i32, body: comp::Body, main: Option<String>) {
         self.postbox.send_message(ClientMsg::Character {
             character_id,
             body,
             main,
-            stats,
         });
 
         self.client_state = ClientState::Pending;
     }
 
     /// Load the current players character list
-    pub fn load_characters(&mut self) {
+    pub fn load_character_list(&mut self) {
         self.character_list.loading = true;
         self.postbox.send_message(ClientMsg::RequestCharacterList);
     }

common/src/character.rs

@@ -11,11 +11,20 @@ pub struct Character {
     pub tool: Option<String>, // TODO: Remove once we start persisting inventories
 }
 
-/// Represents the character data sent by the server after loading from the
-/// database.
+/// Represents a single character item in the character list presented during
+/// character selection. This is a subset of the full character data used for
+/// presentation purposes.
 #[derive(Clone, Debug, Serialize, Deserialize)]
 pub struct CharacterItem {
     pub character: Character,
+    pub body: comp::Body,
+    pub level: usize,
+}
+
+/// The full representation of the data we store in the database for each
+/// character
+#[derive(Clone, Debug, Serialize, Deserialize)]
+pub struct CharacterData {
     pub body: comp::Body,
     pub stats: comp::Stats,
 }

common/src/event.rs

@@ -95,7 +95,6 @@ pub enum ServerEvent {
         character_id: i32,
         body: comp::Body,
         main: Option<String>,
-        stats: comp::Stats,
     },
     ExitIngame {
         entity: EcsEntity,

common/src/msg/client.rs

@@ -18,7 +18,6 @@ pub enum ClientMsg {
         character_id: i32,
         body: comp::Body,
         main: Option<String>, // Specifier for the weapon
-        stats: comp::Stats,
     },
     /// Request `ClientState::Registered` from an ingame state
     ExitIngame,

common/src/msg/server.rs

@@ -79,6 +79,7 @@ pub enum RequestStateError {
 pub enum RegisterError {
     AlreadyLoggedIn,
     AuthError(String),
+    InvalidCharacter,
     //TODO: InvalidAlias,
 }
 

server/src/events/entity_creation.rs

@@ -15,12 +15,11 @@ pub fn handle_create_character(
     character_id: i32,
     body: Body,
     main: Option<String>,
-    stats: Stats,
 ) {
     let state = &mut server.state;
     let server_settings = &server.server_settings;
 
-    state.create_player_character(entity, character_id, body, main, stats, server_settings);
+    state.create_player_character(entity, character_id, body, main, server_settings);
     sys::subscription::initialize_region_subscription(state.ecs(), entity);
 }
 

server/src/events/mod.rs

@@ -74,8 +74,7 @@ impl Server {
                     character_id,
                     body,
                     main,
-                    stats,
-                } => handle_create_character(self, entity, character_id, body, main, stats),
+                } => handle_create_character(self, entity, character_id, body, main),
                 ServerEvent::ExitIngame { entity } => handle_exit_ingame(self, entity),
                 ServerEvent::CreateNpc {
                     pos,

server/src/lib.rs

@@ -103,12 +103,15 @@ impl Server {
         state.ecs_mut().insert(sys::TerrainSyncTimer::default());
         state.ecs_mut().insert(sys::TerrainTimer::default());
         state.ecs_mut().insert(sys::WaypointTimer::default());
+        state
+            .ecs_mut()
+            .insert(sys::StatsPersistenceTimer::default());
 
         // System schedulers to control execution of systems
         state
             .ecs_mut()
             .insert(sys::StatsPersistenceScheduler::every(Duration::from_secs(
-                60,
+                10,
             )));
 
         // Server-only components
@@ -383,7 +386,13 @@ impl Server {
             .nanos as i64;
         let terrain_nanos = self.state.ecs().read_resource::<sys::TerrainTimer>().nanos as i64;
         let waypoint_nanos = self.state.ecs().read_resource::<sys::WaypointTimer>().nanos as i64;
+        let stats_persistence_nanos = self
+            .state
+            .ecs()
+            .read_resource::<sys::StatsPersistenceTimer>()
+            .nanos as i64;
         let total_sys_ran_in_dispatcher_nanos = terrain_nanos + waypoint_nanos;
+
         // Report timing info
         self.metrics
             .tick_time
@@ -440,6 +449,11 @@ impl Server {
             .tick_time
             .with_label_values(&["waypoint"])
             .set(waypoint_nanos);
+        self.metrics
+            .tick_time
+            .with_label_values(&["persistence:stats"])
+            .set(stats_persistence_nanos);
+
         // Report other info
         self.metrics
             .player_online

server/src/migrations/2020-04-20-072214_stats/up.sql

@@ -1,6 +1,6 @@
 CREATE TABLE "stats" (
     character_id INT NOT NULL PRIMARY KEY,
-    "level" INT NOT NULL DEFAULT 1,
+    level INT NOT NULL DEFAULT 1,
     exp INT NOT NULL DEFAULT 0,
     endurance INT NOT NULL DEFAULT 0,
     fitness INT NOT NULL DEFAULT 0,

server/src/persistence/character.rs

@@ -12,10 +12,32 @@ use diesel::prelude::*;
 
 type CharacterListResult = Result<Vec<CharacterItem>, Error>;
 
-// Loading of characters happens immediately after login, and the data is only
-// for the purpose of rendering the character and their level in the character
-// list.
-pub fn load_characters(player_uuid: &str) -> CharacterListResult {
+/// Load stored data for a character.
+///
+/// After first logging in, and after a character is selected, we fetch this
+/// data for the purpose of inserting their persisted data for the entity.
+pub fn load_character_data(character_id: i32) -> Result<comp::Stats, Error> {
+    let (character_data, body_data, stats_data) = schema::character::dsl::character
+        .filter(schema::character::id.eq(character_id))
+        .inner_join(schema::body::table)
+        .inner_join(schema::stats::table)
+        .first::<(Character, Body, Stats)>(&establish_connection())?;
+
+    Ok(comp::Stats::from(StatsJoinData {
+        alias: &character_data.alias,
+        body: &comp::Body::from(&body_data),
+        stats: &stats_data,
+    }))
+}
+
+/// Loads a list of characters belonging to the player. This data is a small
+/// subset of the character's data, and is used to render the character and
+/// their level in the character list.
+///
+/// In the event that a join fails, for a character (i.e. they lack an entry for
+/// stats, body, etc...) the character is skipped, and no entry will be
+/// returned.
+pub fn load_character_list(player_uuid: &str) -> CharacterListResult {
     let data: Vec<(Character, Body, Stats)> = schema::character::dsl::character
         .filter(schema::character::player_uuid.eq(player_uuid))
         .order(schema::character::id.desc())
@@ -28,22 +50,19 @@ pub fn load_characters(player_uuid: &str) -> CharacterListResult {
         .map(|(character_data, body_data, stats_data)| {
             let character = CharacterData::from(character_data);
             let body = comp::Body::from(body_data);
-            let stats = comp::Stats::from(StatsJoinData {
-                character: &character,
-                body: &body,
-                stats: stats_data,
-            });
+            let level = stats_data.level as usize;
 
             CharacterItem {
                 character,
                 body,
-                stats,
+                level,
             }
         })
         .collect())
 }
 
 /// Create a new character with provided comp::Character and comp::Body data.
+///
 /// Note that sqlite does not support returning the inserted data after a
 /// successful insert. To workaround, we wrap this in a transaction which
 /// inserts, queries for the newly created chaacter id, then uses the character
@@ -117,15 +136,16 @@ pub fn create_character(
         Ok(())
     })?;
 
-    load_characters(uuid)
+    load_character_list(uuid)
 }
 
+/// Delete a character. Returns the updated character list.
 pub fn delete_character(uuid: &str, character_id: i32) -> CharacterListResult {
     use schema::character::dsl::*;
 
     diesel::delete(character.filter(id.eq(character_id))).execute(&establish_connection())?;
 
-    load_characters(uuid)
+    load_character_list(uuid)
 }
 
 fn check_character_limit(uuid: &str) -> Result<(), Error> {

server/src/persistence/error.rs

@@ -8,6 +8,8 @@ pub enum Error {
     CharacterLimitReached,
     // An error occured when performing a database action
     DatabaseError(diesel::result::Error),
+    // Unable to load body or stats for a character
+    CharacterDataError,
 }
 
 impl fmt::Display for Error {
@@ -15,6 +17,7 @@ impl fmt::Display for Error {
         write!(f, "{}", match self {
             Self::DatabaseError(diesel_error) => diesel_error.to_string(),
             Self::CharacterLimitReached => String::from("Character limit exceeded"),
+            Self::CharacterDataError => String::from("Error while loading character data"),
         })
     }
 }

server/src/persistence/models.rs

@@ -2,10 +2,9 @@ use super::schema::{body, character, stats};
 use crate::comp;
 use common::character::Character as CharacterData;
 
-/// When we want to build player stats from database data, we need data from the
-/// character, body and stats tables
+/// The required elements to build comp::Stats from database data
 pub struct StatsJoinData<'a> {
-    pub character: &'a CharacterData,
+    pub alias: &'a str,
     pub body: &'a comp::Body,
     pub stats: &'a Stats,
 }
@@ -88,7 +87,7 @@ pub struct Stats {
 
 impl From<StatsJoinData<'_>> for comp::Stats {
     fn from(data: StatsJoinData) -> comp::Stats {
-        let mut base_stats = comp::Stats::new(String::from(&data.character.alias), *data.body);
+        let mut base_stats = comp::Stats::new(String::from(data.alias), *data.body);
 
         base_stats.level.set_level(data.stats.level as u32);
         base_stats.exp.set_current(data.stats.exp as u32);
@@ -106,7 +105,7 @@ impl From<StatsJoinData<'_>> for comp::Stats {
     }
 }
 
-#[derive(AsChangeset)]
+#[derive(AsChangeset, Debug, PartialEq)]
 #[primary_key(character_id)]
 #[table_name = "stats"]
 pub struct StatsUpdate {
@@ -116,3 +115,44 @@ pub struct StatsUpdate {
     pub fitness: i32,
     pub willpower: i32,
 }
+
+impl From<&comp::Stats> for StatsUpdate {
+    fn from(stats: &comp::Stats) -> StatsUpdate {
+        StatsUpdate {
+            level: stats.level.level() as i32,
+            exp: stats.exp.current() as i32,
+            endurance: stats.endurance as i32,
+            fitness: stats.fitness as i32,
+            willpower: stats.willpower as i32,
+        }
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use crate::comp;
+
+    #[test]
+    fn stats_update_from_stats() {
+        let mut stats = comp::Stats::new(
+            String::from("Test"),
+            comp::Body::Humanoid(comp::humanoid::Body::random()),
+        );
+
+        stats.level.set_level(2);
+        stats.exp.set_current(20);
+
+        stats.endurance = 2;
+        stats.fitness = 3;
+        stats.willpower = 4;
+
+        assert_eq!(StatsUpdate::from(&stats), StatsUpdate {
+            level: 2,
+            exp: 20,
+            endurance: 2,
+            fitness: 3,
+            willpower: 4,
+        })
+    }
+}

server/src/persistence/stats.rs

@@ -1,83 +1,25 @@
 extern crate diesel;
 
-use super::establish_connection;
+use super::{establish_connection, models::StatsUpdate, schema};
 use crate::comp;
 use diesel::prelude::*;
 
-/// Update DB rows for stats given a Vec of (character_id, Stats) tuples
-pub fn update(data: Vec<(i32, &comp::Stats)>) {
-    match establish_connection().execute(&build_query(data)) {
-        Err(diesel_error) => log::warn!("Error updating stats: {:?}", diesel_error),
-        _ => {},
-    }
-}
-
-/// Takes a Vec of (character_id, Stats) tuples and builds an SQL UPDATE query
-/// Since there is apprently no sensible way to update > 1 row using diesel, we
-/// just construct the raw SQL
-fn build_query(data: Vec<(i32, &comp::Stats)>) -> String {
-    data.iter()
-        .map(|(character_id, stats)| {
-            String::from(format!(
-                "UPDATE stats SET level = {}, exp = {}, endurance = {}, fitness = {}, willpower = \
-                 {} WHERE character_id = {};",
-                stats.level.level() as i32,
-                stats.exp.current() as i32,
-                stats.endurance as i32,
-                stats.fitness as i32,
-                stats.willpower as i32,
-                *character_id as i32
-            ))
-        })
-        .collect::<Vec<String>>()
-        .join(" ")
-}
-
-#[cfg(test)]
-mod tests {
-    use super::*;
-
-    #[test]
-    fn builds_query_for_multiple_characters() {
-        let mut stats_one = comp::Stats::new(
-            String::from("One"),
-            comp::Body::Humanoid(comp::humanoid::Body::random()),
-        );
-
-        stats_one.endurance = 1;
-        stats_one.fitness = 1;
-        stats_one.willpower = 1;
-
-        let mut stats_two = comp::Stats::new(
-            String::from("Two"),
-            comp::Body::Humanoid(comp::humanoid::Body::random()),
-        );
-
-        stats_two.endurance = 2;
-        stats_two.fitness = 2;
-        stats_two.willpower = 2;
-
-        let mut stats_three = comp::Stats::new(
-            String::from("Three"),
-            comp::Body::Humanoid(comp::humanoid::Body::random()),
-        );
-
-        stats_three.endurance = 3;
-        stats_three.fitness = 3;
-        stats_three.willpower = 3;
-
-        let data = vec![
-            (1_i32, &stats_one),
-            (2_i32, &stats_two),
-            (3_i32, &stats_three),
-        ];
-
-        assert_eq!(
-            build_query(data),
-            "UPDATE stats SET level = 1, exp = 0, endurance = 1, fitness = 1, willpower = 1 WHERE \
-             character_id = 1; UPDATE stats SET level = 1, exp = 0, endurance = 2, fitness = 2, \
-             willpower = 2 WHERE character_id = 2; UPDATE stats SET level = 1, exp = 0, endurance \
-             = 3, fitness = 3, willpower = 3 WHERE character_id = 3;"
-        );
-    }
+pub fn update<'a>(updates: impl Iterator<Item = (i32, &'a comp::Stats)>) {
+    use schema::stats;
+
+    let connection = establish_connection();
+
+    updates.for_each(|(character_id, stats)| {
+        if let Err(error) =
+            diesel::update(stats::table.filter(schema::stats::character_id.eq(character_id)))
+                .set(&StatsUpdate::from(stats))
+                .execute(&connection)
+        {
+            log::warn!(
+                "Failed to update stats for character: {:?}: {:?}",
+                character_id,
+                error
+            )
+        }
+    });
 }

server/src/state_ext.rs

@@ -1,9 +1,12 @@
-use crate::{client::Client, settings::ServerSettings, sys::sentinel::DeletedEntities, SpawnPoint};
+use crate::{
+    client::Client, persistence, settings::ServerSettings, sys::sentinel::DeletedEntities,
+    SpawnPoint,
+};
 use common::{
     assets,
     comp::{self, item},
     effect::Effect,
-    msg::{ClientState, ServerMsg},
+    msg::{ClientState, RegisterError, RequestStateError, ServerMsg},
     state::State,
     sync::{Uid, WorldSyncExt},
     util::Dir,
@@ -36,7 +39,6 @@ pub trait StateExt {
         character_id: i32,
         body: comp::Body,
         main: Option<String>,
-        stats: comp::Stats,
         server_settings: &ServerSettings,
     );
     fn notify_registered_clients(&self, msg: ServerMsg);
@@ -153,19 +155,39 @@ impl StateExt for State {
     fn create_player_character(
         &mut self,
         entity: EcsEntity,
-        character_id: i32, // TODO
+        character_id: i32,
         body: comp::Body,
         main: Option<String>,
-        stats: comp::Stats,
         server_settings: &ServerSettings,
     ) {
+        // Grab persisted character data from the db and insert their associated
+        // components. If for some reason the data can't be returned (missing
+        // data, DB error), kick the client back to the character select screen.
+        match persistence::character::load_character_data(character_id) {
+            Ok(stats) => self.write_component(entity, stats),
+            Err(error) => {
+                log::warn!(
+                    "{}",
+                    format!(
+                        "Failed to load character data for character_id {}: {}",
+                        character_id, error
+                    )
+                );
+
+                if let Some(client) = self.ecs().write_storage::<Client>().get_mut(entity) {
+                    client.error_state(RequestStateError::RegisterDenied(
+                        RegisterError::InvalidCharacter,
+                    ))
+                }
+            },
+        }
+
         // Give no item when an invalid specifier is given
         let main = main.and_then(|specifier| assets::load_cloned::<comp::Item>(&specifier).ok());
 
         let spawn_point = self.ecs().read_resource::<SpawnPoint>().0;
 
         self.write_component(entity, body);
-        self.write_component(entity, stats);
         self.write_component(entity, comp::Energy::new(1000));
         self.write_component(entity, comp::Controller::default());
         self.write_component(entity, comp::Pos(spawn_point));
@@ -251,6 +273,7 @@ impl StateExt for State {
         ) {
             self.write_component(entity, comp::Admin);
         }
+
         // Tell the client its request was successful.
         if let Some(client) = self.ecs().write_storage::<Client>().get_mut(entity) {
             client.allow_state(ClientState::Character);

server/src/sys/message.rs

@@ -175,7 +175,6 @@ impl<'a> System<'a> for Sys {
                         character_id,
                         body,
                         main,
-                        stats,
                     } => match client.client_state {
                         // Become Registered first.
                         ClientState::Connected => client.error_state(RequestStateError::Impossible),
@@ -201,7 +200,6 @@ impl<'a> System<'a> for Sys {
                                 character_id,
                                 body,
                                 main,
-                                stats,
                             });
                         },
                         ClientState::Character => client.error_state(RequestStateError::Already),
@@ -317,7 +315,7 @@ impl<'a> System<'a> for Sys {
                     },
                     ClientMsg::RequestCharacterList => {
                         if let Some(player) = players.get(entity) {
-                            match persistence::character::load_characters(
+                            match persistence::character::load_character_list(
                                 &player.uuid().to_string(),
                             ) {
                                 Ok(character_list) => {

server/src/sys/mod.rs

@@ -20,6 +20,7 @@ pub type SubscriptionTimer = SysTimer<subscription::Sys>;
 pub type TerrainTimer = SysTimer<terrain::Sys>;
 pub type TerrainSyncTimer = SysTimer<terrain_sync::Sys>;
 pub type WaypointTimer = SysTimer<waypoint::Sys>;
+pub type StatsPersistenceTimer = SysTimer<persistence::stats::Sys>;
 pub type StatsPersistenceScheduler = SysScheduler<persistence::stats::Sys>;
 
 // System names

server/src/sys/persistence/stats.rs

@@ -1,4 +1,7 @@
-use crate::{persistence::stats, sys::SysScheduler};
+use crate::{
+    persistence::stats,
+    sys::{SysScheduler, SysTimer},
+};
 use common::comp::{Player, Stats};
 use specs::{Join, ReadStorage, System, Write};
 
@@ -9,24 +12,20 @@ impl<'a> System<'a> for Sys {
         ReadStorage<'a, Player>,
         ReadStorage<'a, Stats>,
         Write<'a, SysScheduler<Self>>,
+        Write<'a, SysTimer<Self>>,
     );
 
-    fn run(&mut self, (players, player_stats, mut scheduler): Self::SystemData) {
+    fn run(&mut self, (players, player_stats, mut scheduler, mut timer): Self::SystemData) {
         if scheduler.should_run() {
-            let updates: Vec<(i32, &Stats)> = (&players, &player_stats)
-                .join()
-                .filter_map(|(player, stats)| {
-                    if let Some(character_id) = player.character_id {
-                        Some((character_id, stats))
-                    } else {
-                        None
-                    }
-                })
-                .collect::<Vec<_>>();
+            timer.start();
 
-            if !updates.is_empty() {
-                stats::update(updates);
-            }
+            stats::update(
+                (&players, &player_stats)
+                    .join()
+                    .filter_map(|(player, stats)| player.character_id.map(|id| (id, stats))),
+            );
+
+            timer.end();
         }
     }
 }

voxygen/src/menu/char_selection/mod.rs

@@ -40,7 +40,7 @@ impl PlayState for CharSelectionState {
         let mut clock = Clock::start();
 
         // Load the player's character list
-        self.client.borrow_mut().load_characters();
+        self.client.borrow_mut().load_character_list();
 
         let mut current_client_state = self.client.borrow().get_client_state();
         while let ClientState::Pending | ClientState::Registered = current_client_state {
@@ -92,7 +92,6 @@ impl PlayState for CharSelectionState {
                                     character_id,
                                     selected_character.body,
                                     selected_character.character.tool.clone(),
-                                    selected_character.stats.clone(),
                                 );
                             }
                         }

voxygen/src/menu/char_selection/ui.rs

@@ -351,7 +351,7 @@ impl CharSelectionUi {
                         tool: tool.map(|specifier| specifier.to_string()),
                     },
                     body,
-                    stats: comp::Stats::new(String::from(name), body),
+                    level: 1,
                 }])
             },
         }
@@ -803,10 +803,12 @@ impl CharSelectionUi {
                         .color(TEXT_COLOR)
                         .set(self.ids.character_names[i], ui_widgets);
 
-                    Text::new(&self.voxygen_i18n.get("char_selection.level_fmt").replace(
-                        "{level_nb}",
-                        &character_item.stats.level.level().to_string(),
-                    ))
+                    Text::new(
+                        &self
+                            .voxygen_i18n
+                            .get("char_selection.level_fmt")
+                            .replace("{level_nb}", &character_item.level.to_string()),
+                    )
                     .down_from(self.ids.character_names[i], 4.0)
                     .font_size(self.fonts.cyri.scale(17))
                     .font_id(self.fonts.cyri.conrod_id)

voxygen/src/menu/main/mod.rs

@@ -126,6 +126,9 @@ impl PlayState for MainMenuState {
                                     ),
                                     client::AuthClientError::ServerError(_, e) => format!("{}", e),
                                 },
+                                client::Error::InvalidCharacter => {
+                                    localized_strings.get("main.login.invalid_character").into()
+                                },
                             },
                             InitError::ClientCrashed => {
                                 localized_strings.get("main.login.client_crashed").into()