Join GitHub today
GitHub is home to over 36 million developers working together to host and review code, manage projects, and build software together.
Sign upFramework for adjusting gun ranges [RDY] #17656
Conversation
mugling
added some commits
Jul 12, 2016
mugling
force-pushed the
mugling:arc
branch
to
5df5f31
Jul 12, 2016
This comment has been minimized.
This comment has been minimized.
What does spanning from nil to maximum aim mean? That a 50% chance of a good hit is possible at nil aim or at maximum aim? |
codemime
reviewed
Jul 12, 2016
| double dispersion = get_weapon_dispersion( &gun, false ) + penalty + driving; | ||
|
|
||
| // cap at min 1MOA as at zero dispersion would result in an infinite effective range | ||
| dispersion = std::max( dispersion, 1.0 ); |
This comment has been minimized.
This comment has been minimized.
codemime
Jul 12, 2016
Member
The dispersion still can become zero on the next line (given zero chance). I guess this check should be either moved down below or dropped entirely (it's not a big deal since min( +x, +INF ) is always +x, but it must be guaranteed that accuracy is always greater than zero. Troubles otherwise).
This comment has been minimized.
This comment has been minimized.
mugling
Jul 12, 2016
Author
Contributor
The question is do you show 0 or maximum range for a chance of zero. I'm leaning towards the latter as this would this function to eventually absorb item::gun_range
codemime
reviewed
Jul 12, 2016
| @@ -552,6 +552,10 @@ class npc : public player | |||
| npc &operator=(const npc &) = default; | |||
| npc &operator=(npc &&) = default; | |||
| ~npc() override; | |||
|
|
|||
| /** Construct an NPC with standard stats */ | |||
| static npc standard(); | |||
This comment has been minimized.
This comment has been minimized.
codemime
Jul 12, 2016
Member
I don't particularly like it here. This function is used only for debugging purposes (statistics, testing) and the name looks like a getter to me, but it's a constructor.
This comment has been minimized.
This comment has been minimized.
mugling
Jul 12, 2016
Author
Contributor
Possibly make_standard? I want to start using it more where we have fake NPC's as opposed to manually constructing them.
codemime
reviewed
Jul 12, 2016
| @@ -192,8 +190,16 @@ dealt_projectile_attack Creature::projectile_attack( const projectile &proj_arg, | |||
| trajectory = g->m.find_clear_path( source, target ); | |||
| } | |||
|
|
|||
| add_msg( m_debug, "%s proj_atk: shot_dispersion: %.2f", | |||
| disp_name().c_str(), shot_dispersion ); | |||
| if( proj_effects.count( "MUZZLE_SMOKE" ) ) { | |||
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
If effective range is reported as |
mugling
added some commits
Jul 12, 2016
This comment has been minimized.
This comment has been minimized.
|
Ah. I see. That makes a fair amount of sense after explanation, but it |
codemime
reviewed
Jul 12, 2016
View changes
| @@ -554,7 +554,7 @@ class npc : public player | |||
| ~npc() override; | |||
|
|
|||
| /** Construct an NPC with standard stats */ | |||
| static npc standard(); | |||
| static npc make_standard(); | |||
This comment has been minimized.
This comment has been minimized.
codemime
Jul 12, 2016
•
Member
That's better. Yet another option is make_fake() (assigns the flag) or even npc( bool fake = false ) or npc( bool random = true ) (pros: it's a constructor, we don't need to know what "standard" means and why other NPCs aren't standard; cons: passing boolean flags is evil - can be solved with an enum). However, the best possible option is getting rid of fake NPCs entirely.
This comment has been minimized.
This comment has been minimized.
mugling
Jul 12, 2016
Author
Contributor
I don't want to confound it with creation of fake NPC's which I agree are a bad idea. The purpose of the named constructor is I want to define a standard NPC for test cases which can be updated in one place if our definition of standard varies.
This comment has been minimized.
This comment has been minimized.
BevapDin
Jul 12, 2016
Contributor
An effectively empty fake_npc or standard_npc class could solve this:
class fake_npc : public npc {
using npc::npc;
};
void test() {
fake_npc my_npc;
my_npc.do_something();
}It allows using all the constructor of npc, but gives it a separate name.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
mugling
Jul 12, 2016
Author
Contributor
allows using all the constructor of npc, but gives it a separate name.
Would need to override each constructor separately?
This comment has been minimized.
This comment has been minimized.
codemime
Jul 12, 2016
Member
Would need to override each constructor separately?
Not for now. The compiler will make a set of default constructors that will call associated constructors of the base class implicitly. There are no custom constructors in npc.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
I think It could possibly be clearer if the next line was |
mugling
force-pushed the
mugling:arc
branch
2 times, most recently
to
4737d43
Jul 12, 2016
mugling
added some commits
Jul 12, 2016
This comment has been minimized.
This comment has been minimized.
|
Some initial observations - these are not implemented changes. The difference between
An (abitrary) increase in
Each value scale linearly with dispersion so adjusting AR-15 to have 6x less dispersion results in:
Allowing ammo to provide a hard cap on range is an existing useful feature we should preserve. For example it is used to limit Whilst not perfect this example data is considerably more sane and would not be too difficult to write unit tests to enforce. Along with some reworking of This PR done for now subject to review. |
mugling
changed the title
Framework for adjusting gun ranges
Framework for adjusting gun ranges [RDY]
Jul 12, 2016
This comment has been minimized.
This comment has been minimized.
|
Re: clarity: I meant that it seems like it would be hard to convey through
UI that the difference between the 3 and 5 in a `3-5 effective range` is
due to aiming.
Capping "max" range is a very good idea. It might seem arbitrary to some
players, but it's much more effective communication than a number in the
weapon readout.
|
mugling
force-pushed the
mugling:arc
branch
to
2d383e6
Jul 12, 2016
This comment has been minimized.
This comment has been minimized.
To be clear this PR doesn't impose any additional cap (it has no intentional behavioral changes). If it were to impose tl;dr this PR doesn't impose a cap but if it did the total effect would be < 1% |
mugling
force-pushed the
mugling:arc
branch
to
c758af2
Jul 12, 2016
mugling
added some commits
Jul 12, 2016
This comment has been minimized.
This comment has been minimized.
|
I think it's fair to just cap players off from operating at <2% effectiveness; it prevents people from wasting time in a very easily intuited manner. Should probably discuss this in further PRs though. |
This comment has been minimized.
This comment has been minimized.
Indeed. I want to provide a framework here for future changes without actually making any. The plan would be then to expand the unit tests in a series of further PR's until we have a fully working model. |
This comment has been minimized.
This comment has been minimized.
|
I think effective range is fairly clear that the
gun is effective between 3 and 5 tiles
It clearly indicates that you can't hit things closer than 3 tiles, is this
what you meant to say? Range at which a good hit is achievable without
aiming is nonsensical, this should just list maximum effective range with
maximum aiming.
Indeed automatically capping the targeting UI
at that distance wouldn't meaningfully affect
gameplay
If you want to fire in the general direction of a target, you should be
able to. Don't forget that we have weapons with splash damage as well as
weapons with a high fire rate that might be worth using beyond what we call
"max effective range".
|
This comment has been minimized.
This comment has been minimized.
I wanted to try and express the notion of how much aiming could improve the effective range...
This is however much simpler and has no chance for confusion. EDIT: Pushed this change in 3440110 |
Coolthulhu
self-assigned this
Jul 14, 2016
Coolthulhu
reviewed
Jul 14, 2016
| per_cur = 8; | ||
| int_cur = 8; | ||
| } | ||
| }; |
This comment has been minimized.
This comment has been minimized.
Coolthulhu
Jul 14, 2016
Contributor
If this is only used for testing, it should be in a file visible only to tests.
This comment has been minimized.
This comment has been minimized.
Coolthulhu
reviewed
Jul 14, 2016
| double range = rl_dist( source, target_arg ); | ||
| double missed_by = shot_dispersion * moa * to_tangent * range; | ||
| double missed_by = sqrt( 2 * pow( range, 2 ) - ( 2 * pow( range, 2 ) * cos( ARCMIN( dispersion ) ) ) ); |
This comment has been minimized.
This comment has been minimized.
Coolthulhu
Jul 14, 2016
Contributor
Can be simplified to:
sqrt( 2 * ( range * range ) * ( 1 - cos( ARCMIN( dispersion ) ) );
The formula is ~10% less forgiving than the current one. That's quite important but not a huge deal.
Coolthulhu
reviewed
Jul 14, 2016
|
|
||
| // aim_per_time() returns improvement (in MOA) per 10 moves | ||
| for( int i = aim * 10; i != 0; --i ) { | ||
| int adj = aim_per_time( gun, penalty ); |
This comment has been minimized.
This comment has been minimized.
Coolthulhu
Jul 14, 2016
•
Contributor
The limiting factor in aiming is sight_dispersion( -1 ), no need to loop here.
EDIT: When aim < 0, that is.
This comment has been minimized.
This comment has been minimized.
mugling
Jul 14, 2016
Author
Contributor
Noted, but is that perhaps not depending upon an implementation detail of aim_per_time?
Coolthulhu
reviewed
Jul 14, 2016
| // calculate maximum potential dispersion | ||
| double dispersion = get_weapon_dispersion( &gun, false ) + penalty + driving; | ||
|
|
||
| // dispersion is uniformly distributed at random so scale linearly with chance |
This comment has been minimized.
This comment has been minimized.
Coolthulhu
Jul 14, 2016
Contributor
Comment is wrong
Dispersion is rather heavily weighted towards the expected value.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Coolthulhu
Jul 14, 2016
Contributor
It is a sum of uniform rngs. A sum of uniform distributions is weighted towards the expected value.
Otherwise dice( x, y ) would be equivalent to rng( x, x * y ).
This comment has been minimized.
This comment has been minimized.
mugling
Jul 14, 2016
Author
Contributor
So we need to sum everything first then just rng once at the end?
This comment has been minimized.
This comment has been minimized.
Coolthulhu
Jul 14, 2016
Contributor
Not sure if it is a good idea. It could work, but it would make OK shooters randomly miss completely and bad shooters much more likely to score perfect shots than now.
This comment has been minimized.
This comment has been minimized.
mugling
Jul 14, 2016
Author
Contributor
Or perhaps then weighted rng, say from 0.5 to 1.0 of original value?
This comment has been minimized.
This comment has been minimized.
kevingranade
Jul 15, 2016
Member
I think you're reading Coolthulu's comment backwards, the comment is wrong, the code is working exactly as intended.
This comment has been minimized.
This comment has been minimized.
mugling
Jul 16, 2016
•
Author
Contributor
The comment doesn't match the current behavior but I'd like to change the latter as it's very hard to write deterministic unit tests for a non-uniform distribution.
For the current code the random value is ~57% of the maximum at MIN_RECOIL rising to ~72% at MIN_RECOIL * 3 (both calculated via 10⁴ iterations). Any changes within player::get_weapon_dispersion will affect this distribution.
I'd like to separate out weapon and player recoil to be returned separately before randomness is applied so that the unit tests can be deterministic. A weighted rng can still be used providing it's bias is known.
EDIT: Presently player::gun_engagement_range is a good approximation at MIN_RECOIL but overly optimistic at any higher amount of recoil
This comment has been minimized.
This comment has been minimized.
kevingranade
via email
Jul 16, 2016
Member
Coolthulhu
merged commit e0109ce
into
CleverRaven:master
Jul 14, 2016
1 check passed
This comment has been minimized.
This comment has been minimized.
|
Has some imperfections, but those are rather minor. I didn't find anything really bad. |
mugling commentedJul 12, 2016
•
edited
Follows #17643 and intentionally does not adjust balance but provides a framework for doing so.
Firstly some minor refactoring of the code to use standard trigonometric functions (ad7bc2b). We seem to use varying definitions of the accuracy levels so a start is made in 33d46b7 to standardise these. There is one behavioral change in that the full range [0-1] is now considered a hit.
With one exception (corrected in 79617ca) effective recoil is uniformly distributed from 0 to maximum and this is exploited to allow the inverse calculation in
player::gun_effective_range. This function allows you to determine at whatrangea player has a givenchanceof achieving a degree ofaccuracy.The skeleton of a unit test is provided with the intention of extending it to include cases along the lines of 9mm pistol should have a 50% chance of a good hit at 3 tiles after 1 turn of aiming.
The stats dumper can now export various effective ranges for addressing future overall balance and finding outliers. The definition of a standard survivor is contained there (8/8/8/8 stats, 4/4 skills with relevant kit) and is supposed to represent the typical mid-game survivor. I don't think it needs much adjustment but if anyone feels strongly that could be done.
Less well defined is effective range displayed to the player. I've dropped the previous
rangefield fromitem::infoas its useless to the point of completely misleading. For example an AR-15 does not have a range of even close to 36.Instead I've defined effective range as 50% chance of good hit spanning from nil to maximum aim which is a much more useful metric. Support is included for comparing between weapons and the debug menu provides a useful way of determining how various factors affect it. For example adding adding a scope, increasing marksmanship etc.
Note this isn't the maximum absolute range (although it is constrained by it) and it isn't to say guns aren't usable beyond their effective range. It does however provide a good indication where you have at least a better than average chance of scoring a hit. It is my intention to update turrets and NPCs to consider effective range as both will spam ammo at targets they have no realistic chance of hitting.
EDIT: Note
player::gun_effective_rangewas later renamed ``player::gun_engagement_range`