Remove map and vehicle functions using item indexes

Indexes to colonies are unstable and everything has now been converted
to use iterators and pointers.

Also fixed a few minor style issues and typos.

src/iexamine.cpp

@@ -2537,7 +2537,7 @@ void iexamine::fvat_full( player &p, const tripoint &examp )
         return;
     }
 
-    for( const item &it : items_here ) {
+    for( item &it : items_here ) {
         if( !it.made_of_from_type( LIQUID ) ) {
             add_msg( _( "You remove %s from the vat." ), it.tname() );
             g->m.add_item_or_charges( p.pos(), it );
@@ -4277,7 +4277,7 @@ static void mill_activate( player &p, const tripoint &examp )
     map_stack items = g->m.i_at( examp );
     units::volume food_volume = 0_ml;
 
-    for( const item &it : items ) {
+    for( item &it : items ) {
         if( it.typeId() == "flour" ) {
             add_msg( _( "This mill already contains flour." ) );
             add_msg( _( "Remove it before starting the mill again." ) );
@@ -4869,7 +4869,7 @@ void iexamine::quern_examine( player &p, const tripoint &examp )
             mill_load_food( p, examp, remaining_capacity );
             break;
         case 3: // remove food
-            for( const item &it : items_here ) {
+            for( item &it : items_here ) {
                 if( it.is_food() ) {
                     // get handling cost before the item reference is invalidated
                     const int handling_cost = -p.item_handling_cost( it );
@@ -5081,7 +5081,7 @@ void iexamine::smoker_options( player &p, const tripoint &examp )
             rem_f_opt = true;
         /* fallthrough */
         case 5: { //remove charcoal
-            for( const item &it : items_here ) {
+            for( item &it : items_here ) {
                 if( ( rem_f_opt && it.is_food() ) || ( !rem_f_opt && ( it.typeId() == "charcoal" ) ) ) {
                     // get handling cost before the item reference is invalidated
                     const int handling_cost = -p.item_handling_cost( it );

src/lightmap.cpp

@@ -63,7 +63,7 @@ std::string four_quadrants::to_string() const
 }
 
 void map::add_light_from_items( const tripoint &p, item_stack::iterator begin,
-                                map_stack::iterator end )
+                                item_stack::iterator end )
 {
     for( auto itm_it = begin; itm_it != end; ++itm_it ) {
         float ilum = 0.0; // brightness

src/map.cpp

@@ -4058,12 +4058,7 @@ map_stack::iterator map::i_rem( const point &location, map_stack::const_iterator
     return i_rem( tripoint( location, abs_sub.z ), it );
 }
 
-int map::i_rem( const int x, const int y, const int index )
-{
-    return i_rem( tripoint( x, y, abs_sub.z ), index );
-}
-
-void map::i_rem( const int x, const int y, const item *it )
+void map::i_rem( const int x, const int y, item *it )
 {
     i_rem( tripoint( x, y, abs_sub.z ), it );
 }
@@ -4134,34 +4129,12 @@ map_stack::iterator map::i_rem( const tripoint &p, map_stack::const_iterator it
     return current_submap->itm[l.x][l.y].erase( it );
 }
 
-int map::i_rem( const tripoint &p, const int index )
-{
-    if( index < 0 ) {
-        debugmsg( "i_rem called with negative index %d", index );
-        return index;
-    }
-
-    if( index >= static_cast<int>( i_at( p ).size() ) ) {
-        return index;
-    }
-
-    auto map_items = i_at( p );
-    auto iter = map_items.begin();
-    std::advance( iter, index );
-    map_items.erase( iter );
-    return index;
-}
-
-void map::i_rem( const tripoint &p, const item *it )
+void map::i_rem( const tripoint &p, item *it )
 {
-    auto map_items = i_at( p );
-
-    for( auto iter = map_items.begin(); iter != map_items.end(); iter++ ) {
-        //delete the item if the pointer memory addresses are the same
-        if( it == &*iter ) {
-            map_items.erase( iter );
-            break;
-        }
+    map_stack map_items = i_at( p );
+    map_stack::const_iterator iter = map_items.get_iterator_from_pointer( it );
+    if( iter != map_items.end() ) {
+        i_rem( p, iter );
     }
 }
 
@@ -5153,30 +5126,6 @@ void map::trigger_rc_items( const std::string &signal )
     process_items( false, trigger_radio_item, signal );
 }
 
-item *map::item_from( const tripoint &pos, size_t index )
-{
-    auto items = i_at( pos );
-
-    if( index >= items.size() ) {
-        return nullptr;
-    } else {
-        // This is a hack until this function is deleted
-        return &*items.get_iterator_from_index( index );
-    }
-}
-
-item *map::item_from( vehicle *veh, int cargo_part, size_t index )
-{
-    auto items = veh->get_items( cargo_part );
-
-    if( index >= items.size() ) {
-        return nullptr;
-    } else {
-        // This is a hack until this function is deleted
-        return &*items.get_iterator_from_index( index );
-    }
-}
-
 const trap &map::tr_at( const tripoint &p ) const
 {
     if( !inbounds( p ) ) {
@@ -6968,15 +6917,15 @@ void map::grow_plant( const tripoint &p )
     if( !furn.has_flag( "PLANT" ) ) {
         return;
     }
-    auto items = i_at( p );
+    map_stack items = i_at( p );
     if( items.empty() ) {
         // No seed there anymore, we don't know what kind of plant it was.
         dbg( D_ERROR ) << "a seed item has vanished at " << p.x << "," << p.y << "," << p.z;
         furn_set( p, f_null );
         return;
     }
 
-    auto seed = items.front();
+    item &seed = items.front();
     if( !seed.is_seed() ) {
         // No seed there anymore, we don't know what kind of plant it was.
         dbg( D_ERROR ) << "a planted item at " << p.x << "," << p.y << "," << p.z << " has no seed data";
@@ -6990,14 +6939,14 @@ void map::grow_plant( const tripoint &p )
             if( has_flag_furn( "GROWTH_SEEDLING", p ) ) {
                 return;
             }
-            i_rem( p, 1 );
+            i_rem( p, &seed );
             rotten_item_spawn( seed, p );
             furn_set( p, furn_str_id( furn.plant->transform ) );
         } else if( seed.age() < plantEpoch * 3 * furn.plant->growth_multiplier ) {
             if( has_flag_furn( "GROWTH_MATURE", p ) ) {
                 return;
             }
-            i_rem( p, 1 );
+            i_rem( p, &seed );
             rotten_item_spawn( seed, p );
             //You've skipped the seedling stage so roll monsters twice
             if( !has_flag_furn( "GROWTH_SEEDLING", p ) ) {

src/map.h

@@ -897,8 +897,7 @@ class map
         map_stack i_at( int x, int y );
         void i_clear( const int x, const int y );
         map_stack::iterator i_rem( const point &location, map_stack::const_iterator it );
-        int i_rem( const int x, const int y, const int index );
-        void i_rem( const int x, const int y, const item *it );
+        void i_rem( const int x, const int y, item *it );
         void spawn_item( const int x, const int y, const std::string &itype_id,
                          const unsigned quantity = 1, const int charges = 0,
                          const time_point &birthday = calendar::time_of_cataclysm, const int damlevel = 0 );
@@ -922,8 +921,7 @@ class map
         // i_rem() methods that return values act like container::erase(),
         // returning an iterator to the next item after removal.
         map_stack::iterator i_rem( const tripoint &p, map_stack::const_iterator it );
-        int i_rem( const tripoint &p, const int index );
-        void i_rem( const tripoint &p, const item *it );
+        void i_rem( const tripoint &p, item *it );
         void spawn_artifact( const tripoint &p );
         void spawn_natural_artifact( const tripoint &p, const artifact_natural_property prop );
         void spawn_item( const tripoint &p, const std::string &itype_id,
@@ -1024,15 +1022,6 @@ class map
 
         void create_anomaly( const tripoint &p, artifact_natural_property prop, bool create_rubble = true );
 
-        /**
-         * Fetch an item from this map location, with sanity checks to ensure it still exists.
-         */
-        item *item_from( const tripoint &pos, const size_t index );
-
-        /**
-         * Fetch an item from this vehicle, with sanity checks to ensure it still exists.
-         */
-        item *item_from( vehicle *veh, const int cargo_part, const size_t index );
         // Partial construction functions
         void partial_con_set( const tripoint &p, const partial_con &con );
         void partial_con_remove( const tripoint &p );
@@ -1578,7 +1567,8 @@ 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, item_stack::iterator begin, map_stack::iterator end );
+        void add_light_from_items( const tripoint &p, item_stack::iterator begin,
+                                   item_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

src/mission_companion.cpp

@@ -2066,7 +2066,7 @@ void talk_function::loot_building( const tripoint &site )
                 critter->die( nullptr );
             }
             //Hoover up tasty items!
-            for( const item &it : bay.i_at( p ) ) {
+            for( item &it : bay.i_at( p ) ) {
                 if( ( ( it.is_food() || it.is_food_container() ) && !one_in( 8 ) ) ||
                     ( it.made_of( LIQUID ) && !one_in( 8 ) ) ||
                     ( it.price( true ) > 1000 && !one_in( 4 ) ) || one_in( 5 ) ) {

src/monattack.cpp

@@ -336,7 +336,7 @@ bool mattack::antqueen( monster *z )
             add_msg( m_warning, _( "The %s tends nearby eggs, and they hatch!" ), z->name() );
         }
         for( const tripoint &egg_pos : egg_points ) {
-            for( const item &it : g->m.i_at( egg_pos ) ) {
+            for( item &it : g->m.i_at( egg_pos ) ) {
                 if( it.typeId() != "ant_egg" ) {
                     continue;
                 }

src/mondeath.cpp

@@ -600,7 +600,7 @@ void mdeath::explode( monster &z )
 void mdeath::focused_beam( monster &z )
 {
 
-    for( const item &it : g->m.i_at( z.pos() ) ) {
+    for( item &it : g->m.i_at( z.pos() ) ) {
         if( it.typeId() == "processor" ) {
             g->m.i_rem( z.pos(), &it );
         }

src/savegame_json.cpp

@@ -3489,7 +3489,7 @@ void submap::load( JsonIn &jsin, const std::string &member_name, bool rubpow_upd
                     return VisitResponse::NEXT;
                 } );
 
-                colony<item>::iterator it = itm[p.x][p.y].insert( tmp );
+                const colony<item>::iterator it = itm[p.x][p.y].insert( tmp );
                 if( tmp.needs_processing() ) {
                     active_items.add( it, p );
                 }

src/vehicle.cpp

@@ -4443,32 +4443,17 @@ cata::optional<vehicle_stack::iterator> vehicle::add_item( int part, const item
     return cata::optional<vehicle_stack::iterator>( new_pos );
 }
 
-bool vehicle::remove_item( int part, int itemdex )
+bool vehicle::remove_item( int part, item *it )
 {
-    if( itemdex < 0 || itemdex >= static_cast<int>( parts[part].items.size() ) ) {
+    const colony<item> &veh_items = parts[part].items;
+    const colony<item>::const_iterator iter = veh_items.get_iterator_from_pointer( it );
+    if( iter == veh_items.end() ) {
         return false;
     }
-
-    remove_item( part, std::next( parts[part].items.begin(), itemdex ) );
+    remove_item( part, iter );
     return true;
 }
 
-bool vehicle::remove_item( int part, const item *it )
-{
-    bool rc = false;
-    colony<item> &veh_items = parts[part].items;
-
-    for( auto iter = veh_items.begin(); iter != veh_items.end(); ++iter ) {
-        //delete the item if the pointer memory addresses are the same
-        if( it == &*iter ) {
-            remove_item( part, iter );
-            rc = true;
-            break;
-        }
-    }
-    return rc;
-}
-
 vehicle_stack::iterator vehicle::remove_item( int part, vehicle_stack::const_iterator it )
 {
     colony<item> &veh_items = parts[part].items;

src/vehicle.h

@@ -1267,8 +1267,7 @@ class vehicle
         int add_charges( int part, const item &itm );
 
         // remove item from part's cargo
-        bool remove_item( int part, int itemdex );
-        bool remove_item( int part, const item *it );
+        bool remove_item( int part, item *it );
         vehicle_stack::iterator remove_item( int part, vehicle_stack::const_iterator it );
 
         vehicle_stack get_items( int part ) const;

src/vehicle_use.cpp

@@ -1209,18 +1209,16 @@ void vehicle::operate_scoop()
                 continue;
             }
             item *that_item_there = nullptr;
-            const map_stack q = g->m.i_at( position );
+            map_stack items = g->m.i_at( position );
             if( g->m.has_flag( "SEALED", position ) ) {
                 // Ignore it. Street sweepers are not known for their ability to harvest crops.
                 continue;
             }
-            size_t itemdex = 0;
-            for( const item &it : q ) {
+            for( item &it : items ) {
                 if( it.volume() < max_pickup_volume ) {
-                    that_item_there = g->m.item_from( position, itemdex );
+                    that_item_there = &it;
                     break;
                 }
-                itemdex++;
             }
             if( !that_item_there ) {
                 continue;
@@ -1235,7 +1233,7 @@ void vehicle::operate_scoop()
             }
             //This attempts to add the item to the scoop inventory and if successful, removes it from the map.
             if( add_item( scoop, *that_item_there ) ) {
-                g->m.i_rem( position, itemdex );
+                g->m.i_rem( position, that_item_there );
             } else {
                 break;
             }

tests/invlet_test.cpp

@@ -236,7 +236,7 @@ static void wear_from_feet( player &p, int pos )
     REQUIRE( size_before > pos );
     // This is a temporary hack to get this (currently broken) test to compile
     p.wear_item( *g->m.i_at( p.pos() ).get_iterator_from_index( pos ), false );
-    g->m.i_rem( p.pos(), pos );
+    g->m.i_rem( p.pos(), &*g->m.i_at( p.pos() ).get_iterator_from_index( pos ) );
 }
 
 static void wield_from_feet( player &p, int pos )
@@ -245,7 +245,7 @@ static void wield_from_feet( player &p, int pos )
     REQUIRE( size_before > pos );
     // This is a temporary hack to get this (currently broken) test to compile
     p.wield( *g->m.i_at( p.pos() ).get_iterator_from_index( pos ) );
-    g->m.i_rem( p.pos(), pos );
+    g->m.i_rem( p.pos(), &*g->m.i_at( p.pos() ).get_iterator_from_index( pos ) );
 }
 
 static void add_item( player &p, item &it, const inventory_location loc )