melee: Make sure that melee weapon accuracy improves hit rates #39564

mlangsdorf · 2020-04-14T15:59:18Z

Summary

SUMMARY: Bugfixes "melee: Make sure that melee weapon accuracy improves hit rates"

Purpose of change

jtbw suggested in #39362 that unit tests would be helpful for ensuring that the simplified DPS
calculation would match the actual DPS values observed by going through the melee attack code.

In the process of implementing that suggestion, it became apparent that melee weapon accuracy
had little or no bearing on the chance to hit with a weapon. weapon.type->m_to_hit was only being applied via mod_hit_bonus() in avatar::reset_stats() which was called from avatar::reset(), so it seemed unlikely that changing weapons would necessarily change the
hit bonus.

Fix that bug, so that melee weapon accuracy clearly effects hit rates, and add unit test cases to make sure that future regressions will be caught. Then go back to the original point of the exercise and add unit test cases for effective dps, and fix effective_dps to match the results of those test cases. Finally, extend effective_dps so that NPCs will use it to value weapons.

Describe the solution

There are a lot of moving parts here:
wapcaplet wrote the initial effective DPS test cases based on some pseudo-code I wrote. Those are in #39476 and this PR includes and supersedes that one.

The melee weapon accuracy fix moves weapon.type->m_to_hit out of get_hit_bonus() and into get_hit_weapon() as a static bonus after calculating skill bonuses. Adding unit tests cases for that required the ability to easily count the number of successful attacks, and after considering the options, I decided that adding an out-of-band set of statistic counters was easier than rewriting the melee attack sequence to be accessible for test cases.

The effective DPS test cases demonstrated that the algorithm for effective_dps had a lot of problems, so parts of it were rewritten to correctly calculate critical hit chances and the damage calculation was rewritten to more closely match the melee code. At the same time, the code was adjusted so that callers could specify if they wanted monsters for display or monsters for evaluation, and calculations for unneeded monsters were skipped.

The DPS unit tests repeat running trials in blocks of 1000 until 100 crits occur in an effort to reduce the burstiness of the results without requiring that the tests run for 90 seconds.

Finally, there was no reason to keep an alternate set of melee DPS calculations in weapon_value(), so that got removed and replaced by using the effective_dps() infrastructure.

Describe alternatives you've considered

player::melee_attack() has an extremely weird structure, with code flow jumping back and forth between the attacker and defender in weird ways. Restructuring it to have a more logical flow and to be easier to test would be great, but it would also be a large project. It should be easier now that there are unit test cases to confirm that changes are just refactors and don't have accidental semantic changes.

Testing

I spent hours and hours running test cases to make all this work. The proof is that the test cases mostly work. Actual calculated dps over 1000s of trials can still vary by more than 20% in extreme cases, and I'm not sure what to do about that.

jbytheway · 2020-04-14T16:25:37Z

src/melee.h

+/*
+ * statistics data for melee attacks, used for test purposes
+ */
+struct melee_statistic_data {


These melee stats are a good candidate to migrate to the event bus, then the tests can hook in to the hit events that way. I'm not asking you to do that in this PR, because melee events aren't a thing yet; this is more of a note-to-self.

Sounds great =)

mlangsdorf · 2020-04-14T17:26:59Z

clang-tidy errors are not my fault; code had them already.

kevingranade · 2020-04-14T18:27:15Z

Yep, that's pending #39562

lgtm-com · 2020-04-14T18:59:31Z

This pull request fixes 1 alert when merging 0b459de into 32e6aab - view on LGTM.com

fixed alerts:

1 for Use of c-style math functions

lgtm-com · 2020-04-15T20:50:06Z

This pull request fixes 1 alert when merging b9463f9 into 5db5df0 - view on LGTM.com

fixed alerts:

1 for Use of c-style math functions

mlangsdorf · 2020-04-17T11:06:29Z

WTF, Xcode on MacOS 10.1, why are you consistently failing the accuracy tests:

effective_dps_test.cpp:90: FAILED:
  CHECK( wpn2_hit_rate > wpn1_hit_rate )
with expansion:
  0.566 > 0.5725
2.357 s: soldier zombie
2.358 s: accuracy increases success
-------------------------------------------------------------------------------
accuracy increases success
  smoker zombie
-------------------------------------------------------------------------------
effective_dps_test.cpp:207
...............................................................................
effective_dps_test.cpp:90: FAILED:
  CHECK( wpn2_hit_rate > wpn1_hit_rate )
with expansion:
  0.3316 > 0.3365
effective_dps_test.cpp:92: FAILED:
  CHECK( dps_wpn2 > dps_wpn1 )
with expansion:
  12.4415197372 > 12.4911880493
5.160 s: smoker zombie
5.161 s: accuracy increases success
-------------------------------------------------------------------------------
accuracy increases success
  survivor zombie
-------------------------------------------------------------------------------
effective_dps_test.cpp:211
...............................................................................
effective_dps_test.cpp:91: FAILED:
  CHECK( wpn3_hit_rate > wpn2_hit_rate )
with expansion:
  0.4173333333 > 0.4226666667

lgtm-com · 2020-04-17T20:28:28Z

This pull request fixes 1 alert when merging 93296d2 into 5dd7e74 - view on LGTM.com

fixed alerts:

1 for Use of c-style math functions

lgtm-com · 2020-04-18T05:34:52Z

This pull request fixes 1 alert when merging e20aa3a into 5dd7e74 - view on LGTM.com

fixed alerts:

1 for Use of c-style math functions

mlangsdorf · 2020-04-18T13:49:01Z

fris0uman inadvertently added a regression with his recent refactoring of melee.cpp to move things from player:: to Character::, which the test caught erratically. A bugfix was added.

lgtm-com · 2020-04-18T16:11:02Z

This pull request fixes 1 alert when merging 0e7c596 into f94cfe1 - view on LGTM.com

fixed alerts:

1 for Use of c-style math functions

Results are closer to expectations now, though not quite there yet. ex. trench knife vs. debug monster: Actual 13.0157, Expected 20.160

commit 8ef3706 moved a bunch of melee related functions out of player:: and into Character::, but failed to call the new renamed version of get_hit() correctly.

Fix some long lines and use std:: functions in place of math.h functions in a few places.

Move the accuracy bonus out of the avatar's hit bonuses where they may or may not be set correctly via reset() and into get_hit_weapon() where they are definitely set and applied correctly. Add some statistics counters to the melee code to make writing test cases easier. Add unit tests to verify that increase a weapon's accuracy increases to hit rates and DPS.

Don't calculate dps for monsters that aren't going to be displayed. Adjust the unit tests based on relative values (epsilon) instead of fixed values (margin).

use average of effective dps against evaluation monsters in melee_value(), with an additional bonus for reach weapons and for weapons that work with a martial art the NPC knows.

Include the Melee Evaluation spreadsheet and a description of it, and update GAME_BALANCE.md with the new formulas.

lgtm-com · 2020-04-18T19:55:58Z

This pull request fixes 1 alert when merging cedb55f into 074dd18 - view on LGTM.com

fixed alerts:

1 for Use of c-style math functions

mlangsdorf · 2020-04-19T00:10:56Z

Yay!

jbytheway reviewed Apr 14, 2020

View reviewed changes

mlangsdorf mentioned this pull request Apr 14, 2020

Add unit tests for get_hit, get_dodge and related functions #39491

Merged

mlangsdorf force-pushed the melee_accuracy_fix branch from 0b459de to b9463f9 Compare April 15, 2020 17:25

mlangsdorf force-pushed the melee_accuracy_fix branch from ab44527 to b9463f9 Compare April 17, 2020 17:36

mlangsdorf force-pushed the melee_accuracy_fix branch 4 times, most recently from 72770fb to 977e3fb Compare April 18, 2020 01:48

wapcaplet mentioned this pull request Apr 18, 2020

[WIP] Test effective damage per second #39476

Closed

mlangsdorf force-pushed the melee_accuracy_fix branch from 977e3fb to e20aa3a Compare April 18, 2020 03:14

mlangsdorf force-pushed the melee_accuracy_fix branch from e20aa3a to 0e7c596 Compare April 18, 2020 13:46

This was referenced Apr 18, 2020

melee: make polearms do less damage against adjacent targets #39694

Merged

Unarmed weapon balance (Part 1 of melee weapon balance project) #39644

Merged

wapcaplet added 2 commits April 18, 2020 12:34

Add tests for damage per second

0b3ba14

Improve DPS trials with better re-initialization

69afce4

Results are closer to expectations now, though not quite there yet. ex. trench knife vs. debug monster: Actual 13.0157, Expected 20.160

mlangsdorf added 6 commits April 18, 2020 12:34

melee: actually call get_melee_hit_base()

d24b8d3

commit 8ef3706 moved a bunch of melee related functions out of player:: and into Character::, but failed to call the new renamed version of get_hit() correctly.

melee: clean up the code slightly

fef593b

Fix some long lines and use std:: functions in place of math.h functions in a few places.

item: improve the effective dps display

2d6a48a

Don't calculate dps for monsters that aren't going to be displayed. Adjust the unit tests based on relative values (epsilon) instead of fixed values (margin).

NPCs: use effective dps for melee_value()

a4f50b1

use average of effective dps against evaluation monsters in melee_value(), with an additional bonus for reach weapons and for weapons that work with a martial art the NPC knows.

melee: update the docs for the changes

cedb55f

Include the Melee Evaluation spreadsheet and a description of it, and update GAME_BALANCE.md with the new formulas.

mlangsdorf force-pushed the melee_accuracy_fix branch from 21a9d19 to cedb55f Compare April 18, 2020 17:34

kevingranade merged commit 61d262e into CleverRaven:master Apr 19, 2020

mlangsdorf deleted the melee_accuracy_fix branch April 19, 2020 00:11

weekly-digest bot mentioned this pull request Apr 19, 2020

Weekly Digest (12 April, 2020 - 19 April, 2020) #39724

Closed

TealcOneill mentioned this pull request Feb 12, 2022

Zweihander fix #55321

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

melee: Make sure that melee weapon accuracy improves hit rates #39564

melee: Make sure that melee weapon accuracy improves hit rates #39564

mlangsdorf commented Apr 14, 2020 •

edited

Loading

jbytheway Apr 14, 2020

mlangsdorf Apr 14, 2020

mlangsdorf commented Apr 14, 2020

kevingranade commented Apr 14, 2020

lgtm-com bot commented Apr 14, 2020

lgtm-com bot commented Apr 15, 2020

mlangsdorf commented Apr 17, 2020

lgtm-com bot commented Apr 17, 2020

lgtm-com bot commented Apr 18, 2020

mlangsdorf commented Apr 18, 2020

lgtm-com bot commented Apr 18, 2020

lgtm-com bot commented Apr 18, 2020

mlangsdorf commented Apr 19, 2020

melee: Make sure that melee weapon accuracy improves hit rates #39564

melee: Make sure that melee weapon accuracy improves hit rates #39564

Conversation

mlangsdorf commented Apr 14, 2020 • edited Loading

Summary

Purpose of change

Describe the solution

Describe alternatives you've considered

Testing

jbytheway Apr 14, 2020

Choose a reason for hiding this comment

mlangsdorf Apr 14, 2020

Choose a reason for hiding this comment

mlangsdorf commented Apr 14, 2020

kevingranade commented Apr 14, 2020

lgtm-com bot commented Apr 14, 2020

lgtm-com bot commented Apr 15, 2020

mlangsdorf commented Apr 17, 2020

lgtm-com bot commented Apr 17, 2020

lgtm-com bot commented Apr 18, 2020

mlangsdorf commented Apr 18, 2020

lgtm-com bot commented Apr 18, 2020

lgtm-com bot commented Apr 18, 2020

mlangsdorf commented Apr 19, 2020

mlangsdorf commented Apr 14, 2020 •

edited

Loading