feat(port): Add book binder with many QoL #2170

leoCottret · 2022-11-06T16:02:45Z

Summary

SUMMARY: Features "Add book binder with many QoL"

Purpose of change

Being able to copy and keep the most interesting recipes on you, even if they were above your level when you copied them.
Also, it was one of the (many) PR to port before
CleverRaven/Cataclysm-DDA#46304
and
CleverRaven/Cataclysm-DDA#49408 (the final boss)

Describe the solution

Port CleverRaven/Cataclysm-DDA#44399
and CleverRaven/Cataclysm-DDA#50367

Changes from the original PR:

when you copy a new recipe, the old recipes that you already know are removed with a message. This means you'll be able to keep the same book binder for the entire game.
the PR added some randomness to the spawn of paper, I didn't port it
you could only copy recipes that match your skill level, I removed the requirement
- but instead, I added a check on the skill level when you open your craft menu
- which means, you can copy any recipe from any book, but you will only be able to craft it when you met the requirement
- I also removed the learnt recipes, and the recipes already in the book binder from the selection
- hopefully, those changes will make it more user friendly
the pen can be used to write too (it has some charges now)
the copying time has been greatly reduced (it was quite long before, probably aligned with DDA pace)
lots of change to the book binder item
- it's not a GENERIC item, but a TOOL
- it uses paper as charges
- by default (when you craft it, and when it spawns), it has respectively 0 or 1 charges (the first page is used)
- then, copying a recipe increases the amount of charges in it
- when it's full of charges, you can't add new recipes in this one
- I could make it infinite, but since it's a tool and not a GENERIC item that contains paper, only the weight increases, not the volume. So I set the maximum at 500 pages (roughly 100 recipes). Although with the first point of this section, it doesn't matter much now.
- lots of extra check have been added, or moved before the recipe to copy selection, to warn the player earlier that the action can't be done (cf tests)

Describe alternatives you've considered

Not porting it, and port the e-book one directly (but way too many changes)
Porting the e-book one in this PR too (too big)

Testing

remove the flags UNLOAD, and RELOAD, spawn new character with 0 skills
test the following conditions: book is full, no paper on player, no writing tool, in the dark, you get special messages before selecting a recipe to copy
test the following conditions: 2 papers on the player, or "1 paper left" in the book: only the recipes with a number of page below or equal to those requirements can be selected (wasn't the case)
spawn the "A History of Firefighting" book
copy the fire axe and the crash axe recipe
the book has now 5 + 4 pages in it
open the copy recipe menu, they are not here anymore
drop the "A History of Firefighting" book, and go away from it
select view recipe, they are here
increase your fabrication skill to 5, try to craft them, they don't appear
increase your fabrication skill to 6, the crash axe appears
increase your fabrication skill to 9, the fire axe appears
drop the book binder, go away from it
they don't appear in the craft menu (not memorised yet)
read the "A History of Firefighting" book once
try to copy a recipe, then view the recipes. They disappeared because you learnt them by reading the book

…ok binder from selection

leoCottret · 2022-11-07T01:21:44Z

If it passes clang-tidy checks, this is ready for review

scarf005 · 2022-11-07T03:25:18Z

that's great. one more idea: maybe it could also extract recipies from a book without requiring pen and paper, at the cost of destroying the book

leoCottret · 2022-11-07T11:20:46Z

that's great. one more idea: maybe it could also extract recipies from a book without requiring pen and paper, at the cost of destroying the book

Would it be worth it though? You would lose the ability to potentially read the book, for you and your followers.
I asked a bit on discord what other people think, because I don't mind to add it if needed.

src/item.h

Coolthulhu · 2022-11-18T20:39:20Z

/home/runner/work/Cataclysm-BN/Cataclysm-BN/src/iuse.cpp:9867:10: error: the 'empty' method should be used to check for emptiness instead of 'size' [readability-container-size-empty,-warnings-as-errors]
    if( !not_learnt_recipes.size() ) {
        ~^~~~~~~~~~~~~~~~~~~~~~~~~
        not_learnt_recipes.empty()

Coolthulhu · 2022-11-21T11:45:12Z

src/item.h

+         *
+         * @return true if the recipe was added, false if it is a duplicate
+         */
+        bool eipc_recipe_add( const recipe_id &recipe_id );


This should be changed just like eipc_recipe_remove.

This doesn't work, eipc_recipe_add is used in iuse.cpp and activity_actor.cpp

They both could be extracted into item_functions.h (declarations) + item_functions.cpp (definitions), that's a file specifically for new functions that use public members of item class.

olanti-p

The binder is working in-game, so functional part is good, this is more code style nitpicks and commentary.

olanti-p · 2022-12-08T23:40:11Z

data/json/items/comestibles/other.json

+    "id": "napkin",
+    "name": { "str": "napkin" },
    "category": "spare_parts",
    "weight": "3 g",
    "color": "white",
    "comestible_type": "FOOD",
    "symbol": "`",
-    "description": "A piece of paper.  Can be used for fires.",
+    "description": "A napkin.  Can be used for fires.",


Is adding this new item intentional? It looks like a merge conflict.
There are no references to napkin anywhere, it won't spawn in game world.

olanti-p · 2022-12-09T00:06:04Z

src/activity_actor.cpp

+
+void bookbinder_copy_activity_actor::do_turn( player_activity &, Character &p )
+{
+    if( character_funcs::fine_detail_vision_mod( p ) > 4.0f ) {


A function has been added for this, no need for magic numbers anymore

Suggested change

if( character_funcs::fine_detail_vision_mod( p ) > 4.0f ) {

if( !character_funcs::can_see_fine_details( p ) ) {

olanti-p · 2022-12-09T00:07:17Z

src/activity_actor.cpp

+
+        player->consume_tools( writing_tools, pages );
+    } else {
+        debugmsg( "Recipe book already has '%s' recipe when it should not.", rec_id.str() );


The cast here is done automatically

Suggested change

debugmsg( "Recipe book already has '%s' recipe when it should not.", rec_id.str() );

debugmsg( "Recipe book already has '%s' recipe when it should not.", rec_id );

olanti-p · 2022-12-09T00:17:52Z

src/iuse.cpp

+        return 0;
+    }
+
+    if( character_funcs::fine_detail_vision_mod( *p ) > 4.0f ) {


Suggested change

if( character_funcs::fine_detail_vision_mod( *p ) > 4.0f ) {

if( !character_funcs::can_see_fine_details( *p ) ) {

olanti-p · 2022-12-09T00:22:02Z

src/iuse.cpp

+    std::string current_recipes = it.get_var( "EIPC_RECIPES" );
+    if( current_recipes.empty() ) {
+        return false;
+    } else if( current_recipes.find( "," + recipe_id.str() + "," ) != std::string::npos ) {


This pattern

std::string current_recipes = it.get_var( "EIPC_RECIPES" ); . . . check if current_recipes.find( "," + recipe_id.str() + "," ) != std::string::npos is true

repeats multiple times thorough the PR and should ideally be extracted into a dedicated function (say, bool eipc_recipe_check( const item &it, const recipe_id &recipe_id ), but that's more of a nit towards original implementation.

olanti-p · 2022-12-09T00:33:47Z

src/iuse.cpp

+        p->add_msg_if_player( m_info, _
+                              ( string_format( "You already know some recipes.  You remove %d pages from the book binder.",
+                                               total_pages_removed ) ) );


_() must receive either a string literal or a string specifically marked for translation via translate_marker(). Passing it a dynamically generated string won't work.

Suggested change

p->add_msg_if_player( m_info, _

( string_format( "You already know some recipes. You remove %d pages from the book binder.",

total_pages_removed ) ) );

p->add_msg_if_player( m_info, string_format( _( "You already know some recipes. You remove %d pages from the book binder." ),

total_pages_removed ) );

olanti-p · 2022-12-09T00:37:09Z

src/iuse.cpp

+
+
+    std::vector<const recipe *> not_learnt_recipes;
+    std::string old_recipes = binder->get_var( "EIPC_RECIPES" );


Unused store (the variable gets assigned here, but nothing uses the value, and a few lines down it get re-assigned again with new value).

Suggested change

std::string old_recipes = binder->get_var( "EIPC_RECIPES" );

olanti-p · 2022-12-09T00:37:28Z

src/iuse.cpp

+    }
+
+    // only keep not learnt recipes and those that are not already in the book
+    old_recipes = binder->get_var( "EIPC_RECIPES" );


(unused store part 2)

Suggested change

old_recipes = binder->get_var( "EIPC_RECIPES" );

std::string old_recipes = binder->get_var( "EIPC_RECIPES" );

olanti-p · 2022-12-09T00:40:17Z

src/iuse.cpp

+
+    for( const recipe *rec : not_learnt_recipes ) {
+
+        const int pages = 1 + rec->difficulty / 2;


This pattern should also have been extracted into a function, say

int calc_recipe_binder_usage( const recipe_id& r ) { return 1 + r->difficulty / 2; }

olanti-p · 2022-12-09T00:45:18Z

src/recipe_dictionary.cpp

+std::vector<const recipe *> recipe_subset::get_recipes()
+{
+    std::vector<const recipe *> ret( recipes.begin(), recipes.end() );
+    return ret;
+}
+


I really hate how all functions here straight up allocate vectors on each call.

Lamandus · 2024-03-26T23:13:51Z

is this PR still worked on?

Lamandus · 2024-04-02T15:15:03Z

Same here, open it up again, if you are willing to work on it.

leoCottret added 6 commits November 6, 2022 03:45

add ultra WIP version without paper usage, and spawn modif

765dc6f

add paper usage, and change recipes shown logic

d447c3d

revert ammo remaining changes

14c2fe6

add misc changes and update doc

5e3263d

increase book binder max charges to non debug amount

5de617a

add usable pen (for the book binder)

4005677

github-actions bot added JSON related to game datas in JSON format. src changes related to source code. labels Nov 6, 2022

leoCottret marked this pull request as draft November 6, 2022 16:03

leoCottret added 8 commits November 6, 2022 17:36

change pen to tool

2540320

improve requirement checks, add some(?), remove recipes already in bo…

7d27343

…ok binder from selection

remove tmp file

1475cea

add automatic pages removal after learning a recipe + misc things

6f92979

astyle (here we go)

5e34eb5

astyle

65a75c8

fix sneaky unrelated change

9300817

fix clang tidy with new property

2ca2ac2

leoCottret changed the title ~~[Port] Add book binder~~ [Port] Add book binder with many QoL Nov 6, 2022

astyle, fix new change on recipe comparison

01ac7a7

leoCottret marked this pull request as ready for review November 7, 2022 01:21

fix duplicated word

c9ce4a6

leoCottret mentioned this pull request Nov 9, 2022

feat(port): Add eBook reader to Store / Read books #2178

Closed

Coolthulhu self-assigned this Nov 15, 2022

Coolthulhu reviewed Nov 18, 2022

View reviewed changes

src/item.h Outdated Show resolved Hide resolved

refactoring, clang tidy

42d8093

Coolthulhu reviewed Nov 21, 2022

View reviewed changes

leoCottret marked this pull request as draft November 21, 2022 21:20

leoCottret marked this pull request as ready for review November 22, 2022 17:35

olanti-p reviewed Dec 9, 2022

View reviewed changes

Coolthulhu removed their assignment Dec 11, 2022

scarf005 changed the title ~~[Port] Add book binder with many QoL~~ feat(port): Add book binder with many QoL Nov 13, 2023

Lamandus closed this Apr 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(port): Add book binder with many QoL #2170

feat(port): Add book binder with many QoL #2170

leoCottret commented Nov 6, 2022 •

edited

Loading

leoCottret commented Nov 7, 2022

scarf005 commented Nov 7, 2022

leoCottret commented Nov 7, 2022

Coolthulhu commented Nov 18, 2022

Coolthulhu Nov 21, 2022

leoCottret Nov 22, 2022

olanti-p Dec 9, 2022

olanti-p left a comment

olanti-p Dec 8, 2022

olanti-p Dec 9, 2022

olanti-p Dec 9, 2022

olanti-p Dec 9, 2022

olanti-p Dec 9, 2022

olanti-p Dec 9, 2022

olanti-p Dec 9, 2022

olanti-p Dec 9, 2022

olanti-p Dec 9, 2022

olanti-p Dec 9, 2022

Lamandus commented Mar 26, 2024

Lamandus commented Apr 2, 2024

	if( character_funcs::fine_detail_vision_mod( p ) > 4.0f ) {
	if( !character_funcs::can_see_fine_details( p ) ) {

	debugmsg( "Recipe book already has '%s' recipe when it should not.", rec_id.str() );
	debugmsg( "Recipe book already has '%s' recipe when it should not.", rec_id );



		std::vector<const recipe *> not_learnt_recipes;
		std::string old_recipes = binder->get_var( "EIPC_RECIPES" );

	old_recipes = binder->get_var( "EIPC_RECIPES" );
	std::string old_recipes = binder->get_var( "EIPC_RECIPES" );


		for( const recipe *rec : not_learnt_recipes ) {

		const int pages = 1 + rec->difficulty / 2;

feat(port): Add book binder with many QoL #2170

feat(port): Add book binder with many QoL #2170

Conversation

leoCottret commented Nov 6, 2022 • edited Loading

Summary

Purpose of change

Describe the solution

Describe alternatives you've considered

Testing

leoCottret commented Nov 7, 2022

scarf005 commented Nov 7, 2022

leoCottret commented Nov 7, 2022

Coolthulhu commented Nov 18, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olanti-p left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lamandus commented Mar 26, 2024

Lamandus commented Apr 2, 2024

leoCottret commented Nov 6, 2022 •

edited

Loading