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

Fix some of the NPC attacking logic #49907

Merged
merged 22 commits into from
Jul 26, 2021
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
241 changes: 174 additions & 67 deletions src/npc_attack.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,9 @@ const std::map<Creature::Attitude, int> attitude_multiplier = {
{ Creature::Attitude::NEUTRAL, -1 }
};
// if you are attacking your target, multiply potential by this number
const int target_modifier = 3;
const float target_modifier = 1.5f;
// if you kill the creature, multiply the potential by this number
const int kill_modifier = 2;
const float kill_modifier = 1.5f;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why are we switching to float math?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a change to use small multipliers to not boost the priority too much for targets/kill targets. This can be reverted.

// the amount of penalty if the npc has to change what it's wielding
// update this number and comment when that is no longer a flat -15 moves
const int base_time_penalty = 3;
Expand All @@ -32,20 +32,30 @@ const int base_throw_now = 10'000;

// TODO: make a better, more generic "check if this projectile is blocked" function
// TODO: put this in a namespace for reuse
static bool has_obstruction( const tripoint &from, const tripoint &to )
static bool has_obstruction( const tripoint &from, const tripoint &to, bool check_ally = false )
{
std::vector<tripoint> line = line_to( from, to );
// @to is what we want to hit. we don't need to check for obstruction there.
line.pop_back();
const map &here = get_map();
for( const tripoint &line_point : line ) {
if( here.impassable( line_point ) ) {
if( here.impassable( line_point ) || ( check_ally && g->critter_at( line_point ) ) ) {
return true;
}
}
return false;
}

static bool can_move( const npc &source )
{
return !source.in_vehicle && source.rules.engagement != combat_engagement::NO_MOVE;
}

static bool can_move_melee( const npc &source )
{
return can_move( source ) && source.rules.engagement != combat_engagement::FREE_FIRE;
}

bool npc_attack_rating::operator>( const npc_attack_rating &rhs ) const
{
if( !rhs._value ) {
Expand Down Expand Up @@ -83,8 +93,10 @@ npc_attack_rating npc_attack_rating::operator-=( const int rhs )

void npc_attack_melee::use( npc &source, const tripoint &location ) const
{
if( !source.wield( weapon ) ) {
debugmsg( "ERROR: npc tried to melee monster with weapon it couldn't wield" );
if( !source.is_wielding( weapon ) ) {
if( !source.wield( weapon ) ) {
debugmsg( "ERROR: npc tried to equip a weapon it couldn't wield" );
}
return;
}
Creature *critter = g->critter_at( location );
Expand All @@ -93,9 +105,32 @@ void npc_attack_melee::use( npc &source, const tripoint &location ) const
return;
}
if( !source.is_adjacent( critter, true ) ) {
add_msg_debug( debugmode::debug_filter::DF_NPC, "%s is attempting a reach attack",
source.disp_name() );
source.reach_attack( location );
if( rl_dist( source.pos(), location ) <= weapon.reach_range( source ) ) {
add_msg_debug( debugmode::debug_filter::DF_NPC, "%s is attempting a reach attack",
source.disp_name() );
// TODO: Avoid friendly fire
source.reach_attack( location );
} else {
source.update_path( location );
if( source.path.size() > 1 ) {
if( can_move_melee( source ) ) {
source.move_to_next();
} else {
source.look_for_player( get_player_character() );
}
} else if( source.path.size() == 1 ) {
if( critter != nullptr ) {
if( source.can_use_offensive_cbm() ) {
source.activate_bionic_by_id( bionic_id( "bio_hydraulics" ) );
}
add_msg_debug( debugmode::debug_filter::DF_NPC, "%s is attempting a melee attack",
source.disp_name() );
source.melee_attack( *critter, true );
}
} else {
source.look_for_player( get_player_character() );
}
}
} else {
add_msg_debug( debugmode::debug_filter::DF_NPC, "%s is attempting a melee attack",
source.disp_name() );
Expand All @@ -105,8 +140,7 @@ void npc_attack_melee::use( npc &source, const tripoint &location ) const

tripoint_range<tripoint> npc_attack_melee::targetable_points( const npc &source ) const
{
const int reach_range{ weapon.reach_range( source ) };
return get_map().points_in_radius( source.pos(), reach_range );
return get_map().points_in_radius( source.pos(), 8 );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

an npc can't technically target these points. explain?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's needed to let NPCs target monsters out of reach. Without checking those tiles NPCs will not even consider melee as an option for any targets that aren't in range and won't move towards enemies.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moving toward enemies is in a different part of the code. this function is "the npc has already chosen to use melee, what are the targets?"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't work that way though. As long as the NPC has any items it will decide throwing said items is the way to go and won't bother moving into melee range.

}

npc_attack_rating npc_attack_melee::evaluate( const npc &source,
Expand Down Expand Up @@ -177,35 +211,62 @@ npc_attack_rating npc_attack_melee::evaluate_critter( const npc &source,
const Creature::Attitude att = source.attitude_to( *critter );
double potential = damage * npc_attack_constants::attitude_multiplier.at( att ) -
( distance_to_me - 1 );

const int reach_range{ weapon.reach_range( source ) };
const int distance = rl_dist( source.pos(), critter->pos() );
int range_penalty = std::max( 0, distance - reach_range );
potential *= range_penalty ? 0.7f : 1.5f;

if( damage >= critter->get_hp() ) {
potential *= npc_attack_constants::kill_modifier;
}
if( target && target->pos() == critter->pos() ) {
potential *= npc_attack_constants::target_modifier;
}

return npc_attack_rating( std::round( potential ), critter->pos() );
}

void npc_attack_gun::use( npc &source, const tripoint &location ) const
{
const item &weapon = *gunmode;
if( !weapon.ammo_sufficient() ) {
source.do_reload( weapon );
if( !source.is_wielding( gun ) ) {
if( !source.wield( gun ) ) {
debugmsg( "ERROR: npc tried to equip a weapon it couldn't wield" );
}
return;
}

if( !gun.ammo_sufficient() ) {
GoLoT marked this conversation as resolved.
Show resolved Hide resolved
source.do_reload( gun );
add_msg_debug( debugmode::debug_filter::DF_NPC, "%s is reloading %s", source.disp_name(),
weapon.display_name() );
gun.display_name() );
return;
}

if( has_obstruction( source.pos(), location, false ) ||
( source.rules.has_flag( ally_rule::avoid_friendly_fire ) &&
!source.wont_hit_friend( location, gun, false ) ) ) {
if( can_move( source ) ) {
source.avoid_friendly_fire();
} else {
source.move_pause();
}
return;
}

const int dist = rl_dist( source.pos(), location );

if( source.aim_per_move( weapon, source.recoil ) > 0 &&
source.confident_shoot_range( weapon, source.get_most_accurate_sight( weapon ) ) >= dist ) {
// Only aim if we aren't in risk of being hit
// TODO: Get distance to closest enemy
Comment on lines +252 to +253
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these comments refer to something that needs to be in evaluate(), not use().

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Evaluate won't really work for this though. Evaluate will give a target and preferred attack, but in some cases the NPC will choose to try and finish an enemy that isn't the closest and the NPC might get stuck trying to aim while being whacked by a nearby enemy constantly messing up the aim.

It has to be done here so the NPC tries to hipfire.

if( dist > 1 && source.aim_per_move( gun, source.recoil ) > 0 &&
/*source.confident_shoot_range(gun, source.get_most_accurate_sight(gun) ) >= dist*/
source.confident_gun_mode_range( gunmode, source.recoil ) < dist ) {
add_msg_debug( debugmode::debug_filter::DF_NPC, "%s is aiming", source.disp_name() );
source.aim();
} else {
source.fire_gun( location );
add_msg_debug( debugmode::debug_filter::DF_NPC, "%s fires %s", source.disp_name(),
weapon.display_name() );
gun.display_name() );
}
}

Expand Down Expand Up @@ -298,17 +359,26 @@ npc_attack_rating npc_attack_gun::evaluate_tripoint(
}
const int distance_to_me = rl_dist( location, source.pos() );
const Creature::Attitude att = source.attitude_to( *critter );
const bool friendly_fire = att == Creature::Attitude::FRIENDLY &&
!source.rules.has_flag( ally_rule::avoid_friendly_fire );
int attitude_mult = npc_attack_constants::attitude_multiplier.at( att );
if( friendly_fire ) {
// hitting a neutral creature isn't exactly desired, but it's a lot less than a friendly.
// if friendly fire is on, we don't care too much, though if an available hit doesn't damage them it would be better.
attitude_mult = npc_attack_constants::attitude_multiplier.at( Creature::Attitude::NEUTRAL );
}
const int distance_penalty = std::max( distance_to_me - 1,
( source.closest_enemy_to_friendly_distance() - 1 ) * 2 );
double potential = damage * attitude_mult - distance_penalty;

if( att != Creature::Attitude::HOSTILE ) {
// No point in throwing stuff at neutral/friendly targets
return npc_attack_rating( cata::nullopt, location );
}

bool avoids_friendly_fire = source.rules.has_flag( ally_rule::avoid_friendly_fire );
GoLoT marked this conversation as resolved.
Show resolved Hide resolved

double potential = damage;

// Make attacks that involve moving to find clear LOS slightly less likely
if( has_obstruction( source.pos(), location, avoids_friendly_fire ) ) {
potential *= 0.9f;
} else if( avoids_friendly_fire && !source.wont_hit_friend( location, gun, false ) ) {
potential *= 0.95f;
}

// Prefer targets not directly next to you
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why? npcs still have to deal with those

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trying to reduce weights for closer targets so melee weapons can be a preferred choice for those targets even if the damage would be lower. Shooting point blank comes with issues as you can't aim properly.

potential *= distance_to_me > 2 ? 1.15f : 0.85f;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps the 2 should be adjusted (increased?) for reach weapons?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The multiplier here is simply trying to get the NPC to pick a melee weapon (if a decent one is available) when in close range by lowering the potential for this weapon as ranged weapons usually have a much higher damage value. I don't recall seeing any enemies with reach attacks but if there are any, it won't really matter that much.

The weights need work but the main goal was to fix the logic issues that led to NPCs throwing all their inventories, throwing errors and not being able to chase targets in melee.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. (There are monsters with reach special attacks, BTW.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is probably not the best way to deal with nearby targets anyway. I think a new PR to balance/tweak weights and add some checks that are missing is in order.


if( damage >= critter->get_hp() ) {
potential *= npc_attack_constants::kill_modifier;
}
Expand All @@ -329,10 +399,13 @@ void npc_attack_activate_item::use( npc &source, const tripoint &/*location*/ )

bool npc_attack_activate_item::can_use( const npc &source ) const
{
const bool can_use_grenades = ( source.is_player_ally() &&
!source.rules.has_flag( ally_rule::use_grenades ) ) || source.is_hallucination();
//TODO: make this more complete. only does BOMB type items
return can_use_grenades && activatable_item.has_flag( flag_NPC_ACTIVATE );
if( !activatable_item.has_flag( flag_NPC_ACTIVATE ) ) {
KorGgenT marked this conversation as resolved.
Show resolved Hide resolved
//TODO: make this more complete. only does BOMB type items
return false;
}
const bool can_use_grenades = !source.is_hallucination() && ( !source.is_player_ally() ||
source.rules.has_flag( ally_rule::use_grenades ) );
return can_use_grenades;
}

npc_attack_rating npc_attack_activate_item::evaluate(
Expand Down Expand Up @@ -361,21 +434,34 @@ std::vector<npc_attack_rating> npc_attack_activate_item::all_evaluations( const

void npc_attack_throw::use( npc &source, const tripoint &location ) const
{
// WARNING! wielding the item invalidates the reference!
KorGgenT marked this conversation as resolved.
Show resolved Hide resolved
if( source.wield( thrown_item ) ) {
add_msg_debug( debugmode::debug_filter::DF_NPC, "%s throws the %s", source.disp_name(),
source.weapon.display_name() );
item thrown( source.weapon );
if( source.weapon.count_by_charges() && source.weapon.charges > 1 ) {
source.weapon.mod_charges( -1 );
thrown.charges = 1;
if( !source.is_wielding( source.weapon ) ) {
if( !source.wield( source.weapon ) ) {
debugmsg( "ERROR: npc tried to equip a weapon it couldn't wield" );
}
return;
}

if( has_obstruction( source.pos(), location, false ) ||
( source.rules.has_flag( ally_rule::avoid_friendly_fire ) &&
!source.wont_hit_friend( location, thrown_item, false ) ) ) {
if( can_move( source ) ) {
source.avoid_friendly_fire();
} else {
source.remove_weapon();
source.move_pause();
}
source.throw_item( location, thrown );
return;
}

add_msg_debug( debugmode::debug_filter::DF_NPC, "%s throws the %s", source.disp_name(),
source.weapon.display_name() );
item thrown( source.weapon );
if( source.weapon.count_by_charges() && source.weapon.charges > 1 ) {
source.weapon.mod_charges( -1 );
thrown.charges = 1;
} else {
debugmsg( "%s couldn't wield %s to throw it", source.disp_name(), thrown_item.display_name() );
source.remove_weapon();
}
source.throw_item( location, thrown );
}

bool npc_attack_throw::can_use( const npc &source ) const
Expand All @@ -384,30 +470,30 @@ bool npc_attack_throw::can_use( const npc &source ) const
if( single_item.count_by_charges() ) {
single_item.charges = 1;
}
// please don't throw your pants...
return !source.is_worn( thrown_item ) &&
// we would rather throw than activate this. unless we need to throw it now.
!( thrown_item.has_flag( flag_NPC_THROW_NOW ) || thrown_item.has_flag( flag_NPC_ACTIVATE ) ) &&
// i don't trust you as far as i can throw you.
source.throw_range( single_item ) != 0;
if( thrown_item.has_flag( flag_NPC_THROW_NOW ) || thrown_item.has_flag( flag_NPC_THROWN ) ) {
//Always allow throwing items that are flagged as throw now or npc_thrown
return true;
}

bool throwable = source.throw_range( single_item ) > 0 && !source.is_worn( thrown_item ) &&
!thrown_item.has_flag( flag_NPC_ACTIVATE );
KorGgenT marked this conversation as resolved.
Show resolved Hide resolved
throwable = throwable && !thrown_item.is_gun() && !thrown_item.is_armor() &&
!thrown_item.is_comestible() && !thrown_item.is_magazine() && !thrown_item.is_tool() &&
!thrown_item.is_unarmed_weapon();
Comment on lines +480 to +482
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems really wonky tbh. we really should have some better logic here that doesn't require you to check the item's type in code like this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup. Left a comment below so it can be addressed eventually.

// TODO: Better choose what should be thrown
return throwable;
}

int npc_attack_throw::base_penalty( const npc &source ) const
{
// hot potato! HOT POTATO!
int throw_now = 0;
if( thrown_item.has_flag( flag_NPC_THROW_NOW ) ) {
throw_now = npc_attack_constants::base_throw_now;
}

item single_item( thrown_item );
if( single_item.count_by_charges() ) {
single_item.charges = 1;
}
const int time_penalty = source.is_wielding( single_item ) ? 0 :
npc_attack_constants::base_time_penalty;

return time_penalty - throw_now;
return time_penalty;
}

tripoint_range<tripoint> npc_attack_throw::targetable_points( const npc &source ) const
Expand All @@ -429,7 +515,23 @@ npc_attack_rating npc_attack_throw::evaluate(
return effectiveness;
}
const int penalty = base_penalty( source );
// TODO: Should this be a field to cache the result?
bool avoids_friendly_fire = source.rules.has_flag( ally_rule::avoid_friendly_fire );
GoLoT marked this conversation as resolved.
Show resolved Hide resolved
for( const tripoint &potential : targetable_points( source ) ) {

// hot potato! HOT POTATO!
// Calculated for all targetable points, not just those with targets
if( thrown_item.has_flag( flag_NPC_THROW_NOW ) ) {
// TODO: Take into account distance to allies too
const int distance_to_me = rl_dist( potential, source.pos() );
int result = npc_attack_constants::base_throw_now + distance_to_me;
if( !has_obstruction( source.pos(), potential, avoids_friendly_fire ) ) {
// More likely to pick a target tile that isn't obstructed
result += 100;
}
return npc_attack_rating( result, potential );
}

if( Creature *critter = g->critter_at( potential ) ) {
if( source.attitude_to( *critter ) != Creature::Attitude::HOSTILE ) {
// no point in friendly fire!
Expand Down Expand Up @@ -486,23 +588,28 @@ npc_attack_rating npc_attack_throw::evaluate_tripoint(
if( critter ) {
att = source.attitude_to( *critter );
}
const bool friendly_fire = att == Creature::Attitude::FRIENDLY &&
!source.rules.has_flag( ally_rule::avoid_friendly_fire );
int attitude_mult = npc_attack_constants::attitude_multiplier.at( att );
if( friendly_fire ) {
// hitting a neutral creature isn't exactly desired, but it's a lot less than a friendly.
// if friendly fire is on, we don't care too much, though if an available hit doesn't damage them it would be better.
attitude_mult = npc_attack_constants::attitude_multiplier.at( Creature::Attitude::NEUTRAL );

if( att != Creature::Attitude::HOSTILE ) {
// No point in throwing stuff at neutral/friendly targets
return npc_attack_rating( cata::nullopt, location );
}

if( source.rules.has_flag( ally_rule::avoid_friendly_fire ) &&
!source.wont_hit_friend( location, thrown_item, false ) ) {
// Avoid friendy fire
return npc_attack_rating( cata::nullopt, location );
}

const float throw_mult = throw_cost( source, single_item ) * source.speed_rating() / 100.0f;
const int damage = source.thrown_item_total_damage_raw( single_item );
float dps = damage / throw_mult;
const int distance_to_me = rl_dist( location, source.pos() );
const int distance_penalty = std::max( distance_to_me - 1,
( source.closest_enemy_to_friendly_distance() - 1 ) * 2 );
const int distance_penalty = std::max( std::min( distance_to_me,
GoLoT marked this conversation as resolved.
Show resolved Hide resolved
source.closest_enemy_to_friendly_distance() ) - 3, 0 );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const int distance_penalty = std::max( std::min( distance_to_me,
source.closest_enemy_to_friendly_distance() ) - 3, 0 );
const int closest_enemy_dist = source.closest_enemy_to_friendly_distance();
int distance_penalty = std::abs( distance_to_me - 3) ;
// Ideally, the 3 in the above should be adjusted for reach weapons, etc
if ( closest_enemy_dist < distance_penalty + 2 ) { // not sure exactly how to adjust distance_penalty here
distance_penalty = std::min( distance_penalty, std::abs( closest_enemy_dist - 1 ) * 2 );
}

This will need considerable formatting! It penalizes throwing when something is close enough that melee should be preferred, or when throwing too close to a friendly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll add this change along with one of KorGgenT suggestions when I have access to my environment to test it, it will be in a few hours. The math still is a placeholder and will need more tweaking in a new PR. Hopefully from somene that can work it out as right now the modifiers for each kind of attack are inconsistent.

const float suitable_item_mult = thrown_item.has_flag( flag_NPC_THROWN ) ? 0.2f : -0.15f;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is a melee/true ranged weapon without flag_NPC_THROW, perhaps base this penalty on the weapon dps?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At first I tried to add a check to see if the weapon had higher dps when wielded than thrown but it didn't work out as expected. I eventually reduced the multiplier for items not flagged and it made the NPCs not throw weapons as often. I think cudgels are still thrown in some circunstamces but that's why the math still needs a lot of work.

It's mainly the logic when executing actions that I fixed but the math is just placeholder and random values that simply worked for my tests.

const float distance_mult = -distance_penalty / 10.0f;

double potential = dps * attitude_mult - distance_penalty;
double potential = dps * ( 1.0f + distance_mult + suitable_item_mult );
if( critter && damage >= critter->get_hp() ) {
GoLoT marked this conversation as resolved.
Show resolved Hide resolved
potential *= npc_attack_constants::kill_modifier;
}
Expand Down
3 changes: 2 additions & 1 deletion src/npc_attack.h
Original file line number Diff line number Diff line change
Expand Up @@ -77,8 +77,9 @@ class npc_attack_melee : public npc_attack
class npc_attack_gun : public npc_attack
{
const gun_mode gunmode;
item &gun;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wait, why? you get that from gunmode!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tried to make the gunmode member non-const but it still complained about const-ness whenever I tried to get the weapon with *gunmode. It seems to always choose the const version of the function over the non-const.

public:
explicit npc_attack_gun( const gun_mode &gunmode ) : gunmode( gunmode ) {}
explicit npc_attack_gun( item &gun, const gun_mode &gunmode ) : gunmode( gunmode ), gun( gun ) {}
npc_attack_rating evaluate( const npc &source, const Creature *target ) const override;
std::vector<npc_attack_rating> all_evaluations( const npc &source,
const Creature *target ) const override;
Expand Down
Loading