Replace the Achievement system with just plain events which are processed as part of the server tick. Instead of adding Achievement updates to a component belonging to the entity, use an event queue - this allows an entity to have multiple achievement checks in a single tick.

client/src/lib.rs

@@ -997,6 +997,7 @@ impl Client {
                 },
                 ServerMsg::CharacterAchievementDataError(error) => {
                     // TODO handle somehow
+                    tracing::info!(?error, "Failed to load achievements");
                 },
                 ServerMsg::AchievementCompletion(achievement) => {
                     // TODO handle in UI

common/src/comp/achievement.rs

@@ -1,10 +1,16 @@
-use crate::comp::{
+use crate::comp::item::{Consumable, Item, ItemKind};
-    self,
+use specs::{Component, Entity, FlaggedStorage};
-    item::{Consumable, Item, ItemKind},
-};
-use specs::{Component, FlaggedStorage};
 use specs_idvs::IDVStorage;
 
+/// Used for in-game events that contribute towards player achievements.
+///
+/// For example, when an `InventoryManip` is detected, we record that event in
+/// order to process achievements which depend on collecting items.
+pub struct AchievementTrigger {
+    pub entity: Entity,
+    pub event: AchievementEvent,
+}
+
 #[derive(Clone, Debug, Serialize, Deserialize)]
 pub enum AchievementEvent {
     None,
@@ -35,7 +41,7 @@ pub struct AchievementItem {
 }
 
 impl AchievementItem {
-    pub fn matches_event(&self, event: AchievementEvent) -> bool {
+    pub fn matches_event(&self, event: &AchievementEvent) -> bool {
         match event {
             AchievementEvent::KilledNpc => self.action == AchievementAction::KillNpcs,
             AchievementEvent::KilledPlayer => self.action == AchievementAction::KillPlayers,
@@ -51,14 +57,6 @@ impl AchievementItem {
                 _ => false,
             },
             AchievementEvent::None => false,
-            _ => {
-                tracing::warn!(
-                    ?event,
-                    "An AchievementEvent was processed but the event was not handled"
-                );
-
-                false
-            },
         }
     }
 }
@@ -79,10 +77,10 @@ impl Achievement {
     /// incremented by 1. This covers many cases, but using this method allows
     /// handling of unique types of achievements which are not simple
     /// counters for events
-    pub fn increment_progress(&mut self, event: AchievementEvent) -> bool {
+    pub fn increment_progress(&mut self, event: &AchievementEvent) -> bool {
         match event {
             AchievementEvent::LevelUp(level) => {
-                self.progress = level as usize;
+                self.progress = *level as usize;
             },
             _ => self.progress += 1,
         };
@@ -123,7 +121,7 @@ impl AchievementList {
     pub fn process_achievement(
         &mut self,
         achievement: Achievement,
-        event: AchievementEvent,
+        event: &AchievementEvent,
     ) -> bool {
         let id = achievement.id;
 
@@ -131,7 +129,7 @@ impl AchievementList {
             self.0.push(achievement);
         }
 
-        return if let Some(char_achievement) = self.item_by_id(id) {
+        if let Some(char_achievement) = self.item_by_id(id) {
             if char_achievement.completed {
                 return false;
             }
@@ -141,30 +139,10 @@ impl AchievementList {
             tracing::warn!("Failed to find achievement after inserting");
 
             false
-        };
+        }
     }
 }
 
-/// Used as a container for in-game events that contribute towards player
-/// achievements.
-///
-/// For example, when an `InventoryManip` is detected, we record that event in
-/// order to process achievements which depend on collecting items.
-#[derive(Clone, Debug, Serialize, Deserialize)]
-pub struct AchievementUpdate {
-    event: AchievementEvent,
-}
-
-impl AchievementUpdate {
-    pub fn new(event: AchievementEvent) -> Self { Self { event } }
-
-    pub fn event(&self) -> AchievementEvent { self.event.clone() }
-}
-
-impl Component for AchievementUpdate {
-    type Storage = IDVStorage<Self>;
-}
-
 #[cfg(test)]
 mod tests {
     use super::*;
@@ -181,7 +159,7 @@ mod tests {
         let event =
             AchievementEvent::CollectedItem(assets::load_expect_cloned("common.items.apple"));
 
-        assert!(item.matches_event(event));
+        assert!(item.matches_event(&event));
     }
 
     #[test]
@@ -195,7 +173,7 @@ mod tests {
         let event =
             AchievementEvent::CollectedItem(assets::load_expect_cloned("common.items.apple"));
 
-        assert_eq!(item.matches_event(event), false);
+        assert_eq!(item.matches_event(&event), false);
     }
 
     #[test]
@@ -208,7 +186,7 @@ mod tests {
 
         let event = AchievementEvent::LevelUp(3);
 
-        assert_eq!(item.matches_event(event), true);
+        assert_eq!(item.matches_event(&event), true);
     }
 
     #[test]
@@ -233,12 +211,12 @@ mod tests {
 
         // The first two increments should not indicate that it is complete
         assert_eq!(
-            achievement_list.process_achievement(achievement.clone(), event.clone()),
+            achievement_list.process_achievement(achievement.clone(), &event),
             false
         );
 
         assert_eq!(
-            achievement_list.process_achievement(achievement.clone(), event.clone()),
+            achievement_list.process_achievement(achievement.clone(), &event),
             false
         );
 
@@ -246,7 +224,7 @@ mod tests {
 
         // It should return true when completed
         assert_eq!(
-            achievement_list.process_achievement(achievement, event),
+            achievement_list.process_achievement(achievement, &event),
             true
         );
 
@@ -274,7 +252,8 @@ mod tests {
         let mut achievement_list = AchievementList::default();
 
         assert_eq!(
-            achievement_list.process_achievement(achievement.clone(), AchievementEvent::LevelUp(6)),
+            achievement_list
+                .process_achievement(achievement.clone(), &AchievementEvent::LevelUp(6)),
             false
         );
 
@@ -283,7 +262,7 @@ mod tests {
         assert_eq!(achievement_list.0.get(0).unwrap().completed, false);
 
         assert_eq!(
-            achievement_list.process_achievement(achievement, AchievementEvent::LevelUp(10)),
+            achievement_list.process_achievement(achievement, &AchievementEvent::LevelUp(10)),
             true
         );
 
@@ -313,7 +292,7 @@ mod tests {
             AchievementEvent::CollectedItem(assets::load_expect_cloned("common.items.mushroom"));
 
         assert_eq!(
-            achievement_list.process_achievement(achievement, event),
+            achievement_list.process_achievement(achievement, &event),
             false
         );
 

common/src/comp/mod.rs

@@ -23,7 +23,7 @@ mod visual;
 pub use ability::{CharacterAbility, CharacterAbilityType, ItemConfig, Loadout};
 pub use achievement::{
     Achievement, AchievementAction, AchievementEvent, AchievementItem, AchievementList,
-    AchievementUpdate,
+    AchievementTrigger,
 };
 pub use admin::{Admin, AdminList};
 pub use agent::{Agent, Alignment};

common/src/state.rs

@@ -150,7 +150,6 @@ impl State {
         ecs.register::<comp::WaypointArea>();
         ecs.register::<comp::ForceUpdate>();
         ecs.register::<comp::InventoryUpdate>();
-        ecs.register::<comp::AchievementUpdate>();
         ecs.register::<comp::Admin>();
         ecs.register::<comp::Waypoint>();
         ecs.register::<comp::Projectile>();

server/src/events/entity_manipulation.rs

@@ -2,9 +2,10 @@ use crate::{client::Client, Server, SpawnPoint, StateExt};
 use common::{
     assets,
     comp::{
-        self, item::lottery::Lottery, object, AchievementEvent, Alignment, Body, HealthChange,
+        self, item::lottery::Lottery, object, AchievementEvent, AchievementTrigger, Alignment,
-        HealthSource, Player, Stats,
+        Body, HealthChange, HealthSource, Player, Stats,
     },
+    event::EventBus,
     msg::{PlayerListUpdate, ServerMsg},
     state::BlockChange,
     sync::{Uid, WorldSyncExt},
@@ -90,10 +91,10 @@ pub fn handle_destroy(server: &mut Server, entity: EcsEntity, cause: HealthSourc
                         },
                         _ => None,
                     } {
-                        let _ = state
+                        state
                             .ecs()
-                            .write_storage()
+                            .read_resource::<EventBus<AchievementTrigger>>()
-                            .insert(entity, comp::AchievementUpdate::new(event));
+                            .emit_now(AchievementTrigger { entity, event });
                     }
                 });
             }
@@ -339,11 +340,15 @@ pub fn handle_level_up(server: &mut Server, entity: EcsEntity, new_level: u32) {
         .get(entity)
         .expect("Failed to fetch uid component for entity.");
 
-    // Write an achievement update to trigger level achievements
+    // Emit an achievement check for the level up
-    let _ = server.state.ecs().write_storage().insert(
+    server
-        entity,
+        .state
-        comp::AchievementUpdate::new(AchievementEvent::LevelUp(new_level)),
+        .ecs()
-    );
+        .read_resource::<EventBus<AchievementTrigger>>()
+        .emit_now(AchievementTrigger {
+            entity,
+            event: AchievementEvent::LevelUp(new_level),
+        });
 
     server
         .state

server/src/events/inventory_manip.rs

@@ -3,9 +3,9 @@ use common::{
     comp::{
         self, item,
         slot::{self, Slot},
-        AchievementEvent, Pos, MAX_PICKUP_RANGE_SQR,
+        AchievementEvent, AchievementTrigger, Pos, MAX_PICKUP_RANGE_SQR,
     },
-    event::AchievementEvent,
+    event::{AchievementEvent, EventBus},
     sync::{Uid, WorldSyncExt},
     terrain::block::Block,
     vol::{ReadVol, Vox},
@@ -86,10 +86,13 @@ pub fn handle_inventory(server: &mut Server, entity: EcsEntity, manip: comp::Inv
 
                 let item = picked_up_item.unwrap();
 
-                state.write_component(
+                state
-                    entity,
+                    .ecs()
-                    comp::AchievementUpdate::new(AchievementEvent::CollectedItem(item.clone())),
+                    .read_resource::<EventBus<AchievementTrigger>>()
-                );
+                    .emit_now(AchievementTrigger {
+                        entity,
+                        event: AchievementEvent::CollectedItem(item.clone()),
+                    });
 
                 comp::InventoryUpdate::new(comp::InventoryUpdateEvent::Collected(item))
             } else {
@@ -119,12 +122,13 @@ pub fn handle_inventory(server: &mut Server, entity: EcsEntity, manip: comp::Inv
                     && state.try_set_block(pos, Block::empty()).is_some()
                 {
                     comp::Item::try_reclaim_from_block(block).map(|item| {
-                        state.write_component(
+                        state
-                            entity,
+                            .ecs()
-                            comp::AchievementUpdate::new(AchievementEvent::CollectedItem(
+                            .read_resource::<EventBus<AchievementTrigger>>()
-                                item.clone(),
+                            .emit_now(AchievementTrigger {
-                            )),
+                                entity,
-                        );
+                                event: AchievementEvent::CollectedItem(item.clone()),
+                            });
 
                         state.give_item(entity, item);
                     });

server/src/events/mod.rs

@@ -125,4 +125,8 @@ impl Server {
 
         frontend_events
     }
+
+    // pub fn handle_achievement_events(&mut self) -> {
+
+    // }
 }

server/src/lib.rs

@@ -43,7 +43,7 @@ use futures_util::{select, FutureExt};
 use metrics::{ServerMetrics, TickMetrics};
 use network::{Address, Network, Pid};
 use persistence::{
-    achievement::{AchievementLoader, AchievementLoaderResponse, AvailableAchievements},
+    achievement::{Achievement, AchievementLoader, AchievementLoaderResponse},
     character::{CharacterLoader, CharacterLoaderResponseType, CharacterUpdater},
 };
 use specs::{join::Join, Builder, Entity as EcsEntity, RunNow, SystemData, WorldExt};
@@ -89,6 +89,8 @@ pub struct Server {
 
     metrics: ServerMetrics,
     tick_metrics: TickMetrics,
+
+    achievement_data: Vec<Achievement>,
 }
 
 impl Server {
@@ -101,6 +103,9 @@ impl Server {
 
         // Event Emitters
         state.ecs_mut().insert(EventBus::<ServerEvent>::default());
+        state
+            .ecs_mut()
+            .insert(EventBus::<comp::AchievementTrigger>::default());
 
         state.ecs_mut().insert(AuthProvider::new(
             settings.auth_server_address.clone(),
@@ -261,13 +266,14 @@ impl Server {
 
         // TODO I switched this to return comp::Achievement but that's not right...we
         // want the id really,
-        match persistence::achievement::sync(&settings.persistence_db_dir) {
+        let achievement_data = match persistence::achievement::sync(&settings.persistence_db_dir) {
-            Ok(achievements) => {
+            Ok(achievements) => achievements,
-                info!("Achievement data loaded...");
+            Err(e) => {
-                state.ecs_mut().insert(AvailableAchievements(achievements));
+                error!(?e, "Achievement data migration error");
+
+                Vec::new()
             },
-            Err(e) => error!(?e, "Achievement data migration error"),
+        };
-        }
 
         let this = Self {
             state,
@@ -280,6 +286,8 @@ impl Server {
 
             metrics,
             tick_metrics,
+
+            achievement_data,
         };
 
         debug!(?settings, "created veloren server with");
@@ -429,6 +437,43 @@ impl Server {
             }
         }
 
+        // Achievement processing
+        let achievement_events = self
+            .state
+            .ecs()
+            .read_resource::<EventBus<comp::AchievementTrigger>>()
+            .recv_all();
+
+        for trigger in achievement_events {
+            // Get the achievement that matches this event
+            for achievement in &self.achievement_data {
+                let achievement_item = comp::AchievementItem::from(&achievement.details);
+
+                if achievement_item.matches_event(&trigger.event) {
+                    // Calls to `process_achievement` return true to indicate that the
+                    // achievement is complete. In this case, we notify the client to notify them of
+                    // completing the achievement
+                    if let Some(achievement_list) = self
+                        .state
+                        .ecs()
+                        .write_storage::<comp::AchievementList>()
+                        .get_mut(trigger.entity)
+                    {
+                        if achievement_list.process_achievement(
+                            comp::Achievement::from(achievement),
+                            &trigger.event,
+                        ) == true
+                        {
+                            self.notify_client(
+                                trigger.entity,
+                                ServerMsg::AchievementCompletion(achievement_item),
+                            )
+                        }
+                    }
+                }
+            }
+        }
+
         // 7 Persistence updates
         let before_persistence_updates = Instant::now();
 

server/src/persistence/achievement.rs

@@ -21,6 +21,8 @@ use std::{
 };
 use tracing::{error, info, warn};
 
+pub type Achievement = AchievementModel;
+
 /// Available database operations when modifying a player's characetr list
 enum AchievementLoaderRequestKind {
     LoadCharacterAchievementList {
@@ -237,10 +239,3 @@ pub fn hash<T: Hash>(t: &T) -> u64 {
     t.hash(&mut s);
     s.finish()
 }
-
-/// Holds a list of achievements available to players.
-///
-/// This acts as the reference for checks on achievements, and holds id's as
-/// well as details of achievement
-#[derive(Debug)]
-pub struct AvailableAchievements(pub Vec<AchievementModel>);

server/src/sys/achievement.rs

@@ -1,48 +0,0 @@
-use crate::persistence::achievement::AvailableAchievements;
-
-use crate::client::Client;
-use common::{
-    comp::{Achievement, AchievementItem, AchievementList, AchievementUpdate},
-    msg::ServerMsg,
-};
-use specs::{Join, ReadExpect, System, WriteStorage};
-pub struct Sys;
-
-impl<'a> System<'a> for Sys {
-    #[allow(clippy::type_complexity)] // TODO: Pending review in #587
-    type SystemData = (
-        WriteStorage<'a, Client>,
-        WriteStorage<'a, AchievementList>,
-        WriteStorage<'a, AchievementUpdate>,
-        ReadExpect<'a, AvailableAchievements>,
-    );
-
-    fn run(
-        &mut self,
-        (mut clients, mut achievement_lists, mut achievement_updates, available_achievements): Self::SystemData,
-    ) {
-        // TODO filter out achievements which do not care about this event here, then
-        // iterate over them
-        for (client, achievement_list, ach_update) in
-            (&mut clients, &mut achievement_lists, &achievement_updates).join()
-        {
-            (available_achievements.0).iter().for_each(|achievement| {
-                let achievement_item = AchievementItem::from(&achievement.details);
-
-                if achievement_item.matches_event(ach_update.event()) {
-                    // Calls to `process_achievement` return true to indicate that the
-                    // achievement is complete. In this case, we notify the client to notify them of
-                    // completing the achievement
-                    if achievement_list
-                        .process_achievement(Achievement::from(achievement), ach_update.event())
-                        == true
-                    {
-                        client.notify(ServerMsg::AchievementCompletion(achievement_item));
-                    }
-                }
-            });
-        }
-
-        achievement_updates.clear();
-    }
-}

server/src/sys/mod.rs

@@ -1,4 +1,3 @@
-pub mod achievement;
 pub mod entity_sync;
 pub mod message;
 pub mod object;
@@ -54,9 +53,6 @@ pub fn run_sync_systems(ecs: &mut specs::World) {
     // Sync
     terrain_sync::Sys.run_now(ecs);
     entity_sync::Sys.run_now(ecs);
-
-    // Test
-    achievement::Sys.run_now(ecs);
 }
 
 /// Used to schedule systems to run at an interval