Refactored skills code.

client/src/lib.rs

@@ -753,7 +753,6 @@ impl Client {
                     | ClientGeneral::ExitInGame
                     | ClientGeneral::PlayerPhysics { .. }
                     | ClientGeneral::UnlockSkill(_)
-                    | ClientGeneral::RefundSkill(_)
                     | ClientGeneral::RequestSiteInfo(_)
                     | ClientGeneral::UnlockSkillGroup(_)
                     | ClientGeneral::RequestPlayerPhysics { .. }

common/net/src/msg/client.rs

@@ -76,7 +76,6 @@ pub enum ClientGeneral {
         ori: comp::Ori,
     },
     UnlockSkill(Skill),
-    RefundSkill(Skill),
     UnlockSkillGroup(SkillGroupKind),
     RequestSiteInfo(SiteId),
     //Only in Game, via terrain stream
@@ -128,7 +127,6 @@ impl ClientMsg {
                         | ClientGeneral::PlayerPhysics { .. }
                         | ClientGeneral::TerrainChunkRequest { .. }
                         | ClientGeneral::UnlockSkill(_)
-                        | ClientGeneral::RefundSkill(_)
                         | ClientGeneral::RequestSiteInfo(_)
                         | ClientGeneral::UnlockSkillGroup(_)
                         | ClientGeneral::RequestPlayerPhysics { .. }

common/src/comp/skillset/mod.rs

@@ -145,6 +145,8 @@ pub struct SkillGroup {
     pub earned_exp: u32,
     pub available_sp: u16,
     pub earned_sp: u16,
+    // Used for persistence
+    pub ordered_skills: Vec<Skill>,
 }
 
 impl SkillGroup {
@@ -155,6 +157,7 @@ impl SkillGroup {
             earned_exp: 0,
             available_sp: 0,
             earned_sp: 0,
+            ordered_skills: Vec::new(),
         }
     }
 
@@ -183,7 +186,6 @@ impl SkillGroup {
 pub struct SkillSet {
     pub skill_groups: Vec<SkillGroup>,
     pub skills: HashMap<Skill, Option<u16>>,
-    pub ordered_skills: Vec<Skill>,
     pub modify_health: bool,
     pub modify_energy: bool,
 }
@@ -203,7 +205,6 @@ impl Default for SkillSet {
                 SkillGroup::new(SkillGroupKind::Weapon(ToolKind::Pick)),
             ],
             skills: HashMap::new(),
-            ordered_skills: Vec::new(),
             modify_health: false,
             modify_energy: false,
         }
@@ -229,70 +230,41 @@ impl SkillSet {
         }
     }
 
-    /// Unlocks a skill for a player, assuming they have the relevant skill
+    /// Returns a reference to a particular skill group in a skillset
-    /// group unlocked and available SP in that skill group.
+    fn skill_group(&self, skill_group: SkillGroupKind) -> Option<&SkillGroup> {
-    pub fn unlock_skill(&mut self, skill: Skill) {
+        self.skill_groups
-        if let Some(skill_group_kind) = skill.skill_group_kind() {
+            .iter()
-            let next_level = self.next_skill_level(skill);
+            .find(|s_g| s_g.skill_group_kind == skill_group)
-            let prerequisites_met = self.prerequisites_met(skill);
+    }
-            if !matches!(self.skills.get(&skill), Some(level) if *level == skill.max_level()) {
+
+    /// Returns a mutable reference to a particular skill group in a skillset
+    fn skill_group_mut(&mut self, skill_group: SkillGroupKind) -> Option<&mut SkillGroup> {
+        self.skill_groups
+            .iter_mut()
+            .find(|s_g| s_g.skill_group_kind == skill_group)
+    }
+
+    /// Adds experience to the skill group within an entity's skill set
+    pub fn add_experience(&mut self, skill_group_kind: SkillGroupKind, amount: u32) {
         if let Some(mut skill_group) = self.skill_group_mut(skill_group_kind) {
-                    if prerequisites_met {
+            skill_group.earned_exp = skill_group.earned_exp.saturating_add(amount);
-                        if skill_group.available_sp >= skill.skill_cost(next_level) {
-                            skill_group.available_sp -= skill.skill_cost(next_level);
-                            if let Skill::UnlockGroup(group) = skill {
-                                self.unlock_skill_group(group);
-                            }
-                            if matches!(skill, Skill::General(GeneralSkill::HealthIncrease)) {
-                                self.modify_health = true;
-                            }
-                            if matches!(skill, Skill::General(GeneralSkill::EnergyIncrease)) {
-                                self.modify_energy = true;
-                            }
-                            self.skills.insert(skill, next_level);
         } else {
-                            trace!("Tried to unlock skill for skill group with insufficient SP");
+            warn!("Tried to add experience to a skill group that player does not have");
-                        }
-                    } else {
-                        trace!("Tried to unlock skill without meeting prerequisite skills");
-                    }
-                } else {
-                    trace!("Tried to unlock skill for a skill group that player does not have");
-                }
-            } else {
-                trace!("Tried to unlock skill the player already has")
-            }
-        } else {
-            warn!(
-                ?skill,
-                "Tried to unlock skill that does not exist in any skill group!"
-            );
         }
     }
 
-    /// Removes a skill from a player and refunds 1 skill point in the relevant
+    /// Gets the available experience for a particular skill group
-    /// skill group.
+    pub fn available_experience(&self, skill_group: SkillGroupKind) -> u32 {
-    pub fn refund_skill(&mut self, skill: Skill) {
+        self.skill_group(skill_group)
-        if let Ok(level) = self.skill_level(skill) {
+            .map_or(0, |s_g| s_g.available_experience())
-            if let Some(skill_group_kind) = skill.skill_group_kind() {
-                if let Some(mut skill_group) = self.skill_group_mut(skill_group_kind) {
-                    skill_group.available_sp += skill.skill_cost(level);
-                    if level.map_or(false, |l| l > 1) {
-                        self.skills.insert(skill, level.map(|l| l - 1));
-                    } else {
-                        self.skills.remove(&skill);
     }
+
+    /// Checks how much experience is needed for the next skill point in a tree
+    pub fn skill_point_cost(&self, skill_group: SkillGroupKind) -> u32 {
+        if let Some(level) = self.skill_group(skill_group).map(|sg| sg.earned_sp) {
+            skill_group.skill_point_cost(level)
         } else {
-                    warn!("Tried to refund skill for a skill group that player does not have");
+            skill_group.skill_point_cost(0)
-                }
-            } else {
-                warn!(
-                    ?skill,
-                    "Tried to refund skill that does not exist in any skill group"
-                )
-            }
-        } else {
-            warn!("Tried to refund skill that has not been unlocked");
         }
     }
 
@@ -325,38 +297,6 @@ impl SkillSet {
         }
     }
 
-    /// Adds/subtracts experience to the skill group within an entity's skill
-    /// set
-    pub fn add_experience(&mut self, skill_group_kind: SkillGroupKind, amount: u32) {
-        if let Some(mut skill_group) = self.skill_group_mut(skill_group_kind) {
-            skill_group.earned_exp = skill_group.earned_exp.saturating_add(amount);
-        } else {
-            warn!("Tried to add experience to a skill group that player does not have");
-        }
-    }
-
-    /// Checks that the skill set contains all prerequisite skills for a
-    /// particular skill
-    pub fn prerequisites_met(&self, skill: Skill) -> bool {
-        skill
-            .prerequisite_skills()
-            .all(|(s, l)| self.skill_level(s).map_or(false, |l_b| l_b >= l))
-    }
-
-    /// Returns a reference to a particular skill group in a skillset
-    fn skill_group(&self, skill_group: SkillGroupKind) -> Option<&SkillGroup> {
-        self.skill_groups
-            .iter()
-            .find(|s_g| s_g.skill_group_kind == skill_group)
-    }
-
-    /// Returns a reference to a particular skill group in a skillset
-    fn skill_group_mut(&mut self, skill_group: SkillGroupKind) -> Option<&mut SkillGroup> {
-        self.skill_groups
-            .iter_mut()
-            .find(|s_g| s_g.skill_group_kind == skill_group)
-    }
-
     /// Gets the available points for a particular skill group
     pub fn available_sp(&self, skill_group: SkillGroupKind) -> u16 {
         self.skill_group(skill_group)
@@ -368,10 +308,11 @@ impl SkillSet {
         self.skill_group(skill_group).map_or(0, |s_g| s_g.earned_sp)
     }
 
-    /// Gets the available experience for a particular skill group
+    /// Checks that the skill set contains all prerequisite skills of the required level for a particular skill
-    pub fn available_experience(&self, skill_group: SkillGroupKind) -> u32 {
+    pub fn prerequisites_met(&self, skill: Skill) -> bool {
-        self.skill_group(skill_group)
+        skill
-            .map_or(0, |s_g| s_g.available_experience())
+            .prerequisite_skills()
+            .all(|(s, l)| self.skill_level(s).map_or(false, |l_b| l_b >= l))
     }
 
     /// Gets skill point cost to purchase skill of next level
@@ -398,6 +339,63 @@ impl SkillSet {
         }
     }
 
+    /// Checks the next level of a skill
+    fn next_skill_level(&self, skill: Skill) -> Option<u16> {
+        if let Ok(level) = self.skill_level(skill) {
+            // If already has skill, and that skill has levels, level + 1
+            level.map(|l| l + 1)
+        } else {
+            // Else if the skill has levels, 1
+            skill.max_level().map(|_| 1)
+        }
+    }
+
+    /// 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) {
+        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);
+            // 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 prerequisites_met {
+                        if skill_group.available_sp >= skill.skill_cost(next_level) {
+                            skill_group.available_sp -= skill.skill_cost(next_level);
+                            skill_group.ordered_skills.push(skill);
+                            match skill {
+                                Skill::UnlockGroup(group) => {
+                                    self.unlock_skill_group(group);
+                                },
+                                Skill::General(GeneralSkill::HealthIncrease) => {
+                                    self.modify_health = true;
+                                },
+                                Skill::General(GeneralSkill::EnergyIncrease) => {
+                                    self.modify_energy = true;
+                                },
+                                _ => {},
+                            }
+                            self.skills.insert(skill, next_level);
+                        } else {
+                            trace!("Tried to unlock skill for skill group with insufficient SP");
+                        }
+                    } else {
+                        trace!("Tried to unlock skill without meeting prerequisite skills");
+                    }
+                } else {
+                    trace!("Tried to unlock skill for a skill group that player does not have");
+                }
+            } else {
+                trace!("Tried to unlock skill the player already has")
+            }
+        } else {
+            warn!(
+                ?skill,
+                "Tried to unlock skill that does not exist in any skill group!"
+            );
+        }
+    }
+
     /// Checks if the player has available SP to spend
     pub fn has_available_sp(&self) -> bool {
         self.skill_groups.iter().any(|sg| {
@@ -406,15 +404,6 @@ impl SkillSet {
         })
     }
 
-    /// Checks how much experience is needed for the next skill point in a tree
-    pub fn skill_point_cost(&self, skill_group: SkillGroupKind) -> u32 {
-        if let Some(level) = self.skill_group(skill_group).map(|sg| sg.earned_sp) {
-            skill_group.skill_point_cost(level)
-        } else {
-            skill_group.skill_point_cost(0)
-        }
-    }
-
     /// Checks if the skill is at max level in a skill set
     pub fn is_at_max_level(&self, skill: Skill) -> bool {
         if let Ok(level) = self.skill_level(skill) {
@@ -444,15 +433,6 @@ impl SkillSet {
             default
         }
     }
-
-    /// Checks the next level of a skill
-    fn next_skill_level(&self, skill: Skill) -> Option<u16> {
-        if let Ok(level) = self.skill_level(skill) {
-            level.map(|l| l + 1)
-        } else {
-            skill.max_level().map(|_| 1)
-        }
-    }
 }
 
 pub enum SkillError {

common/src/comp/skillset/skills.rs

@@ -18,11 +18,12 @@ pub enum Skill {
     Bow(BowSkill),
     Staff(StaffSkill),
     Sceptre(SceptreSkill),
-    UnlockGroup(SkillGroupKind),
     Roll(RollSkill),
     Climb(ClimbSkill),
     Swim(SwimSkill),
     Pick(MiningSkill),
+    // TODO: Don't do this, maybe Sharp has idea?
+    UnlockGroup(SkillGroupKind),
 }
 
 #[derive(Clone, Copy, Debug, Hash, PartialEq, Eq, Serialize, Deserialize)]

server/src/persistence/character.rs

@@ -16,7 +16,7 @@ use crate::{
             convert_character_from_database, convert_inventory_from_database_items,
             convert_items_to_database_items, convert_loadout_from_database_items,
             convert_skill_groups_to_database, convert_skill_set_from_database,
-            convert_skills_to_database, convert_stats_from_database,
+            convert_stats_from_database,
             convert_waypoint_from_database_json, convert_waypoint_to_database_json,
         },
         character_loader::{CharacterCreationResult, CharacterDataResult, CharacterListResult},
@@ -162,31 +162,11 @@ pub fn load_character_data(
         }
     });
 
-    let mut stmt = connection.prepare_cached(
-        "
-        SELECT  skill,
-                level
-        FROM    skill
-        WHERE   entity_id = ?1",
-    )?;
-
-    let skill_data = stmt
-        .query_map(&[char_id], |row| {
-            Ok(Skill {
-                entity_id: char_id,
-                skill: row.get(0)?,
-                level: row.get(1)?,
-            })
-        })?
-        .filter_map(Result::ok)
-        .collect::<Vec<Skill>>();
-
     let mut stmt = connection.prepare_cached(
         "
         SELECT  skill_group_kind,
-                exp,
+                earned_exp,
-                available_sp,
+                skills
-                earned_sp
         FROM    skill_group
         WHERE   entity_id = ?1",
     )?;
@@ -197,6 +177,7 @@ pub fn load_character_data(
                 entity_id: char_id,
                 skill_group_kind: row.get(0)?,
                 earned_exp: row.get(1)?,
+                skills: row.get(2)?
             })
         })?
         .filter_map(Result::ok)
@@ -252,7 +233,7 @@ pub fn load_character_data(
     Ok((
         convert_body_from_database(&body_data.variant, &body_data.body_data)?,
         convert_stats_from_database(character_data.alias),
-        convert_skill_set_from_database(&skill_data, &skill_group_data),
+        convert_skill_set_from_database(&skill_group_data),
         convert_inventory_from_database_items(
             character_containers.inventory_container_id,
             &inventory_items,
@@ -1029,39 +1010,6 @@ pub fn update(
         ])?;
     }
 
-    let db_skills = convert_skills_to_database(char_id, char_skill_set.skills);
-
-    let known_skills = Rc::new(
-        db_skills
-            .iter()
-            .map(|x| Value::from(x.skill.clone()))
-            .collect::<Vec<Value>>(),
-    );
-
-    let mut stmt = transaction.prepare_cached(
-        "
-        DELETE
-        FROM    skill
-        WHERE   entity_id = ?1
-        AND     skill NOT IN rarray(?2)",
-    )?;
-
-    let delete_count = stmt.execute(&[&char_id as &dyn ToSql, &known_skills])?;
-    trace!("Deleted {} skills", delete_count);
-
-    let mut stmt = transaction.prepare_cached(
-        "
-        REPLACE
-        INTO    skill (entity_id,
-                       skill,
-                       level)
-        VALUES (?1, ?2, ?3)",
-    )?;
-
-    for skill in db_skills {
-        stmt.execute(&[&skill.entity_id as &dyn ToSql, &skill.skill, &skill.level])?;
-    }
-
     let db_waypoint = convert_waypoint_to_database_json(char_waypoint);
 
     let mut stmt = transaction.prepare_cached(

server/src/persistence/character/conversions.rs

@@ -1,6 +1,6 @@
 use crate::persistence::{
     character::EntityId,
-    models::{Character, Item, Skill, SkillGroup},
+    models::{Character, Item, SkillGroup},
 };
 
 use crate::persistence::{
@@ -500,14 +500,10 @@ pub fn convert_stats_from_database(alias: String) -> common::comp::Stats {
     new_stats
 }
 
-pub fn convert_skill_set_from_database(
+pub fn convert_skill_set_from_database(skill_groups: &[SkillGroup]) -> common::comp::SkillSet {
-    skills: &[Skill],
-    skill_groups: &[SkillGroup],
-) -> common::comp::SkillSet {
     skillset::SkillSet {
         skill_groups: convert_skill_groups_from_database(skill_groups),
-        skills: convert_skills_from_database(skills),
+        skills: HashMap::new(),
-        ordered_skills: Vec::new(),
         modify_health: true,
         modify_energy: true,
     }
@@ -532,6 +528,7 @@ fn convert_skill_groups_from_database(skill_groups: &[SkillGroup]) -> Vec<skills
             spent_exp: 0,
             available_sp: 0,
             earned_sp: 0,
+            ordered_skills: Vec::new(),
         };
 
         while new_skill_group.earn_skill_point().is_ok() {}
@@ -541,16 +538,6 @@ fn convert_skill_groups_from_database(skill_groups: &[SkillGroup]) -> Vec<skills
     new_skill_groups
 }
 
-fn convert_skills_from_database(skills: &[Skill]) -> HashMap<skills::Skill, Option<u16>> {
-    let mut new_skills = HashMap::new();
-    for skill in skills.iter() {
-        if let Some(new_skill) = json_models::db_string_to_skill(&skill.skill) {
-            new_skills.insert(new_skill, skill.level.map(|l| l as u16));
-        }
-    }
-    new_skills
-}
-
 pub fn convert_skill_groups_to_database(
     entity_id: CharacterId,
     skill_groups: Vec<skillset::SkillGroup>,
@@ -561,20 +548,7 @@ pub fn convert_skill_groups_to_database(
             entity_id,
             skill_group_kind: json_models::skill_group_to_db_string(sg.skill_group_kind),
             earned_exp: sg.earned_exp as i32,
-        })
+            skills: sg.ordered_skills.iter().map(|s| json_models::skill_to_db_string(*s)).collect(),
-        .collect()
-}
-
-pub fn convert_skills_to_database(
-    entity_id: CharacterId,
-    skills: HashMap<skills::Skill, Option<u16>>,
-) -> Vec<Skill> {
-    skills
-        .iter()
-        .map(|(s, l)| Skill {
-            entity_id,
-            skill: json_models::skill_to_db_string(*s),
-            level: l.map(|l| l as i32),
         })
         .collect()
 }

server/src/persistence/models.rs

@@ -20,16 +20,11 @@ pub struct Body {
     pub body_data: String,
 }
 
-pub struct Skill {
-    pub entity_id: i64,
-    pub skill: String,
-    pub level: Option<i32>,
-}
-
 pub struct SkillGroup {
     pub entity_id: i64,
     pub skill_group_kind: String,
     pub earned_exp: i32,
+    pub skills: String,
 }
 
 pub struct Pet {

server/src/sys/msg/in_game.rs

@@ -250,11 +250,6 @@ impl Sys {
                     .get_mut(entity)
                     .map(|mut skill_set| skill_set.unlock_skill(skill));
             },
-            ClientGeneral::RefundSkill(skill) => {
-                skill_sets
-                    .get_mut(entity)
-                    .map(|mut skill_set| skill_set.refund_skill(skill));
-            },
             ClientGeneral::UnlockSkillGroup(skill_group_kind) => {
                 skill_sets
                     .get_mut(entity)