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.

Sign up for GitHub

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

Better trading UI and fix duplication bug when trading containers with NPCs #48922

Closed
wants to merge 5 commits into from
Closed

Better trading UI and fix duplication bug when trading containers with NPCs #48922

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
48955a1
UI changes for trading with NPCs, and container duplication fix
emomicrowave May 16, 2021
31d9211
Clang-tidy and astyle fixes
emomicrowave May 19, 2021
c69cc00
Fix incorrectly idented code block
emomicrowave May 19, 2021
e944545
Fix an edge cases where the full charge is traded, but the UI shows j…
emomicrowave May 19, 2021
57499c0
Update src/npctrade.cpp
KorGgenT Jul 10, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
275 changes: 236 additions & 39 deletions src/npctrade.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,6 +256,52 @@ void item_pricing::adjust_values( const double adjust, const faction *fac )
}
}

std::vector<item_pricing *> item_pricing::get_contents_rec()
{
std::vector<item_pricing *> tmp;

if( this->is_container && this->contents.size() ) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if( this->is_container && this->contents.size() ) {
if( is_container && !contents.empty() ) {

we don't really do the this-> thing, it's unecessary really. also .empty() is a function, it's a bit more reasonable to use a boolean than an integer as a boolean

for( item_pricing *ip : this->contents ) {
tmp.push_back( ip );

if( ip->is_container && ip->contents.size() ) {
std::vector<item_pricing *> v_ip = ip->get_contents_rec();
tmp.insert( tmp.end(), v_ip.begin(), v_ip.end() );
}
}
}

return tmp;
}

void item_pricing::populate_container_pointers( std::vector<item_pricing> &trading )
{

for( item_pricing &ip : trading ) {
// set parent container
ip.parent = nullptr;
if( ip.loc.has_parent() ) {
for( item_pricing &ip2 : trading ) {
if( ip2.loc.get_item() == ip.loc.parent_item().get_item() ) {
ip.parent = &ip2;
break;
}
}
}

// get contained items
if( ip.is_container ) {
for( item_pricing &ip2 : trading ) {
for( item *contained : ip.loc.get_item()->contents.all_items_top() ) {
if( contained == ip2.loc.get_item() ) {
ip.contents.push_back( &ip2 );
}
}
}
}
}
}

void trading_window::setup_win( ui_adaptor &ui )
{
const int win_they_w = TERMX / 2;
Expand All @@ -276,6 +322,11 @@ void trading_window::setup_trade( int cost, npc &np )
// Trading is not linear - starving NPC may pay $100 for 3 jerky, but not $100000 for 300 jerky
theirs = npc_trading::init_buying( player_character, np, true );
yours = npc_trading::init_buying( np, player_character, false );

// Populate each item_pricing's @parent and @contents pointers, so that it's way faster to
// select / deselect and calculate prices of items and containers during trading
item_pricing::populate_container_pointers( theirs );
item_pricing::populate_container_pointers( yours );

if( np.will_exchange_items_freely() ) {
your_balance = 0;
Expand All @@ -294,13 +345,13 @@ void trading_window::update_win( npc &np, const std::string &deal )
std::vector<item *> added;

for( item_pricing &pricing : yours ) {
if( pricing.selected ) {
added.push_back( pricing.loc.get_item() );
if( pricing.marked ) {
added.push_back( pricing.loc.get_item() );
}
}

for( item_pricing &pricing : theirs ) {
if( pricing.selected ) {
if( pricing.marked ) {
without.insert( pricing.loc.get_item() );
}
}
Expand Down Expand Up @@ -393,20 +444,20 @@ void trading_window::update_win( npc &np, const std::string &deal )
}
}

if( ip.selected ) {
if( ip.marked ) {
color = c_white;
}

trim_and_print( w_whose, point( 1, i - offset + 1 ), win_w, color, "%s %c %s",
right_justify( hotkey.short_description(), 2 ),
ip.selected ? '+' : '-', itname );
ip.marked ? '+' : '-', itname );
#if defined(__ANDROID__)
ctxt.register_manual_key( hotkey.get_first_input(), itname );
#endif
hotkey = ctxt.next_unassigned_hotkey( hotkeys, hotkey );

std::string price_str = format_money( ip.price );
nc_color price_color = np.will_exchange_items_freely() ? c_dark_gray : ( ip.selected ? c_white :
nc_color price_color = np.will_exchange_items_freely() ? c_dark_gray : ( ip.marked ? c_white :
c_light_gray );
mvwprintz( w_whose, point( win_w - utf8_width( price_str ), i - offset + 1 ),
price_color, price_str );
Expand Down Expand Up @@ -612,50 +663,196 @@ bool trading_window::perform_trade( npc &np, const std::string &deal )
ch += offset;
if( ch < target_list.size() ) {
item_pricing &ip = target_list[ch];
int change_amount = 1;
int &owner_sells = focus_them ? ip.u_has : ip.npc_has;
int &owner_sells_charge = focus_them ? ip.u_charges : ip.npc_charges;

if( ip.selected ) {
if( owner_sells_charge > 0 ) {
change_amount = owner_sells_charge;
owner_sells_charge = 0;
} else if( owner_sells > 0 ) {
change_amount = owner_sells;
owner_sells = 0;
}
} else if( ip.charges > 0 ) {
change_amount = get_var_trade( *ip.loc.get_item(), ip.charges );
if( change_amount < 1 ) {

bool old_is_marked = ip.marked;
bool old_is_selected = ip.selected;

// We make the assumption that reaching SELECTED=1 and MARKED=0 is impossible.
// see npctrade.h for difference between variables.
if( !ip.selected && ip.marked )
{
ip.marked = false;
} else
{
ip.selected = !ip.selected;
ip.marked = !ip.marked;
}

// get_change_amount() depends on ip.selected and ip.marked, so it needs to be executed
// after the selection has been changed.
int change_amount = get_change_amount( ip, true );
if( change_amount < 1 )
{
ip.marked = old_is_marked;
ip.selected = old_is_selected;
continue;
}

// Don't trade items in sealed containers without their container.
if( ip.selected && ip.loc.has_parent() )
{
item_contents::sealed_summary sealed_container =
ip.loc.parent_item()->contents.get_sealed_summary();

if( sealed_container == item_contents::sealed_summary::all_sealed ) {
popup( "Cannot trade contents of sealed containers!" );
ip.selected = false;
ip.marked = false;
continue;
}
owner_sells_charge = change_amount;
} else {
if( ip.count > 1 ) {
change_amount = get_var_trade( *ip.loc.get_item(), ip.count );
if( change_amount < 1 ) {
} else if( sealed_container == item_contents::sealed_summary::part_sealed ) {
const item_pocket *pocket = ip.loc.parent_item()->contents.contained_where( *ip.loc.get_item() );
if( pocket->sealed() ) {
popup( "Cannot trade contents of sealed containers!" );
ip.selected = false;
ip.marked = false;
continue;
}
}
owner_sells = change_amount;
}
ip.selected = !ip.selected;
if( ip.selected != focus_them ) {
change_amount *= -1;
}
int delta_price = ip.price * change_amount;
if( !np.will_exchange_items_freely() ) {
your_balance -= delta_price;

// If current item is a container, SELECT and MARK the container. Only MARK all of the contents.
if( ip.is_container )
{
std::vector<item_pricing *> contents = ip.get_contents_rec();

for( item_pricing *content : contents ) {
// Only toggle whether item is selected, if its state is different from the container's
// Otherwise deselecting the container, selects the item inside, which makes no sense from UX standpoint
if( content->marked != ip.marked ) {
content->marked = !content->marked;
// make sure we are trading the whole stack
get_change_amount( *content, false );
}
// Handles price calculation, when you've selected few individual items from a container
// and then select the entire container.
// The state that the container is deselected, but some ( not all! ) of it's contents remain selected shouldn't happen
else if( ip.selected && content->selected ) {
// deselect current charge / count
content->selected = false;
content->marked = false;
int change_amount = get_change_amount( *content, false );
adjust_balance( *content, np, change_amount );
// select full charge coutn
content->marked = true;
get_change_amount( *content, false );
}
}
}
if( ip.loc.where() == item_location::type::character ) {
volume_left += ip.vol * change_amount;
weight_left += ip.weight * change_amount;

// If we deselected an item inside a container, we also want to deselect the container itself,
// cause it would be difficult to handle selling a whole container without one item inside.
if( !ip.selected && !ip.marked && ip.parent && ip.parent->marked )
{

item_pricing *ip_container = &ip;

while( ip_container->parent ) {
// Found the top-most selected container
if( ip_container->parent->selected ) {
ip_container = ip_container->parent;
ip_container->selected = false;
ip_container->marked = false;

adjust_balance( *ip_container, np, 1 );
break;
} else if( !ip_container->parent->selected && ip_container->parent->marked ) {
ip_container = ip_container->parent;
ip_container->marked = false;
} else {
break;
}
}
Comment on lines +749 to +764
Copy link
Member

Choose a reason for hiding this comment

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

i feel like this is ad-hoc code of something we already have in the code base somewhere, but i can't remember what it is off-hand.

Copy link
Member

Choose a reason for hiding this comment

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

my first suspicion is that this is just a reimplementation of the parent-finding code in item_location.


// Because the @item_pricing of the container includes price and weight of the individual items,
// after deselecting the container itself, go through once more and add the @item_pricing of the remaining selected items.
// We want to find orphaned containers, that are marked for trade, and we want to SELECT them too. If we treat them as regular items,
// their price will be used to calculate the balance, and then the price of their contents is going to be duplicately calculated, leading to problems.
std::queue<item_pricing *> nodes;
nodes.push( ip_container );

while( nodes.size() ) {
item_pricing *current = nodes.front();

for( item_pricing *ip2 : current->contents ) {
// We found an orphaned subcontainer. SELECT it for trading. Its contents should be already marked
if( ip2->is_container && ip2->marked && ip2->contents.size() ) {
ip2->selected = true;
adjust_balance( *ip2, np, 1 );
}
// regular item or container without contents. Treat as a regular item, i.e select for trading and add price
else if( ip2->marked ) {
ip2->selected = true;
ip2->marked = true;
adjust_balance( *ip2, np, get_change_amount( *ip2, false ) );
}
// unmarked container. we have to make sure that we check it for orphaned unselected items or orphaned containers
else if( !ip2->marked && ip2->contents.size() ) {
nodes.push( ip2 );
}

}
nodes.pop();
}
continue;
}
Comment on lines +773 to 797
Copy link
Member

Choose a reason for hiding this comment

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

this whole while loop needs to be a separate function. off hand i suspect it would be slightly better as a recursive function but that implementation detail doesn't bother me at this point

adjust_balance( ip, np, change_amount );
}
}
}

return confirm;
return confirm;
}

// Recalculates the money, volume, and weight balance
// As a rule of thumb you want to adjust balance only for items that have been SELECTED.
// If an item is marked it means it's inside a selected container, and its price is already taken into account.
void trading_window::adjust_balance( item_pricing &ip, npc &np, int change_amount )
{
if( ( ip.selected || ip.marked ) != focus_them ) {
change_amount *= -1;
}
int delta_price = ip.price * change_amount;
if( !np.will_exchange_items_freely() ) {
your_balance -= delta_price;
}
if( ip.loc.where() == item_location::type::character ) {
volume_left += ip.vol * change_amount;
weight_left += ip.weight * change_amount;
}
}

// Returns the amount of money that needs to be charged.
// @manual checks whether the user should be asked for input. If True, then the function may return < 1,
// which is usually means the user has cancelled the input, so you need to handle this appropriately.
// If manual is False, then the whole item stack is selected / deselected for trading.
int trading_window::get_change_amount( item_pricing &ip, bool manual )
{
int change_amount = 1;
int &owner_sells = focus_them ? ip.u_has : ip.npc_has;
int &owner_sells_charge = focus_them ? ip.u_charges : ip.npc_charges;

// This the item has been just deselected for trading.
if( !ip.selected && !ip.marked ) {
if( owner_sells_charge > 0 ) {
change_amount = owner_sells_charge;
owner_sells_charge = 0;
} else if( owner_sells > 0 ) {
change_amount = owner_sells;
owner_sells = 0;
}
} else if( ip.charges > 0 ) {
change_amount = manual ? get_var_trade( *ip.loc.get_item(),
ip.charges ) : ( owner_sells_charge ? owner_sells_charge : ip.charges );
owner_sells_charge = change_amount;
} else {
if( ip.count > 1 ) {
change_amount = manual ? get_var_trade( *ip.loc.get_item(),
ip.count ) : ( owner_sells ? owner_sells : ip.count );
}
owner_sells = change_amount;
}

return change_amount;
}

// Returns how much the NPC will owe you after this transaction.
Expand Down
16 changes: 15 additions & 1 deletion src/npctrade.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,12 +32,24 @@ class item_pricing
}
void set_values( int ip_count );
void adjust_values( double adjust, const faction *fac );
// recursively fetches all contents of the item depth first
std::vector<item_pricing *> get_contents_rec();
static void populate_container_pointers( std::vector<item_pricing> &trading );

item_location loc;
int price = 0;
// Whether this is selected for trading
// Whether this is selected for trading. Item's price, volume and weight impact
// will be calculated. Item will be transferred after trading is done.
bool selected = false;
// Items inside selected containers are marked but not selected.
// This way they are highlighted in the UI, but their price is not calculated
// and don't get duplicately transferred after the trade has been done.
// Core assumption is, that the container price includes the combined prices of all items inside.
bool marked = false;
bool is_container = false;
// top-level contents, i.e doesn't return contents of contents, but you can manually get these.
std::vector<item_pricing *> contents;
item_pricing *parent;
int count = 0;
int charges = 0;
int u_has = 0;
Expand All @@ -64,6 +76,8 @@ class trading_window
void setup_win( ui_adaptor &ui );
void update_win( npc &np, const std::string &deal );
void show_item_data( size_t offset, std::vector<item_pricing> &target_list );
void adjust_balance( item_pricing &ip, npc &np, int change_amount );
int get_change_amount( item_pricing &ip, bool manual );

catacurses::window w_head;
catacurses::window w_them;
Expand Down