[CR] Fix nested pockets pickup (#54941)

* Fix nested pockets pickup

There are quite some discrepancies between can_contain and
best_pocket functionality. This commit looks to fix that and actually
allow nested pockets to work properly as long as there is space.
fixes #54725

Author: robkuijper

src/character.cpp

@@ -2945,16 +2945,7 @@ bool Character::can_pickVolume_partial( const item &it, bool, const item *avoid
         copy.charges = 1;
     }
 
-    const item weapon = get_wielded_item();
-    if( ( avoid == nullptr || &weapon != avoid ) && weapon.can_contain( copy ).success() ) {
-        return true;
-    }
-    for( const item &w : worn ) {
-        if( ( avoid == nullptr || &w != avoid ) && w.can_contain( copy ).success() ) {
-            return true;
-        }
-    }
-    return false;
+    return can_pickVolume( copy, avoid );
 }
 
 bool Character::can_pickWeight( const item &it, bool safe ) const

src/character.h

@@ -1234,11 +1234,6 @@ class Character : public Creature, public visitable
         int get_mod( const trait_id &mut, const std::string &arg ) const;
         /** Applies skill-based boosts to stats **/
         void apply_skill_boost();
-        /**
-          * What is the best pocket to put @it into?
-          * the pockets in @avoid do not count
-          */
-        std::pair<item_location, item_pocket *> best_pocket( const item &it, const item *avoid );
     protected:
 
         void on_move( const tripoint_abs_ms &old_pos ) override;
@@ -1950,6 +1945,16 @@ class Character : public Creature, public visitable
         bool can_pickVolume_partial( const item &it, bool safe = false, const item *avoid = nullptr ) const;
         bool can_pickWeight( const item &it, bool safe = true ) const;
         bool can_pickWeight_partial( const item &it, bool safe = true ) const;
+
+        /**
+          * What is the best pocket to put @it into?
+          * @param it the item to try and find a pocket for.
+          * @param avoid pockets in this item are not taken into account.
+          *
+          * @returns nullptr in the value of the returned pair if no valid pocket was found.
+          */
+        std::pair<item_location, item_pocket *> best_pocket( const item &it, const item *avoid = nullptr );
+
         /**
          * Checks if character stats and skills meet minimum requirements for the item.
          * Prints an appropriate message if requirements not met.

src/item.cpp

@@ -9324,24 +9324,27 @@ ret_val<bool> item::is_compatible( const item &it ) const
     return contents.is_compatible( it );
 }
 
-ret_val<bool> item::can_contain( const item &it ) const
+ret_val<bool> item::can_contain( const item &it, const bool nested ) const
 {
     if( this == &it ) {
         // does the set of all sets contain itself?
         return ret_val<bool>::make_failure();
     }
+    if( nested && !this->is_container() ) {
+        return ret_val<bool>::make_failure();
+    }
     // disallow putting portable holes into bags of holding
     if( contents.bigger_on_the_inside( volume() ) &&
         it.contents.bigger_on_the_inside( it.volume() ) ) {
         return ret_val<bool>::make_failure();
     }
     for( const item *internal_it : contents.all_items_top( item_pocket::pocket_type::CONTAINER ) ) {
-        if( internal_it->contents.can_contain_rigid( it ).success() ) {
+        if( internal_it->can_contain( it, true ).success() ) {
             return ret_val<bool>::make_success();
         }
     }
 
-    return contents.can_contain( it );
+    return nested ? contents.can_contain_rigid( it ) : contents.can_contain( it );
 }
 
 bool item::can_contain( const itype &tp ) const
@@ -9359,10 +9362,10 @@ bool item::can_contain_partial( const item &it ) const
 }
 
 std::pair<item_location, item_pocket *> item::best_pocket( const item &it, item_location &parent,
-        const item *avoid, const bool allow_sealed, const bool ignore_settings )
+        const item *avoid, const bool allow_sealed, const bool ignore_settings, const bool nested )
 {
     item_location nested_location( parent, this );
-    return contents.best_pocket( it, nested_location, avoid, allow_sealed, ignore_settings );
+    return contents.best_pocket( it, nested_location, avoid, allow_sealed, ignore_settings, nested );
 }
 
 bool item::spill_contents( Character &c )

src/item.h

@@ -1489,14 +1489,16 @@ class item : public visitable
         /**
          * Can the pocket contain the specified item?
          * @param it the item being put in
+         * @param nested whether or not the current call is nested (used recursively).
          */
         /*@{*/
-        ret_val<bool> can_contain( const item &it ) const;
+        ret_val<bool> can_contain( const item &it, const bool nested = false ) const;
         bool can_contain( const itype &tp ) const;
         bool can_contain_partial( const item &it ) const;
         /*@}*/
         std::pair<item_location, item_pocket *> best_pocket( const item &it, item_location &parent,
-                const item *avoid = nullptr, bool allow_sealed = false, bool ignore_settings = false );
+                const item *avoid = nullptr, bool allow_sealed = false, bool ignore_settings = false,
+                bool nested = false );
 
         units::length max_containable_length( bool unrestricted_pockets_only = false ) const;
         units::length min_containable_length() const;

src/item_contents.cpp

@@ -550,10 +550,12 @@ void item_contents::force_insert_item( const item &it, item_pocket::pocket_type
 }
 
 std::pair<item_location, item_pocket *> item_contents::best_pocket( const item &it,
-        item_location &parent, const item *avoid, const bool allow_sealed, const bool ignore_settings )
+        item_location &parent, const item *avoid, const bool allow_sealed, const bool ignore_settings,
+        const bool nested )
 {
-    std::pair<item_location, item_pocket *> ret;
-    ret.second = nullptr;
+    // @TODO: this could be made better by doing a plain preliminary volume check.
+    // if the total volume of the parent is not sufficient, a child won't have enough either.
+    std::pair<item_location, item_pocket *> ret = { parent, nullptr };
     for( item_pocket &pocket : contents ) {
         if( !pocket.is_type( item_pocket::pocket_type::CONTAINER ) ) {
             // best pocket is for picking stuff up.
@@ -565,29 +567,24 @@ std::pair<item_location, item_pocket *> item_contents::best_pocket( const item &
             // that needs to be something a player explicitly does
             continue;
         }
-        if( !pocket.can_contain( it ).success() ) {
-            continue;
-        }
         if( !ignore_settings && !pocket.settings.accepts_item( it ) ) {
             // Item forbidden by whitelist / blacklist
             continue;
         }
-        if( ret.second == nullptr || ret.second->better_pocket( pocket, it ) ) {
-            // this pocket is the new candidate for "best"
-            ret.first = parent;
+        item_pocket *const nested_content_pocket =
+            pocket.best_pocket_in_contents( parent, it, avoid, allow_sealed, ignore_settings );
+        if( nested_content_pocket != nullptr ) {
+            // item fits in pockets contents, no need to check the pocket itself.
+            // this gives nested pockets priority over parent pockets.
+            ret.second = nested_content_pocket;
+            continue;
+        }
+        if( !pocket.can_contain( it ).success() || ( nested && !pocket.rigid() ) ) {
+            // non-rigid nested pocket makes no sense, item should also be able to fit in parent.
+            continue;
+        }
+        if( ret.second == nullptr || ret.second->better_pocket( pocket, it, /*nested=*/nested ) ) {
             ret.second = &pocket;
-            // check all pockets within to see if they are better
-            for( item *contained : pocket.all_items_top() ) {
-                if( contained == avoid ) {
-                    continue;
-                }
-                std::pair<item_location, item_pocket *> internal_pocket =
-                    contained->best_pocket( it, parent, avoid, /*allow_sealed=*/false, /*ignore_settings=*/false );
-                if( internal_pocket.second != nullptr &&
-                    ret.second->better_pocket( *internal_pocket.second, it, true ) ) {
-                    ret = internal_pocket;
-                }
-            }
         }
     }
     return ret;

src/item_contents.h

@@ -36,10 +36,12 @@ class item_contents
 
         /**
           * returns an item_location and pointer to the best pocket that can contain the item @it
+          * checks all items contained in every pocket
           * only checks CONTAINER pocket type
           */
         std::pair<item_location, item_pocket *> best_pocket( const item &it, item_location &parent,
-                const item *avoid = nullptr, bool allow_sealed = false, bool ignore_settings = false );
+                const item *avoid = nullptr, bool allow_sealed = false, bool ignore_settings = false,
+                bool nested = false );
 
         units::length max_containable_length( bool unrestricted_pockets_only = false ) const;
         units::length min_containable_length() const;

src/item_pocket.cpp

@@ -1274,7 +1274,7 @@ ret_val<item_pocket::contain_code> item_pocket::is_compatible( const item &it )
 
     if( it.length() < data->min_item_length ) {
         return ret_val<item_pocket::contain_code>::make_failure(
-                   contain_code::ERR_TOO_BIG, _( "item is too short" ) );
+                   contain_code::ERR_TOO_SMALL, _( "item is too short" ) );
     }
 
     if( it.volume() < data->min_item_volume ) {
@@ -1349,7 +1349,7 @@ ret_val<item_pocket::contain_code> item_pocket::can_contain( const item &it ) co
         }
     } else if( size() == 1 && contents.front().made_of( phase_id::GAS ) ) {
         return ret_val<item_pocket::contain_code>::make_failure(
-                   contain_code::ERR_LIQUID, _( "can't put non gas into pocket with gas" ) );
+                   contain_code::ERR_GAS, _( "can't put non gas into pocket with gas" ) );
     }
 
 
@@ -1801,6 +1801,26 @@ ret_val<item_pocket::contain_code> item_pocket::insert_item( const item &it )
     return ret;
 }
 
+item_pocket *item_pocket::best_pocket_in_contents(
+    item_location &parent, const item &it, const item *avoid,
+    const bool allow_sealed, const bool ignore_settings )
+{
+    item_pocket *ret = nullptr;
+
+    for( item &contained_item : contents ) {
+        if( &contained_item == &it || &contained_item == avoid ) {
+            continue;
+        }
+        item_pocket *nested_pocket = contained_item.best_pocket( it, parent, avoid,
+                                     allow_sealed, ignore_settings, /*nested=*/true ).second;
+        if( nested_pocket != nullptr &&
+            ( ret == nullptr || ret->better_pocket( *nested_pocket, it, /*nested=*/true ) ) ) {
+            ret = nested_pocket;
+        }
+    }
+    return ret;
+}
+
 int item_pocket::obtain_cost( const item &it ) const
 {
     if( has_item( it ) ) {

src/item_pocket.h

@@ -317,6 +317,20 @@ class item_pocket
         void add( const item &it, item **ret = nullptr );
         bool can_unload_liquid() const;
 
+        /**
+        * @brief Check contents of pocket to see if it contains a valid item/pocket to store the given item.
+        * @param ret Used to cache and return a pocket if a valid one was found.
+        * @param pocket The @a item_pocket pocket to recursively look through.
+        * @param parent The parent item location, most likely provided by initial call to @a best_pocket.
+        * @param it The item to try and store.
+        * @param avoid Item to avoid trying to search for/inside of.
+        *
+        * @returns A non-nullptr if a suitable pocket is found.
+        */
+        item_pocket *best_pocket_in_contents(
+            item_location &parent, const item &it, const item *avoid,
+            const bool allow_sealed, const bool ignore_settings );
+
         // only available to help with migration from previous usage of std::list<item>
         std::list<item> &edit_contents();