Fix proficiency books' effect on crafting (CleverRaven#48650)

* Fix proficiency books' effect on crafting

* Cache recipe batch time in craft_activity_actor::do_turn()

* Make crafting time functions const and/or operate on const Character.

Also makes Character::crafting_inventory() const.

* add a unit test

* also check that proficiency mitigation is not as good as having prof

* use a struct to encapsulate the crafting inventory cache

* clean up one hack

* Add TODO for getting rid of const_cast

src/activity_actor.cpp

@@ -1761,6 +1761,10 @@ std::unique_ptr<activity_actor> unload_activity_actor::deserialize( JsonIn &jsin
 craft_activity_actor::craft_activity_actor( item_location &it, const bool is_long ) :
     craft_item( it ), is_long( is_long )
 {
+    cached_crafting_speed = 0;
+    cached_assistants = 0;
+    cached_base_total_moves = 1;
+    cached_cur_total_moves = 1;
 }
 
 bool craft_activity_actor::check_if_craft_okay( item_location &craft_item, Character &crafter )
@@ -1796,6 +1800,7 @@ void craft_activity_actor::start( player_activity &act, Character &crafter )
     }
     act.moves_left = calendar::INDEFINITELY_LONG;
     activity_override = craft_item.get_item()->get_making().exertion_level();
+    cached_crafting_speed = 0;
 }
 
 void craft_activity_actor::do_turn( player_activity &act, Character &crafter )
@@ -1819,18 +1824,26 @@ void craft_activity_actor::do_turn( player_activity &act, Character &crafter )
         return;
     }
 
+    if( cached_crafting_speed != crafting_speed || cached_assistants != assistants ) {
+        cached_crafting_speed = crafting_speed;
+        cached_assistants = assistants;
+
+        // Base moves for batch size with no speed modifier or assistants
+        // Must ensure >= 1 so we don't divide by 0;
+        cached_base_total_moves = std::max( static_cast<int64_t>( 1 ),
+                                            rec.batch_time( crafter, craft.charges, 1.0f, 0 ) );
+        // Current expected total moves, includes crafting speed modifiers and assistants
+        cached_cur_total_moves = std::max( static_cast<int64_t>( 1 ),
+                                           rec.batch_time( crafter, craft.charges, crafting_speed,
+                                                   assistants ) );
+    }
+    const double base_total_moves = cached_base_total_moves;
+    const double cur_total_moves = cached_cur_total_moves;
+
     // item_counter represents the percent progress relative to the base batch time
     // stored precise to 5 decimal places ( e.g. 67.32 percent would be stored as 6'732'000 )
     const int old_counter = craft.item_counter;
 
-    // Base moves for batch size with no speed modifier or assistants
-    // Must ensure >= 1 so we don't divide by 0;
-    const double base_total_moves = std::max( static_cast<int64_t>( 1 ), rec.batch_time( crafter,
-                                    craft.charges, 1.0f, 0 ) );
-    // Current expected total moves, includes crafting speed modifiers and assistants
-    const double cur_total_moves = std::max( static_cast<int64_t>( 1 ), rec.batch_time( crafter,
-                                   craft.charges, crafting_speed,
-                                   assistants ) );
     // Delta progress in moves adjusted for current crafting speed /
     //crafter.exertion_adjusted_move_multiplier( exertion_level() )
     int spent_moves = crafter.get_moves() * crafter.exertion_adjusted_move_multiplier(
@@ -1863,6 +1876,8 @@ void craft_activity_actor::do_turn( player_activity &act, Character &crafter )
         // Divide by 100 for seconds, 20 for 5%
         const time_duration pct_time = time_duration::from_seconds( base_total_moves / 2000 );
         crafter.craft_proficiency_gain( craft, pct_time * five_percent_steps );
+        // Invalidate the crafting time cache because proficiencies may have changed
+        cached_crafting_speed = 0;
     }
 
     // Unlike skill, tools are consumed once at the start and should not be consumed at the end

src/activity_actor_definitions.h

@@ -698,6 +698,10 @@ class craft_activity_actor : public activity_actor
 
         float activity_override = NO_EXERCISE;
         cata::optional<requirement_data> cached_continuation_requirements;
+        float cached_crafting_speed;
+        int cached_assistants;
+        double cached_base_total_moves;
+        double cached_cur_total_moves;
 
         bool check_if_craft_okay( item_location &craft_item, Character &crafter );
     public:

src/character.cpp

@@ -437,7 +437,6 @@ std::string enum_to_string<blood_type>( blood_type data )
 
 // *INDENT-OFF*
 Character::Character() :
-    cached_time( calendar::before_time_starts ),
     id( -1 ),
     next_climate_control_check( calendar::before_time_starts ),
     last_climate_control_ret( false )
@@ -481,6 +480,7 @@ Character::Character() :
 
     move_mode = move_mode_id( "walk" );
     next_expected_position = cata::nullopt;
+    crafting_cache.time = calendar::before_time_starts;
 }
 // *INDENT-ON*
 

src/character.h

@@ -2011,8 +2011,6 @@ class Character : public Creature, public visitable
         // Save favorite ammo location
         item_location ammo_location;
         std::set<tripoint_abs_omt> camps;
-        /* crafting inventory cached time */
-        time_point cached_time;
 
         std::vector <addiction> addictions;
         /** Adds an addiction to the player */
@@ -2462,7 +2460,7 @@ class Character : public Creature, public visitable
         void clear_morale();
         bool has_morale_to_read() const;
         bool has_morale_to_craft() const;
-        const inventory &crafting_inventory( bool clear_path );
+        const inventory &crafting_inventory( bool clear_path ) const;
         /**
         * Returns items that can be used to craft with. Always includes character inventory.
         * @param src_pos Character position.
@@ -2471,7 +2469,7 @@ class Character : public Creature, public visitable
         * @returns Craftable inventory items found.
         * */
         const inventory &crafting_inventory( const tripoint &src_pos = tripoint_zero,
-                                             int radius = PICKUP_RANGE, bool clear_path = true );
+                                             int radius = PICKUP_RANGE, bool clear_path = true ) const;
         void invalidate_crafting_inventory();
 
         /** Returns a value from 1.0 to 11.0 that acts as a multiplier
@@ -2530,15 +2528,10 @@ class Character : public Creature, public visitable
         /** For use with in progress crafts */
         float crafting_speed_multiplier( const item &craft, const cata::optional<tripoint> &loc ) const;
         int available_assistant_count( const recipe &rec ) const;
-        /**
-         * Time to craft not including speed multiplier
-         */
-        int64_t base_time_to_craft( const recipe &rec, int batch_size = 1 ) const;
         /**
          * Expected time to craft a recipe, with assumption that multipliers stay constant.
          */
-        int64_t expected_time_to_craft( const recipe &rec, int batch_size = 1,
-                                        bool in_progress = false ) const;
+        int64_t expected_time_to_craft( const recipe &rec, int batch_size = 1 ) const;
         std::vector<const item *> get_eligible_containers_for_crafting() const;
         bool check_eligible_containers_for_crafting( const recipe &rec, int batch_size = 1 ) const;
         bool can_make( const recipe *r, int batch_size = 1 );  // have components?
@@ -2945,9 +2938,14 @@ class Character : public Creature, public visitable
 
         struct weighted_int_list<std::string> melee_miss_reasons;
 
-        int cached_moves;
-        tripoint cached_position;
-        pimpl<inventory> cached_crafting_inventory;
+        struct crafting_cache_type {
+            time_point time;
+            int moves;
+            tripoint position;
+            int radius;
+            pimpl<inventory> crafting_inventory;
+        };
+        mutable crafting_cache_type crafting_cache;
 
         time_point melee_warning_turn = calendar::turn_zero;
     protected:

src/crafting.cpp

@@ -375,17 +375,10 @@ int Character::available_assistant_count( const recipe &rec ) const
     } );
 }
 
-int64_t Character::base_time_to_craft( const recipe &rec, int batch_size ) const
+int64_t Character::expected_time_to_craft( const recipe &rec, int batch_size ) const
 {
     const size_t assistants = available_assistant_count( rec );
-    return rec.batch_time( *this, batch_size, 1.0f, assistants );
-}
-
-int64_t Character::expected_time_to_craft( const recipe &rec, int batch_size,
-        bool in_progress ) const
-{
-    const size_t assistants = available_assistant_count( rec );
-    float modifier = crafting_speed_multiplier( rec, in_progress );
+    float modifier = crafting_speed_multiplier( rec );
     return rec.batch_time( *this, batch_size, modifier, assistants );
 }
 
@@ -536,61 +529,62 @@ bool Character::can_start_craft( const recipe *rec, recipe_filter_flags flags, i
                inv, rec->get_component_filter( flags ), batch_size, craft_flags::start_only );
 }
 
-const inventory &Character::crafting_inventory( bool clear_path )
+const inventory &Character::crafting_inventory( bool clear_path ) const
 {
     return crafting_inventory( tripoint_zero, PICKUP_RANGE, clear_path );
 }
 
 const inventory &Character::crafting_inventory( const tripoint &src_pos, int radius,
-        bool clear_path )
+        bool clear_path ) const
 {
     tripoint inv_pos = src_pos;
     if( src_pos == tripoint_zero ) {
         inv_pos = pos();
     }
-    static int radius_mem = radius;
-    if( cached_moves == moves
-        && radius_mem == radius
-        && cached_time == calendar::turn
-        && cached_position == inv_pos ) {
-        return *cached_crafting_inventory;
+    if( moves == crafting_cache.moves
+        && radius == crafting_cache.radius
+        && calendar::turn == crafting_cache.time
+        && inv_pos == crafting_cache.position ) {
+        return *crafting_cache.crafting_inventory;
     }
-    radius_mem = radius;
-    cached_crafting_inventory->clear();
+    crafting_cache.crafting_inventory->clear();
     if( radius >= 0 ) {
-        cached_crafting_inventory->form_from_map( inv_pos, radius, this, false, clear_path );
+        crafting_cache.crafting_inventory->form_from_map( inv_pos, radius, this, false, clear_path );
     }
 
-    for( const item_location &it : all_items_loc() ) {
+    // TODO: Add a const overload of all_items_loc() that returns something like
+    // vector<const_item_location> in order to get rid of the const_cast here.
+    for( const item_location &it : const_cast<Character *>( this )->all_items_loc() ) {
         // can't craft with containers that have items in them
         if( !it->contents.empty_container() ) {
             continue;
         }
-        cached_crafting_inventory->add_item( *it );
+        crafting_cache.crafting_inventory->add_item( *it );
     }
 
     for( const bionic &bio : *my_bionics ) {
         const bionic_data &bio_data = bio.info();
         if( ( !bio_data.activated || bio.powered ) &&
             !bio_data.fake_item.is_empty() ) {
-            *cached_crafting_inventory += item( bio.info().fake_item,
-                                                calendar::turn, units::to_kilojoule( get_power_level() ) );
+            *crafting_cache.crafting_inventory += item( bio.info().fake_item,
+                                                  calendar::turn, units::to_kilojoule( get_power_level() ) );
         }
     }
     if( has_trait( trait_BURROW ) ) {
-        *cached_crafting_inventory += item( "pickaxe", calendar::turn );
-        *cached_crafting_inventory += item( "shovel", calendar::turn );
+        *crafting_cache.crafting_inventory += item( "pickaxe", calendar::turn );
+        *crafting_cache.crafting_inventory += item( "shovel", calendar::turn );
     }
 
-    cached_moves = moves;
-    cached_time = calendar::turn;
-    cached_position = inv_pos;
-    return *cached_crafting_inventory;
+    crafting_cache.moves = moves;
+    crafting_cache.time = calendar::turn;
+    crafting_cache.position = inv_pos;
+    crafting_cache.radius = radius;
+    return *crafting_cache.crafting_inventory;
 }
 
 void Character::invalidate_crafting_inventory()
 {
-    cached_time = calendar::before_time_starts;
+    crafting_cache.time = calendar::before_time_starts;
 }
 
 void Character::make_craft( const recipe_id &id_to_make, int batch_size,
@@ -1020,14 +1014,7 @@ double Character::crafting_success_roll( const recipe &making ) const
         return 2;
     }
 
-    float prof_multiplier = 1.0f;
-    for( const recipe_proficiency &recip : making.proficiencies ) {
-        if( !recip.required && !has_proficiency( recip.id ) ) {
-            prof_multiplier *= recip.fail_multiplier;
-        }
-    }
-
-    return ( skill_roll / diff_roll ) / prof_multiplier;
+    return ( skill_roll / diff_roll ) / making.proficiency_failure_maluses( *this );
 }
 
 int item::get_next_failure_point() const

src/recipe.cpp

@@ -67,16 +67,7 @@ int64_t recipe::time_to_craft_moves( const Character &guy, recipe_time_flag flag
     if( flags == recipe_time_flag::ignore_proficiencies ) {
         return time;
     }
-    int64_t ret = time;
-    for( const recipe_proficiency &prof : proficiencies ) {
-        if( !prof.required ) {
-            if( !guy.has_proficiency( prof.id ) &&
-                !helpers_have_proficiencies( guy, prof.id ) ) {
-                ret *= prof.time_multiplier;
-            }
-        }
-    }
-    return ret;
+    return time * proficiency_time_maluses( guy );
 }
 
 int64_t recipe::batch_time( const Character &guy, int batch, float multiplier,
@@ -658,7 +649,7 @@ std::string recipe::used_proficiencies_string( const Character *c ) const
     return used;
 }
 
-std::string recipe::missing_proficiencies_string( Character *c ) const
+std::string recipe::missing_proficiencies_string( const Character *c ) const
 {
     if( c == nullptr ) {
         return { };
@@ -738,7 +729,7 @@ std::set<proficiency_id> recipe::assist_proficiencies() const
     return ret;
 }
 
-float recipe::proficiency_time_maluses( Character &guy ) const
+float recipe::proficiency_time_maluses( const Character &guy ) const
 {
     float total_malus = 1.0f;
     for( const recipe_proficiency &prof : proficiencies ) {
@@ -752,7 +743,7 @@ float recipe::proficiency_time_maluses( Character &guy ) const
     return total_malus;
 }
 
-float recipe::proficiency_failure_maluses( Character &guy ) const
+float recipe::proficiency_failure_maluses( const Character &guy ) const
 {
     float total_malus = 1.0f;
     for( const recipe_proficiency &prof : proficiencies ) {

src/recipe.h

@@ -174,7 +174,7 @@ class recipe
         // Format the proficiencies string.
         std::string required_proficiencies_string( const Character *c ) const;
         std::string used_proficiencies_string( const Character *c ) const;
-        std::string missing_proficiencies_string( Character *c ) const;
+        std::string missing_proficiencies_string( const Character *c ) const;
         // Proficiencies for search
         std::string recipe_proficiencies_string() const;
         // Required proficiencies
@@ -184,9 +184,9 @@ class recipe
         // Helpful proficiencies
         std::set<proficiency_id> assist_proficiencies() const;
         // The time malus due to proficiencies lacking
-        float proficiency_time_maluses( Character &guy ) const;
+        float proficiency_time_maluses( const Character &guy ) const;
         // The failure malus due to proficiencies lacking
-        float proficiency_failure_maluses( Character &guy ) const;
+        float proficiency_failure_maluses( const Character &guy ) const;
 
         // How active of exercise this recipe is
         float exertion_level() const;