map: update active items cache when removing contents

src/active_item_cache.cpp

@@ -6,12 +6,30 @@
 #include "item.h"
 #include "safe_reference.h"
 
-void active_item_cache::remove( const item *it )
+namespace
+{
+
+void _remove_if( std::list<item_reference> &active_items, item const *it )
 {
-    active_items[it->processing_speed()].remove_if( [it]( const item_reference & active_item ) {
+    active_items.remove_if( [it]( const item_reference & active_item ) {
         item *const target = active_item.item_ref.get();
         return !target || target == it;
     } );
+}
+
+} // namespace
+
+void active_item_cache::remove( const item *it )
+{
+    int const ps = it->processing_speed();
+    if( ps == item::NO_PROCESSING ) {
+        // this item might have contained an active item so remove it from all queues
+        for( decltype( active_items )::value_type &al : active_items ) {
+            _remove_if( al.second, it );
+        }
+    } else {
+        _remove_if( active_items[ps], it );
+    }
     if( it->can_revive() ) {
         special_items[ special_item_type::corpse ].remove_if( [it]( const item_reference & active_item ) {
             item *const target = active_item.item_ref.get();

src/item.cpp

@@ -12537,7 +12537,7 @@ int item::processing_speed() const
     // This item doesn't actually need processing.
     // Either it contains items that need processing. Use processing speed from those.
     // Or it is in same container with items that need processing.
-    int processing_speed = 10000;
+    int processing_speed = item::NO_PROCESSING;
     for( const item *it : contents.all_items_top( item_pocket::pocket_type::CONTAINER ) ) {
         processing_speed = std::min( processing_speed, it->processing_speed() );
     }

src/item.h

@@ -1444,6 +1444,7 @@ class item : public visitable
          * The rate at which an item should be processed, in number of turns between updates.
          */
         int processing_speed() const;
+        static constexpr int NO_PROCESSING = 10000;
         /**
          * Process and apply artifact effects. This should be called exactly once each turn, it may
          * modify character stats (like speed, strength, ...), so call it after those have been reset.

src/item_location.cpp

@@ -628,6 +628,13 @@ class item_location::impl::item_in_container : public item_location::impl
         void remove_item() override {
             on_contents_changed();
             container->remove_item( *target() );
+            item_location ancestor = parent_item();
+            while( ancestor.has_parent() ) {
+                ancestor = ancestor.parent_item();
+            }
+            if( !ancestor->needs_processing() ) {
+                get_map().remove_active_item( ancestor.position(), ancestor.get_item() );
+            }
         }
 
         void on_contents_changed() override {

src/map.cpp

@@ -4656,6 +4656,24 @@ map_stack map::i_at( const tripoint_bub_ms &p )
     return i_at( p.raw() );
 }
 
+void map::remove_active_item( tripoint const &p, item *it )
+{
+    if( !inbounds( p ) ) {
+        return;
+    }
+    point l;
+    submap *const current_submap = get_submap_at( p, l );
+    if( current_submap == nullptr ) {
+        return;
+    }
+    current_submap->active_items.remove( it );
+    if( current_submap->active_items.empty() ) {
+        // TODO: fix point types
+        submaps_with_active_items.erase( tripoint_abs_sm( abs_sub.x() + p.x / SEEX,
+                                         abs_sub.y() + p.y / SEEY, p.z ) );
+    }
+}
+
 map_stack::iterator map::i_rem( const tripoint &p, const map_stack::const_iterator &it )
 {
     point l;
@@ -5243,36 +5261,6 @@ static void process_vehicle_items( vehicle &cur_veh, int part )
     }
 }
 
-std::vector<tripoint_abs_sm> map::check_submap_active_item_consistency()
-{
-    std::vector<tripoint_abs_sm> result;
-    for( int z = -OVERMAP_DEPTH; z < OVERMAP_HEIGHT; ++z ) {
-        for( int x = 0; x < MAPSIZE; ++x ) {
-            for( int y = 0; y < MAPSIZE; ++y ) {
-                tripoint p( x, y, z );
-                submap *s = get_submap_at_grid( p );
-                if( s == nullptr ) {
-                    debugmsg( "Tried to access items at (%d,%d,%d) but the submap is not loaded", p.x, p.y, p.z );
-                    continue;
-                }
-                bool has_active_items = !s->active_items.get().empty();
-                bool map_has_active_items = submaps_with_active_items.count( p + abs_sub.xy() );
-                if( has_active_items != map_has_active_items ) {
-                    result.push_back( p + abs_sub.xy() );
-                }
-            }
-        }
-    }
-    for( const tripoint_abs_sm &p : submaps_with_active_items ) {
-        tripoint_rel_sm rel = p - abs_sub.xy();
-        half_open_rectangle<point_rel_sm> map( point_rel_sm(), point_rel_sm( MAPSIZE, MAPSIZE ) );
-        if( !map.contains( rel.xy() ) ) {
-            result.push_back( p );
-        }
-    }
-    return result;
-}
-
 void map::process_items()
 {
     const int minz = zlevels ? -OVERMAP_DEPTH : abs_sub.z();

src/map.h

@@ -1212,7 +1212,7 @@ class map
 
         // Returns points for all submaps with inconsistent state relative to
         // the list in map.  Used in tests.
-        std::vector<tripoint_abs_sm> check_submap_active_item_consistency();
+        void check_submap_active_item_consistency();
         // Accessor that returns a wrapped reference to an item stack for safe modification.
         // TODO: fix point types (remove the first overload)
         map_stack i_at( const tripoint &p );
@@ -1231,6 +1231,7 @@ class map
         map_stack::iterator i_rem( const point &location, const map_stack::const_iterator &it ) {
             return i_rem( tripoint( location, abs_sub.z() ), it );
         }
+        void remove_active_item( tripoint const &p, item *it );
         // TODO: fix point types (remove the first overload)
         void i_rem( const tripoint &p, item *it );
         void i_rem( const tripoint_bub_ms &p, item *it );

src/visitable.cpp

@@ -705,12 +705,12 @@ std::list<item> map_cursor::remove_items_with( const
             iter = stack.erase( iter );
 
             if( --count == 0 ) {
-                return res;
+                break;
             }
         } else {
             iter->remove_internal( filter, count, res );
             if( count == 0 ) {
-                return res;
+                break;
             }
             ++iter;
         }

tests/map_test.cpp

@@ -11,6 +11,7 @@
 #include "game_constants.h"
 #include "map_helpers.h"
 #include "point.h"
+#include "submap.h"
 #include "type_id.h"
 
 TEST_CASE( "map_coordinate_conversion_functions" )
@@ -136,11 +137,78 @@ TEST_CASE( "tinymap_bounds_checking" )
     }
 }
 
+void map::check_submap_active_item_consistency()
+{
+    for( int z = -OVERMAP_DEPTH; z < OVERMAP_HEIGHT; ++z ) {
+        for( int x = 0; x < MAPSIZE; ++x ) {
+            for( int y = 0; y < MAPSIZE; ++y ) {
+                tripoint p( x, y, z );
+                submap *s = get_submap_at_grid( p );
+                REQUIRE( s != nullptr );
+                bool submap_has_active_items = !s->active_items.get().empty();
+                bool cache_has_active_items = submaps_with_active_items.count( p + abs_sub.xy() ) != 0;
+                CAPTURE( abs_sub.xy(), p, p + abs_sub.xy() );
+                CHECK( submap_has_active_items == cache_has_active_items );
+            }
+        }
+    }
+    for( const tripoint_abs_sm &p : submaps_with_active_items ) {
+        tripoint_rel_sm rel = p - abs_sub.xy();
+        half_open_rectangle<point_rel_sm> map( point_rel_sm(), point_rel_sm( MAPSIZE, MAPSIZE ) );
+        CAPTURE( rel );
+        CHECK( map.contains( rel.xy() ) );
+    }
+}
+
 TEST_CASE( "place_player_can_safely_move_multiple_submaps" )
 {
     // Regression test for the situation where game::place_player would misuse
     // map::shift if the resulting shift exceeded a single submap, leading to a
     // broken active item cache.
     g->place_player( tripoint_zero );
-    CHECK( get_map().check_submap_active_item_consistency().empty() );
+    get_map().check_submap_active_item_consistency();
+}
+
+TEST_CASE( "inactive_container_with_active_contents" )
+{
+    map &here = get_map();
+    clear_map();
+    REQUIRE( here.get_submaps_with_active_items().empty() );
+    here.check_submap_active_item_consistency();
+
+    item bottle_plastic( "bottle_plastic" );
+    REQUIRE( !bottle_plastic.needs_processing() );
+    item disinfectant( "disinfectant" );
+    REQUIRE( disinfectant.needs_processing() );
+    int const bp_speed = bottle_plastic.processing_speed();
+    int const dis_speed = disinfectant.processing_speed();
+    REQUIRE( bp_speed != dis_speed );
+
+    ret_val<void> const ret =
+        bottle_plastic.put_in( disinfectant, item_pocket::pocket_type::CONTAINER );
+    REQUIRE( ret.success() );
+    REQUIRE( bottle_plastic.needs_processing() );
+    REQUIRE( bottle_plastic.processing_speed() == dis_speed );
+
+    item &bp = here.add_item( tripoint_zero, bottle_plastic );
+    item_location bp_loc( map_cursor( tripoint_zero ), &bp );
+    item_location dis_loc( bp_loc, &bp.only_item() );
+
+    REQUIRE( here.get_submaps_with_active_items().count( here.get_abs_sub() ) != 0 );
+    here.check_submap_active_item_consistency();
+
+    bool from_container = GENERATE( true, false );
+
+    if( from_container ) {
+        dis_loc.remove_item();
+        CHECK( !bp.needs_processing() );
+        CHECK( bp.processing_speed() == bp_speed );
+    } else {
+        bp_loc->get_contents().clear_items();
+        CHECK( !bp.needs_processing() );
+        CHECK( bp.processing_speed() == bp_speed );
+        bp_loc.remove_item();
+    }
+    CHECK( here.get_submaps_with_active_items().count( here.get_abs_sub() ) == 0 );
+    here.check_submap_active_item_consistency();
 }