From 68babf92f04b6c35178dce6b3d34e83060e46027 Mon Sep 17 00:00:00 2001 From: BevapDin Date: Fri, 3 Jan 2020 19:32:45 +0100 Subject: [PATCH 1/3] Revert "don't rebuild the item drop list (#36647)" This reverts commit 7aad3abe9133edc4e84fc7415d006e25ac7996fb. --- src/activity_item_handling.cpp | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-) diff --git a/src/activity_item_handling.cpp b/src/activity_item_handling.cpp index 4d66f24b8c7bb..4d4bce6ced17d 100644 --- a/src/activity_item_handling.cpp +++ b/src/activity_item_handling.cpp @@ -536,21 +536,14 @@ static std::list obtain_activity_items( player_activity &act, player &p ) res.insert( res.begin(), excess.begin(), excess.end() ); } // Load anything that remains (if any) into the activity - if( items.size() > act.targets.size() ) { + 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 ); } } - // remove items already dropped from the targets list by checking if pointers were invalidated - for( int i = act.targets.size() - 1; i >= 0; i-- ) { - const auto target_iter = act.targets.cbegin() + i; - const auto value_iter = act.values.cbegin() + i; - if( !*target_iter ) { - act.targets.erase( target_iter ); - act.values.erase( value_iter ); - } - } // 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 // creates a brand new activity every turn and cancels the old activity From 2f4d5282a6fdad8a95e044f427550c42355b7b61 Mon Sep 17 00:00:00 2001 From: BevapDin Date: Fri, 3 Jan 2020 21:30:49 +0100 Subject: [PATCH 2/3] Fix dropping only two or so items when selecting to drop all of a stack. 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. --- src/activity_item_handling.cpp | 37 +++++++++++++++------------------- 1 file changed, 16 insertions(+), 21 deletions(-) diff --git a/src/activity_item_handling.cpp b/src/activity_item_handling.cpp index 4d4bce6ced17d..d840146b2cf16 100644 --- a/src/activity_item_handling.cpp +++ b/src/activity_item_handling.cpp @@ -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 &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 convert_to_items( Character &p, const drop_locations &drop, std::function filter ) { @@ -394,6 +378,16 @@ static std::list 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 ) { @@ -517,6 +511,9 @@ static std::list 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 ) ) { @@ -538,11 +535,9 @@ static std::list 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 From 5f339ebb4e98654a42978c963258b51dd58e237f Mon Sep 17 00:00:00 2001 From: BevapDin Date: Sat, 4 Jan 2020 16:33:32 +0100 Subject: [PATCH 3/3] Fix accessing invalid item locations: see comment for an explanation. --- src/activity_item_handling.cpp | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/activity_item_handling.cpp b/src/activity_item_handling.cpp index d840146b2cf16..115da69ce12d2 100644 --- a/src/activity_item_handling.cpp +++ b/src/activity_item_handling.cpp @@ -361,6 +361,11 @@ static drop_locations convert_to_locations( const player_activity &act ) return res; } for( size_t i = 0; i < act.values.size(); i++ ) { + // locations may have become invalid as items are forcefully dropped + // when they exceed the storage volume of the character + if( !act.targets[i] || !act.targets[i].get_item() ) { + continue; + } res.emplace_back( act.targets[i], act.values[i] ); } return res;