QOL Don't pick up books before reading

Also obsoleted "hates books" trait, it's not worth a point.

data/json/mutations/mutations.json

@@ -398,7 +398,7 @@
     "description": "There's nothing quite like the smell of a good book!  Books are more fun (or less boring) for you!",
     "starting_trait": true,
     "valid": false,
-    "cancels": [ "ILLITERATE", "HATES_BOOKS" ]
+    "cancels": [ "ILLITERATE" ]
   },
   {
     "type": "mutation",
@@ -825,7 +825,7 @@
     "description": "You never learned to read!  Books and computers are off-limits to you.",
     "starting_trait": true,
     "valid": false,
-    "cancels": [ "FASTREADER", "SLOWREADER", "PROF_DICEMASTER", "LOVES_BOOKS", "HATES_BOOKS" ]
+    "cancels": [ "FASTREADER", "SLOWREADER", "PROF_DICEMASTER", "LOVES_BOOKS" ]
   },
   {
     "type": "mutation",
@@ -903,16 +903,6 @@
     "vitamins_absorb_multi": [ [ "veggy", [ [ "vitA", 0 ], [ "vitB", 0 ], [ "vitC", 0 ], [ "calcium", 0 ], [ "iron", 0 ] ] ] ],
     "cancels": [ "VEGETARIAN" ]
   },
-  {
-    "type": "mutation",
-    "id": "HATES_BOOKS",
-    "name": { "str": "Hates Books" },
-    "points": -1,
-    "description": "Reading is for nerds!  Boring books are more boring, and you can't have fun by reading books.",
-    "starting_trait": true,
-    "valid": false,
-    "cancels": [ "ILLITERATE", "LOVES_BOOKS" ]
-  },
   {
     "type": "mutation",
     "id": "ANTIFRUIT",

data/json/obsoletion/mutations.json

@@ -52,5 +52,14 @@
     "description": "You've been taught proper table manners from your early childhood on.  Now you can't even think about eating without a table.  Eating without it frustrates you, but eating like a civilized person gives you a bigger morale bonus.",
     "starting_trait": false,
     "valid": false
+  },
+  {
+    "type": "mutation",
+    "id": "HATES_BOOKS",
+    "name": { "str": "Hates Books" },
+    "points": -1,
+    "description": "Reading is for nerds!  Boring books are more boring, and you can't have fun by reading books.",
+    "valid": false,
+    "cancels": [ "ILLITERATE", "LOVES_BOOKS" ]
   }
 ]

src/activity_handlers.cpp

@@ -3272,6 +3272,14 @@ void activity_handlers::read_do_turn( player_activity *act, player *p )
     } else {
         p->moves = 0;
     }
+
+    if( calendar::once_every( 1_minutes ) ) {
+        item_location loc = act->targets[0];
+        if( !loc || !loc->is_book() ) {
+            p->add_msg_if_player( m_bad, _( "You lost your book!  You stop reading." ) );
+            act->set_to_null();
+        }
+    }
 }
 
 void activity_handlers::read_finish( player_activity *act, player *p )
@@ -3282,10 +3290,10 @@ void activity_handlers::read_finish( player_activity *act, player *p )
     }
     if( p->is_npc() ) {
         npc *guy = dynamic_cast<npc *>( p );
-        guy->finish_read( * act->targets.front().get_item() );
+        guy->finish_read( act->targets.front() );
     } else {
         if( avatar *u = dynamic_cast<avatar *>( p ) ) {
-            u->do_read( *act->targets.front().get_item() );
+            u->do_read( act->targets.front() );
         } else {
             act->set_to_null();
         }

src/avatar.cpp

@@ -383,12 +383,13 @@ int avatar::time_to_read( const item &book, const player &reader, const player *
  * str_values: Parallel to values, these contain the learning penalties (as doubles in string form) as follows:
  *             Experience gained = Experience normally gained * penalty
  */
-bool avatar::read( item &it, const bool continuous )
+bool avatar::read( item_location loc, const bool continuous )
 {
-    if( it.is_null() ) {
+    if( !loc ) {
         add_msg( m_info, _( "Never mind." ) );
         return false;
     }
+    item &it = *loc;
     if( !has_identified( it.typeId() ) ) {
         // We insta-identify the book, then try to read it
         items_identified.insert( it.typeId() );
@@ -408,7 +409,7 @@ bool avatar::read( item &it, const bool continuous )
     add_msg( m_debug, "avatar::read: time_taken = %d", time_taken );
     player_activity act( ACT_READ, time_taken, continuous ? activity.index : 0,
                          reader->getID().get_value() );
-    act.targets.emplace_back( item_location( *this, &it ) );
+    act.targets.emplace_back( loc );
 
     if( it.typeId() == "guidebook" ) {
         // special guidebook effect: print a misc. hint when read
@@ -728,8 +729,14 @@ static void skim_book_msg( const item &book, avatar &u )
     add_msg( _( "You note that you have a copy of %s in your possession." ), book.type_name() );
 }
 
-void avatar::do_read( item &book )
+void avatar::do_read( item_location loc )
 {
+    if( !loc ) {
+        activity.set_to_null();
+        return;
+    }
+
+    item &book = *loc;
     const auto &reading = book.type->book;
     if( !reading ) {
         activity.set_to_null();
@@ -925,7 +932,7 @@ void avatar::do_read( item &book )
 
     if( continuous ) {
         activity.set_to_null();
-        read( book, true );
+        read( loc, true );
         if( activity ) {
             return;
         }
@@ -1023,9 +1030,9 @@ int avatar::calc_focus_equilibrium( bool ignore_pain ) const
     int focus_equilibrium = 100;
 
     if( activity.id() == ACT_READ ) {
-        const item &book = *activity.targets[0].get_item();
-        if( book.is_book() && get_item_position( &book ) != INT_MIN ) {
-            auto &bt = *book.type->book;
+        item_location loc = activity.targets[0];
+        if( loc && loc->is_book() ) {
+            auto &bt = *loc->type->book;
             // apply a penalty when we're actually learning something
             const SkillLevel &skill_level = get_skill_level_object( bt.skill );
             if( skill_level.can_train() && skill_level < bt.level ) {
@@ -1119,17 +1126,7 @@ int avatar::calc_focus_change() const
 
 void avatar::update_mental_focus()
 {
-
     focus_pool += calc_focus_change();
-
-    // Moved from calc_focus_equilibrium, because it is now const
-    if( activity.id() == ACT_READ ) {
-        const item *book = activity.targets[0].get_item();
-        if( get_item_position( book ) == INT_MIN || !book->is_book() ) {
-            add_msg_if_player( m_bad, _( "You lost your book!  You stop reading." ) );
-            activity.set_to_null();
-        }
-    }
 }
 
 void avatar::reset_stats()

src/avatar.h

@@ -151,9 +151,9 @@ class avatar : public player
          */
         int time_to_read( const item &book, const player &reader, const player *learner = nullptr ) const;
         /** Handles reading effects and returns true if activity started */
-        bool read( item &it, bool continuous = false );
+        bool read( item_location loc, bool continuous = false );
         /** Completes book reading action. **/
-        void do_read( item &book );
+        void do_read( item_location loc );
         /** Note that we've read a book at least once. **/
         bool has_identified( const std::string &item_id ) const override;
 

src/game.cpp

@@ -2205,7 +2205,7 @@ int game::inventory_item_menu( item_location locThisItem, int iStartX, int iWidt
                     avatar_action::mend( u, locThisItem );
                     break;
                 case 'R':
-                    u.read( oThisItem );
+                    u.read( locThisItem );
                     break;
                 case 'D':
                     u.disassemble( locThisItem, false );

src/game_inventory.cpp

@@ -942,10 +942,10 @@ item_location game_menus::inv::gun_to_modify( player &p, const item &gunmod )
                          _( "You don't have any guns to modify." ) );
 }
 
-class read_inventory_preset: public pickup_inventory_preset
+class read_inventory_preset: public inventory_selector_preset
 {
     public:
-        read_inventory_preset( const player &p ) : pickup_inventory_preset( p ), p( p ) {
+        read_inventory_preset( const player &p ) : p( p ) {
             const std::string unknown = _( "<color_dark_gray>?</color>" );
 
             append_cell( [ this, &p ]( const item_location & loc ) -> std::string {
@@ -1028,18 +1028,12 @@ class read_inventory_preset: public pickup_inventory_preset
                 !loc->type->can_use( "learn_spell" ) && u->has_identified( loc->typeId() ) ) {
                 return denials.front();
             }
-            return pickup_inventory_preset::get_denial( loc );
+            return std::string();
         }
 
         std::function<bool( const inventory_entry & )> get_filter( const std::string &filter ) const
         override {
-            auto base_filter = pickup_inventory_preset::get_filter( filter );
-
-            return [this, base_filter, filter]( const inventory_entry & e ) {
-                if( base_filter( e ) ) {
-                    return true;
-                }
-
+            return [this, filter]( const inventory_entry & e ) {
                 if( !is_known( e.any_item() ) ) {
                     return false;
                 }

src/handle_action.cpp

@@ -1270,7 +1270,7 @@ static void read()
             item spell_book = *loc.get_item();
             spell_book.get_use( "learn_spell" )->call( u, spell_book, spell_book.active, u.pos() );
         } else {
-            u.read( *loc.obtain( u ) );
+            u.read( loc );
         }
     } else {
         add_msg( _( "Never mind." ) );

src/npc.cpp

@@ -887,8 +887,14 @@ int npc::time_to_read( const item &book, const player &reader ) const
     return retval;
 }
 
-void npc::finish_read( item &book )
+void npc::finish_read( item_location loc )
 {
+    if( !loc ) {
+        revert_after_activity();
+        return;
+    }
+
+    item &book = *loc;
     const auto &reading = book.type->book;
     if( !reading ) {
         revert_after_activity();
@@ -976,7 +982,7 @@ void npc::finish_read( item &book )
         activity.set_to_null();
         player *pl = dynamic_cast<player *>( this );
         if( pl ) {
-            start_read( book, pl );
+            start_read( loc, pl );
         }
         if( activity ) {
             return;
@@ -986,12 +992,13 @@ void npc::finish_read( item &book )
     revert_after_activity();
 }
 
-void npc::start_read( item &chosen, player *pl )
+void npc::start_read( item_location loc, player *pl )
 {
+    item &chosen = *loc;
     const int time_taken = time_to_read( chosen, *pl );
     const double penalty = static_cast<double>( time_taken ) / time_to_read( chosen, *pl );
     player_activity act( ACT_READ, time_taken, 0, pl->getID().get_value() );
-    act.targets.emplace_back( item_location( *this, &chosen ) );
+    act.targets.emplace_back( loc );
     act.str_values.push_back( to_string( penalty ) );
     // push an identifier of martial art book to the action handling
     if( chosen.type->use_methods.count( "MA_MANUAL" ) ) {
@@ -1016,12 +1023,11 @@ void npc::do_npc_read()
         if( !ch ) {
             return;
         }
-        item &chosen = *loc.obtain( *ch );
-        if( can_read( chosen, fail_reasons ) ) {
+        if( can_read( *loc, fail_reasons ) ) {
             if( g->u.sees( pos() ) ) {
                 add_msg( m_info, _( "%s starts reading." ), disp_name() );
             }
-            start_read( chosen, pl );
+            start_read( loc, pl );
         } else {
             for( const auto &elem : fail_reasons ) {
                 say( elem );

src/npc.h

@@ -937,8 +937,8 @@ class npc : public player
         int value( const item &it ) const;
         int value( const item &it, int market_price ) const;
         bool wear_if_wanted( const item &it, std::string &reason );
-        void start_read( item &chosen, player *pl );
-        void finish_read( item &book );
+        void start_read( item_location loc, player *pl );
+        void finish_read( item_location loc );
         bool can_read( const item &book, std::vector<std::string> &fail_reasons );
         int time_to_read( const item &book, const player &reader ) const;
         void do_npc_read();

src/player.cpp

@@ -135,7 +135,6 @@ static const trait_id trait_FASTLEARNER( "FASTLEARNER" );
 static const trait_id trait_FAT( "FAT" );
 static const trait_id trait_FELINE_FUR( "FELINE_FUR" );
 static const trait_id trait_FUR( "FUR" );
-static const trait_id trait_HATES_BOOKS( "HATES_BOOKS" );
 static const trait_id trait_HOOVES( "HOOVES" );
 static const trait_id trait_HUGE( "HUGE" );
 static const trait_id trait_HUGE_OK( "HUGE_OK" );
@@ -3426,7 +3425,7 @@ void player::use( item_location loc )
     } else if( used.is_book() ) {
         // TODO: Handle this with dynamic dispatch.
         if( avatar *u = as_avatar() ) {
-            u->read( used );
+            u->read( loc );
         }
     } else if( used.type->has_use() ) {
         invoke_item( &used, loc.position() );
@@ -3701,12 +3700,6 @@ int player::book_fun_for( const item &book, const player &p ) const
 
     if( has_trait( trait_LOVES_BOOKS ) ) {
         fun_bonus++;
-    } else if( has_trait( trait_HATES_BOOKS ) ) {
-        if( book.type->book->fun > 0 ) {
-            fun_bonus = 0;
-        } else {
-            fun_bonus--;
-        }
     }
 
     if( fun_bonus > 1 && book.get_chapters() > 0 && book.get_remaining_chapters( p ) == 0 ) {

tests/crafting_test.cpp

@@ -165,7 +165,7 @@ TEST_CASE( "available_recipes", "[recipes]" )
         REQUIRE_FALSE( dummy.knows_recipe( r ) );
 
         WHEN( "the player read it and has an appropriate skill" ) {
-            dummy.do_read( craftbook );
+            dummy.do_read( item_location( dummy, &craftbook ) );
             dummy.set_skill_level( r->skill_used, 2 );
             // Secondary skills are just set to be what the autolearn requires
             // but the primary is not
@@ -305,16 +305,6 @@ static void prep_craft( const recipe_id &rid, const std::vector<item> &tools,
 static time_point midnight = calendar::turn_zero + 0_hours;
 static time_point midday = calendar::turn_zero + 12_hours;
 
-static void set_time( const time_point &time )
-{
-    calendar::turn = time;
-    g->reset_light_level();
-    int z = g->u.posz();
-    g->m.update_visibility_cache( z );
-    g->m.invalidate_map_cache( z );
-    g->m.build_map_cache( z );
-}
-
 // This tries to actually run the whole craft activity, which is more thorough,
 // but slow
 static int actually_test_craft( const recipe_id &rid, const std::vector<item> &tools,

tests/map_helpers.cpp

@@ -7,6 +7,7 @@
 #include <vector>
 
 #include "avatar.h"
+#include "calendar.h"
 #include "field.h"
 #include "game.h"
 #include "game_constants.h"
@@ -129,3 +130,13 @@ void build_test_map( const ter_id &terrain )
     g->m.invalidate_map_cache( 0 );
     g->m.build_map_cache( 0, true );
 }
+
+void set_time( const time_point &time )
+{
+    calendar::turn = time;
+    g->reset_light_level();
+    int z = g->u.posz();
+    g->m.update_visibility_cache( z );
+    g->m.invalidate_map_cache( z );
+    g->m.build_map_cache( z );
+}

tests/map_helpers.h

@@ -8,6 +8,7 @@
 
 class monster;
 struct tripoint;
+class time_point;
 
 void wipe_map_terrain();
 void clear_creatures();
@@ -19,5 +20,6 @@ void clear_map_and_put_player_underground();
 monster &spawn_test_monster( const std::string &monster_type, const tripoint &start );
 void clear_vehicles();
 void build_test_map( const ter_id &terrain );
+void set_time( const time_point &time );
 
 #endif // CATA_TESTS_MAP_HELPERS_H