Enchantment/relic cleanup 2 #400

olanti-p · 2021-03-16T14:01:41Z

Purpose of change

Tweak the math a bit when multiple enchantments modify the same value.
Makes it so bonuses don't affect each other in weird ways, but rather are
calculated based on same baseline and then applied simultaneously.
Check, implement and/or fix enchantments that modify the following values:
- STRENGTH
- DEXTERITY
- PERCEPTION
- INTELLIGENCE
- SPEED
- ATTACK_COST and item-specific ITEM_ATTACK_COST
- MOVE_COST
- METABOLISM
- MANA_CAP
- MANA_REGEN
Add tests for enchantments, moves gain and mana pool
Update docs for enchantments

Describe alternatives you've considered

Porting the code from DDA repo, but after encountering various merge conflicts, bugs (like CleverRaven#47450) and lack of tests, decided writing everything from scratch would save time on bugfixing later.

Testing

Unit tests; tried relics with these enchantments in-game.

Additional context

Using process_turn() is likely to collect a lot of global state related failures, but I'd rather keep it, as it makes the tests work close to the "real thing".

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

Coolthulhu · 2021-03-16T16:17:03Z

src/magic_enchantment.cpp


-    guy.mod_num_dodges_bonus( get_value_add( enchant_vals::mod::BONUS_DODGE ) );
-    guy.mod_num_dodges_bonus( mult_bonus( enchant_vals::mod::BONUS_DODGE, guy.get_num_dodges_base() ) );
+    guy.mod_num_dodges_bonus( calc_bonus( enchant_vals::mod::BONUS_DODGE, guy.get_num_dodges_base() ) );


"BONUS_DODGE" is another unfortunate name. It sounds like bonus to the ability to dodge, but actually adds dodge attempts before penalty.

My guess is it should've been BONUS_DODGES, which would've conveyed the intended effect.

Coolthulhu · 2021-03-16T16:18:17Z

src/melee.cpp

@@ -419,12 +419,12 @@ void Character::melee_attack( Creature &t, bool allow_special, const matec_id &f
    }
    item &cur_weapon = allow_unarmed ? used_weapon() : weapon;

-    if( cur_weapon.attack_time() > attack_speed( cur_weapon ) * 20 ) {
+    if( cur_weapon.attack_cost() > attack_cost( cur_weapon ) * 20 ) {


Not really changed here, but that sure is a puzzling line.

Coolthulhu · 2021-03-16T17:57:03Z

I'm getting test failures in the new tests, all of them off with the character getting +1 in a stat or speed.

Big block

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cata_test is a Catch v2.12.0 host application.
Run with -? for options

Randomness seeded to: 1615917198

-------------------------------------------------------------------------------
Enchantments modify stats
  base stats 8
       When: Character receives relic
   And when: Turn passes
       Then: Stats are modified and don't overflow
-------------------------------------------------------------------------------
enchantment_test.cpp:189
...............................................................................

enchantment_test.cpp:171: FAILED:
  CHECK( guy.get_per() == p )
with expansion:
  6 == 5

-------------------------------------------------------------------------------
Enchantments modify stats
  base stats 8
       When: Character receives relic
   And when: Turn passes
   And when: Character loses relic
       Then: Stats remain unchanged
-------------------------------------------------------------------------------
enchantment_test.cpp:194
...............................................................................

enchantment_test.cpp:171: FAILED:
  CHECK( guy.get_per() == p )
with expansion:
  6 == 5

-------------------------------------------------------------------------------
Enchantments modify stats
  base stats 12
       When: Character receives relic
   And when: Turn passes
       Then: Stats are modified and don't overflow
-------------------------------------------------------------------------------
enchantment_test.cpp:189
...............................................................................

enchantment_test.cpp:171: FAILED:
  CHECK( guy.get_per() == p )
with expansion:
  8 == 7

-------------------------------------------------------------------------------
Enchantments modify stats
  base stats 12
       When: Character receives relic
   And when: Turn passes
   And when: Character loses relic
       Then: Stats remain unchanged
-------------------------------------------------------------------------------
enchantment_test.cpp:194
...............................................................................

enchantment_test.cpp:171: FAILED:
  CHECK( guy.get_per() == p )
with expansion:
  8 == 7

-------------------------------------------------------------------------------
Enchantments modify stats
  base stats 4
       When: Character receives relic
   And when: Turn passes
       Then: Stats are modified and don't overflow
-------------------------------------------------------------------------------
enchantment_test.cpp:189
...............................................................................

enchantment_test.cpp:171: FAILED:
  CHECK( guy.get_per() == p )
with expansion:
  4 == 3

-------------------------------------------------------------------------------
Enchantments modify stats
  base stats 4
       When: Character receives relic
   And when: Turn passes
   And when: Character loses relic
       Then: Stats remain unchanged
-------------------------------------------------------------------------------
enchantment_test.cpp:194
...............................................................................

enchantment_test.cpp:171: FAILED:
  CHECK( guy.get_per() == p )
with expansion:
  4 == 3

-------------------------------------------------------------------------------
Enchantments modify speed
  base = 100
       When: Character receives relic
   And when: Turn passes
       Then: Speed changes, moves gain changes
-------------------------------------------------------------------------------
enchantment_test.cpp:258
...............................................................................

enchantment_test.cpp:238: FAILED:
  REQUIRE( guy.get_speed() == speed )
with expansion:
  76 == 75

-------------------------------------------------------------------------------
Enchantments modify speed
  base = 100
       When: Character receives relic
   And when: Turn passes
   And when: Character loses relic
       Then: Nothing changes
-------------------------------------------------------------------------------
enchantment_test.cpp:263
...............................................................................

enchantment_test.cpp:238: FAILED:
  REQUIRE( guy.get_speed() == speed )
with expansion:
  76 == 75

-------------------------------------------------------------------------------
Enchantments modify speed
  base = 120
       When: Character receives relic
   And when: Turn passes
       Then: Speed changes, moves gain changes
-------------------------------------------------------------------------------
enchantment_test.cpp:258
...............................................................................

enchantment_test.cpp:238: FAILED:
  REQUIRE( guy.get_speed() == speed )
with expansion:
  86 == 85

-------------------------------------------------------------------------------
Enchantments modify speed
  base = 120
       When: Character receives relic
   And when: Turn passes
   And when: Character loses relic
       Then: Nothing changes
-------------------------------------------------------------------------------
enchantment_test.cpp:263
...............................................................................

enchantment_test.cpp:238: FAILED:
  REQUIRE( guy.get_speed() == speed )
with expansion:
  86 == 85

===============================================================================
test cases:   8 |   6 passed |  2 failed
assertions: 410 | 400 passed | 10 failed

Ended test at Tue Mar 16 18:53:30 2021
The test took 3.117 seconds

18:53:30.097 INFO : Randomness seeded to: 1615917198

Platform:

gcc (Ubuntu 9.3.0-17ubuntu1~20.04) 9.3.0

Building with:

make RELEASE=1 TILES=1 SOUND=1 PROFILE=-pg LINTJSON=0 OPTLEVEL=-O3 -k

olanti-p · 2021-03-17T11:39:31Z

Turns out JSON double parser produces different values for different compilers.

Parsing -0.5 gives -0.5 for clang version 10.0.0-4ubuntu1 and -0.499999999999999889 for gcc version 9.3.0 (Ubuntu 9.3.0-17ubuntu1~20.04). This difference is enough to screw up truncation...

I guess I'll just limit minimum multiplicative bonus to some reasonable amount (0.00001 should be more than enough).

src/magic_enchantment.cpp

Coolthulhu · 2021-03-17T16:37:39Z

src/creature.cpp

+
+void Creature::clear_effects_for_testing()
+{
+    effects->clear();


Does it need to exist? It may be a bug in how I designed effect removal if it does.

No idea, it was me checking if effects were correctly removed (they seem to)

Coolthulhu · 2021-03-17T16:46:28Z

doc/MAGIC.md

-  "add": 13.37,
+  // Additive bonus. Optional integer number, default is 0.
+  // May be ignored for some values.
+  "add": 13,


Good catch there - most of the code pretty clearly tries to imply that it's a double, by using doubles to accumulate it.

I actually thought about converting it to double, but there doesn't seem to be any use cases to warrant that (yet?)

In theory, it would work if multiple effects had subtle effects that only make sense when added together.
But it sounds like UI trouble to even try.

olanti-p added 13 commits March 16, 2021 11:48

Additional cleanup for tests

cbe7218

Tests for effects & mutations from enchantments

ad9e849

Enchantment values: stats

7bac741

Enchantment values: SPEED

dcf4664

Some tests for speed / moves gain

a7f2118

Simplify check for enchantment 'has' condition

3c36a06

Clean up enchantment bonus calculation

9224986

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

2dec401

Enchantment values: ATTACK_COST, ITEM_ATTACK_COST

303df00

Enchantment values: MOVE_COST

1153a8b

Enchantment values: METABOLISM

a6ed4db

Enchantment values: MANA_CAP, MANA_REGEN + mana tests

80bdfb4

Migration for old enchant val enum strings

4dc21a6

Coolthulhu reviewed Mar 16, 2021

View reviewed changes

Consistent rounding for enchantment bonuses

a0103f6

olanti-p commented Mar 17, 2021

View reviewed changes

src/magic_enchantment.cpp Outdated Show resolved Hide resolved

fix typo

7a2ed32

Coolthulhu reviewed Mar 17, 2021

View reviewed changes

Revert effects cleanup change

998abb5

Coolthulhu merged commit b741a5d into cataclysmbnteam:upload Mar 18, 2021

olanti-p deleted the ench-cleanup-2 branch March 19, 2021 14:19

olanti-p mentioned this pull request Aug 28, 2022

[TODO] Finish moving from artifacts to relics #1816

Open

olanti-p mentioned this pull request Dec 30, 2022

Enchantment/relic cleanup 3 #2264

Merged

Maleclypse mentioned this pull request Jan 7, 2023

Improving the Test suite for enchantments CleverRaven/Cataclysm-DDA#62875

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enchantment/relic cleanup 2 #400

Enchantment/relic cleanup 2 #400

olanti-p commented Mar 16, 2021 •

edited

Loading

Coolthulhu Mar 16, 2021

chaosvolt Mar 17, 2021

Coolthulhu Mar 16, 2021

Coolthulhu commented Mar 16, 2021

olanti-p commented Mar 17, 2021 •

edited

Loading

Coolthulhu Mar 17, 2021

olanti-p Mar 17, 2021

Coolthulhu Mar 17, 2021

olanti-p Mar 17, 2021 •

edited

Loading

Coolthulhu Mar 17, 2021

Enchantment/relic cleanup 2 #400

Enchantment/relic cleanup 2 #400

Conversation

olanti-p commented Mar 16, 2021 • edited Loading

Purpose of change

Describe alternatives you've considered

Testing

Additional context

Coolthulhu Mar 16, 2021

Choose a reason for hiding this comment

chaosvolt Mar 17, 2021

Choose a reason for hiding this comment

Coolthulhu Mar 16, 2021

Choose a reason for hiding this comment

Coolthulhu commented Mar 16, 2021

olanti-p commented Mar 17, 2021 • edited Loading

Coolthulhu Mar 17, 2021

Choose a reason for hiding this comment

olanti-p Mar 17, 2021

Choose a reason for hiding this comment

Coolthulhu Mar 17, 2021

Choose a reason for hiding this comment

olanti-p Mar 17, 2021 • edited Loading

Choose a reason for hiding this comment

Coolthulhu Mar 17, 2021

Choose a reason for hiding this comment

olanti-p commented Mar 16, 2021 •

edited

Loading

olanti-p commented Mar 17, 2021 •

edited

Loading

olanti-p Mar 17, 2021 •

edited

Loading