From 6d2496b7de107ffcdb8e2a8d7292308c8fcf6272 Mon Sep 17 00:00:00 2001 From: Sam Date: Wed, 1 Dec 2021 23:38:33 -0500 Subject: [PATCH] Changed skill group to store available_exp instead of spent_exp to help enforce certain invariants. Addressed more review coments. --- common/src/comp/ability.rs | 26 +++--- common/src/comp/skillset/mod.rs | 88 ++++++++++--------- common/src/comp/skillset/skills.rs | 11 +-- common/systems/src/stats.rs | 2 +- server/src/events/interaction.rs | 10 ++- .../src/persistence/character/conversions.rs | 65 ++++++++++---- 6 files changed, 117 insertions(+), 85 deletions(-) diff --git a/common/src/comp/ability.rs b/common/src/comp/ability.rs index 32c1df965a..1c5daf23d9 100644 --- a/common/src/comp/ability.rs +++ b/common/src/comp/ability.rs @@ -1242,7 +1242,7 @@ impl CharacterAbility { if skillset.has_skill(Sword(TsCombo)) { let speed_segments = f32::from(Sword(TsSpeed).max_level()) + 1.0; - let speed_level = f32::from(skillset.skill_level_or(Sword(TsSpeed), 0)); + let speed_level = f32::from(skillset.skill_level(Sword(TsSpeed)).unwrap_or(0)); *speed_increase = (speed_level + 1.0) / speed_segments; *max_speed_increase = (speed_level + 1.0) / speed_segments; } else { @@ -1250,7 +1250,7 @@ impl CharacterAbility { *max_speed_increase = 0.0; } - let energy_level = skillset.skill_level_or(Sword(TsRegen), 0); + let energy_level = skillset.skill_level(Sword(TsRegen)).unwrap_or(0); let stages = u16::try_from(stage_data.len()) .expect("number of stages can't be more than u16"); @@ -1258,7 +1258,7 @@ impl CharacterAbility { *max_energy_gain *= f32::from((energy_level + 1) * stages - 1) * f32::from(stages - 1) / f32::from(Sword(TsRegen).max_level() + 1); - *scales_from_combo = skillset.skill_level_or(Sword(TsDamage), 0).into(); + *scales_from_combo = skillset.skill_level(Sword(TsDamage)).unwrap_or(0).into(); }, CharacterAbility::DashMelee { ref mut is_interruptible, @@ -1308,7 +1308,7 @@ impl CharacterAbility { if let Ok(level) = skillset.skill_level(Sword(SCost)) { *energy_cost *= modifiers.energy_cost.powi(level.into()); } - let spin_level = skillset.skill_level_or(Sword(SSpins), 0); + let spin_level = skillset.skill_level(Sword(SSpins)).unwrap_or(0); *num_spins = u32::from(spin_level) * modifiers.num + 1; }, _ => {}, @@ -1333,11 +1333,11 @@ impl CharacterAbility { stage_data.pop(); } let speed_segments = f32::from(Axe(DsSpeed).max_level()); - let speed_level = f32::from(skillset.skill_level_or(Axe(DsSpeed), 0)); + let speed_level = f32::from(skillset.skill_level(Axe(DsSpeed)).unwrap_or(0)); *speed_increase *= speed_level / speed_segments; *max_speed_increase *= speed_level / speed_segments; - let energy_level = skillset.skill_level_or(Axe(DsRegen), 0); + let energy_level = skillset.skill_level(Axe(DsRegen)).unwrap_or(0); let stages = u16::try_from(stage_data.len()) .expect("number of stages can't be more than u16"); @@ -1345,7 +1345,7 @@ impl CharacterAbility { *max_energy_gain *= f32::from((energy_level + 1) * stages - 1).max(1.0) * f32::from(stages - 1).max(1.0) / f32::from(Axe(DsRegen).max_level() + 1); - *scales_from_combo = skillset.skill_level_or(Axe(DsDamage), 0).into(); + *scales_from_combo = skillset.skill_level(Axe(DsDamage)).unwrap_or(0).into(); }, CharacterAbility::SpinMelee { ref mut base_damage, @@ -1424,11 +1424,11 @@ impl CharacterAbility { .collect::>(); } let speed_segments = f32::from(Hammer(SsSpeed).max_level()); - let speed_level = f32::from(skillset.skill_level_or(Hammer(SsSpeed), 0)); + let speed_level = f32::from(skillset.skill_level(Hammer(SsSpeed)).unwrap_or(0)); *speed_increase *= speed_level / speed_segments; *max_speed_increase *= speed_level / speed_segments; - let energy_level = skillset.skill_level_or(Hammer(SsRegen), 0); + let energy_level = skillset.skill_level(Hammer(SsRegen)).unwrap_or(0); let stages = u16::try_from(stage_data.len()) .expect("number of stages can't be more than u16"); @@ -1436,7 +1436,7 @@ impl CharacterAbility { *max_energy_gain *= f32::from((energy_level + 1) * stages) / f32::from((Hammer(SsRegen).max_level() + 1) * stages); - *scales_from_combo = skillset.skill_level_or(Hammer(SsDamage), 0).into(); + *scales_from_combo = skillset.skill_level(Hammer(SsDamage)).unwrap_or(0).into(); }, CharacterAbility::ChargedMelee { ref mut scaled_damage, @@ -1604,9 +1604,9 @@ impl CharacterAbility { ref mut projectile, .. } => { let modifiers = SKILL_MODIFIERS.staff_tree.fireball; - let damage_level = skillset.skill_level_or(Staff(BDamage), 0); - let regen_level = skillset.skill_level_or(Staff(BRegen), 0); - let range_level = skillset.skill_level_or(Staff(BRadius), 0); + let damage_level = skillset.skill_level(Staff(BDamage)).unwrap_or(0); + let regen_level = skillset.skill_level(Staff(BRegen)).unwrap_or(0); + let range_level = skillset.skill_level(Staff(BRadius)).unwrap_or(0); let power = modifiers.power.powi(damage_level.into()); let regen = modifiers.regen.powi(regen_level.into()); let range = modifiers.range.powi(range_level.into()); diff --git a/common/src/comp/skillset/mod.rs b/common/src/comp/skillset/mod.rs index a7b64113d9..b5cedf1e71 100644 --- a/common/src/comp/skillset/mod.rs +++ b/common/src/comp/skillset/mod.rs @@ -42,6 +42,9 @@ impl Asset for SkillLevelMap { const EXTENSION: &'static str = "ron"; } +/// Contains the prerequisite skills for each skill. It cannot currently detect +/// cyclic dependencies, so if you modify the prerequisite map ensure that there +/// are no cycles of prerequisites. #[derive(Clone, Debug, Serialize, Deserialize)] pub struct SkillPrerequisitesMap(HashMap>); @@ -157,10 +160,10 @@ impl SkillGroupKind { #[derive(Clone, Debug, Eq, Hash, PartialEq, Serialize, Deserialize)] pub struct SkillGroup { pub skill_group_kind: SkillGroupKind, - // How much exp has been used for skill points - pub spent_exp: u32, - // How much exp has been earned in total + // The invariant that (earned_exp >= available_exp) should not be violated + pub available_exp: u32, pub earned_exp: u32, + // The invariant that (earned_sp >= available_sp) should not be violated pub available_sp: u16, pub earned_sp: u16, // Used for persistence @@ -171,7 +174,7 @@ impl SkillGroup { fn new(skill_group_kind: SkillGroupKind) -> SkillGroup { SkillGroup { skill_group_kind, - spent_exp: 0, + available_exp: 0, earned_exp: 0, available_sp: 0, earned_sp: 0, @@ -179,33 +182,37 @@ impl SkillGroup { } } - /// Returns the available experience that could be used to earn another - /// skill point in a particular skill group. - pub fn available_experience(&self) -> u32 { self.earned_exp - self.spent_exp } + /// Returns the amount of experience in a skill group that has been spent to + /// acquire skill points Relies on the invariant that (earned_exp >= + /// available_exp) to ensure function does not underflow + pub fn spent_exp(&self) -> u32 { self.earned_exp - self.available_exp } /// Adds a skill point while subtracting the necessary amount of experience pub fn earn_skill_point(&mut self) -> Result<(), SpRewardError> { let sp_cost = self.skill_group_kind.skill_point_cost(self.earned_sp); - if self.available_experience() >= sp_cost { - let new_spent_exp = self - .spent_exp - .checked_add(sp_cost) - .ok_or(SpRewardError::Overflow)?; - let new_earned_sp = self - .earned_sp - .checked_add(1) - .ok_or(SpRewardError::Overflow)?; - let new_available_sp = self - .available_sp - .checked_add(1) - .ok_or(SpRewardError::Overflow)?; - self.spent_exp = new_spent_exp; - self.earned_sp = new_earned_sp; - self.available_sp = new_available_sp; - Ok(()) - } else { - Err(SpRewardError::InsufficientExp) - } + // If there is insufficient available exp, checked sub will fail as the result + // would be less than 0 + let new_available_exp = self + .available_exp + .checked_sub(sp_cost) + .ok_or(SpRewardError::InsufficientExp)?; + let new_earned_sp = self + .earned_sp + .checked_add(1) + .ok_or(SpRewardError::Overflow)?; + let new_available_sp = self + .available_sp + .checked_add(1) + .ok_or(SpRewardError::Overflow)?; + self.available_exp = new_available_exp; + self.earned_sp = new_earned_sp; + self.available_sp = new_available_sp; + Ok(()) + } + + pub fn add_experience(&mut self, amount: u32) { + self.earned_exp = self.earned_exp.saturating_add(amount); + self.available_exp = self.available_exp.saturating_add(amount); } } @@ -254,7 +261,7 @@ impl SkillSet { pub fn load_from_database( skill_groups: Vec, - mut all_skills: HashMap>, + mut all_skills: HashMap, SkillsPersistenceError>>, ) -> Self { let mut skillset = SkillSet { skill_groups, @@ -274,7 +281,7 @@ impl SkillSet { { // Remove valid skill group kind from the hash map so that loop eventually // terminates. - if let Some(skills) = all_skills.remove(&skill_group_kind) { + if let Some(Ok(skills)) = all_skills.remove(&skill_group_kind) { let backup_skillset = skillset.clone(); // Iterate over all skills and make sure that unlocking them is successful. If // any fail, fall back to skillset before unlocking any to allow a full respec @@ -336,8 +343,8 @@ impl SkillSet { /// 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) { - skill_group.earned_exp = skill_group.earned_exp.saturating_add(amount); + if let Some(skill_group) = self.skill_group_mut(skill_group_kind) { + skill_group.add_experience(amount); } else { warn!("Tried to add experience to a skill group that player does not have"); } @@ -346,7 +353,7 @@ impl SkillSet { /// Gets the available experience for a particular skill group pub fn available_experience(&self, skill_group: SkillGroupKind) -> u32 { self.skill_group(skill_group) - .map_or(0, |s_g| s_g.available_experience()) + .map_or(0, |s_g| s_g.available_exp) } /// Checks how much experience is needed for the next skill point in a tree @@ -521,15 +528,6 @@ impl SkillSet { Err(SkillError::MissingSkill) } } - - /// Returns the level of the skill or passed value as default - pub fn skill_level_or(&self, skill: Skill, default: u16) -> u16 { - if let Ok(level) = self.skill_level(skill) { - level - } else { - default - } - } } #[derive(Debug)] @@ -546,8 +544,16 @@ pub enum SkillUnlockError { NoParentSkillTree, } +#[derive(Debug)] pub enum SpRewardError { InsufficientExp, UnavailableSkillGroup, Overflow, } + +#[derive(Debug)] +pub enum SkillsPersistenceError { + HashMismatch, + DeserializationFailure, + SpentExpMismatch, +} diff --git a/common/src/comp/skillset/skills.rs b/common/src/comp/skillset/skills.rs index 8a4aa1ed7c..5ecbf8db35 100644 --- a/common/src/comp/skillset/skills.rs +++ b/common/src/comp/skillset/skills.rs @@ -187,18 +187,15 @@ impl Skill { /// Returns a vec of prerequisite skills (it should only be necessary to /// note direct prerequisites) /// Automatically filters itself from the skills returned + /// Is unable to detect cyclic dependencies, so ensure that there are no + /// cycles if you modify the prerequisite map. pub fn prerequisite_skills(&self) -> impl Iterator + '_ { SKILL_PREREQUISITES .get(self) .into_iter() .flatten() - .filter_map(move |(skill, level)| { - if self == skill { - None - } else { - Some((*skill, *level)) - } - }) + .filter(move |(skill, _)| self != *skill) + .map(|(skill, level)| (*skill, *level)) } /// Returns the cost in skill points of unlocking a particular skill diff --git a/common/systems/src/stats.rs b/common/systems/src/stats.rs index eff0f52bd5..b63535dcd7 100644 --- a/common/systems/src/stats.rs +++ b/common/systems/src/stats.rs @@ -133,7 +133,7 @@ impl<'a> System<'a> for Sys { .skill_groups() .iter() .filter_map(|s_g| { - (s_g.available_experience() >= skill_set.skill_point_cost(s_g.skill_group_kind)) + (s_g.available_exp >= skill_set.skill_point_cost(s_g.skill_group_kind)) .then(|| s_g.skill_group_kind) }) .collect::>(); diff --git a/server/src/events/interaction.rs b/server/src/events/interaction.rs index 4533275950..a90946b404 100644 --- a/server/src/events/interaction.rs +++ b/server/src/events/interaction.rs @@ -355,15 +355,17 @@ pub fn handle_mine_block( let need_double_ore = |rng: &mut rand::rngs::ThreadRng| { let chance_mod = f64::from(SKILL_MODIFIERS.mining_tree.ore_gain); - let skill_level = - skillset.skill_level_or(Skill::Pick(MiningSkill::OreGain), 0); + let skill_level = skillset + .skill_level(Skill::Pick(MiningSkill::OreGain)) + .unwrap_or(0); rng.gen_bool(chance_mod * f64::from(skill_level)) }; let need_double_gem = |rng: &mut rand::rngs::ThreadRng| { let chance_mod = f64::from(SKILL_MODIFIERS.mining_tree.gem_gain); - let skill_level = - skillset.skill_level_or(Skill::Pick(MiningSkill::GemGain), 0); + let skill_level = skillset + .skill_level(Skill::Pick(MiningSkill::GemGain)) + .unwrap_or(0); rng.gen_bool(chance_mod * f64::from(skill_level)) }; diff --git a/server/src/persistence/character/conversions.rs b/server/src/persistence/character/conversions.rs index 7cf9922536..17587b9db4 100644 --- a/server/src/persistence/character/conversions.rs +++ b/server/src/persistence/character/conversions.rs @@ -24,7 +24,7 @@ use core::{convert::TryFrom, num::NonZeroU64}; use hashbrown::HashMap; use lazy_static::lazy_static; use std::{collections::VecDeque, str::FromStr, sync::Arc}; -use tracing::trace; +use tracing::{trace, warn}; #[derive(Debug)] pub struct ItemModelPair { @@ -510,24 +510,30 @@ pub fn convert_stats_from_database(alias: String) -> common::comp::Stats { } pub fn convert_skill_set_from_database(skill_groups: &[SkillGroup]) -> common::comp::SkillSet { - let (skillless_skill_groups, skills) = convert_skill_groups_from_database(skill_groups); - common::comp::SkillSet::load_from_database(skillless_skill_groups, skills) + let (skillless_skill_groups, deserialized_skills) = + convert_skill_groups_from_database(skill_groups); + common::comp::SkillSet::load_from_database(skillless_skill_groups, deserialized_skills) } +#[allow(clippy::type_complexity)] fn convert_skill_groups_from_database( skill_groups: &[SkillGroup], ) -> ( + // Skill groups in the vec do not contain skills, those are added later. The skill group only + // contains fields related to experience and skill points Vec, - HashMap>, + // + HashMap, skillset::SkillsPersistenceError>>, ) { let mut new_skill_groups = Vec::new(); - let mut all_skills = HashMap::new(); + let mut deserialized_skills = HashMap::new(); for skill_group in skill_groups.iter() { let skill_group_kind = json_models::db_string_to_skill_group(&skill_group.skill_group_kind); let mut new_skill_group = skillset::SkillGroup { skill_group_kind, - earned_exp: skill_group.earned_exp as u32, - spent_exp: 0, + // Available and earned exp and sp are reconstructed below + earned_exp: 0, + available_exp: 0, available_sp: 0, earned_sp: 0, // Ordered skills empty here as skills get inserted later as they are unlocked, so long @@ -535,26 +541,47 @@ fn convert_skill_groups_from_database( ordered_skills: Vec::new(), }; - // Convert exp into skill points + // Add experience to skill group through method to ensure invariant of + // (earned_exp >= available_exp) are maintained + let skill_group_exp = skill_group.earned_exp.clamp(0, i64::from(u32::MAX)) as u32; + new_skill_group.add_experience(skill_group_exp); + + // Convert exp into skill points, earn_skill_point will only return an error + // when it is no longer able to spend exp to acquire another skill point while new_skill_group.earn_skill_point().is_ok() {} - // If the stored spent exp is the same as the spent exp after adding skill - // points, and the hash stored of the skill group is the same as the current - // hash of the skill group, don't invalidate skills; otherwise invalidate the - // skills in this skill_group. - let skills = if skill_group.spent_exp as u32 == new_skill_group.spent_exp - && Some(&skill_group.hash_val) == skillset::SKILL_GROUP_HASHES.get(&skill_group_kind) + use skillset::SkillsPersistenceError; + + let skills_result = if skill_group.spent_exp != i64::from(new_skill_group.spent_exp()) { + // If persisted spent exp does not equal the spent exp after reacquiring skill + // points, force a respec + Err(SkillsPersistenceError::SpentExpMismatch) + } else if Some(&skill_group.hash_val) != skillset::SKILL_GROUP_HASHES.get(&skill_group_kind) { - serde_json::from_str::>(&skill_group.skills).unwrap_or_default() + // Else if persisted hash for skill group does not match current hash for skill + // group, force a respec + Err(SkillsPersistenceError::HashMismatch) } else { - Vec::new() + // Else attempt to deserialize skills from a json string + match serde_json::from_str::>(&skill_group.skills) { + // If it correctly deserializes, return the persisted skills + Ok(skills) => Ok(skills), + // Else if doesn't deserialize correctly, force a respec + Err(err) => { + warn!( + "Skills failed to correctly deserialized\nError: {:#?}\nRaw JSON: {:#?}", + err, &skill_group.skills + ); + Err(SkillsPersistenceError::DeserializationFailure) + }, + } }; - all_skills.insert(skill_group_kind, skills); + deserialized_skills.insert(skill_group_kind, skills_result); new_skill_groups.push(new_skill_group); } - (new_skill_groups, all_skills) + (new_skill_groups, deserialized_skills) } pub fn convert_skill_groups_to_database( @@ -568,7 +595,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: i64::from(sg.earned_exp), - spent_exp: i64::from(sg.spent_exp), + spent_exp: i64::from(sg.spent_exp()), // If fails to convert, just forces a respec on next login skills: serde_json::to_string(&sg.ordered_skills).unwrap_or_else(|_| "".to_string()), hash_val: skill_group_hashes