Updates to client-side requests and limiting payloads. Ensuring that

the client only ever requests the full list of achievements once,
and that the server send individual achievement updates as they
happen, which we then merge into that list.

Serialise the AchievementAction for now, would be nice to find a
non-serialised mechanism based on item enums or string ids.

client/src/lib.rs

@@ -72,6 +72,7 @@ pub struct Client {
     pub world_map: (Arc<DynamicImage>, Vec2<u32>),
     pub player_list: HashMap<Uid, PlayerInfo>,
     pub character_list: CharacterList,
+    pub achievement_list: AchievementList,
     pub active_character_id: Option<i32>,
 
     _network: Network,
@@ -103,6 +104,15 @@ pub struct CharacterList {
     pub error: Option<String>,
 }
 
+/// Holds data related to the current character's achievements, as well as some
+/// additional state to handle UI.
+#[derive(Default)]
+pub struct AchievementList {
+    pub achievements: comp::AchievementList,
+    pub loading: bool,
+    pub error: Option<String>,
+}
+
 impl Client {
     /// Create a new `Client`.
     pub fn new<A: Into<SocketAddr>>(addr: A, view_distance: Option<u32>) -> Result<Self, Error> {
@@ -197,6 +207,7 @@ impl Client {
             world_map,
             player_list: HashMap::new(),
             character_list: CharacterList::default(),
+            achievement_list: AchievementList::default(),
             active_character_id: None,
 
             _network: network,
@@ -335,6 +346,21 @@ impl Client {
         // Can't fail
     }
 
+    /// Requests a full achievement list from the server, merged with the
+    /// characters achievements. This only needs to be called once for the
+    /// character, subsequent updates to the characetr's achievements are sent
+    /// from the server and merged into this result.
+    pub fn load_achievements(&mut self) {
+        if let (true, Some(character_id)) = (
+            self.achievement_list.achievements.is_empty(),
+            self.active_character_id,
+        ) {
+            self.singleton_stream
+                .send(ClientMsg::RequestCharacterAchievementList(character_id))
+                .unwrap();
+        }
+    }
+
     pub fn use_slot(&mut self, slot: comp::slot::Slot) {
         self.singleton_stream
             .send(ClientMsg::ControlEvent(ControlEvent::InventoryManip(
@@ -993,15 +1019,16 @@ impl Client {
                     frontend_events.push(Event::SetViewDistance(vd));
                 },
                 ServerMsg::CharacterAchievementDataLoaded(achievement_list) => {
-                    self.state.write_component(self.entity, achievement_list);
+                    self.achievement_list.achievements = achievement_list;
+                    self.achievement_list.loading = false;
                 },
                 ServerMsg::CharacterAchievementDataError(error) => {
-                    // TODO handle somehow
+                    self.achievement_list.loading = false;
-                    tracing::info!(?error, "Failed to load achievements");
+                    self.achievement_list.error = Some(error)
                 },
-                ServerMsg::AchievementCompletion(achievement) => {
+                ServerMsg::AchievementCompletion(_achievement) => {
-                    // TODO handle in UI
+                    // TODO: We receieve a single achievement here, and
-                    tracing::info!(?achievement, "Completed achievement");
+                    // update the client's achievement list
                 },
             }
         }

common/src/comp/achievement.rs

@@ -1,7 +1,8 @@
 use crate::comp::item::{Consumable, Item, ItemKind};
 use hashbrown::HashMap;
+use serde::{Deserialize, Serialize};
 use specs::{Component, Entity, FlaggedStorage};
-use specs_idvs::IDVStorage;
+use specs_idvs::IdvStorage;
 
 /// Used for in-game events that contribute towards player achievements.
 ///
@@ -130,10 +131,12 @@ impl AchievementList {
 }
 
 impl Component for AchievementList {
-    type Storage = FlaggedStorage<Self, IDVStorage<Self>>; // TODO check
+    type Storage = FlaggedStorage<Self, IdvStorage<Self>>; // TODO check
 }
 
 impl AchievementList {
+    pub fn is_empty(&self) -> bool { self.0.is_empty() }
+
     /// Process a single CharacterAchievement item based on the occurance of an
     /// `AchievementEvent`.
     ///
@@ -224,12 +227,12 @@ mod tests {
 
         // The first two increments should not indicate that it is complete
         assert_eq!(
-            achievement_list.process_achievement(&achievement.clone(), &event),
+            achievement_list.process_achievement(&achievement, &event),
             None
         );
 
         assert_eq!(
-            achievement_list.process_achievement(&achievement.clone(), &event),
+            achievement_list.process_achievement(&achievement, &event),
             None
         );
 

common/src/event.rs

@@ -60,7 +60,7 @@ pub enum ServerEvent {
     },
     UpdateCharacterData {
         entity: EcsEntity,
-        components: (i32, comp::Body, comp::Stats, comp::Inventory, comp::Loadout),
+        components: (comp::Body, comp::Stats, comp::Inventory, comp::Loadout),
     },
     ExitIngame {
         entity: EcsEntity,

common/src/lib.rs

@@ -11,7 +11,6 @@
     option_zip
 )]
 
-#[macro_use] extern crate serde_derive;
 pub mod assets;
 pub mod astar;
 pub mod character;

common/src/msg/client.rs

@@ -14,6 +14,7 @@ pub enum ClientMsg {
         token_or_username: String,
     },
     RequestCharacterList,
+    RequestCharacterAchievementList(i32),
     CreateCharacter {
         alias: String,
         tool: Option<String>,

server/data/achievements.ron

@@ -1,62 +1,82 @@
 [
     (
-        id: "9d48b606-f70a-4e7f-a3b7-6630e75ba8fe",
+        uuid: "9d48b606-f70a-4e7f-a3b7-6630e75ba8fe",
         title: "Collect 10 apples",
-        action: CollectConsumable(Apple),
+        action: (
+            CollectConsumable(Apple),
+        ),
         target: 10
     ),
     (
-        id: "ffccb88c-2bb0-41bd-942b-3cbdf9882295",
+        uuid: "ffccb88c-2bb0-41bd-942b-3cbdf9882295",
         title: "Collect 50 apples",
-        action: CollectConsumable(Apple),
+        action: (
+            CollectConsumable(Apple),
+        ),
         target: 50
     ),
     (
-        id: "290c0bc1-ab0a-450e-82d7-6917d9ea7497",
+        uuid: "290c0bc1-ab0a-450e-82d7-6917d9ea7497",
         title: "Pick a mushroom",
-        action: CollectConsumable(Mushroom),
+        action: (
+            CollectConsumable(Mushroom),
+        ),
         target: 1
     ),
     (
-        id: "598aff28-01f6-46a9-a4c1-d75aead98794",
+        uuid: "598aff28-01f6-46a9-a4c1-d75aead98794",
         title: "Kill an NPC",
-        action: KillNpcs,
+        action: (
+            KillNpcs,
+        ),
         target: 1
     ),
     (
-        id: "046e08c5-a512-4ee1-b6e4-76e4e11dd502",
+        uuid: "046e08c5-a512-4ee1-b6e4-76e4e11dd502",
         title: "Kill 10 NPCs",
-        action: KillNpcs,
+        action: (
+            KillNpcs,
+        ),
         target: 10
     ),
     (
-        id: "9c2a326b-c4fa-45bf-9dd6-d4bf9dd85bb5",
+        uuid: "9c2a326b-c4fa-45bf-9dd6-d4bf9dd85bb5",
         title: "Kill 100 NPCs",
-        action: KillNpcs,
+        action: (
+            KillNpcs,
+        ),
         target: 100
     ),
     (
-        id: "de782069-4366-41dc-9645-1c02c64b8037",
+        uuid: "de782069-4366-41dc-9645-1c02c64b8037",
         title: "Kill another player",
-        action: KillPlayers,
+        action: (
+            KillPlayers,
+        ),
         target: 1
     ),
     (
-        id: "df6bf984-efdf-40f7-8bac-e09e9927acba",
+        uuid: "df6bf984-efdf-40f7-8bac-e09e9927acba",
         title: "Kill 10 players",
-        action: KillPlayers,
+        action: (
+            KillPlayers,
+        ),
         target: 10
     ),
     (
-        id: "d733c6cc-57c9-4a22-aea6-13f4850d750f",
+        uuid: "d733c6cc-57c9-4a22-aea6-13f4850d750f",
         title: "Find a Velorite fragment",
-        action: CollectConsumable(VeloriteFrag),
+        action: (
+            CollectConsumable(VeloriteFrag),
+        ),
         target: 1
     ),
     (
-        id: "50140fe9-1811-40c4-bb3b-89503ac60ccf",
+        uuid: "50140fe9-1811-40c4-bb3b-89503ac60ccf",
         title: "Find Velorite",
-        action: CollectConsumable(Velorite),
+        action: (
+            CollectConsumable(Velorite),
+        ),
         target: 1
     )
 ]

server/src/events/entity_creation.rs

@@ -16,7 +16,7 @@ pub fn handle_initialize_character(server: &mut Server, entity: EcsEntity, chara
 pub fn handle_loaded_character_data(
     server: &mut Server,
     entity: EcsEntity,
-    loaded_components: (i32, comp::Body, comp::Stats, comp::Inventory, comp::Loadout),
+    loaded_components: (comp::Body, comp::Stats, comp::Inventory, comp::Loadout),
 ) {
     server
         .state

server/src/events/inventory_manip.rs

@@ -5,7 +5,7 @@ use common::{
         slot::{self, Slot},
         AchievementEvent, AchievementTrigger, Pos, MAX_PICKUP_RANGE_SQR,
     },
-    event::{AchievementEvent, EventBus},
+    event::EventBus,
     sync::{Uid, WorldSyncExt},
     terrain::block::Block,
     vol::{ReadVol, Vox},

server/src/lib.rs

@@ -55,7 +55,7 @@ use std::{
 };
 #[cfg(not(feature = "worldgen"))]
 use test_world::{World, WORLD_SIZE};
-use tracing::{debug, error, info};
+use tracing::{debug, error, info, warn};
 use uvth::{ThreadPool, ThreadPoolBuilder};
 use vek::*;
 #[cfg(feature = "worldgen")]
@@ -265,12 +265,10 @@ impl Server {
         // Sync and Load Achievement Data
         debug!("Syncing Achievement data...");
 
-        // TODO I switched this to return comp::Achievement but that's not right...we
-        // want the id really,
         let achievement_data = match persistence::achievement::sync(&settings.persistence_db_dir) {
             Ok(achievements) => achievements,
             Err(e) => {
-                error!(?e, "Achievement data migration error");
+                warn!(?e, "Achievement data migration error");
 
                 Vec::new()
             },

server/src/migrations/2020-06-11-231641_achievements/down.sql

@@ -1 +1 @@
-DROP TABLE IF EXISTS "achievement";
+DROP TABLE IF EXISTS "achievements";

server/src/migrations/2020-06-11-231641_achievements/up.sql

@@ -1,7 +1,6 @@
 CREATE TABLE IF NOT EXISTS "achievements" (
     "uuid" TEXT PRIMARY KEY NOT NULL,
-    "checksum" TEXT NOT NULL UNIQUE,
     "title" TEXT NOT NULL,
-    "action" INT NOT NULL,
+    "action" TEXT NOT NULL,
     "target" INT NOT NULL
 );

server/src/persistence/achievement.rs

@@ -135,8 +135,6 @@ pub fn sync(db_dir: &str) -> Result<Vec<comp::Achievement>, Error> {
         .filter(schema::data_migration::title.eq(String::from("achievements")))
         .first::<DataMigration>(&connection);
 
-    info!(?result, "result: ");
-
     let should_run = match result {
         Ok(migration_entry) => {
             // If these don't match, we need to sync data
@@ -185,7 +183,7 @@ pub fn sync(db_dir: &str) -> Result<Vec<comp::Achievement>, Error> {
                             .set(item)
                             .execute(&connection)
                             {
-                                Ok(_) => warn!(?existing_item.checksum, "Updated achievement"),
+                                Ok(_) => warn!(?existing_item.uuid, "Updated achievement"),
                                 Err(err) => return Err(Error::DatabaseError(err)),
                             }
                         }

server/src/persistence/character.rs

@@ -49,7 +49,7 @@ enum CharacterLoaderRequestKind {
 }
 
 /// A tuple of the components that are persisted to the DB for each character
-pub type PersistedComponents = (i32, comp::Body, comp::Stats, comp::Inventory, comp::Loadout);
+pub type PersistedComponents = (comp::Body, comp::Stats, comp::Inventory, comp::Loadout);
 
 type CharacterListResult = Result<Vec<CharacterItem>, Error>;
 type CharacterDataResult = Result<PersistedComponents, Error>;
@@ -236,7 +236,6 @@ impl Drop for CharacterLoader {
 fn load_character_data(player_uuid: &str, character_id: i32, db_dir: &str) -> CharacterDataResult {
     let connection = establish_connection(db_dir);
 
-<<<<<<< HEAD
     let result = schema::character::dsl::character
         .filter(schema::character::id.eq(character_id))
         .filter(schema::character::player_uuid.eq(player_uuid))

server/src/persistence/models.rs

@@ -5,7 +5,7 @@ use super::schema::{
 };
 use crate::comp;
 use chrono::NaiveDateTime;
-use common::{achievement::AchievementItem, character::Character as CharacterData};
+use common::character::Character as CharacterData;
 use diesel::sql_types::Text;
 use serde::{Deserialize, Serialize};
 use tracing::warn;
@@ -376,18 +376,55 @@ pub struct NewDataMigration<'a> {
 #[primary_key(uuid)]
 pub struct Achievement {
     pub uuid: String,
-    pub checksum: String,
     pub title: String,
-    pub action: i32,
+    pub action: AchievementActionData,
     pub target: i32,
 }
 
+/// A wrapper type for the AchievementAction JSON column on achievements
+#[derive(AsExpression, Debug, Deserialize, Hash, Serialize, PartialEq, FromSqlRow)]
+#[sql_type = "Text"]
+pub struct AchievementActionData(pub comp::AchievementAction);
+
+impl<DB> diesel::deserialize::FromSql<Text, DB> for AchievementActionData
+where
+    DB: diesel::backend::Backend,
+    String: diesel::deserialize::FromSql<Text, DB>,
+{
+    fn from_sql(
+        bytes: Option<&<DB as diesel::backend::Backend>::RawValue>,
+    ) -> diesel::deserialize::Result<Self> {
+        let t = String::from_sql(bytes)?;
+
+        match serde_json::from_str(&t) {
+            Ok(data) => Ok(Self(data)),
+            Err(e) => {
+                warn!(?e, "Failed to deserialize achievement action data");
+                Ok(Self(comp::AchievementAction::None))
+            },
+        }
+    }
+}
+
+impl<DB> diesel::serialize::ToSql<Text, DB> for AchievementActionData
+where
+    DB: diesel::backend::Backend,
+{
+    fn to_sql<W: std::io::Write>(
+        &self,
+        out: &mut diesel::serialize::Output<W, DB>,
+    ) -> diesel::serialize::Result {
+        let s = serde_json::to_string(&self.0)?;
+        <String as diesel::serialize::ToSql<Text, DB>>::to_sql(&s, out)
+    }
+}
+
 impl From<&Achievement> for comp::Achievement {
     fn from(achievement: &Achievement) -> comp::Achievement {
         comp::Achievement {
             uuid: achievement.uuid.clone(),
             title: achievement.title.clone(),
-            action: comp::AchievementAction::None, // TODO find a way to store this data
+            action: achievement.action.0.clone(),
             target: achievement.target as usize,
         }
     }

server/src/persistence/schema.rs

@@ -1,9 +1,8 @@
 table! {
     achievements (uuid) {
         uuid -> Text,
-        checksum -> Text,
         title -> Text,
-        action -> Integer,
+        action -> Text,
         target -> Integer,
     }
 }

server/src/state_ext.rs

@@ -1,9 +1,6 @@
 use crate::{
-    client::Client,
+    client::Client, persistence::character::PersistedComponents, settings::ServerSettings,
-    persistence::{achievement::AchievementLoader, character::PersistedComponents},
+    sys::sentinel::DeletedEntities, SpawnPoint,
-    settings::ServerSettings,
-    sys::sentinel::DeletedEntities,
-    SpawnPoint,
 };
 use common::{
     comp,
@@ -211,13 +208,7 @@ impl StateExt for State {
     }
 
     fn update_character_data(&mut self, entity: EcsEntity, components: PersistedComponents) {
-        let (character_id, body, stats, inventory, loadout) = components;
+        let (body, stats, inventory, loadout) = components;
-
-        // Now that data essential for loading into the world has returned, kick of a
-        // request for supplemental game data, such as achievements
-        self.ecs()
-            .read_resource::<AchievementLoader>()
-            .load_character_achievement_list(entity, character_id);
 
         // Make sure physics are accepted.
         self.write_component(entity, comp::ForceUpdate);

server/src/sys/message.rs

@@ -41,6 +41,7 @@ impl Sys {
         client: &mut Client,
         cnt: &mut u64,
         character_loader: &ReadExpect<'_, CharacterLoader>,
+        achievement_loader: &ReadExpect<'_, AchievementLoader>,
         terrain: &ReadExpect<'_, TerrainGrid>,
         uids: &ReadStorage<'_, Uid>,
         can_build: &ReadStorage<'_, CanBuild>,
@@ -383,6 +384,11 @@ impl Sys {
                         .get_mut(entity)
                         .map(|s| s.skill_set.unlock_skill_group(skill_group_type));
                 },
+                ClientMsg::RequestCharacterAchievementList(character_id) => {
+                    if players.get(entity).is_some() {
+                        achievement_loader.load_character_achievement_list(entity, character_id)
+                    }
+                },
             }
         }
     }
@@ -490,6 +496,7 @@ impl<'a> System<'a> for Sys {
                     &mut cnt,
 
                     &character_loader,
+                    &achievement_loader,
                     &terrain,
                     &uids,
                     &can_build,
@@ -524,14 +531,9 @@ impl<'a> System<'a> for Sys {
             }
         }
 
-        // Handle new players: Tell all clients to add them to the player list, and load
+        // Handle new players: Tell all clients to add them to the player list
-        // non-critical data for their character
         for entity in new_players {
             if let (Some(uid), Some(player)) = (uids.get(entity), players.get(entity)) {
-                if let Some(character_id) = player.character_id {
-                    achievement_loader.load_character_achievement_list(entity, character_id);
-                }
-
                 let msg = ServerMsg::PlayerListUpdate(PlayerListUpdate::Add(*uid, PlayerInfo {
                     player_alias: player.alias.clone(),
                     is_online: true,