Rework active_item_cache to use safe references

After CleverRaven#31796 item processing invalidates item_locations, which
causes issues with all activities using item_locations.

This commit rewrites active_item_cache to store a safe_reference<item>
instead of an item_stack::iterator, removing the now unnecessary
active_item_set and has() functions.

This also fixes a bug with active item reording after processing introduced
by CleverRaven#31406 and a minor bug with the submaps_with_active_items cache.

src/active_item_cache.cpp

@@ -7,72 +7,75 @@
 #include "debug.h"
 #include "item.h"
 #include "item_stack.h"
+#include "safe_reference.h"
 
-void active_item_cache::remove( item_stack::iterator it, point location )
+void active_item_cache::remove( const item *it )
 {
-    const auto predicate = [&]( const item_reference & active_item ) {
-        return location == active_item.location && active_item.item_iterator == it;
-    };
-    // The iterator is expected to be in this list, as it was added that way in `add`.
-    // But the processing_speed may have changed, so if it's not in the expected container,
-    // remove it from all containers to ensure no stale iterator remains.
-    auto &expected = active_items[it->processing_speed()];
-    const auto iter = std::find_if( expected.begin(), expected.end(), predicate );
-    if( iter != expected.end() ) {
-        expected.erase( iter );
-    } else {
-        for( auto &e : active_items ) {
-            e.second.remove_if( predicate );
-        }
-    }
-    if( active_item_set.erase( &*it ) == 0 ) {
-        // map erase returns number of elements erased
-        debugmsg( "The item isn't there!" );
-    }
+    active_items[it->processing_speed()].remove_if( [it]( const item_reference & active_item ) {
+        item *const target = active_item.safe_ref.get();
+        return !target || target == it;
+    } );
 }
 
-void active_item_cache::add( item_stack::iterator it, point location )
+void active_item_cache::add( safe_reference<item> it, point location )
 {
-    if( has( it, location ) ) {
+    item *const target = it.get();
+    if( !target ) {
+        debugmsg( "Tried to add nonexistant item to the active item cache at (%d, %d)", location.x,
+                  location.y );
         return;
     }
-    active_items[it->processing_speed()].push_back( item_reference{ location, it, &*it } );
-    active_item_set[ &*it ] = false;
-}
-
-bool active_item_cache::has( item_stack::iterator it, point ) const
-{
-    return active_item_set.find( &*it ) != active_item_set.end();
+    // If the item is alread in the cache for some reason, don't add a second reference
+    std::list<item_reference> &target_list = active_items[target->processing_speed()];
+    if( std::find_if( target_list.begin(),
+    target_list.end(), [target]( const item_reference & active_item_ref ) {
+    return target == active_item_ref.safe_ref.get();
+    } ) != target_list.end() ) {
+        return;
+    }
+    target_list.emplace_back( item_reference{ location, it } );
 }
 
-bool active_item_cache::has( const item_reference &itm ) const
+bool active_item_cache::empty() const
 {
-    const auto found = active_item_set.find( itm.item_id );
-    return found != active_item_set.end() && found->second;
+    return active_items.empty();
 }
 
-bool active_item_cache::empty() const
+std::vector<item_reference> active_item_cache::get()
 {
-    return active_items.empty();
+    std::vector<item_reference> all_cached_items;
+    for( std::pair<const int, std::list<item_reference>> &kv : active_items ) {
+        for( std::list<item_reference>::iterator it = kv.second.begin(); it != kv.second.end(); ) {
+            if( it->safe_ref ) {
+                all_cached_items.emplace_back( *it );
+            } else {
+                it = kv.second.erase( it );
+            }
+        }
+    }
+    return all_cached_items;
 }
 
-// get() only returns the first size() / processing_speed() elements of each list, rounded up.
-// It relies on the processing logic to remove and reinsert the items to they
-// move to the back of their respective lists (or to new lists).
-// Otherwise only the first n items will ever be processed.
-std::list<item_reference> active_item_cache::get()
+std::vector<item_reference> active_item_cache::get_for_processing()
 {
-    std::list<item_reference> items_to_process;
-    for( auto &tuple : active_items ) {
+    std::vector<item_reference> items_to_process;
+    for( std::pair<const int, std::list<item_reference>> &kv : active_items ) {
         // Rely on iteration logic to make sure the number is sane.
-        int num_to_process = tuple.second.size() / tuple.first;
-        for( auto &an_iter : tuple.second ) {
-            active_item_set[an_iter.item_id] = true;
-            items_to_process.push_back( an_iter );
-            if( --num_to_process < 0 ) {
-                break;
+        int num_to_process = kv.second.size() / kv.first;
+        std::list<item_reference>::iterator it = kv.second.begin();
+        for( ; it != kv.second.end() && num_to_process >= 0; ) {
+            if( it->safe_ref ) {
+                items_to_process.push_back( *it );
+                --num_to_process;
+                ++it;
+            } else {
+                // The item has been destroyed, so remove the reference from the cache
+                it = kv.second.erase( it );
             }
         }
+        // Rotate the returned items to the end of their list so that the items that weren't
+        // returned this time will be first in line on the next call
+        kv.second.splice( kv.second.end(), kv.second, kv.second.begin(), it );
     }
     return items_to_process;
 }

src/active_item_cache.h

@@ -9,32 +9,52 @@
 #include "enums.h"
 #include "item.h"
 #include "item_stack.h"
+#include "safe_reference.h"
 
 // A struct used to uniquely identify an item within a submap or vehicle.
 struct item_reference {
     point location;
-    item_stack::iterator item_iterator;
-    // Do not access this from outside this module, it is only used as an ID for active_item_set.
-    item *item_id;
+    safe_reference<item> safe_ref;
 };
 
 class active_item_cache
 {
     private:
         std::unordered_map<int, std::list<item_reference>> active_items;
-        // Cache for fast lookup when we're iterating over the active items to verify the item is present.
-        // Key is item_id, value is whether it was returned in the last call to get
-        std::unordered_map<item *, bool> active_item_set;
 
     public:
-        void remove( item_stack::iterator it, point location );
-        void add( item_stack::iterator it, point location );
-        bool has( item_stack::iterator it, point ) const;
-        // Use this one if there's a chance that the item being referenced has been invalidated.
-        bool has( const item_reference &itm ) const;
+        /**
+         * Removes the item if it is in the cache. Does nothing if the item is not in the cache.
+         * Relies on the fact that item::processing_speed() is a constant.
+         * Also removes any items that have been destroyed in the list containing it
+         */
+        void remove( const item *it );
+
+        /**
+         * Adds the reference to the cache. Does nothing if the reference is already in the cache.
+         * Relies on the fact that item::processing_speed() is a constant.
+         */
+        void add( safe_reference<item> it, point location );
+
+        /**
+         * Returns true if the cache is empty
+         */
         bool empty() const;
 
-        std::list<item_reference> get();
+        /**
+         * Returns a vector of all cached active item references.
+         * Broken references are removed from the cache.
+         */
+        std::vector<item_reference> get();
+
+        /**
+         * Returns the first size() / processing_speed() elements of each list, rounded up.
+         * Items returned are rotated to the back of their respective lists, otherwise only the
+         * first n items will ever be processed.
+         * Broken references are removed from the cache.
+         * Relies on the fact that item::processing_speed() is a constant.
+         */
+        std::vector<item_reference> get_for_processing();
 
         /** Subtract delta from every item_reference's location */
         void subtract_locations( const point &delta );