NPC AI logic and test issues #27438
Merged
NPC AI logic and test issues #27438
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
One of the NPC movement tests checks that an NPC in a vehicle on hazardous terrain does not move. The intention is that the NPC should not be fleeing the hazardous terrain. Unfortunately there are other reasons why NPCs may move (such as long-term goals to acquire ammo etc.). Resolve this by giving the NPCs in the test a mission which causes them to stay still unless in peril.
To see which NPC is causing a test failure.
npc::scan_new_items() was supposed to return true if some action was taken, but the logic was reversed, because npc::wield_better_weapon() returns true if a weapon was wielded. The upshot was that NPCs were being charged for the time to equip a weapon when they got new items but *didn't* equip a weapon. Rearrange the logic to make more sense.
mlangsdorf
approved these changes
Jan 5, 2019
mlangsdorf
added
NPC / Factions
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
SUMMARY: Bugfixes "Fix NPC weapon wielding turn cost"
Purpose of change
I started looking into this because of the occasional test failure involving
npc-movement, specifically theNPC in vehicle should not escape from dangerous terrainwas failing.After some investigation it turned out that this test was only ever passing most of the time due to a bug, and when that bug was avoided it would fail.
This PR fixes the bug and the test.
Describe the solution
The point of this test is that NPCs should normally try to escape hazardous terrain, but not if they are in a vehicle. So it checks that the non-vehicle NPC moves, and the in-vehicle one does not.
The trouble is that there are many other possible reasons for an NPC to move. When there's nothing else in particular to do they will go looking for some distant item, and move; that's what caused the test failure.
Usually this didn't happen, because of a bug in the weapon-wielding logic. When an NPC gets new items they check to see whether any is a better weapon. These newly-created NPCs all had new items and all did this check.
It was supposed to be that when they had a better weapon they would wield it (spending their turn) instead of moving. What actually happened is that they would spend their turn when they didn't wield a new weapon (and not spend it when they did).
So, the test failure would happen for NPCs who either had no new items or happened to be holding a better weapon (a somewhat rare event). It didn't always fail even then; it also had to be that they found a useful path towards whichever item they decided to seek.
So, there are two changes here:
NPC_MISSION_SHOPKEEPERmission. This pre-empts the more general 'go find random stuff' logic and means that they will stand still unless in peril.Describe alternatives you've considered
I thought about removing the non-follower NPC in this situation (because I think the follower NPC wouldn't have had this issue, and would still test the feature adequately).
Additional context
@mlangsdorf Review would be appreciated.