[WIP] Tailoring without thread charges

[ZhilkinSerg]

Overview of the changes:

• Removed thread charges from all sewing tools;
• Changed thread ammo type and changed all thread ammuntion to generic inherited from generic_filament;
• Changed advanced sewing action to use filament requirement added earlier;
• Updated tailor profession starting items (removed scissors, changed thread quanity);
• Changed repair action to use filament requirement when using tools with sewing quality for repair.

To-do:

• Remove hardcoded qualities and requirements;
• Do not ask for components each repair tick.

[Coolthulhu]

Pretty sure volume is for a full stack.
Volume for 1 thread was 5ml.
Don't specify stack_size and volume like that: stack size is a divisor on volume.

[ZhilkinSerg]

When I set volume to 1000 ml, stackable to true and stack_size to 200 it gives thread enormous volume in-game (like 8 liters for 8 sinews), so volume should be defined per single item.

[Coolthulhu]

Tabs here: you should replace all tabs with 4 spaces each.
If you're adding const everywhere, also add static so that they aren't regenerated every time the function is executed.

[ZhilkinSerg]

Replaced all tabs with 4 spaces and made const arrays also static.

[Coolthulhu]

Nowadays we use @todo for TODOs because the documentation system detects them.

[ZhilkinSerg]

Replaced TODOs with @todo

[Coolthulhu]

Those 3 filaments below differ from each other by name, description, material and color.
You should define a base filament and have all of them copy-from it instead of redefining everything anew. Check out how it's done for car batteries.

[ZhilkinSerg]

Thanks, I've changed that.

[Coolthulhu]

This won't happen in unmodded game and since you're hardcoding the IDs, you don't really care much about anything but hardcoded.

[ZhilkinSerg]

That was actually a debug code block which I wrote first to check available quantities (with

p->add_msg_if_player(m_info, _("DEBUG: Caching filament=%s, quantity=%d, quantity_total=%d"), filament, filament_quantity, filament_have_total);

).

I will remove this block it as it is not really used.

[Coolthulhu]

A better way than hardcode would be to create a requirement for filament and check that. That way it would not be hardcoded and you would not need to manually replicate what crafting process already does.
You access requirement by requirement_id( "id_here" ).obj(), and use it like:

const auto &req = making->requirements();
bool can_craft = req.can_make_with_inventory( crafting_inv, batch_size );

Then consume the resources:

for( const auto &it : req.get_components() ) {
    consume_items( it, batch_size );
}
for( const auto &it : req.get_tools() ) {
    consume_tools( it, batch_size );
}

Doing it this way would automatically ask for resources, so you may skip the second part and just consume manually. Still, the first part is great for asking if resources exist.

[ZhilkinSerg]

I've already added filament requirement earlier when I changed recipes in #20270. I will try to implement its usage in sew_advanced just like you described. Thanks!

[ZhilkinSerg]

I've ran into issue - requirements come from the recipe. We are repairing or modifying item, so there's no recipe for that and I simply cannot call:

const auto &req = making->requirements();

[Coolthulhu]

const auto &req = requirement_id( "id_here" ).obj()
bool can_craft = req.can_make_with_inventory( crafting_inv, batch_size );

[ZhilkinSerg]

Thanks - it seems that works good.

[Coolthulhu]

All this block looks equivalent to filament_used = std::min( filament_have, filament_left ).

[ZhilkinSerg]

Changed that.

[Coolthulhu]

Repeating a whole block like that is not acceptable.
Just do not return at the end of the block and instead have the consumption happen after all the if/else here.

[Coolthulhu]

Also coding style:

if( x ) {
    // Stuff
} else {
}

One newline for if, one newline for closing, no newline for else.
No extra semicolons at the end of blocks.

[ZhilkinSerg]

Coding style updated.

[Coolthulhu]

Looks like you didn't push the commits - I can't see the changes.

[ZhilkinSerg]

I'm still struggling with switching to usage of requirements.

It seems for me that it is impossible to mix different filement types in single crafting: character has 8 sinews and 8 thread, it needs 16 filament to pad polo shirt with leather, but it is not possible until I have exactly 16 of any type. On the other hand, same goes for recipes, so it should okay for now.

[Coolthulhu]

It seems for me that it is impossible to mix different filement types in single crafting

This is not worth working around. All crafting works like this.

[Coolthulhu]

You changed all the TODO lines in iuse.cpp. This is not a huge problem, but whole file search+replace should be avoided unless you're sure you won't cause conflicts.

[Coolthulhu]

You consume the filament twice. You don't need batch_size when using the default value (1).

[ZhilkinSerg]

Hmm... I tested it in-game and it works fine - when I need to consume 16 filament to pad item with leather - 16 threads are consumed.

I will test it again.

[Coolthulhu]

This could mean you aren't actually using the comps here.
Note that you are calling consume_items twice.

[ZhilkinSerg]

I believe consume_items is used twice for various reasons: first for filament from the requirement and second for modding material (leather, kevlar, etc) - isn't it ?

[Coolthulhu]

Oh
Yeah, that makes sense

[Coolthulhu]

Still, it would be cleaner to sum up those requirements. You can add them just like numbers.

[ZhilkinSerg]

It is possible to create and use padding requirements for leather, kevlar, but there are cases when materials aren't spent, but filament is spent and also cases where double amount of filament is used.

You damage your %s trying to modify it and also waste filament (%d)!
You fail to modify the clothing, and you waste filament (%d) and materials.
You modify your %s, but waste a lot of filament (%d).
You modify your %s using a total of %d filament!

[Coolthulhu]

I thought they're already a requirement. Nevermind then, can stay as is.

[Coolthulhu]

You still have multiple copies of the consumption code. Keep it in one place.

[ZhilkinSerg]

Reduced to single copy.

[Coolthulhu]

The word "filament" feels really weird here, even if technically correct.
Any native English speakers to confirm/deny?

[Coolthulhu]

Instead of using batch_size, you can multiply a requirement:

const auto req = requirement_id("filament").obj() * filament_needed;

It would be safer than using batch_size, because someday batch rules may change to allow lower costs or similar effects.

[ZhilkinSerg]

Changed that.

[Coolthulhu]

If the array itself is const, the type in it doesn't need to be, because constness of array will be passed down to its types when its used (unless you have pointers in the array, but you don't have them here). You can also make the arrays static.

static const std::array<std::string 4> array_name_here {

[ZhilkinSerg]

Arrays were updated.

[ZhilkinSerg]

    const int filament_required = mod->volume() / 125_ml + 10;     // We need extra filament to lose it on bad rolls
    const auto req = requirement_id("filament").obj() * filament_required;

    const bool has_enough_filament = req.can_make_with_inventory(crafting_inv);

+10 is not enough for bad rolls (which use x2 materials) in case item volume is bigger than 1250_ml, so player will get debug message Attempted a recipe with no available component!, item will be modified and filament won't be spent in that case.

How should we avoid this? Increasing +10 modifier to some bigger value is not a good solution in my opinion.

[Coolthulhu]

How should we avoid this?

Drop the "you manage to modify but waste extra thread" case and just make it fail in those cases.

[Coolthulhu]

Do not compare bools - just if( consume_mats ) {.

[ZhilkinSerg]

I'm really sorry about that :)

[Coolthulhu]

Is this ever not true?

[ZhilkinSerg]

consume_reqs is set to true only in one case - when you damage item.

    p->add_msg_if_player(m_bad, _("You damage your %s trying to modify it and also waste %d filament charges!"),

[Coolthulhu]

consume_reqs looks true everywhere.
consume_mats has one case where it is false. Still, in all the other cases you overwrite the true with another true, which is 3 extra lines that do nothing.

[ZhilkinSerg]

Fixed this.

[Coolthulhu]

In every section, you recalculate the number just to print what you used. Would be cleaner if you had it once in consumption section. It's fine for it to be split into 2 messages, for example
"You use %d charges"
"You fail to modify %s"

[ZhilkinSerg]

Thanks for suggestion

[ZhilkinSerg]

There is very strange bug now (I guess it is related to damaged clothes and removed sewing tools charges):

Occasionally filament/materials appear in crafting selection menu as they are nearby player, though filament is only in inventory of the player (see GIF below).

It seems nearby materials disappear from crafting menu after I remove all modifications from the item.
When nearby materials are selected, crafting always fails.

[ZhilkinSerg]

I've set breakpoint in the debugger to line 821 in crafting.cpp.

Locals tab in VS shows that both player_has and map_has have 4 members and contents are exactly the same - how does it even possible?

-		map_has	{ size=4 }	std::vector<item_comp,std::allocator<item_comp> >
		[capacity]	4	__int64
+		[allocator]	allocator	std::_Compressed_pair<std::_Wrap_alloc<std::allocator<item_comp> >,std::_Vector_val<std::_Simple_types<item_comp> >,1>
-		[0]	{...}	item_comp
-		component	{type="thread" count=6 available=a_false ...}	component
+		type	"thread"	std::basic_string<char,std::char_traits<char>,std::allocator<char> >
		count	6	int
		available	a_false	available_status
		recoverable	true	bool
		requirement	false	bool
-		[1]	{...}	item_comp
-		component	{type="sinew" count=6 available=a_false ...}	component
+		type	"sinew"	std::basic_string<char,std::char_traits<char>,std::allocator<char> >
		count	6	int
		available	a_false	available_status
		recoverable	true	bool
		requirement	false	bool
-		[2]	{...}	item_comp
-		component	{type="plant_fibre" count=6 available=a_true (1) ...}	component
+		type	"plant_fibre"	std::basic_string<char,std::char_traits<char>,std::allocator<char> >
		count	6	int
		available	a_true (1)	available_status
		recoverable	true	bool
		requirement	false	bool
-		[3]	{...}	item_comp
-		component	{type="yarn" count=6 available=a_false ...}	component
+		type	"yarn"	std::basic_string<char,std::char_traits<char>,std::allocator<char> >
		count	6	int
		available	a_false	available_status
		recoverable	true	bool
		requirement	false	bool

-		player_has	{ size=4 }	std::vector<item_comp,std::allocator<item_comp> >
		[capacity]	4	__int64
+		[allocator]	allocator	std::_Compressed_pair<std::_Wrap_alloc<std::allocator<item_comp> >,std::_Vector_val<std::_Simple_types<item_comp> >,1>
-		[0]	{...}	item_comp
-		component	{type="thread" count=6 available=a_false ...}	component
+		type	"thread"	std::basic_string<char,std::char_traits<char>,std::allocator<char> >
		count	6	int
		available	a_false	available_status
		recoverable	true	bool
		requirement	false	bool
-		[1]	{...}	item_comp
-		component	{type="sinew" count=6 available=a_false ...}	component
+		type	"sinew"	std::basic_string<char,std::char_traits<char>,std::allocator<char> >
		count	6	int
		available	a_false	available_status
		recoverable	true	bool
		requirement	false	bool
-		[2]	{...}	item_comp
-		component	{type="plant_fibre" count=6 available=a_true (1) ...}	component
+		type	"plant_fibre"	std::basic_string<char,std::char_traits<char>,std::allocator<char> >
		count	6	int
		available	a_true (1)	available_status
		recoverable	true	bool
		requirement	false	bool
-		[3]	{...}	item_comp
-		component	{type="yarn" count=6 available=a_false ...}	component
+		type	"yarn"	std::basic_string<char,std::char_traits<char>,std::allocator<char> >
		count	6	int
		available	a_false	available_status
		recoverable	true	bool
		requirement	false	bool

[ZhilkinSerg]

It seems issue was caused by non-integer consumption_modifier which was provided as parameter to consume_items when crafting failed:

double consumption_modifier = 1;
...
consumption_modifier = 1/2;
...
p->consume_items(it, consumption_modifier);

Did I forgot some cast and 1/2 became 0?

[codemime]

Did I forgot some cast and 1/2 became 0?

1/2 is an int implicitly casted to dobule. You can either write 1.0 / 2 or (much better) simply 0.5.

[ZhilkinSerg]

Thanks a lot! I should've learn a bit about types in c++ before typing on my keyboard :)

#include <iostream>
#include <typeinfo>

int main()
{
    double a;

    a = 1;
    std::cout << typeid(a).name() << std::endl;
    std::cout << a << std::endl;
    
    a = 1/2;
    std::cout << typeid(a).name() << std::endl;
    std::cout << a << std::endl;

    a = 0.5;
    std::cout << typeid(a).name() << std::endl;
    std::cout << a << std::endl;

}

d
1
d
0
d
0.5

[Coolthulhu]

Try this with more than one thread type. For example, one thread in inventory and one on the floor and also yarn on the floor.
If this asks for thread type more than once, it is unacceptable.

[ZhilkinSerg]

Game asks for components each repair tick now - I know that's very bad and I will change that.

[DangerNoodle]

Any word on reasons for failed commits? Attempting to view details is returning a 404 error.

[AMurkin]

It's says
20:22:48 Linting data/json/items/tools.json 20:22:49 ERROR: Unmatched contex 'TOOL<TOOL>:use_action<repair_item>:requirements' 20:22:49 Makefile:943: recipe for target 'obj/lint.cache' failed 20:22:49 make: *** [obj/lint.cache] Error 65 20:22:49 Build step 'Conditional step (single)' marked build as failure

[DangerNoodle]

@ZhilkinSerg please make use of the online linter: http://dev.narc.ro/cataclysm/format.html

[ZhilkinSerg]

I've updated TOOL json data structure with requirements node for use_action.

Lint rules should be updated with:

:(use_action(:@)?|countdown_action|drop_action)<repair_item>:requirements=NOWRAP

Lint rules update was not commited yet.

[DangerNoodle]

It was been over a month since last commit, and you were unable or unwilling to make use of the online linter?

I have would attempted a pull request superseding this, had I not had difficulty understanding the code well enough to resolve the utterly unworkable problem of it prompting for thread every repair tick.

[ZhilkinSerg]

Online linter won't help - it simply doesn't contain new rule for the change i did in tools.json (see my previous message). Other than that there are should be no issues in the json.

The issue with prompting for components each tick happened to be not so easy to fix, otherwise I would've been resolved it already. Recently I've found possible way to avoid it. We need to use same approach as done in crafting - components are cached and crafting is done without asking until these cached components are available. See craft_command::execute() method.

[BevapDin]

This copy is pointless, requirement is already an requirement id. Just use it directly or change the loop variable to req_id.

[BevapDin]

Same here. Just use req_id as loop variable and remove this line.

[BevapDin]

Wrong style. { should be on the same line as if.

[BevapDin]

This check is redundant. The following loop will work just fine with an empty array.

[angoddu]

Any progress on this?

[ZhilkinSerg]

Any progress on this?

Sorry, only rebasing occasionally. Have no time to work on that issue with asking of components each repair tick yet. I'm not dropping this change though and will continue working on it.

[Leland]

This will close #20983, I believe. Be sure to add to issue description if so.

[BorkBorkGoesTheCode]

@ZhilkinSerg Are you working on this?