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: NPCs drop excess items if overburdened, merchants pick up and renew inventory if restocking #3386

Merged
merged 3 commits into from
Oct 7, 2023

Conversation

OrenAudeles
Copy link
Collaborator

Summary

SUMMARY: Bugfixes "NPCs drop items if overburdened instead of just overvolume, merchant restocking fix"

Purpose of change

Merchant NPCs in the Refugee Center, Christopher Flood, Merchant and Makayla Sanchez, Arsonist, stay way overburdened even after calling drop_invalid_inventory because it was only able to drop items above volume limits. Combined with the checking for the "LIFT" quality when overburdened in weight_carried_reduced_by, a very slow check because it looks over all items in tiles (map and vehicle) within PICKUP_RANGE distance of the NPC, these two NPCs were using a lot of CPU time to do literally nothing but stand in place.
This problem is mitigated in #3385 since these NPCs do not carry weapons a lot of the time. They're also relatively shielded from the monsters hidden within the Refugee Center so are unlikely to carry weapons unless things go very poorly for the building.

To go with dropping inventory when overburdened the NPC merchants also need to properly restock their wares. If they've dropped their inventory then on restocking their remaining undropped inventory will be deleted and replaced by whatever is generated as new stock. This can lead to an accumulation of items at and around the NPC as new items are stocked then spilled onto the floor. To fix this I have the NPC pick up nearby owned items before clearing the inventory and restocking.
To prevent an issue with the Arsonist, and any other NPC Merchants with the "no_faction" faction defined, where restocking items will not actually generate any due to the faction wealth being too low and thus delete the NPC's inventory without replacing with anything I have limited picking up, clearing, and finally replacing the inventory by requiring there be something to restock.

Describe the solution

Dropping items by weight works very similarly to dropping items by volume. The primary difference is that it is not done within the inventory class but outside of it. There is no reason to make it a class method when all necessary functionality is available from outside of the class. By keeping NPCs from being overburdened as much as possible we prevent the costly "LIFT" quality check in most cases.

Picking up owned items prevents accumulation of items on the floor, and restricting it to if there are items replacing the old prevents the Arsonist from completely losing their stock. The Arsonist still will not ever restock but that makes some sense as they're a random fire starter, presumably without any backer and unknown means to produce more incendiary devices.

Picking up owned items is made a free action because they're about to get deleted and replaced, and then probably dropped onto the floor again which does take moves away from the merchant.

Describe alternatives you've considered

Instead of requiring new wares to allow deletion of old inventory the minimum shop_value could have been modified to allow some items, or allow generation of a few "free" items before they start deducting from shop_value.

Testing

NPCs drop items when overburdened, and Merchants pick up nearby items properly when refreshing their stocks. Arsonist no longer deletes their inventory without replacing any of it.

Additional context

If the NPC drops items onto a vehicle tile they will not be picked up again when restocking inventory. I'm not sure if that would be desirable and erred on the side of minimizing the NPC's reach. These items picked up ARE about to get deleted so being a bit conservative seemed best.

@github-actions github-actions bot added the src changes related to source code. label Oct 7, 2023
Copy link
Member

@scarf005 scarf005 left a comment

Choose a reason for hiding this comment

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

image

  1. npcs properly drop heavy stuff.

image

  1. perf looks good

image

  1. trading doesn't destroy the game

image
image

  1. that merchant really hates owing stuff debug moved time few days forward, stuff seems to be replenished.

@scarf005 scarf005 added this pull request to the merge queue Oct 7, 2023
Merged via the queue into cataclysmbnteam:upload with commit 000e5da Oct 7, 2023
7 of 15 checks passed
@OrenAudeles OrenAudeles deleted the merchant-drop-excess branch October 7, 2023 16:43
@olanti-p
Copy link
Contributor

olanti-p commented Oct 7, 2023

The tests failed for this PR

-------------------------------------------------------------------------------
Character is slowed down while overburdened
  Carry weight over carry capacity
       When: Character carries specified weight
       Then: No effect on speed
   And when: Turn passes
-------------------------------------------------------------------------------
../tests/speed_test.cpp:90
...............................................................................

../tests/speed_test.cpp:92: FAILED:
  REQUIRE( guy.weight_carried() == 1_kilogram * load_kg + 633_gram )
with expansion:
Error:   44,633,000mg == 66,633,000mg
with messages:
  load_kg := 66
  speed_exp := 88

-------------------------------------------------------------------------------
Character is slowed down while overburdened
  Carry weight significantly over carry capacity
       When: Character carries specified weight
       Then: No effect on speed
   And when: Turn passes
-------------------------------------------------------------------------------
../tests/speed_test.cpp:90
...............................................................................

../tests/speed_test.cpp:92: FAILED:
  REQUIRE( guy.weight_carried() == 1_kilogram * load_kg + 633_gram )
with expansion:
Error:   44,633,000mg == 102,633,000mg
with messages:
  load_kg := 102
  speed_exp := 68

@OrenAudeles
Copy link
Collaborator Author

that makes sense. I forgot to run tests and fix as I went. It's because this condition is true for the test

if( activity.targets.empty() ) {
    drop_invalid_inventory();
}

so the test character is immediately dropping items once overburdened.

@OrenAudeles
Copy link
Collaborator Author

I was using npc::drop_items as a reference for what drop_invalid_inventory was supposed to do, and didn't take into account that it's ONLY the NPC that should drop items if overencumbered.

These tests pass again in #3387

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
src changes related to source code.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants