fix milling products

[PatrikLundell]

Summary

None

Purpose of change

Fix #71652, i.e. milling yields the wrong nutrients.

Describe the solution

Note: Changed implementation yet again:

• Change the JSON milling info to contain the product (as before) and a recipe id (instead of a conversion factor).

• Changed the JSON item definitions with milling entries to reference recipes with the appropriate minimum source / product ratios.

• Added milling info entries for copy-from items where the source item had milling info. The overriding milling info typically is for the "null" object, and no recipe is provided.

• Find how many of each millable input we have in the mill.

• Iterate for each type of source input we have:

• Retrieve the corresponding recipe.

• Iterate over all input alternatives to find the one we have in the mill.

• Calculate how many batches we can make out of that input, produce the corresponding batch results, and remove the number of input items consumed.

• Check that there is a recipe that takes the input to produce the milling result and don't accept input if none exists.

• Check mill content at mill start and remove any content that won't result in full batches.

• Note that the logic is duplicated for mill start and mill finalization, because it's possible to change recipes in between these points.

Describe alternatives you've considered

The first attempt was a 4 line quick fix...

Testing

Played around with cooked acorns and buckwheat.

• Buckwheat is processed in batches of 1, so there is never any left overs (and the calories remain the same regardless of the number of items processed).
• Did the corresponding thing with cooked acorns (batch 10) and the expected number of unprocessed acorns remained afterward (with the expected messages), and the number of calories remained steady and approximately 2/3 of the number of calories displayed for a single batch of acorns.
• Mixed buckwheat and acorns, and recovered two types of wheat free flour (with the same calorie count as above) and left over acorns.
• After the addition of tests on input and mill start:
• Removed buckwheat from all 3 milling recipes (should be sufficient to do that for the base one) and verified buckwheat was no longer accepted as input and that a debug message was generated. Restored the recipes afterwards.
• Input of 11 buckwheat and 11 cooked acorns.
• Started the mill and verified one acorn was removed.
• Verified 165 + 150 wheat free flour units were produced after debug waiting and removal of the products. Verified the calorie content of the two lots were as expected.
• Plus other variations of the tests above throughout the course of the development.
• Bring up the debug spawn menu and iterate over every object. This results in debug messages for the ones that don't reference appropriate recipes (caught two such cases during the testing).
• Tossed every primary (i.e. non copy-from) item into the mill and verified they milled and produced the correct number of output as per the recipes. This does not verify that the recipes were translated or set up correctly, however. That's a task for the Mk I Eyeball.

Additional context

The mill has the luxury of using as many recipes as is needed to handle all the different source / product ratios. The player facing recipes, however, are left as is (with some minor corrections). This PR does not attempt to handle the large number of inconsistencies, omissions, and other oddities in existing recipes beyond that. A number of comments about issues have been left in comments below.

Note that this implementation doesn't actually use the source item's milling conversion factor, but rather that of the (first) corresponding recipe. It also relies on that recipe actually listing all the inputs that produces that particular product.

[mqrause]

From what I can see your solution is trying to fix the wrong end of the issue. The recipe_charges are actually correct but the components assign only a single item instead of the whole stack of items that get milled.

For example if you're milling 50x item A with a conversion rate of 0.5, they will turn into 25x item B. Those item B should then all have 50x A as components and recipe_charges should be 25 so nutrients can be calculated correctly.

[PatrikLundell]

Yes, the total number of charges is items * conversion rate, but, as you said, the item is a single item. As far as I could see, with my compiler deciding not to help, there is no way to set a count on the item unless it's one with charges, and those are to be removed.

If I remember correctly, there is something used by recipes for their components that deals with counts, and it might be possible to go via that rather than manipulation of an item.

Edit: The item_components class seems to be useful only if you have a recipe, so it seems to me that you'd have to make a recipe either on the fly or as a JSON resource, or you'd have to hack the item used to multiply all of its relevant properties (calories and "vitamins" as a minimum, but possibly weight/volume and more) by the number of items actually used and then split those resources over the resultant total number of product items.

[mqrause]

Recipes have two advantages for the purposes of this. The first is that they essentially define the "conversion ratio" as a proper fraction with created items as the numerator and required items as the denominator. And the second is that they also have an explicit batch size and not just convert arbitrary numbers of items.

What you could do here as a probably not too bad solution is about this:

1. Construct and item_components object with all the input items instead of std::map<itype_id, item> millable_items;
2. Get the greatest common denominator of input amount and output amount.
3. Split the components by gcd and set recipe_charges to output amount / gcd.

2 and 3 are mostly to avoid excessive duplication of components. E.g. if you were to mill 100 wheat into 1500 flour without those steps, you'd get 1500 flour items with 100 wheat as components each, meaning you'd have 150000 wheat in memory and saves for kinda no reason.

It's still not perfect for non-integer conversion rates because you could have extra input items that result in the gcd becoming 1, but what you currently have results in int(conversion_rate) / conversion_rate times the expected nutrients. Which is 0 for conversion rates < 1 and e.g. 0.67 when milling cooked acorns into wheat-free flour.

[PatrikLundell]

Clang-tidy 16 / build (src) (pull_request) seems to be screwed up:

Error: /home/runner/work/Cataclysm-DDA/Cataclysm-DDA/src/overmap_ui.cpp:498:67: error: insufficient spaces at this location.  2 required, but only 1 found. [cata-text-style,-warnings-as-errors]
                                               _( "DANGEROUS AREA! (R=%d)" ), note_danger_radius ) : is_dangerous ? _( "IN DANGEROUS AREA!" ) : "";

Untouched file, and that's not what's on that line, it's distance_player, is_dangerous ? "DANGEROUS AREA!" : "" ) );

Error: /home/runner/work/Cataclysm-DDA/Cataclysm-DDA/src/iexamine.cpp:5964:22: error: static member accessed through instance [readability-static-accessed-through-instance,-warnings-as-errors] recipe rec = recipe_dict.get_craft( product.typeId() ); ^~~~~~~~~~~~ recipe_dictionary::
My compiler disagrees with the addition of a context of the class defined prior to the object and refuses to compile if it's added.

Error: /home/runner/work/Cataclysm-DDA/Cataclysm-DDA/src/iexamine.cpp:6011:17: error: use range-based for loop instead [modernize-loop-convert,-warnings-as-errors] for( map_stack::iterator iter = items.begin(); iter != items.end(); ++iter ) { ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ (auto & it : items)
It demands the change of the iterator to the new style, but ignores that iteration has to deal with the removal of an element in the map it iterates over. That may or may not be possible to deal with, but I can't get a working syntax.

[mqrause]

I didn't give it a real review, yet, just addressing the clang-tidy issues.

Untouched file, and that's not what's on that line, it's distance_player, is_dangerous ? "DANGEROUS AREA!" : "" ) );

Qrox already has a PR up to fix that, so you can ignore it.

Error: /home/runner/work/Cataclysm-DDA/Cataclysm-DDA/src/iexamine.cpp:5964:22: error: static member accessed through instance [readability-static-accessed-through-instance,-warnings-as-errors] recipe rec = recipe_dict.get_craft( product.typeId() ); ^~~~~~~~~~~~ recipe_dictionary:: My compiler disagrees with the addition of a context of the class defined prior to the object and refuses to compile if it's added.

You're supposed to use recipe_dictionary::get_craft, because the get_craft method is static.

Error: /home/runner/work/Cataclysm-DDA/Cataclysm-DDA/src/iexamine.cpp:6011:17: error: use range-based for loop instead [modernize-loop-convert,-warnings-as-errors] for( map_stack::iterator iter = items.begin(); iter != items.end(); ++iter ) { ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ (auto & it : items) It demands the change of the iterator to the new style, but ignores that iteration has to deal with the removal of an element in the map it iterates over. That may or may not be possible to deal with, but I can't get a working syntax.

You have two loops like that, the first one is the offender that doesn't remove elements and could be changed, the second one is fine because as you said it removes elements.

[github-actions]

Spell checker encountered unrecognized words in the in-game text added in this pull request. See below for details.

Click to expand

• Failed to find milling recipe for %s. It wasn't milled.
• This mill doesn't contain a full last batch of %s, which requires a batch size of %d.

This alert is automatically generated. You can simply disregard if this is inaccurate, or (optionally) you can also add the new words to tools/spell_checker/dictionary.txt so they will not trigger an alert next time.

[mqrause]

I like the recipe approach, I think that might be a potential first step in genericizing things like milling, smoking and other automated processing.

Currently this causes new issues, though. For one, not all items with milling data actually have recipes. I didn't check all, but found dry wild rice at least. That's only obtainable by milling wild rice if I'm not mistaken. And another is that the code can't find any recipes with suffixes. So if you had two alternative recipes that use different components to get the same item, you could only mill the items from the recipe without a suffix.

Another small thing is that with your changes, the conversion rate of the milling data is obsolete (but is still used for item info in a possibly inaccurate way). This together with the above means the islot_milling needs to be adjusted. Essentially instead of what it currently has it simply needs to be pointed to a recipe_id it should use. That should also simplify the code a bit since you don't have to look for a recipe on the fly. But it does mean going through all the milling data in the json.

[PatrikLundell]

Good thoughts. It's a fair bit of JSON work, so I'll make a comment to indicate what I'm trying to do (essentially what you suggest):

• Define one inaccessible (for milling only) recipe for each ratio and product based on the conversion_factor. There are several cases where the existing recipes use different values, partially because they won't fit into the 15 result form factor used, and partially for unknown reasons (e.g. wild rice), where it isn't even the closest fit.
• Too annoying with errors in existing recipes, so I'm going to try to fix them to a "best fit".
• Move all milling recipe into a new milling.json file.
• Replace the obsolete conversion_rate with the name of the corresponding milling recipe.
• Make the code use the referenced recipe.

Apart from the above, I also found the large_bone had a mill entry, but it won't fit into a mill, and it had a conversion factor of 4 while the calcium was 144 times higher than the regular bone (which also had a factor of 4). I just struck the milling entry from the large bone (with a "comment").

Edit:
Wild rice is weird:
mill into dry_wild_rice and then
mill into flour_wheat_free...
Also, the first step can only be done via the mill.

paste_nut and paste_seed only has a quern recipe, missing the mortar and food processor ones.

paste_nut are suspect, as it have very low conversion rates, which leads to a suspicion it's meant for individual seeds rather than standard units (it also overrides nutrients).

The paste_seed recipe has a much better conversion rate than the milling info does. I went with the recipe.

chili-p is inconsistent. Conversion factor 20, but 2 recipes yield 50 and the last 1. I went with 50 for all of them. Also note that there are 2 recipes for chili_pepper and one for dry_chili, but only dry_chili has a milling entry. Finally, dry_chili has 1 recipe, chili_pepper 2, out of the standard lot of 3 for different tools.

garlic_powder had a conversion_rate of 5 but a recipe rate of 50. I went with the latter. While there are 3 recipes for garlic_powder, only one of them uses dry_garlic, which has the milling entry. The other two use garlic_clove (and one of those uses an additional heat source). Of course, 3 recipes over 2 ingredient variant means not the standard set of 3 sets of tools is provided for either.

Edit 2:
cornmeal has 3 recipes, one for each tool group, which is good, but two of them use an external heat source, and one of those accepts kernels as an alternative component (and kernels doesn't have a milling entry: that's carried by dry_corn).

There's probably a need for an audit of milling tools, as I don't think they're consistent in how the 3 recipes (when present) are implemented. I won't address that side track, though.

skewer_bone gets milled into meal_bone_tainted, for some reason. Doesn't make sense as they're extracted from bones and bone_sturdy, which only contain "normal" bones (although human and demi_human is included).

meal_chitin_piece takes insect_dust as an input in 2 out of 3 recipes. It doesn't have any milling entry, and its description does not seem like something you'd actually be able to use for meal production. I leave that for others to deal with.

Edit 3:
Response to the next entry: I try to keep it down to a reasonable amount of scope creep, which is why I record things I noticed here.
Anyway, I'm about to make a JSON only update in preparation for the code and last JSON work. This is compatible with the code as it stands now.
It moves all the milling recipes into its own file and adds a lot of recipes intended for use by the milling code (i.e. recipes to be referenced by modified milling entries).

The reason there are as many recipes as there are is that I've tried to use the smallest source material lots possible for each material, as opposed to the crafting recipes that tries to cram everything into a standard package. Code doesn't care about UI clutter, though.

[mqrause]

Yeah, a lot of the stuff is inconsistent (or outright bugged, which is why I made #71768 after reviewing this). But beware of scope creep. It's perfectly fine to just fix the milling mechanics in this PR and address inconsistencies in another one.

[PatrikLundell]

I'm stuck with the tests' refusal of accepting flour_wheat_free from rice and wild rice. Apparently factoring out these two from a larger combined recipe runs afoul of the bounds within which the product calories are allowed to lie.
As far as I understand packaging multiple source materials alternatives together masks that some of them were out of bounds.
Thus, I need some competent ruling on how to change rice to both fall inside the bounds and to make real world sense. I can do the former (easy, just increase the conversion_rate. It could also be achieved by overriding the nutrients of the product (done with paste_nut), or reduce the number of calories in rice), but not the latter.

Edit:
I've also run into another problem: item "copy-from" also copies the milling info from the base item, so you could probably mill cooked buckwheat, for instance (found that when trying to spawn buckwheat for milling, and milling info is generated for cooked buckwheat when you pass through it in the list, but with the modified logic it complains about a missing recipe for it).
I need to find a way to either delete the milling entry (which seems to be supported only for flags and similar stuff) or find a way to overwrite it with something that's interpreted as a "don't process" entry. Blank strings doesn't work, for instance.

Edit 2: Looks like "null" for into_ and a check for is_null works.

[PatrikLundell]

Further oddities noted:

• Cannabis, cotton, and grape seeds have no nutrients, but have nutrient_override flags. This causes seed paste made from them to have zero calories.
• Wheat free flour ground from dry_rice is claimed to be made from cooked rice. It's probably caused by dry_rice's cook_like field, which says rice_cooked. It's the same story for wild rice.
• tainted bone meal from skewers have no nutrients.

[Maleclypse]

Current test failure looks like the average just needs to be moved. Ping me if it's more complicated and I'm misunderstanding.

[PatrikLundell]

@Maleclypse: I base this on the test error messages, so it might be wrong. I haven't seen the test code.

As far as I understand the test attempts to ensure crafted items don't stray too far from the definition of the item crafted. In this case the base definition has 40 kcal while both dry_rice and dry_wild_rice has 240 with a factor of 3, resulting in 80 kcal per unit of wheat_free_flour. The test comes up with different numbers for some reason, but in the same ballpark.
Increasing the yield to 4 ought to "solve" the problem by resulting in 60 kcal which should be within the limit (I believe I've seen 76 for something else during my tests). However, the person who came up with the yield factor presumably based it on something relating to real world values.

There's also the issue that the test itself is buggy, since it doesn't complain about the same yield in the manual crafting recipes (5->15), for whatever reason (my guess is that it's caused by a failure to handle the multiple alternatives sensibly, possible throwing them into a blender rather than picking out min and max).
It's also possible to "solve" the problem by reducing the kcal value in dry_rice and dry_wild_rice, and 200 is lower than the 202.0238 value the test pulled out of somewhere. You could probably get away with a value of 210, since the test reduces 240 to 231, and thus ought to reduce 210 to lower than 202.0238. But, again, this is just number fudging to pass the test, without connecting to what the values should be in reality.

Either of those "solutions" would cause the tests to pass, but I can't judge if either would make sense from a real world perspective.

[mqrause]

There's also the issue that the test itself is buggy, since it doesn't complain about the same yield in the manual crafting recipes (5->15), for whatever reason (my guess is that it's caused by a failure to handle the multiple alternatives sensibly, possible throwing them into a blender rather than picking out min and max).

What the test does is check all possible permutations of components for a given recipe and averages that out, then checks if that average falls within default calories +/- two times the standard deviation of the permutations (but at least 25%). So you basically found an outlier hidden within a recipe that won't be accepted as their own separate recipe.

If all the food and recipe stats are correct, that is indeed a bug within the test. But on a fairly superficial glance it just seems like the conversion rate is wrong and rice should produce more flour.

[PatrikLundell]

I searched the net for calories in a cup of wheat flour and rice flour respectively, threw the values into a calculator to determine a ratio, and then multiplied the calories of "flour" by that ratio, resulting in 57.xxx, which is close enough to the 60 you get with a conversion rate of 4 rather than 3.

[PatrikLundell]

The Aftershock mod adjustment of milling was straightforward, just an modification of the entries, as the list used by the milling recipe had already been extended (and it uses the same factor).
The Megafauna entries were simply deleted, because the conversion rate is silly (inherited from the similarly deleted large bone base game entry) with all the nutrients of large items into 4 measly lots of flour, and the items were too large to fit into a mill anyway (which is why the entries are removed rather than corrected). Don't know why the bot adds the Aftershock tag but not a Megafauna one, though.

[mqrause]

Don't know why the bot adds the Aftershock tag but not a Megafauna one, though.

That's simply because there isn't one.

[PatrikLundell]

Should a maintainer be notified?

[Maleclypse]

Should a maintainer be notified?

We should probably add a label for Megafauna in the future.