Fix some issues with minetest.clear_craft (#8712)

* Fix some issues with minetest.clear_craft

- Fix memory leak
- Fix crafts with an output count not being cleared when clearing by
  input.
- Fix recipe list being reversed when clearing by input.

* Add CraftInput::empty()

games/minimal/mods/test/crafting.lua

@@ -0,0 +1,71 @@
+local function test_clear_craft()
+	minetest.log("info", "Testing clear_craft")
+	-- Clearing by output
+	minetest.register_craft({
+		output = "foo",
+		recipe = {{"bar"}}
+	})
+	minetest.register_craft({
+		output = "foo 4",
+		recipe = {{"foo", "bar"}}
+	})
+	assert(#minetest.get_all_craft_recipes("foo") == 2)
+	minetest.clear_craft({output="foo"})
+	assert(minetest.get_all_craft_recipes("foo") == nil)
+	-- Clearing by input
+	minetest.register_craft({
+		output = "foo 4",
+		recipe = {{"foo", "bar"}}
+	})
+	assert(#minetest.get_all_craft_recipes("foo") == 1)
+	minetest.clear_craft({recipe={{"foo", "bar"}}})
+	assert(minetest.get_all_craft_recipes("foo") == nil)
+end
+test_clear_craft()
+
+local function test_get_craft_result()
+	minetest.log("info", "test_get_craft_result()")
+	-- normal
+	local input = {
+		method = "normal",
+		width = 2,
+		items = {"", "default:coal_lump", "", "default:stick"}
+	}
+	minetest.log("info", "torch crafting input: "..dump(input))
+	local output, decremented_input = minetest.get_craft_result(input)
+	minetest.log("info", "torch crafting output: "..dump(output))
+	minetest.log("info", "torch crafting decremented input: "..dump(decremented_input))
+	assert(output.item)
+	minetest.log("info", "torch crafting output.item:to_table(): "..dump(output.item:to_table()))
+	assert(output.item:get_name() == "default:torch")
+	assert(output.item:get_count() == 4)
+	-- fuel
+	local input = {
+		method = "fuel",
+		width = 1,
+		items = {"default:coal_lump"}
+	}
+	minetest.log("info", "coal fuel input: "..dump(input))
+	local output, decremented_input = minetest.get_craft_result(input)
+	minetest.log("info", "coal fuel output: "..dump(output))
+	minetest.log("info", "coal fuel decremented input: "..dump(decremented_input))
+	assert(output.time)
+	assert(output.time > 0)
+	-- cook
+	local input = {
+		method = "cooking",
+		width = 1,
+		items = {"default:cobble"}
+	}
+	minetest.log("info", "cobble cooking input: "..dump(output))
+	local output, decremented_input = minetest.get_craft_result(input)
+	minetest.log("info", "cobble cooking output: "..dump(output))
+	minetest.log("info", "cobble cooking decremented input: "..dump(decremented_input))
+	assert(output.time)
+	assert(output.time > 0)
+	assert(output.item)
+	minetest.log("info", "cobble cooking output.item:to_table(): "..dump(output.item:to_table()))
+	assert(output.item:get_name() == "default:stone")
+	assert(output.item:get_count() == 1)
+end
+test_get_craft_result()

games/minimal/mods/test/init.lua

@@ -9,5 +9,7 @@ pseudo = PseudoRandom(13)
 assert(pseudo:next() == 22290)
 assert(pseudo:next() == 13854)
 
-dofile(minetest.get_modpath("test") .. "/player.lua")
-dofile(minetest.get_modpath("test") .. "/formspec.lua")
+local modpath = minetest.get_modpath("test")
+dofile(modpath .. "/player.lua")
+dofile(modpath .. "/formspec.lua")
+dofile(modpath .. "/crafting.lua")

games/minimal/mods/test/player.lua

@@ -74,51 +74,3 @@ local function run_player_tests(player)
 	minetest.chat_send_all("All tests pass!")
 end
 minetest.register_on_joinplayer(run_player_tests)
-
-
-local function test_get_craft_result()
-	minetest.log("info", "test_get_craft_result()")
-	-- normal
-	local input = {
-		method = "normal",
-		width = 2,
-		items = {"", "default:coal_lump", "", "default:stick"}
-	}
-	minetest.log("info", "torch crafting input: "..dump(input))
-	local output, decremented_input = minetest.get_craft_result(input)
-	minetest.log("info", "torch crafting output: "..dump(output))
-	minetest.log("info", "torch crafting decremented input: "..dump(decremented_input))
-	assert(output.item)
-	minetest.log("info", "torch crafting output.item:to_table(): "..dump(output.item:to_table()))
-	assert(output.item:get_name() == "default:torch")
-	assert(output.item:get_count() == 4)
-	-- fuel
-	local input = {
-		method = "fuel",
-		width = 1,
-		items = {"default:coal_lump"}
-	}
-	minetest.log("info", "coal fuel input: "..dump(input))
-	local output, decremented_input = minetest.get_craft_result(input)
-	minetest.log("info", "coal fuel output: "..dump(output))
-	minetest.log("info", "coal fuel decremented input: "..dump(decremented_input))
-	assert(output.time)
-	assert(output.time > 0)
-	-- cook
-	local input = {
-		method = "cooking",
-		width = 1,
-		items = {"default:cobble"}
-	}
-	minetest.log("info", "cobble cooking input: "..dump(output))
-	local output, decremented_input = minetest.get_craft_result(input)
-	minetest.log("info", "cobble cooking output: "..dump(output))
-	minetest.log("info", "cobble cooking decremented input: "..dump(decremented_input))
-	assert(output.time)
-	assert(output.time > 0)
-	assert(output.item)
-	minetest.log("info", "cobble cooking output.item:to_table(): "..dump(output.item:to_table()))
-	assert(output.item:get_name() == "default:stone")
-	assert(output.item:get_count() == 1)
-end
-test_get_craft_result()

src/craftdef.cpp

@@ -287,6 +287,15 @@ std::string craftDumpMatrix(const std::vector<ItemStack> &items,
 	CraftInput
 */
 
+bool CraftInput::empty() const
+{
+	for (const auto &item : items) {
+		if (!item.empty())
+			return false;
+	}
+	return true;
+}
+
 std::string CraftInput::dump() const
 {
 	std::ostringstream os(std::ios::binary);
@@ -906,18 +915,7 @@ public:
 			std::vector<ItemStack> &output_replacement, bool decrementInput,
 			IGameDef *gamedef) const
 	{
-		output.item = "";
-		output.time = 0;
-
-		// If all input items are empty, abort.
-		bool all_empty = true;
-		for (const auto &item : input.items) {
-			if (!item.empty()) {
-				all_empty = false;
-				break;
-			}
-		}
-		if (all_empty)
+		if (input.empty())
 			return false;
 
 		std::vector<std::string> input_names;
@@ -1002,84 +1000,48 @@ public:
 		return recipes;
 	}
 
-	virtual bool clearCraftRecipesByOutput(const CraftOutput &output, IGameDef *gamedef)
+	virtual bool clearCraftsByOutput(const CraftOutput &output, IGameDef *gamedef)
 	{
-		auto vec_iter = m_output_craft_definitions.find(output.item);
+		auto to_clear = m_output_craft_definitions.find(output.item);
 
-		if (vec_iter == m_output_craft_definitions.end())
+		if (to_clear == m_output_craft_definitions.end())
 			return false;
 
-		std::vector<CraftDefinition*> &vec = vec_iter->second;
-		for (auto def : vec) {
+		for (auto def : to_clear->second) {
 			// Recipes are not yet hashed at this point
-			std::vector<CraftDefinition*> &unhashed_inputs_vec = m_craft_defs[(int) CRAFT_HASH_TYPE_UNHASHED][0];
-			std::vector<CraftDefinition*> new_vec_by_input;
-			/* We will preallocate necessary memory addresses, so we don't need to reallocate them later.
-				This would save us some performance. */
-			new_vec_by_input.reserve(unhashed_inputs_vec.size());
-			for (auto &i2 : unhashed_inputs_vec) {
-				if (def != i2) {
-					new_vec_by_input.push_back(i2);
-				}
-			}
-			m_craft_defs[(int) CRAFT_HASH_TYPE_UNHASHED][0].swap(new_vec_by_input);
+			std::vector<CraftDefinition *> &defs = m_craft_defs[(int)CRAFT_HASH_TYPE_UNHASHED][0];
+			defs.erase(std::remove(defs.begin(), defs.end(), def), defs.end());
+			delete def;
 		}
-		m_output_craft_definitions.erase(output.item);
+		m_output_craft_definitions.erase(to_clear);
 		return true;
 	}
 
-	virtual bool clearCraftRecipesByInput(CraftMethod craft_method, unsigned int craft_grid_width,
-		const std::vector<std::string> &recipe, IGameDef *gamedef)
+	virtual bool clearCraftsByInput(const CraftInput &input, IGameDef *gamedef)
 	{
-		bool all_empty = true;
-		for (const auto &i : recipe) {
-			if (!i.empty()) {
-				all_empty = false;
-				break;
-			}
-		}
-		if (all_empty)
+		if (input.empty())
 			return false;
 
-		CraftInput input(craft_method, craft_grid_width, craftGetItems(recipe, gamedef));
 		// Recipes are not yet hashed at this point
-		std::vector<CraftDefinition*> &unhashed_inputs_vec = m_craft_defs[(int) CRAFT_HASH_TYPE_UNHASHED][0];
-		std::vector<CraftDefinition*> new_vec_by_input;
+		std::vector<CraftDefinition *> &defs = m_craft_defs[(int)CRAFT_HASH_TYPE_UNHASHED][0];
+		std::vector<CraftDefinition *> new_defs;
 		bool got_hit = false;
-		for (std::vector<CraftDefinition*>::size_type
-				i = unhashed_inputs_vec.size(); i > 0; i--) {
-			CraftDefinition *def = unhashed_inputs_vec[i - 1];
-			/* If the input doesn't match the recipe definition, this recipe definition later
-				will be added back in source map. */
+		for (auto def : defs) {
 			if (!def->check(input, gamedef)) {
-				new_vec_by_input.push_back(def);
+				new_defs.push_back(def);
 				continue;
 			}
-			CraftOutput output = def->getOutput(input, gamedef);
 			got_hit = true;
-			auto vec_iter = m_output_craft_definitions.find(output.item);
-			if (vec_iter == m_output_craft_definitions.end())
+			std::string output = def->getOutput(input, gamedef).item;
+			delete def;
+			auto it = m_output_craft_definitions.find(craftGetItemName(output, gamedef));
+			if (it == m_output_craft_definitions.end())
 				continue;
-			std::vector<CraftDefinition*> &vec = vec_iter->second;
-			std::vector<CraftDefinition*> new_vec_by_output;
-			/* We will preallocate necessary memory addresses, so we don't need
-				to reallocate them later. This would save us some performance. */
-			new_vec_by_output.reserve(vec.size());
-			for (auto &vec_i : vec) {
-				/* If pointers from map by input and output are not same,
-					we will add 'CraftDefinition*' to a new vector. */
-				if (def != vec_i) {
-					/* Adding dereferenced iterator value (which are
-						'CraftDefinition' reference) to a new vector. */
-					new_vec_by_output.push_back(vec_i);
-				}
-			}
-			// Swaps assigned to current key value with new vector for output map.
-			m_output_craft_definitions[output.item].swap(new_vec_by_output);
+			std::vector<CraftDefinition *> &outdefs = it->second;
+			outdefs.erase(std::remove(outdefs.begin(), outdefs.end(), def), outdefs.end());
 		}
 		if (got_hit)
-			// Swaps value with new vector for input map.
-			m_craft_defs[(int) CRAFT_HASH_TYPE_UNHASHED][0].swap(new_vec_by_input);
+			defs.swap(new_defs);
 
 		return got_hit;
 	}

src/craftdef.h

@@ -80,6 +80,9 @@ struct CraftInput
 		method(method_), width(width_), items(items_)
 	{}
 
+	// Returns true if all items are empty.
+	bool empty() const;
+
 	std::string dump() const;
 };
 
@@ -431,9 +434,8 @@ public:
 	virtual std::vector<CraftDefinition*> getCraftRecipes(CraftOutput &output,
 			IGameDef *gamedef, unsigned limit=0) const=0;
 
-	virtual bool clearCraftRecipesByOutput(const CraftOutput &output, IGameDef *gamedef) = 0;
-	virtual bool clearCraftRecipesByInput(CraftMethod craft_method,
-			unsigned int craft_grid_width, const std::vector<std::string> &recipe, IGameDef *gamedef) = 0;
+	virtual bool clearCraftsByOutput(const CraftOutput &output, IGameDef *gamedef) = 0;
+	virtual bool clearCraftsByInput(const CraftInput &input, IGameDef *gamedef) = 0;
 
 	// Print crafting recipes for debugging
 	virtual std::string dump() const=0;

src/script/lua_api/l_craft.cpp

@@ -294,7 +294,7 @@ int ModApiCraft::l_clear_craft(lua_State *L)
 	std::string type = getstringfield_default(L, table, "type", "shaped");
 	CraftOutput c_output(output, 0);
 	if (!output.empty()) {
-		if (craftdef->clearCraftRecipesByOutput(c_output, getServer(L))) {
+		if (craftdef->clearCraftsByOutput(c_output, getServer(L))) {
 			lua_pushboolean(L, true);
 			return 1;
 		}
@@ -351,7 +351,13 @@ int ModApiCraft::l_clear_craft(lua_State *L)
 		throw LuaError("Unknown crafting definition type: \"" + type + "\"");
 	}
 
-	if (!craftdef->clearCraftRecipesByInput(method, width, recipe, getServer(L))) {
+	std::vector<ItemStack> items;
+	items.reserve(recipe.size());
+	for (const auto &item : recipe)
+		items.emplace_back(item, 1, 0, getServer(L)->idef());
+	CraftInput input(method, width, items);
+
+	if (!craftdef->clearCraftsByInput(input, getServer(L))) {
 		warningstream << "No craft recipe matches input" << std::endl;
 		lua_pushboolean(L, false);
 		return 1;