Skip to content

Commit

Permalink
Fix dropping only two or so items when selecting to drop all of a stack.
Browse files Browse the repository at this point in the history
The problem is within `convert_to_items`. It has code that converts an item pointer to a stack of items in the main inventory (e.g. 100 flyers) into multiple pointers, each to a different item within that stack.

E.g. when the dropping is started, the user has selected the stack of 100 flyers and a count of 100. This is a **single** pair of information added to the activity: the pointer to the first item of the stack and the number 100.

`convert_to_items` converts that into 100 `act_item` instances (each contains a pointer to a different item within that stack of flyers, a move cost, and a count of 1 for each instance).

This is later given to the actual dropping code, which works fine for now, it drops the first item of that stack.

But at the end of the dropping code (after dropping one item), the remaining values used to be converted back into item position and count values (two integers) in order to store them in the activity to resume this upon the next turn.

This was done in `convert_to_locations` (the other overload), which used to look up the item position and merge the multiple instance that all pointed to the same item position (again: **old** code). The new code only compares the `item_location`, which compares the actual item pointers. Those don't match (one points to the first item in the stack, the next one to the second item in the stack and so on).

So the pairs are not merged back into a single item position and count pair.

Now the activity contains 99 instance of items to be dropped.

Upon loading those the next time, they go through `convert_to_items`, which gets their item position and creates **new** `item_location` instances, but because the count for each of those items is 1, they all point to the first item in the stack (caused by the loop over the item stack in `convert_to_items` (the loop there over `p.inv.const_stack` only visits the first one, after visiting that one `obtained` is 1, as is `count`).

Now the code drops the first of those items. But this invalidates all the remaining instances (remember: they all point to the same item!) and so dropping stops and no more items are stored for dropping next time.
  • Loading branch information
BevapDin committed Jan 3, 2020
1 parent 68babf9 commit 2f4d528
Showing 1 changed file with 16 additions and 21 deletions.
37 changes: 16 additions & 21 deletions src/activity_item_handling.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -366,22 +366,6 @@ static drop_locations convert_to_locations( const player_activity &act )
return res;
}

static drop_locations convert_to_locations( const std::list<act_item> &items )
{
drop_locations res;

for( const act_item &ait : items ) {
if( ait.loc && ait.count > 0 ) {
if( res.empty() || res.back().first != ait.loc ) {
res.emplace_back( ait.loc, ait.count );
} else {
res.back().second += ait.count;
}
}
}
return res;
}

static std::list<act_item> convert_to_items( Character &p, const drop_locations &drop,
std::function<bool( item_location loc )> filter )
{
Expand All @@ -394,6 +378,16 @@ static std::list<act_item> convert_to_items( Character &p, const drop_locations
if( !filter( loc ) ) {
continue;
} else if( !p.is_worn( *loc ) && !p.is_wielding( *loc ) ) {
// Special case. After dropping the first few items, the remaining items are already separated.
// That means: `drop` already contains references to each of the items in
// `p.inv.const_stack`, and `count` will be 1 for each of them.
// If we continued without this check, we iterate over `p.inv.const_stack` multiple times,
// but each time stopping after visiting the first item.
// In the end, we would add references to the same item (the first one in the stack) multiple times.
if( count == 1 ) {
res.emplace_back( loc, 1, loc.obtain_cost( p, 1 ) );
continue;
}
int obtained = 0;
for( const item &it : p.inv.const_stack( p.get_item_position( &*loc ) ) ) {
if( obtained >= count ) {
Expand Down Expand Up @@ -517,6 +511,9 @@ static std::list<item> obtain_activity_items( player_activity &act, player &p )
while( !items.empty() && ( p.is_npc() || p.moves > 0 || items.front().consumed_moves == 0 ) ) {
act_item &ait = items.front();

assert( ait.loc );
assert( ait.loc.get_item() );

p.mod_moves( -ait.consumed_moves );

if( p.is_worn( *ait.loc ) ) {
Expand All @@ -538,11 +535,9 @@ static std::list<item> obtain_activity_items( player_activity &act, player &p )
// Load anything that remains (if any) into the activity
act.targets.clear();
act.values.clear();
if( !items.empty() ) {
for( const drop_location &drop : convert_to_locations( items ) ) {
act.targets.push_back( drop.first );
act.values.push_back( drop.second );
}
for( const act_item &ait : items ) {
act.targets.push_back( ait.loc );
act.values.push_back( ait.count );
}
// And cancel if its empty. If its not, we modified in place and we will continue
// to resolve the drop next turn. This is different from the pickup logic which
Expand Down

0 comments on commit 2f4d528

Please sign in to comment.