Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[READY] Ranged balance unit test #21468

Merged
merged 36 commits into from Sep 27, 2017

Conversation

Projects
None yet
8 participants
@kevingranade
Copy link
Member

commented Jul 23, 2017

First stab at an automated test to ensure ranged balance as outlined in #21244
Fixes #21244
Fixes #19718
Fixes #21158

  • NPCs need gear loadouts, they're currently naked.
  • Decrease quickdraw accuracy.
    - [ ] Apply 5x increase to MAX_RECOIL and related values in a more consistent way.
  • Need to increase mid-range accuracy across the board.
  • Reduce accuracy at very high aim levels in some cases.
    - [ ] Apply dispersion scaling changes to item definitions instead of as an override.
  • Evaluate new use cases.
  • add upper and lower caps on aim speed.
  • communicate new system to player.

@kevingranade kevingranade force-pushed the kevingranade:ranged_balance_unit_test branch Jul 24, 2017

@Coolthulhu

This comment has been minimized.

Copy link
Contributor

commented Jul 24, 2017

RNG has to be there until you write a CD function for your distribution.
CDF for a sum of uniform variables is basically brute force approach: simulate all possible rolls, calculate distribution. Still, most likely better than relying on RNG.
That's a good reason not to use sum of random variables (except maybe as Irvin-Hall, for which there are known formulas).

One important thing those tests should test should be distribution of hit types, not just binary hit/miss.
And even if not test exactly yet, at least print.

@kevingranade kevingranade force-pushed the kevingranade:ranged_balance_unit_test branch Jul 29, 2017

@kevingranade

This comment has been minimized.

Copy link
Member Author

commented Jul 29, 2017

Wild firing without aiming fails, it's too accurate.

@Coolthulhu Coolthulhu self-assigned this Jul 30, 2017

@kevingranade kevingranade force-pushed the kevingranade:ranged_balance_unit_test branch Jul 30, 2017

tests/ranged_balance.cpp Outdated
item &ammo = shooter.i_add( item( ammo_id, calendar::turn, gun.ammo_capacity() ) );
REQUIRE( gun.is_reloadable_with( ammo_id ) );
REQUIRE( shooter.can_reload( gun, ammo_id ) );
gun.reload( shooter, item_location( shooter, &ammo ), gun.ammo_capacity() );

This comment has been minimized.

Copy link
@Coolthulhu

Coolthulhu Jul 30, 2017

Contributor

The test files could use some styling. For example, tabs->spaces.

This comment has been minimized.

Copy link
@kevingranade

kevingranade Jul 30, 2017

Author Member

I'm partially waiting on #21494 and partially just being sloppy since I'm still rewriting large sections of it as I go. Will be 100% astyled by the time I mark it ready to merge.

tests/ranged_balance.cpp Outdated
shooter.get_weapon_dispersion( gun,
rl_dist( shooter.pos(), test_target.pos() ) );
dispersion.add_range( std::max( static_cast<double>( gun.sight_dispersion() ),
shooter.recoil_total() * ( 1.0 - aim_ratio ) ) );

This comment has been minimized.

Copy link
@Coolthulhu

Coolthulhu Jul 30, 2017

Contributor

recoil_total would probably just return RECOIL_MIN, except when something is weirdly initialized or after burst.
But projectile_attack doesn't increase recoil.

This comment has been minimized.

Copy link
@kevingranade

kevingranade Jul 30, 2017

Author Member

Good catch, this isn't doing remotely the right thing.

This comment has been minimized.

Copy link
@kevingranade

kevingranade Jul 30, 2017

Author Member

Actually I think it is doing what I want right now, but it's farly fragile, I should just use RECOL_MIN.

tests/ranged_balance.cpp Outdated

statistics minimum_stats = firing_test( shooter, gun_type, 0.0, minimum_range );
INFO( "Accumulated " << minimum_stats.n() << " samples." );
CHECK( minimum_stats.avg() < 0.01 );

This comment has been minimized.

Copy link
@Coolthulhu

Coolthulhu Jul 30, 2017

Contributor

Why 1%? That's very strict, especially when you're counting grazes and not just good hits.

This comment has been minimized.

Copy link
@kevingranade

kevingranade Jul 30, 2017

Author Member

Why not? If it starts failing we can adjust, but in this case we're asserting "almost never hits", so I'd like to be as strong as is practical.
Note I'm not rejecting your request to factor in hit quality, I'm just filling this out in what I think is the best order, I'll circe back around to check if hit quality is practical to extract once I have the use cases covered.

This comment has been minimized.

Copy link
@Coolthulhu

Coolthulhu Jul 30, 2017

Contributor

Well, the test cases may need adjusting then.
Almost always missing at 5 tiles is comparable to reliably missing broad side of the barn from inside.

This comment has been minimized.

Copy link
@kevingranade

kevingranade Jul 30, 2017

Author Member

You might have missed the detail that this is with no aiming at all, this is effectively the quickdraw scenario.
Also it's not remotely a barn-sized target, it's exactly analagous to shooting at a person-sized and shaped target at the stated range. Depending on how we set up our range adjustment, that 5-tile range will almost certainly be equivalent to 20' or so, at which point a novice always missing a man-sized target when quickdrawing is perfectly reasonable.

This comment has been minimized.

Copy link
@Coolthulhu

Coolthulhu Jul 30, 2017

Contributor

I wouldn't enforce "always misses" on novices. It may be reasonable to expect it, but it only complicates things when balancing.
It does matter for experts - those certainly need to be capped from above, but again - 1% cap is really strict, at this point it's a problematic limitation that doesn't test an in-game situation.

Any hit rate below 10% that includes grazes is well into not being worth it for proper firearms. It would make sense for pneumatics or lasers, where ammunition is very cheap, but firearms are expensive to maintain and also loud.
It's basically a statistical outlier - one that is not worth including in the model.
If you bumped the rate to at least 10%, it would both become more practical and easier to actually enforce without breaking other values.

But then again, going for capping the 10% "good hit" rate would be even better. It would test actual in-game effects, not implementation details.

This comment has been minimized.

Copy link
@kevingranade

kevingranade Jul 31, 2017

Author Member

I'll agree to the extent that assertions about "solid hits" are higher value than assertions about "technical hits", and if we do encounter a deadlock while balancing, the "technical hits" are the ones to relax.

tests/ranged_balance.cpp Outdated
dealt_projectile_attack shot =
projectile_attack( make_gun_projectile( gun ), shooter.pos(), test_target.pos(),
dispersion, &shooter, nullptr );
firing_stats.add( shot.hit_critter != nullptr );

This comment has been minimized.

Copy link
@Coolthulhu

Coolthulhu Jul 30, 2017

Contributor

Would be more meaningful to count only hits above certain threshold of accuracy.
Grazes are relatively common and their damage is at most 1/8 of base damage. For weak firearms, it means no damage against even moderately armored targets.

This comment has been minimized.

Copy link
@kevingranade

kevingranade Jul 30, 2017

Author Member

If it's practical to make assertions about hit quality, I'll also make assertions about it, but grazes aren't meaningless to make assertions about.

@kevingranade kevingranade changed the title Ranged balance unit test [WIP] Ranged balance unit test Jul 30, 2017

@Coolthulhu Coolthulhu removed their assignment Jul 30, 2017

@kevingranade kevingranade force-pushed the kevingranade:ranged_balance_unit_test branch 2 times, most recently Aug 2, 2017

@kevingranade

This comment has been minimized.

Copy link
Member Author

commented Aug 2, 2017

Needed a massive overhaul for performance reasons, it was taking multiple seconds to complete, and it's just going to continue to get worse over time. (after changes they're running in under 0.05s)
Two major changes were to call projectile_attack_roll() and make assertions about missed_by (solid order of magnitude improvement) and to inject the desired thresholds into the test fixture so it could bail out once the confidence intervals shrunk to not include any desired thresholds. This also allows us to make assertions about several hit qualities at the same range, but that might end up being unecessary.

This covers the bases outlined in #21244 but needs some additional tweaking and then of course the actual balance changes.

  • NPCs need gear loadouts, they're currently naked.
  • Guns need to be geared up when appropriate, currently it only uses vanilla guns.

@kevingranade kevingranade force-pushed the kevingranade:ranged_balance_unit_test branch Aug 2, 2017

@kevingranade kevingranade force-pushed the kevingranade:ranged_balance_unit_test branch 4 times, most recently Aug 17, 2017

@kevingranade

This comment has been minimized.

Copy link
Member Author

commented Aug 19, 2017

Constraint for quickdraw turned out to be overly-agressive after all, it took dropping it to one hit out of 10, increasing the range slightly and increasing MAX_RECOIL (and associated values) by 5x to enforce it properly, but that did it and it seems like a workable adjustment, the original MAX_RECOIL was just a shot in the dark after all rimshot.

@kevingranade

This comment has been minimized.

Copy link
Member Author

commented Aug 19, 2017

For context, "completely unaimed" is now 3,000 MOA, which in more accessable terms, is roughly 3" (radius) per tle of distance. That doesn't sound like much, but it's 5x as high as it used to be, and it's plenty to make you miss when added to other sources of inaccuracy. I'm pretty happy with that result, it actually gives us a basis for what unaimed accuracy should be as opposed to it being totally arbitrary.

This does cause a problem though, now that aiming is a lot faster, it's more of a no-brainer to keep aiming to the maximum, since the spike in accuracy at the very end is extremely high.
I think we need a progressive aim speed, but before that, we need some expectations around aiming speed as well, otherwise the fixes to accuracy are quite likely to end up wrecking other parts of the system.

@Firestorm01X2

This comment has been minimized.

Copy link
Contributor

commented Aug 23, 2017

How about add relation between aming speed and player weapon skill so unexpirienced player take aim slower?

@kevingranade

This comment has been minimized.

Copy link
Member Author

commented Aug 23, 2017

It's already there.

@kevingranade

This comment has been minimized.

Copy link
Member Author

commented Aug 27, 2017

Additional info, I wrote a test to find the dispersion value where hit rate exceeds 50% at various ranges, and here's the output:
Range: 1 Dispersion: 1785
Range: 2 Dispersion: 891
Range: 3 Dispersion: 582
Range: 4 Dispersion: 426
Range: 5 Dispersion: 342
Range: 6 Dispersion: 301
Range: 7 Dispersion: 245
Range: 8 Dispersion: 216
Range: 9 Dispersion: 192
Range: 10 Dispersion: 169
Range: 11 Dispersion: 147
Range: 12 Dispersion: 146
Range: 13 Dispersion: 133
Range: 14 Dispersion: 120
Range: 15 Dispersion: 114
Range: 16 Dispersion: 107
Range: 17 Dispersion: 101
Range: 18 Dispersion: 95
Range: 19 Dispersion: 90
Range: 20 Dispersion: 85
Range: 21 Dispersion: 82
Range: 22 Dispersion: 78
Range: 23 Dispersion: 74
Range: 24 Dispersion: 72
Range: 25 Dispersion: 69
Range: 26 Dispersion: 66
Range: 27 Dispersion: 63
Range: 28 Dispersion: 49
Range: 29 Dispersion: 49
Range: 30 Dispersion: 49
Range: 31 Dispersion: 49
Range: 32 Dispersion: 49
Range: 33 Dispersion: 49
Range: 34 Dispersion: 49
Range: 35 Dispersion: 49
Range: 36 Dispersion: 49
Range: 37 Dispersion: 46
Range: 38 Dispersion: 45
Range: 39 Dispersion: 44
Range: 40 Dispersion: 42
Range: 41 Dispersion: 41
Range: 42 Dispersion: 40
Range: 43 Dispersion: 40
Range: 44 Dispersion: 39
Range: 45 Dispersion: 38
Range: 46 Dispersion: 37
Range: 47 Dispersion: 36
Range: 48 Dispersion: 36
Range: 49 Dispersion: 34
Range: 50 Dispersion: 34
Range: 51 Dispersion: 34
Range: 52 Dispersion: 33
Range: 53 Dispersion: 32
Range: 54 Dispersion: 21
Range: 55 Dispersion: 21
Range: 56 Dispersion: 21
Range: 57 Dispersion: 21
Range: 58 Dispersion: 21
Range: 59 Dispersion: 21
Range: 60 Dispersion: 21

@kevingranade kevingranade force-pushed the kevingranade:ranged_balance_unit_test branch 2 times, most recently Aug 31, 2017

@kevingranade kevingranade changed the title [WIP] Ranged balance unit test [CR] Ranged balance unit test Sep 2, 2017

src/character.cpp Outdated
int limit = 0;
if( !gun.has_flag( "DISABLE_SIGHTS" ) && effective_dispersion( gun.type->gun->sight_dispersion ) < recoil ) {
cost = std::max( std::min( gun.volume() / 250_ml, 8 ), 1 );

This comment has been minimized.

Copy link
@kevingranade

kevingranade Sep 2, 2017

Author Member

Balance aside, this is a bugfix. The previous version based aiming speed for unmodded guns on gun size, then completely ignored it once a sight ws added.

@kevingranade kevingranade force-pushed the kevingranade:ranged_balance_unit_test branch Sep 26, 2017

@kevingranade kevingranade force-pushed the kevingranade:ranged_balance_unit_test branch to ae61a67 Sep 26, 2017

@kevingranade

This comment has been minimized.

Copy link
Member Author

commented Sep 26, 2017

Ok, that's everything except the overhaul of range_with_even_chance_of_good_hit().

I made the multipliers for dex and per the same (2.25), which is the furthest I could adjust it without making the tests start failing.

@Coolthulhu

This comment has been minimized.

Copy link
Contributor

commented Sep 27, 2017

All right, let's get it in for now. The rebases get heavy and it's good enough for now.

Problems not fixed yet:

  • Dexterity is strictly better than perception. The tests should not break as long as sum of modifiers from dex+perception is ~5, meaning it should be safe to make perception give 5 per point and dexterity not help dispersion at all (just aim speed)
  • The new TICKS_TO_SECONDS function returns int. We want float, otherwise it rounds wayyy too much. Also, the name is misleading: everywhere else those units are called "moves", while ticks are variable sized.
  • As a consequence of the rounding above, "time to reach" always gets two trailing zeroes. Some extra precision isn't bad (one decimal would be perfect, because difference between 2.1 and 2.9 is almost 30%), but that ugly truncation above breaks it.
  • Shotguns don't have own test or any other measure of whether they are balanced or not
  • The "regular aim" etc. pseudo-headers in gun item display for guns aren't colored in any way, which makes it harder to spot where they start and end at a glance
  • Aiming is too fast and doesn't slow down fast enough. This brings back part of the "aim perfectly or not at all" dysfunction. Aiming speed should probably get its own set of tests later.

@Coolthulhu Coolthulhu merged commit 41e8209 into CleverRaven:master Sep 27, 2017

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.7%) to 23.215%
Details
gorgon-ghprb Build finished.
Details
@Coolthulhu

This comment has been minimized.

Copy link
Contributor

commented Sep 27, 2017

Shit, should have been more careful - I totally missed the part where the tests for high-level characters got adjusted in the wrong way and relaxed into uselessness.

It still solves the part where guns are totally useless that resulted from the revert earlier on, but does not achieve balance.

@Firestorm01X2

This comment has been minimized.

Copy link
Contributor

commented Sep 27, 2017

Create issue on that. Better with "Priority" tag.

@Firestorm01X2

This comment has been minimized.

Copy link
Contributor

commented Sep 27, 2017

There is chance that this PR caused crashes on throwing. Look for referenced issue.

@DanmakuDan

This comment has been minimized.

Copy link
Contributor

commented Sep 27, 2017

In ranged.cpp,

int time_to_fire( const Character &p, const itype &firingt )

The item being thrown expects a gun type to be associated with it, but there is only a null pointer available for items that can only be thrown (firingt.gun.get() = 0x0), causing the crash in #22001.

const skill_id &skill_used = firingt.gun.get()->skill_used;

The game doesn't crash if you try to throw a gun. This apparently means if I try to throw a pistol, it will use my pistol skill to determine the time it takes to throw it. I tested changing handgun level 0 to level 20, and it caused the aim expected throw time to decrease from 80 to 10.

Additionally, the actual throw time cost is calculated separately at int throw_cost( const player &c, const item &to_throw ) after a target has been confirmed.

@kevingranade kevingranade deleted the kevingranade:ranged_balance_unit_test branch Sep 28, 2017

@Sagan-Ro

This comment has been minimized.

Copy link

commented Oct 13, 2017

Okay, so I have a few questions that are really bugging me:

  1. Why does my laser rifle on burst fire hit an enemy a couple of times then start shooting to the sides of the enemy. As in two-to-five tiles left and right of my target's actual position? If that is due to "recoil"... lasers aren't supposed to have recoil, right?

  2. Why does my character, even when debugged to level 40 skill in everything with 20 STR/INT/PRC/DEX with a scoped and fully outfitted M107 .50 caliber sniper rifle capable of accurately killing human-sized targets over a mile away... LITERALLY have trouble hitting a target across the street? In what universe could that possibly make sense?

I am really, really hoping the absolutely craptastic new "performance" of long-range weaponry and burst firing is just because the new system isn't finished yet. I was playing release 6723 for over a month, and could tag enemies more than 2/3 the time with the Heavy Rail Rifle (scope+gyroscopic stabilizer+tuned mechanism+match trigger) around 55+ squares away. Now I just get grazing hits over 20 squares, and any hit at all seems to go to zero at or past 25 squares.

Thank you so much, I would really like it if my sniper could actually, um... snipe. It's pointless to have long-distance weapons in this game if long-distance attacks are no longer possible.

@kevingranade

This comment has been minimized.

Copy link
Member Author

commented Oct 13, 2017

Please don't post in closed issues.

@CleverRaven CleverRaven locked and limited conversation to collaborators Oct 13, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.