Enchantment/relic cleanup 2 (#400)

* Additional cleanup for tests

* Tests for effects & mutations from enchantments

* Enchantment values: stats

* Enchantment values: SPEED

* Some tests for speed / moves gain

* Simplify check for enchantment 'has' condition

* Clean up enchantment bonus calculation

1. Fix final value being out of bounds in some places.
2. Changed the formula for combining multiple enchantments to produce more easily predictable results.
3. Remove dead code

* Renamed 2 functions to be less ambiguous

* Enchantment values: ATTACK_COST, ITEM_ATTACK_COST

* Enchantment values: MOVE_COST

* Enchantment values: METABOLISM

* Enchantment values: MANA_CAP, MANA_REGEN + mana tests

* Migration for old enchant val enum strings

* Consistent rounding for enchantment bonuses

* fix typo

* Revert effects cleanup change

data/mods/TEST_DATA/items.json

@@ -21,6 +21,18 @@
     "effects": [ "NEVER_MISFIRES", "NON-FOULING", "RECOVER_80" ],
     "qualities": [ [ "HAMMER", 1 ] ]
   },
+  {
+    "type": "GENERIC",
+    "id": "test_1kg_cube",
+    "name": "TEST 1kg cube",
+    "description": "A tiny super-dense cube with nice round weight of 1 kg.",
+    "weight": "1 kg",
+    "color": "light_gray",
+    "symbol": "*",
+    "material": [ "steel" ],
+    "volume": "10 ml",
+    "category": "spare_parts"
+  },
   {
     "id": "test_rag",
     "type": "TOOL",

data/mods/TEST_DATA/relics.json

@@ -0,0 +1,112 @@
+[
+  {
+    "type": "GENERIC",
+    "id": "test_relic_base",
+    "name": "TEST relic base",
+    "description": "A relic for test purposes",
+    "category": "spare_parts",
+    "weight": "1 kg",
+    "volume": "100 ml",
+    "material": [ "steel" ],
+    "color": "red",
+    "symbol": "*"
+  },
+  {
+    "type": "GENERIC",
+    "id": "test_relic_gives_trait",
+    "copy-from": "test_relic_base",
+    "name": "TEST relic gives trait",
+    "relic_data": { "passive_effects": [ { "has": "HELD", "condition": "ALWAYS", "mutations": [ "CARNIVORE" ] } ] }
+  },
+  {
+    "type": "GENERIC",
+    "id": "test_relic_mods_stats",
+    "copy-from": "test_relic_base",
+    "name": "TEST relic mods stats",
+    "relic_data": {
+      "passive_effects": [
+        {
+          "has": "HELD",
+          "condition": "ALWAYS",
+          "values": [
+            { "value": "STRENGTH", "add": 4, "multiply": 1 },
+            { "value": "DEXTERITY", "add": -2 },
+            { "value": "PERCEPTION", "add": 1, "multiply": -0.5 },
+            { "value": "INTELLIGENCE", "add": -11 }
+          ]
+        }
+      ]
+    }
+  },
+  {
+    "type": "GENERIC",
+    "id": "test_relic_mods_speed",
+    "copy-from": "test_relic_base",
+    "name": "TEST relic mods speed",
+    "relic_data": {
+      "passive_effects": [ { "has": "HELD", "condition": "ALWAYS", "values": [ { "value": "SPEED", "add": 25, "multiply": -0.5 } ] } ]
+    }
+  },
+  {
+    "type": "GENERIC",
+    "id": "test_relic_mods_atk_cost",
+    "copy-from": "test_relic_base",
+    "name": "TEST relic mods attack cost",
+    "relic_data": {
+      "passive_effects": [ { "has": "HELD", "condition": "ALWAYS", "values": [ { "value": "ATTACK_COST", "multiply": -0.2 } ] } ]
+    }
+  },
+  {
+    "type": "GENERIC",
+    "id": "test_relic_sword",
+    "name": "TEST relic sword",
+    "description": "A relic sword for test purposes",
+    "category": "spare_parts",
+    "weight": "1250 g",
+    "to_hit": 1,
+    "color": "dark_gray",
+    "symbol": "/",
+    "material": [ "steel" ],
+    "volume": "1 L",
+    "bashing": 32,
+    "cutting": 32,
+    "price": 7500,
+    "price_postapoc": 300,
+    "relic_data": {
+      "passive_effects": [ { "has": "HELD", "condition": "ALWAYS", "values": [ { "value": "ITEM_ATTACK_COST", "add": -15 } ] } ]
+    }
+  },
+  {
+    "type": "GENERIC",
+    "id": "test_relic_mods_mv_cost",
+    "copy-from": "test_relic_base",
+    "name": "TEST relic mods movement cost",
+    "relic_data": {
+      "passive_effects": [ { "has": "HELD", "condition": "ALWAYS", "values": [ { "value": "MOVE_COST", "multiply": -0.1 } ] } ]
+    }
+  },
+  {
+    "type": "GENERIC",
+    "id": "test_relic_mods_metabolism",
+    "copy-from": "test_relic_base",
+    "name": "TEST relic mods metabolic rate",
+    "relic_data": {
+      "passive_effects": [ { "has": "HELD", "condition": "ALWAYS", "values": [ { "value": "METABOLISM", "multiply": -0.1 } ] } ]
+    }
+  },
+  {
+    "type": "GENERIC",
+    "id": "test_relic_mods_manapool",
+    "copy-from": "test_relic_base",
+    "name": "TEST relic mods mana pool",
+    "relic_data": {
+      "passive_effects": [
+        {
+          "has": "HELD",
+          "condition": "ALWAYS",
+          "values": [ { "value": "MANA_CAP", "add": 100, "multiply": -0.3 }, { "value": "MANA_REGEN", "multiply": -0.3 } ]
+        }
+      ]
+    }
+  }
+]

doc/MAGIC.md

@@ -330,48 +330,109 @@ Syntax:
 ```
 
 ### values
-(array) List of miscellaneous values to modify.
+(array) List of miscellaneous character/item values to modify.
 
 Syntax for single entry:
 ```c++
 {
   // (required) Value ID to modify, refer to list below.
   "value": "VALUE_ID_STRING"
 
-  // Additive bonus. Optional, default is 0.
-  "add": 13.37,
+  // Additive bonus. Optional integer number, default is 0.
+  // May be ignored for some values.
+  "add": 13,
 
   // Multiplicative bonus. Optional, default is 0.
   "multiply": -0.3,
 }
 ```
 
-Additive bonus is applied before multiplicative, like so:
+Additive bonus is applied separately from multiplicative, like so:
 ```c++
 bonus = add + base_value * multiply
 ```
 
 Thus, a `multiply` value of -0.8 is -80%, and a `multiply` of 2.5 is +250%.
+When modifying integer values, final bonus is rounded towards 0 (truncated).
+
+When multiple enchantments (e.g. one from an item and one from a bionic) modify the same value,
+their bonuses are added together without rounding, then the sum is rounded (if necessary)
+before being applied to the base value.
+
+Since there's no limit on number of enchantments the character can have at a time,
+the final calculated values have hardcoded bounds to prevent unintended behavior.
 
 #### IDs of modifiable values
 
+#### Character values
+
+##### STRENGTH
+Strength stat.
+`base_value` here is the base stat value.
+The final value cannot go below 0.
+
+##### DEXTERITY
+Dexterity stat.
+`base_value` here is the base stat value.
+The final value cannot go below 0.
+
+##### PERCEPTION
+Perception stat.
+`base_value` here is the base stat value.
+The final value cannot go below 0.
+
+##### INTELLIGENCE
+Intelligence stat.
+`base_value` here is the base stat value.
+The final value cannot go below 0.
+
+##### SPEED
+Character speed.
+`base_value` here is character speed including pain/hunger/weight penalties.
+Final speed value cannot go below 25% of base speed.
+
+##### ATTACK_COST
+Melee attack cost. The lower, the better.
+`base_value` here is attack cost for given weapon including modifiers from stats and skills.
+The final value cannot go below 25.
+
+##### MOVE_COST
+Movement cost.
+`base_value` here is tile movement cost including modifiers from clothing and traits.
+The final value cannot go below 20.
+
+##### METABOLISM
+Metabolic rate.
+This modifier ignores `add` field.
+`base_value` here is `PLAYER_HUNGER_RATE` modified by traits.
+The final value cannot go below 0.
+
+##### MANA_CAP
+Mana capacity.
+`base_value` here is character's base mana capacity modified by traits.
+The final value cannot go below 0.
+
+##### MANA_REGEN
+Mana regeneration rate.
+This modifier ignores `add` field.
+`base_value` here is character's base mana gain rate modified by traits.
+The final value cannot go below 0.
+
+#### Item values
+
+##### ITEM_ATTACK_COST
+Attack cost (melee or throwing) for this item.
+Ignores condition / location, and is always active.
+`base_value` here is base item attack cost.
+Note that the final value cannot go below 0.
+
 ##### TODO
 
 TODO: docs for each
 
 TODO: some of these are broken/unimplemented
 
-* STRENGTH
-* DEXTERITY
-* PERCEPTION
-* INTELLIGENCE
-* SPEED
-* ATTACK_COST
-* ATTACK_SPEED
-* MOVE_COST
-* METABOLISM
-* MAX_MANA
-* REGEN_MANA
+
 * BIONIC_POWER
 * MAX_STAMINA
 * REGEN_STAMINA
@@ -424,7 +485,6 @@ Effects for the item that has the enchantment:
 * ITEM_ENCUMBRANCE
 * ITEM_VOLUME
 * ITEM_COVERAGE
-* ITEM_ATTACK_SPEED
 * ITEM_WET_PROTECTION
 
 ## Examples

src/character.cpp

@@ -4550,7 +4550,7 @@ void Character::update_body( const time_point &from, const time_point &to )
     update_stomach( from, to );
     recalculate_enchantment_cache();
     if( ticks_between( from, to, 3_minutes ) > 0 ) {
-        magic->update_mana( *this->as_player(), to_turns<float>( 3_minutes ) );
+        magic->update_mana( *this->as_player(), to_turns<double>( 3_minutes ) );
     }
     const int five_mins = ticks_between( from, to, 5_minutes );
     if( five_mins > 0 ) {
@@ -7701,15 +7701,9 @@ void Character::recalculate_enchantment_cache()
     }
 }
 
-double Character::calculate_by_enchantment( double modify, enchant_vals::mod value,
-        bool round_output ) const
+double Character::bonus_from_enchantments( double base, enchant_vals::mod value, bool round ) const
 {
-    modify += enchantment_cache->get_value_add( value );
-    modify *= 1.0 + enchantment_cache->get_value_multiply( value );
-    if( round_output ) {
-        modify = std::round( modify );
-    }
-    return modify;
+    return enchantment_cache->calc_bonus( value, base, round );
 }
 
 void Character::passive_absorb_hit( body_part bp, damage_unit &du ) const
@@ -7749,28 +7743,28 @@ static void item_armor_enchantment_adjust( Character &guy, damage_unit &du, item
 {
     switch( du.type ) {
         case DT_ACID:
-            du.amount = armor.calculate_by_enchantment( guy, du.amount, enchant_vals::mod::ITEM_ARMOR_ACID );
+            du.amount += armor.bonus_from_enchantments( guy, du.amount, enchant_vals::mod::ITEM_ARMOR_ACID );
             break;
         case DT_BASH:
-            du.amount = armor.calculate_by_enchantment( guy, du.amount, enchant_vals::mod::ITEM_ARMOR_BASH );
+            du.amount += armor.bonus_from_enchantments( guy, du.amount, enchant_vals::mod::ITEM_ARMOR_BASH );
             break;
         case DT_BIOLOGICAL:
-            du.amount = armor.calculate_by_enchantment( guy, du.amount, enchant_vals::mod::ITEM_ARMOR_BIO );
+            du.amount += armor.bonus_from_enchantments( guy, du.amount, enchant_vals::mod::ITEM_ARMOR_BIO );
             break;
         case DT_COLD:
-            du.amount = armor.calculate_by_enchantment( guy, du.amount, enchant_vals::mod::ITEM_ARMOR_COLD );
+            du.amount += armor.bonus_from_enchantments( guy, du.amount, enchant_vals::mod::ITEM_ARMOR_COLD );
             break;
         case DT_CUT:
-            du.amount = armor.calculate_by_enchantment( guy, du.amount, enchant_vals::mod::ITEM_ARMOR_CUT );
+            du.amount += armor.bonus_from_enchantments( guy, du.amount, enchant_vals::mod::ITEM_ARMOR_CUT );
             break;
         case DT_ELECTRIC:
-            du.amount = armor.calculate_by_enchantment( guy, du.amount, enchant_vals::mod::ITEM_ARMOR_ELEC );
+            du.amount += armor.bonus_from_enchantments( guy, du.amount, enchant_vals::mod::ITEM_ARMOR_ELEC );
             break;
         case DT_HEAT:
-            du.amount = armor.calculate_by_enchantment( guy, du.amount, enchant_vals::mod::ITEM_ARMOR_HEAT );
+            du.amount += armor.bonus_from_enchantments( guy, du.amount, enchant_vals::mod::ITEM_ARMOR_HEAT );
             break;
         case DT_STAB:
-            du.amount = armor.calculate_by_enchantment( guy, du.amount, enchant_vals::mod::ITEM_ARMOR_STAB );
+            du.amount += armor.bonus_from_enchantments( guy, du.amount, enchant_vals::mod::ITEM_ARMOR_STAB );
             break;
         default:
             return;
@@ -7784,28 +7778,28 @@ static void armor_enchantment_adjust( Character &guy, damage_unit &du )
 {
     switch( du.type ) {
         case DT_ACID:
-            du.amount = guy.calculate_by_enchantment( du.amount, enchant_vals::mod::ARMOR_ACID );
+            du.amount += guy.bonus_from_enchantments( du.amount, enchant_vals::mod::ARMOR_ACID );
             break;
         case DT_BASH:
-            du.amount = guy.calculate_by_enchantment( du.amount, enchant_vals::mod::ARMOR_BASH );
+            du.amount += guy.bonus_from_enchantments( du.amount, enchant_vals::mod::ARMOR_BASH );
             break;
         case DT_BIOLOGICAL:
-            du.amount = guy.calculate_by_enchantment( du.amount, enchant_vals::mod::ARMOR_BIO );
+            du.amount += guy.bonus_from_enchantments( du.amount, enchant_vals::mod::ARMOR_BIO );
             break;
         case DT_COLD:
-            du.amount = guy.calculate_by_enchantment( du.amount, enchant_vals::mod::ARMOR_COLD );
+            du.amount += guy.bonus_from_enchantments( du.amount, enchant_vals::mod::ARMOR_COLD );
             break;
         case DT_CUT:
-            du.amount = guy.calculate_by_enchantment( du.amount, enchant_vals::mod::ARMOR_CUT );
+            du.amount += guy.bonus_from_enchantments( du.amount, enchant_vals::mod::ARMOR_CUT );
             break;
         case DT_ELECTRIC:
-            du.amount = guy.calculate_by_enchantment( du.amount, enchant_vals::mod::ARMOR_ELEC );
+            du.amount += guy.bonus_from_enchantments( du.amount, enchant_vals::mod::ARMOR_ELEC );
             break;
         case DT_HEAT:
-            du.amount = guy.calculate_by_enchantment( du.amount, enchant_vals::mod::ARMOR_HEAT );
+            du.amount += guy.bonus_from_enchantments( du.amount, enchant_vals::mod::ARMOR_HEAT );
             break;
         case DT_STAB:
-            du.amount = guy.calculate_by_enchantment( du.amount, enchant_vals::mod::ARMOR_STAB );
+            du.amount += guy.bonus_from_enchantments( du.amount, enchant_vals::mod::ARMOR_STAB );
             break;
         default:
             return;
@@ -9735,8 +9729,12 @@ int Character::run_cost( int base_cost, bool diag ) const
             movecost += 10 * footwear_factor();
         }
 
-        movecost = calculate_by_enchantment( movecost, enchant_vals::mod::MOVE_COST );
+        movecost += bonus_from_enchantments( movecost, enchant_vals::mod::MOVE_COST );
         movecost /= stamina_move_cost_modifier();
+
+        if( movecost < 20.0 ) {
+            movecost = 20.0;
+        }
     }
 
     if( diag ) {