diff --git a/common/src/comp/ability.rs b/common/src/comp/ability.rs index 98e7d67cad..762ff7b200 100644 --- a/common/src/comp/ability.rs +++ b/common/src/comp/ability.rs @@ -1894,3 +1894,55 @@ impl From<(&CharacterAbility, AbilityInfo)> for CharacterState { } } } + +#[cfg(test)] +mod tests { + use super::*; + use comp::{ + inventory::item::{Item, ItemKind}, + tool, + }; + #[test] + // As we have only player skills, test it with starter tools + // Load ability_config of each tool and try each CharacterAbility of it + fn test_adjusting_by_skills() { + let test_tools = [ + "common.items.weapons.sword.starter", + "common.items.weapons.hammer.starter_hammer", + "common.items.weapons.bow.starter", + "common.items.weapons.axe.starter_axe", + "common.items.weapons.staff.starter_staff", + "common.items.weapons.sceptre.starter_sceptre", + "common.items.tool.pickaxe_stone", + ]; + + let dummy_skillset = skills::SkillSet::default(); + // check non-combat abilities + let dummy_ability = CharacterAbility::default(); + dummy_ability.adjusted_by_skills(&dummy_skillset, None); + for tool_id in test_tools { + let item = Item::new_from_asset_expect(tool_id); + let tool::AbilitySet { + primary, + secondary, + abilities, + } = &item.item_config_expect().abilities; + + // It should be a tool, I swear + if let ItemKind::Tool(tool) = &item.kind { + primary + .clone() + .adjusted_by_skills(&dummy_skillset, Some(tool.kind)); + secondary + .clone() + .adjusted_by_skills(&dummy_skillset, Some(tool.kind)); + for entry in abilities { + let (_, ability) = entry; + ability + .clone() + .adjusted_by_skills(&dummy_skillset, Some(tool.kind)); + } + } + } + } +} diff --git a/common/src/comp/skills.rs b/common/src/comp/skills.rs index 9315b0fc12..15efdb7052 100644 --- a/common/src/comp/skills.rs +++ b/common/src/comp/skills.rs @@ -150,34 +150,55 @@ impl From for BoostValue { } pub fn adjust_with_level(skillset: &SkillSet, skill: Skill, effect: impl FnOnce(f32, u16)) { + // NOTE: We are unwrapping before checking skill level, + // because if it falls we want know it even if we don't have this level + let multiplier = match skill.boost().as_mult_maybe() { + Some(m) => m, + None => return invalid_skill_boost(skill), + }; if let Ok(Some(level)) = skillset.skill_level(skill) { - skill - .boost() - .as_mult_maybe() - .map_or_else(|| invalid_skill_boost(skill), |m| effect(m, level)) + effect(multiplier, level); } } pub fn adjust_counter_with_level(skillset: &SkillSet, skill: Skill, effect: impl FnOnce(i16, i16)) { + // NOTE: We are unwrapping before checking skill level, + // because if it falls we want know it even if we don't have this level + let counter = match skill.boost().as_i16_maybe() { + Some(c) => c, + None => return invalid_skill_boost(skill), + }; if let Ok(Some(level)) = skillset.skill_level(skill) { - skill - .boost() - .as_i16_maybe() - .map_or_else(|| invalid_skill_boost(skill), |m| effect(m, level as i16)) + effect(counter, level as i16); } } pub fn set_if_has(skillset: &SkillSet, skill: Skill, effect: impl FnOnce(f32)) { + // NOTE: We are unwrapping before checking skill level, + // because if it falls we want know it even if we don't have this level + let multiplier = match skill.boost().as_mult_maybe() { + Some(c) => c, + None => return invalid_skill_boost(skill), + }; if skillset.has_skill(skill) { - skill - .boost() - .as_mult_maybe() - .map_or_else(|| invalid_skill_boost(skill), |m| effect(m)) + effect(multiplier); } } + +#[track_caller] pub fn invalid_skill_boost(skill: Skill) { let err_msg = format!( - "{:?} produced unexpected BoostValue: {:?}", + r#" + + {:?} produced unexpected BoostValue: {:?} + + Clearly that shouldn't happen and tests should catch this. + + If they didn't, probably because we've added new skills/weapons. + In this case, please find `test_adjusting_skills`, + fix tests and fix corresponding `impl Boost` for this skill. + + "#, skill, skill.boost() );