Make from code review and refactor

Notably `Pickup::pick_up()` now uses item_stack iterators instead of
indexes since the previous hack proved to be too fragile.

The since items are now stored unordered on the map, the order of items
displayed in the pickup ui is now sorted using `item::operator<()`

src/activity_item_handling.cpp

@@ -544,28 +544,30 @@ void activity_on_turn_wear( player_activity &act, player &p )
         act.targets.pop_back();
         act.values.pop_back();
 
-        item *temp_item = target.get_item();
-
-        if( temp_item == nullptr ) {
-            continue; // No such item.
+        if( !target ) {
+            debugmsg( "Lost target item of ACT_WEAR" );
+            continue;
         }
 
-        item leftovers = *temp_item;
+        // Make copies so the original remains untouched if wearing fails
+        item newit = *target;
+        item leftovers = newit;
 
-        if( quantity != 0 && temp_item->count_by_charges() ) {
-            // Reinserting leftovers happens after item removal to avoid stacking issues.
-            leftovers.charges = temp_item->charges - quantity;
+        // Handle charges, quantity == 0 means move all
+        if( quantity != 0 && newit.count_by_charges() ) {
+            leftovers.charges = newit.charges - quantity;
             if( leftovers.charges > 0 ) {
-                temp_item->charges = quantity;
+                newit.charges = quantity;
             }
         } else {
             leftovers.charges = 0;
         }
 
-        // On successful wear remove from map or vehicle, replacing with leftovers if any.
-        if( p.wear_item( *temp_item ) ) {
+        if( p.wear_item( newit ) ) {
+            // If we wore up a whole stack, remove the original item
+            // Otherwise, replace the item with the leftovers
             if( leftovers.charges > 0 ) {
-                *temp_item = std::move( leftovers );
+                *target = std::move( leftovers );
             } else {
                 target.remove_item();
             }
@@ -686,41 +688,38 @@ static void move_items( player &p, const tripoint &relative_dest, bool to_vehicl
         targets.pop_back();
         quantities.pop_back();
 
-        item *temp_item = target.get_item();
-
-        if( temp_item == nullptr ) {
-            continue; // No such item.
+        if( !target ) {
+            debugmsg( "Lost target item of ACT_MOVE_ITEMS" );
+            continue;
         }
 
-        item leftovers = *temp_item;
+        // Don't need to make a copy here since movement can't be canceled
+        item &leftovers = *target;
+        // Make a copy to be put in the destination location
+        item newit = leftovers;
 
-        if( quantity != 0 && temp_item->count_by_charges() ) {
-            // Reinserting leftovers happens after item removal to avoid stacking issues.
-            leftovers.charges = temp_item->charges - quantity;
-            if( leftovers.charges > 0 ) {
-                temp_item->charges = quantity;
-            }
+        // Handle charges, quantity == 0 means move all
+        if( quantity != 0 && newit.count_by_charges() ) {
+            newit.charges = std::min( newit.charges, quantity );
+            leftovers.charges -= quantity;
         } else {
             leftovers.charges = 0;
         }
 
         // Check that we can pick it up.
-        if( !temp_item->made_of_from_type( LIQUID ) ) {
+        if( !newit.made_of_from_type( LIQUID ) ) {
             // This is for hauling across zlevels, remove when going up and down stairs
             // is no longer teleportation
             const tripoint src = target.position();
             int distance = src.z == dest.z ? std::max( rl_dist( src, dest ), 1 ) : 1;
-            p.mod_moves( -Pickup::cost_to_move_item( p, *temp_item ) * distance );
+            p.mod_moves( -Pickup::cost_to_move_item( p, newit ) * distance );
             if( to_vehicle ) {
-                put_into_vehicle_or_drop( p, item_drop_reason::deliberate, { *temp_item }, dest );
+                put_into_vehicle_or_drop( p, item_drop_reason::deliberate, { newit }, dest );
             } else {
-                drop_on_map( p, item_drop_reason::deliberate, { *temp_item }, dest );
+                drop_on_map( p, item_drop_reason::deliberate, { newit }, dest );
             }
-            // Remove from map or vehicle.
-            // If we didn't pick up a whole stack, put the remainder back where it came from.
-            if( leftovers.charges > 0 ) {
-                *target.get_item() = std::move( leftovers );
-            } else {
+            // If we picked up a whole stack, remove the leftover item
+            if( leftovers.charges <= 0 ) {
                 target.remove_item();
             }
         }

src/lightmap.cpp

@@ -62,7 +62,7 @@ std::string four_quadrants::to_string() const
                           ( *this )[quadrant::SW], ( *this )[quadrant::NW] );
 }
 
-void map::add_light_from_items( const tripoint &p, map_stack::iterator begin,
+void map::add_light_from_items( const tripoint &p, item_stack::iterator begin,
                                 map_stack::iterator end )
 {
     for( auto itm_it = begin; itm_it != end; ++itm_it ) {

src/map.cpp

@@ -4499,15 +4499,14 @@ static bool process_item( item_stack &items, item_stack::iterator &n, const trip
     std::swap( temp, *n );
 
     // Store a pointer to the target item so we can check if processing destroyed it.
-    const item *const target = &*n;
+    item *const target = &*n;
 
     if( temp.process( nullptr, location, activate, insulation, flag ) ) {
         // Item is to be destroyed so erase the null item in the map stack
         // unless it was already destroyed by processing.
-        if( std::find_if( items.begin(), items.end(), [target]( const item & it ) {
-        return &it == target;
-    } ) != items.end() ) {
-            items.erase( n );
+        item_stack::iterator it = items.get_iterator_from_pointer( target );
+        if( it != items.end() ) {
+            items.erase( it );
         }
         return true;
     } else {

src/map.h

@@ -1578,7 +1578,7 @@ class map
         void apply_light_arc( const tripoint &p, int angle, float luminance, int wideangle = 30 );
         void apply_light_ray( bool lit[MAPSIZE_X][MAPSIZE_Y],
                               const tripoint &s, const tripoint &e, float luminance );
-        void add_light_from_items( const tripoint &p, map_stack::iterator begin, map_stack::iterator end );
+        void add_light_from_items( const tripoint &p, item_stack::iterator begin, map_stack::iterator end );
         std::unique_ptr<vehicle> add_vehicle_to_map( std::unique_ptr<vehicle> veh, bool merge_wrecks );
 
         // Internal methods used to bash just the selected features