Inconsistency with crafting replacements with multiple inputs

[fluxionary]

Minetest version

5.2.0-dev-e20f2539, 5.1.0, and probably earlier

OS / Hardware

N/A

Summary

There is some inconsistency with crafting w/ the results of crafting recipes w/ replacements, and I don't know what the correct behaviour is. If a recipe lists a replacement, and there are multiple instances of the "source" item in the recipe, you will get a varying number of replacements, depending on various things.

Steps to reproduce

I noticed this behaviour while using unified_inventory (and sometimes unified_inventory_plus), but it works the same w/ the default inventory screen. Because there are no recipes w/ replacements in minetest_game, we'll have to add a mod w/ a few recipes for tests:

-- 1
minetest.register_craft({
    output = "default:stone",
    recipe = {{"default:cactus", "default:cactus", "default:stick"}},
    replacements = {{"default:cactus", "default:cactus"}}
})

-- 2
minetest.register_craft({
    output = "default:stone",
    recipe = {{"default:dirt", "default:dirt", "default:stick"}},
    replacements = {{"default:dirt", "default:dirt 2"}}
})

-- 3
minetest.register_craft({
    output = "default:stone",
    recipe = {{"default:cactus", "default:stick"}},
    replacements = {{"default:cactus", "default:cactus"}}
})

-- 4
minetest.register_craft({
    output = "default:stone",
    recipe = {{"default:dirt", "default:stick"}},
    replacements = {{"default:dirt", "default:dirt 2"}}
})

Note that recipes (1) and (2) demonstrate the issue; recipes (3) and (4) are just provided for reference. Also note that recipes that replace multiple different kinds of items behave like (3) and (4), as long as there is only a single instance of each "source" item in the recipe.

number of replacement items received after	recipe 1	recipe 2	recipe 3	recipe 4
crafting a single item, crafting grid empty afterwards	1	2	1	2
crafting a single item, crafting grid retains more ingredients	2	4	1	2
crafting 10 items, crafting grid empty afterwards	19	38	10	20
crafting 10 items, crafting grid retains more ingredients	20	40	10	20

The real result of this behaviour, is that different mods seem to assume that the replacements in crafting recipes work differently; some have recipes like recipe (1), and some have recipes like (2). With recipes like (1), however, you don't always get the replacements you expected, and with (2), you can easily end up w/ many more replacements than you should get, which is technically an exploit.

My intuition, looking at this data, is that the code that is used to add replacements in the crafting grid after a craft only replaces a single instance of an item in a recipe, whereas the code that adds replacements to the player's inventory replaces all of them, and that the former should be made to behave like the latter. I'd like some input from people more familiar w/ the code and the intentions, though.

[SmallJoker]

So basically the code should spread the items among same ItemStacks. Current behaviour visualized below (recipe 2):

Original MP4: 26 KiB. GitHub-allowed GIF: 147.2 KiB.

[style-nine]

MTG actually does have a replacement recipe in fuel recipe for bucket of lava.

[p-ouellette]

The intended behavior is that each replacement gets applied only once (so recipe 1 should have the same number of replacements as 3, and 2 should have the same numbers as 4) . If you want both cactuses to be replaced in recipe 1, you would use replacements = {{"default:cactus", "default:cactus"}, {"default:cactus", "default:cactus"}}. The bug is that the replacement gets applied to multiple ingredients when the stack has more than 1 item.

Here's the fix:

diff --git a/src/craftdef.cpp b/src/craftdef.cpp
index 0181ceb6..07c3ebc0 100644
--- a/src/craftdef.cpp
+++ b/src/craftdef.cpp
@@ -229,6 +229,7 @@ static void craftDecrementOrReplaceInput(CraftInput &input,
                                rep.deSerialize(j->second, gamedef->idef());
                                item.remove(1);
                                found_replacement = true;
+                               pairs.erase(j);
                                output_replacements.push_back(rep);
                                break;

[fluxionary]

@pauloue Is there a reason you didn't submit that fix as a PR? This seems to still be an issue.

[p-ouellette]

I'll verify that it's correct and submit it when I get a chance, but until then anyone else can feel free to do so.

[appgurueu]

I'll verify that it's correct and submit it when I get a chance, but until then anyone else can feel free to do so.

@pauloue So?

[tenplus1]

Replacements have been an issue for some time, especially with food crafts that need to return buckets for certain items or vessels for others e.g. the following recipe should return dirt for every sand used but it only ever returns one item unless a number is added to the replacements section:

minetest.register_craft( {
	output = "default:sandstone",
	recipe = {
		{"default:sand", "default:sand", "default:sand"},
		{"default:sand", "default:sand", "default:sand"},
		{"default:sand", "default:sand", "default:sand"},
	},
	replacements = {
		{"default:sand", "default:dirt"},
	}
})

[p-ouellette]

@tenplus1, when I read the code I concluded that the intended behavior is that each replacement table specifies the replacement of a single input item, so that recipe should only replace the first sand. To replace all of them you need to use

replacements = {
	{"default:sand", "default:dirt"}, {"default:sand", "default:dirt"}, ... -- repeated 9 times
}

This means it's possible to replace only one sand or replace some input sand with one item and other input sand with another item.
The bug in this issue only shows up when an input stack has more than one item. My 1-line patch above should fix it.

[fluxionary]

it occurred to me recently that the "correct" way, as outlined here, prevents replacing a single item w/ multiple replacements, while the "wrong" way allows for that.

[Wuzzy2]

Thank you. I actually discovered this myself when I started documenting crafting recipes, thanks for confirming.

Bugfix PR: #12819