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

trade_ui: address small bugs #52818

Merged
merged 8 commits into from
Nov 15, 2021
Merged

Conversation

andrei8l
Copy link
Contributor

@andrei8l andrei8l commented Nov 14, 2021

Summary

SUMMARY: None

Purpose of change

Address small bugs with the new trade_ui and remove old workarounds inherited from trading_window

Fixes #43239: contained liquids have negative prices
Fixes #52816: invlet and keybinding conflicts
Fixes #52817: allied NPCs still check balance before accepting
Fixes #46873: npc_trading::trade() can cause a segfault if window refresh is triggered while traded items are manipulated
Can't trade for, or even expand some containers (ex: zipper bag): remove TRADER_AVOID from all container items

Describe the solution

Please see individual commits. These are all small fixes.

Describe alternatives you've considered

N/A

Testing

Additional context

@Maleclypse Maleclypse added <Bugfix> This is a fix for a bug (or closes open issue) Items: Containers Things that hold other things labels Nov 14, 2021
@Night-Pryanik
Copy link
Contributor

Any chance this PR will fix #14619?

@andrei8l
Copy link
Contributor Author

Any chance this PR will fix #14619?

If I'm reading this chain right, that dialogue option first generates a random debt with parameters defined in json here

"effect": [ { "add_debt": [ [ "TRUST", 1 ], [ "VALUE", 3 ], [ "ALTRUISM", 2 ], [ "TOTAL", 300 ] ] }, "give_equipment" ],

then picks an item that can fit in the NPC's total debt, then subtracts the item's value from the debt here
p.op_of_u.owed -= giving[chosen].price;

At the moment I don't see any way to properly remove the fake debt added in the first step, sorry. It probably requires changes to JSON processing so you'd have a single-step effect like "effect": [ { "give_equipment": [ ...paremeters here... ] } ] instead of the two steps used now.

@andrei8l andrei8l marked this pull request as ready for review November 14, 2021 18:01
src/npc.cpp Outdated Show resolved Hide resolved
@wapcaplet
Copy link
Contributor

Nice work! I retested the associated bugs in your branch, and can confirm:

  • Liquids (and their containers, and their containers' containers, etc.) have correct positive prices.
  • Allied NPCs accept trades that put them in debt (or gifts) without complaint.
  • When trading an item inside a wielded backpack for an item that I must wield, I get prompted to drop the backpack, the trade goes through ok, and there is no crash.
  • I already changed my keybinding and can't easily test the new default, but TAB works to swap panes as expected.

Co-authored-by: Eric Pierce <[email protected]>
@andrei8l
Copy link
Contributor Author

@wapcaplet thanks a lot for testing and tracking down these bugs!

@kevingranade kevingranade merged commit b1c74e2 into CleverRaven:master Nov 15, 2021
@andrei8l andrei8l deleted the trade-bugs-small branch November 15, 2021 16:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment