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

NPC trade - bugfixes and improvements #51704

Merged
merged 2 commits into from
Sep 28, 2021

Conversation

RoyBerube
Copy link
Contributor

@RoyBerube RoyBerube commented Sep 19, 2021

Summary

Bugfixes "NPC trade dupe fix and other fixes"

Purpose of change

Fixes #50454 Fixes #46874 Fixes #49209 Fixes #47939 Item duplication when trading with NPC or merchant

Fixes #46765 Ammo in belts and magazines not included in trade value.

Fixes #42432 Fixes #40175 NPC trade volume and weight not properly updating.

Describe the solution

Disallows trading an item twice in two ways:

  1. All non worn containers are collapsed so only the container is listed. This also helps to simplify the trade list for item selection.
  2. Worn containers will list their contents as well as the container. The container price is just for itself, and can be traded by itself without any contents being included. Contents can be included or excluded at will. No duplication is possible.

Some logic was checking the item location the wrong way and resulted in weight and volume updates being incorrect.

Ammo in belts and magazines is now priced correctly as a byproduct of code changes.

Describe alternatives you've considered

Rewriting the trade interface. It would be better imo to behave more like the AIM or inventory screen for selection.

Testing

Full test program found no problems.

Tested with NPC ally and NPC merchant. Assorted weapons, tools, kits, containers, stacked items, liquids, ammo, worn canteen. Trading and pricing seems to be working consistently and correctly.

Additional context

@actual-nh actual-nh added <Bugfix> This is a fix for a bug (or closes open issue) [C++] Changes (can be) made in C++. Previously named `Code` Items: Containers Things that hold other things NPC / Factions NPCs, AI, Speech, Factions, Ownership labels Sep 19, 2021
@Night-Pryanik
Copy link
Contributor

Why did you revert my edit to your OP? Doesn't your PR fixes all these issues?

@PatrikLundell
Copy link
Contributor

Just a sanity check: I assume you're not allowed to trade away closed containers alone, in particular when the contents are liquid.
"Hey Mr. Merchant, I have here this nice locked safe I want to sell to you"...

@RoyBerube
Copy link
Contributor Author

Why did you revert my edit to your OP? Doesn't your PR fixes all these issues?

Sorry, I didn't realize you had edited it. Won't happen again.

@Maleclypse
Copy link
Member

Roy would you be interested in making an NPC trait or other change so that “Merchant” NPCs will never sell an item for less than 115% of its post_apoc_price and never buy for more than 85% of its post_apoc_price?

@RoyBerube
Copy link
Contributor Author

Roy would you be interested in making an NPC trait or other change so that “Merchant” NPCs will never sell an item for less than 115% of its post_apoc_price and never buy for more than 85% of its post_apoc_price?

double npc_trading::net_price_adjustment( const Character &buyer, const Character &seller )

Clamping is already done there, so it would not take much to change the clamp value according to some json setting. I'm not that familiar with the npc json files so cannot comment on that part.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
<Bugfix> This is a fix for a bug (or closes open issue) [C++] Changes (can be) made in C++. Previously named `Code` Items: Containers Things that hold other things NPC / Factions NPCs, AI, Speech, Factions, Ownership
Projects
None yet
6 participants