Fix some of the NPC attacking logic

[GoLoT]

Summary

Bugfixes "Fix some of the NPC attack logic"

Purpose of change

Keep in mind this is a quick fix for the more pressing issues with the new NPC attack logic. The goal is to try to avoid NPCs throwing all their inventory, fixing NPCs trying to shoot unequipped weapons and getting them to use weapons in less stupid ways.
Fixes #49834
Fixes #49830

Describe the solution

This is in no way a perfect solution and it has its issues. For the most part NPCs will now try to behave according to the flags set, trying to avoid friendly fire, not killing themselves while in a car, moving to nearby targets when a melee weapon is the preferred weapon and so on.

EDIT: Updated the npc_attack test so it compiles but the expected values are the old ones, making the test fail. I think it's pointless to try to fix the test now as the values would be meaningless since the evaluation functions will need adjustments.

EDIT: Tests updated to reflect scenarios and outcomes instead of checking evaluation values.

Describe alternatives you've considered

Proposing a revert of the original commit until this is solved. I chose to push a quick fix so NPCs can at least defend themselves while the feature is polished.

Testing

I didn't do nearly enough testing but I did test the most basic stuff. I tried giving an NPC a ranged and a melee weapon and it used the ranged weapon until out of ammo. Then switched to the melee weapon.

I tried driving the NPC around and it stood in the vehicle and tried to shoot enemies until running out of ammo. Then it switched to a melee weapon and waited for enemies to come without jumping from the vehicle or trying to kill himself.

Throwing is severely nerfed now and items that are dangerous (i.e. active grenades) are given a huge boost. The math behind choosing to throw items is... well... missing. I simply gave it some multipliers that seem to be working so far. When an item has the NPC_THROWN flag it will be more likely to be thrown but it still has to be a better choice (as in better damage output) than using a melee weapon.

With the changes I didn't get any more "missing gun" errors or infinite loops and the NPCs at the very least showed a will to live. Also they kept their inventory, and being selfish in a cataclysm is always a good thing.

[GoLoT]

@KorGgenT evaluate() results are close to what they were before but still need tweaking and adding other multipliers (like taking into account being surrounded when choosing weapons). If you feel the values are off or not in line with your original idea, feel free to pick this PR apart and take only the changes needed.

[mlangsdorf]

I may be slightly confused, but it looks like the checks for rules.has_flag( ally_rule::use_guns ) and rules.has_flag( ally_rule::use_silent ) got lost in the initial PR and are still absent. Would you please double-check if that's the case and if so, please add them back to the gun and throw can_use() routines? Thanks.

[GoLoT]

@mlangsdorf those were added in the original PR here and working properly now that weights are favoring ranged weapons over throwing.

The checks aren't present for throwing though. Should throwing be considered non-silent?

[KorGgenT]

why are we switching to float math?

[GoLoT]

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

[KorGgenT]

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

[GoLoT]

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.

[KorGgenT]

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?"

[GoLoT]

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.

[KorGgenT]

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

[GoLoT]

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.

[KorGgenT]

why? npcs still have to deal with those

[GoLoT]

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.

[KorGgenT]

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.

[GoLoT]

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

[KorGgenT]

wait, why? you get that from gunmode!

[GoLoT]

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.

[lgtm-com]

This pull request introduces 1 alert when merging 3641d95 into 4663683 - view on LGTM.com

new alerts:

• 1 for Multiplication result converted to larger type

[actual-nh]

A few thoughts.

[actual-nh]

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

[GoLoT]

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.

[actual-nh]

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.

[GoLoT]

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.

[actual-nh]

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

[GoLoT]

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.

[actual-nh]

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

[GoLoT]

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.

[GoLoT]

I think I addressed every review comment, the UI doesn't make it easy so if I missed something let me know. Same if you feel something isn't clear.

I think this PR is ready for merge if no one has objections. Tweaking the weights is definitely needed but I think that should be done in a new PR as the more pressing issue is trying to leave experimental in a playable state. Trying to do everything in this PR can extend indefinitely and will delay the fix. Fine tuning weights can wait and needs further discussion as right now the attacks and weapons are tested independently without any context of other possible attack methods, making it hard to find a way to make NPCs make sensible choices. Tests have to be updated to reflect the new weights/evaluations when the evaluation functions are stable.

There is also the matter of throwing explosives that I didn't really test and the lack of support for some NPC flags.

I repeated some of my tests right now and everything seems to be working so far. One of the tests that showed multiple issues before was letting the barred zombies in a refugee center out and watching the guard NPCs go crazy. I tried that again and they got rid of the zombies as usual without any apparent issues. Also tested again with a single NPC and he used his ranged weapon until he broke an arm and couldn't wield it. Then he switched to melee and eventually died as a hero.

[personjerry]

My understanding is that this PR isn't a perfect solution; Perhaps as a stopgap there can be a combat setting for NPCs to not consider throwing at all? I.e. if (no_throw_flag) potential = 0;

[GoLoT]

As per @mlangsdorf 's suggestion I added a check to prevent throwing items (unless it's an active explosive) if ranged attacks are disallowed. A separate flag can be added if needed though.

[actual-nh]

I think this PR is ready for merge if no one has objections. Tweaking the weights is definitely needed but I think that should be done in a new PR as the more pressing issue is trying to leave experimental in a playable state. Trying to do everything in this PR can extend indefinitely and will delay the fix. Fine tuning weights can wait and needs further discussion as right now the attacks and weapons are tested independently without any context of other possible attack methods, making it hard to find a way to make NPCs make sensible choices. Tests have to be updated to reflect the new weights/evaluations when the evaluation functions are stable.

I think at least some of the tests can be changed to requiring a specific ordering of result values instead of requiring specific result values - if the question is whether the code is working for doing a melee weapon at melee range, don't check its numeric evaluation, check to make sure it's better than a gun/thrown weapon/etc.

[GoLoT]

Reworked tests to reflect scenarios and outcomes instead of testing specific return values. More tests scenarios will have to be added and evaluation functions updated accordingly (TDD).

If all checks pass this should be ready to merge. There are some behavior issues still but the NPCs should at least defend themselves. Tweaking will come from playtesting and new tests.

[GoLoT]

Wishful thinking: If it passess all checks again without any random failures it should be good to go.

EDIT: Yeah, it didn't happen. Errors are unrelated to the PR so still good to go.

[actual-nh]

Something wrong re ammo_sufficient, possibly other things? (Might not be from this PR, of course...)

[I-am-Erk]

@GoLoT how is this doing? Is it about ready?

[GoLoT]

I think @kevingranade wanted to take a look at it but the ASan issues kept him busy.

Unless someone finds a specific issue that prevents merging, I consider it ready. There are issues that I have more or less pin-pointed but I didn't want to extend this PR. I'll work on those in separate PRs when this is merged while I test everything on my current experimental run.

[cainiaowu]

There was some behavior tree implementation in #39810, perhaps the NPC AI could use some of that.