Optimize item::stacks_with for further load time gains

Co-Authored-By: akrieger <[email protected]>

src/cata_utility.h

@@ -624,6 +624,57 @@ std::map<K, V> map_without_keys( const std::map<K, V> &original, const std::vect
     return filtered;
 }
 
+template<typename Map, typename Set>
+bool map_equal_ignoring_keys( const Map &lhs, const Map &rhs, const Set &ignore_keys )
+{
+    // Since map and set are sorted, we can do this as a single pass with only conditional checks into remove_keys
+    if( ignore_keys.empty() ) {
+        return lhs == rhs;
+    }
+
+    auto lbegin = lhs.begin();
+    auto lend = lhs.end();
+    auto rbegin = rhs.begin();
+    auto rend = rhs.end();
+
+    for( ; lbegin != lend && rbegin != rend; ++lbegin, ++rbegin ) {
+        // Sanity check keys
+        if( lbegin->first != rbegin->first ) {
+            while( lbegin != lend && ignore_keys.count( lbegin->first ) == 1 ) {
+                ++lbegin;
+            }
+            if( lbegin == lend ) {
+                break;
+            }
+            if( rbegin->first != lbegin->first ) {
+                while( rbegin != rend && ignore_keys.count( rbegin->first ) == 1 ) {
+                    ++rbegin;
+                }
+                if( rbegin == rend ) {
+                    break;
+                }
+            }
+            // If we've skipped ignored keys and the keys still don't match,
+            // then the maps are unequal.
+            if( lbegin->first != rbegin->first ) {
+                return false;
+            }
+        }
+        if( lbegin->second != rbegin->second && ignore_keys.count( lbegin->first ) != 1 ) {
+            return false;
+        }
+        // Either the values were equal, or the key was ignored.
+    }
+    // At least one map ran out of keys. The other may still have ignored keys in it.
+    while( lbegin != lend && ignore_keys.count( lbegin->first ) ) {
+        ++lbegin;
+    }
+    while( rbegin != rend && ignore_keys.count( rbegin->first ) ) {
+        ++rbegin;
+    }
+    return lbegin == lend && rbegin == rend;
+}
+
 int modulo( int v, int m );
 
 /** Add elements from one set to another */

src/item.cpp

@@ -1436,7 +1436,8 @@ bool item::stacks_with( const item &rhs, bool check_components, bool combine_liq
           itype_variant().id != rhs.itype_variant().id ) ) {
         return false;
     }
-    if( ammo_remaining() != 0 && rhs.ammo_remaining() != 0 && is_money() ) {
+    const std::set<ammotype> ammo = ammo_types();
+    if( is_money( ammo ) && ammo_remaining( ammo ) != 0 && rhs.ammo_remaining() != 0 ) {
         // Dealing with nonempty cash cards
         // TODO: Fix cash cards not showing total value. Until that is fixed do not stack cash cards.
         // When that is fixed just change this to true.
@@ -1488,12 +1489,11 @@ bool item::stacks_with( const item &rhs, bool check_components, bool combine_liq
     }
     // Guns that differ only by dirt/shot_counter can still stack,
     // but other item_vars such as label/note will prevent stacking
-    const std::vector<std::string> ignore_keys = { "dirt", "shot_counter", "spawn_location_omt" };
-    if( map_without_keys( *item_vars, ignore_keys ) != map_without_keys( *rhs.item_vars,
-            ignore_keys ) ) {
+    static const std::set<std::string> ignore_keys = { "dirt", "shot_counter", "spawn_location_omt" };
+    if( !map_equal_ignoring_keys( item_vars, rhs.item_vars, ignore_keys ) ) {
         return false;
     }
-    const std::string omt_loc_var = "spawn_location_omt";
+    static const std::string omt_loc_var = "spawn_location_omt";
     const bool this_has_location = has_var( omt_loc_var );
     const bool that_has_location = has_var( omt_loc_var );
     if( this_has_location != that_has_location ) {
@@ -8602,7 +8602,12 @@ bool item::ready_to_revive( map &here, const tripoint &pos ) const
 
 bool item::is_money() const
 {
-    return ammo_types().count( ammo_money );
+    return is_money( ammo_types() );
+}
+
+bool item::is_money( const std::set<ammotype> &ammo ) const
+{
+    return ammo.count( ammo_money );
 }
 
 bool item::is_cash_card() const
@@ -10671,7 +10676,8 @@ int item::shots_remaining( const Character *carrier ) const
     return ret;
 }
 
-int item::ammo_remaining( const Character *carrier, const bool include_linked ) const
+int item::ammo_remaining( const std::set<ammotype> &ammo, const Character *carrier,
+                          const bool include_linked ) const
 {
     int ret = 0;
 
@@ -10693,7 +10699,6 @@ int item::ammo_remaining( const Character *carrier, const bool include_linked )
         }
     }
 
-    std::set<ammotype> ammo = ammo_types();
     // Non ammo using item that uses charges
     if( ammo.empty() ) {
         ret += charges;
@@ -10730,6 +10735,11 @@ int item::ammo_remaining( const Character *carrier, const bool include_linked )
 
     return ret;
 }
+int item::ammo_remaining( const Character *carrier, const bool include_linked ) const
+{
+    std::set<ammotype> ammo = ammo_types();
+    return ammo_remaining( ammo, carrier, include_linked );
+}
 
 int item::ammo_remaining( const bool include_linked ) const
 {

src/item.h

@@ -353,6 +353,10 @@ class item : public visitable
         bool ready_to_revive( map &here, const tripoint &pos ) const;
 
         bool is_money() const;
+    private:
+        bool is_money( const std::set<ammotype> &ammo ) const;
+    public:
+
         bool is_cash_card() const;
         bool is_software() const;
         bool is_software_storage() const;
@@ -2376,6 +2380,10 @@ class item : public visitable
          */
         int ammo_remaining( const Character *carrier = nullptr, bool include_linked = false ) const;
         int ammo_remaining( bool include_linked ) const;
+    private:
+        int ammo_remaining( const std::set<ammotype> &ammo, const Character *carrier = nullptr,
+                            bool include_linked = false ) const;
+    public:
 
         /**
          * ammo capacity for a specific ammo

tests/cata_utility_test.cpp

@@ -278,6 +278,76 @@ TEST_CASE( "map_without_keys", "[map][filter]" )
     CHECK_FALSE( map_without_keys( map_dirt_2, dirt ) == map_without_keys( map_name_a_dirt_2, dirt ) );
 }
 
+TEST_CASE( "map_equal_ignoring_keys", "[map][filter]" )
+{
+    std::map<std::string, std::string> map_empty;
+    std::map<std::string, std::string> map_name_a = {
+        { "name", "a" }
+    };
+    std::map<std::string, std::string> map_name_b = {
+        { "name", "b" }
+    };
+    std::map<std::string, std::string> map_dirt_1 = {
+        { "dirt", "1" }
+    };
+    std::map<std::string, std::string> map_dirt_2 = {
+        { "dirt", "2" }
+    };
+    std::map<std::string, std::string> map_name_a_dirt_1 = {
+        { "name", "a" },
+        { "dirt", "1" }
+    };
+    std::map<std::string, std::string> map_name_a_dirt_2 = {
+        { "name", "a" },
+        { "dirt", "2" }
+    };
+    std::set<std::string> dirt = { "dirt" };
+
+    // Empty maps compare equal to maps with all keys filtered out
+    CHECK( map_equal_ignoring_keys( map_empty, map_dirt_1, dirt ) );
+    CHECK( map_equal_ignoring_keys( map_empty, map_dirt_2, dirt ) );
+
+    // Maps are equal when all differing keys are filtered out
+    // (same name, dirt filtered out)
+    CHECK( map_equal_ignoring_keys( map_name_a, map_name_a_dirt_1, dirt ) );
+    CHECK( map_equal_ignoring_keys( map_name_a, map_name_a_dirt_2, dirt ) );
+
+    // Maps are different if some different keys remain after filtering
+    // (different name, no dirt to filter out)
+    CHECK_FALSE( map_equal_ignoring_keys( map_name_a, map_name_b, dirt ) );
+    CHECK_FALSE( map_equal_ignoring_keys( map_name_b, map_name_a, dirt ) );
+    // (different name, dirt filtered out)
+    CHECK_FALSE( map_equal_ignoring_keys( map_dirt_1, map_name_a_dirt_1, dirt ) );
+    CHECK_FALSE( map_equal_ignoring_keys( map_dirt_2, map_name_a_dirt_2, dirt ) );
+
+    // Maps with different ignored keys are equal after filtering them out.
+    std::map<std::string, std::string> rock_and_beer = {
+        { "rock", "granite" },
+        { "beer", "stout" }
+    };
+    std::map<std::string, std::string> beer_and_stone = {
+        { "beer", "stout" },
+        { "stone", "boulder" }
+    };
+    std::map<std::string, std::string> lagers_are_best = {
+        { "beer", "lager" },
+        { "rock", "schist" },
+        { "stone", "pebble" }
+    };
+    std::map<std::string, std::string> major_lager = {
+        {"beer", "lager" }
+    };
+    std::set<std::string> rock_and_stone = { "rock", "stone" };
+    CHECK( map_equal_ignoring_keys( rock_and_beer, beer_and_stone, rock_and_stone ) );
+
+    // Tests still work when one map has more keys than the other, as long as all are ignored.
+    CHECK( map_equal_ignoring_keys( major_lager, lagers_are_best, rock_and_stone ) );
+    CHECK( map_equal_ignoring_keys( lagers_are_best, major_lager, rock_and_stone ) );
+
+    CHECK_FALSE( map_equal_ignoring_keys( rock_and_beer, lagers_are_best, rock_and_stone ) );
+    CHECK_FALSE( map_equal_ignoring_keys( lagers_are_best, beer_and_stone, rock_and_stone ) );
+}
+
 TEST_CASE( "check_debug_menu_string_methods", "[debug_menu]" )
 {
     std::map<std::string, std::vector<std::string>> split_expect = {