Fix error when picking up a node with deprecated tile definition (#40)

If `image` instead of `tile` was used, the mod passed nil to minetest.inventorycube.
The color field of a tile is now also supported (the code is used only for the particle image calculation).

.github/workflows/build.yml

@@ -0,0 +1,11 @@
+on: [push, pull_request]
+name: build
+jobs:
+  lint:
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@master
+      - name: lint
+        uses: Roang-zero1/factorio-mod-luacheck@master
+        with:
+          luacheckrc_url: https://raw.githubusercontent.com/minetest-mods/item_drop/master/.luacheckrc

.luacheckrc

@@ -0,0 +1,19 @@
+unused_args = false
+allow_defined_top = true
+max_line_length = 999
+
+ignore = {
+    "name", "drops", "i",
+}
+
+globals = {
+    "minetest",
+}
+
+read_globals = {
+    string = {fields = {"split", "trim"}},
+    table = {fields = {"copy", "getn"}},
+
+    "vector", "ItemStack",
+    "dump",
+}

README.md

@@ -1,15 +1,14 @@
-# item_drop
+# Item Drop [![](https://github.com/minetest-mods/item_drop/workflows/build/badge.svg)](https://github.com/minetest-mods/item_drop/actions) [![License](https://img.shields.io/badge/license-LGPLv2.1%2B-blue.svg)](https://www.gnu.org/licenses/old-licenses/lgpl-2.1.en.html)
+
+A highly configurable mod providing item magnet and in-world node drops\
 By [PilzAdam](https://github.com/PilzAdam),
 [texmex](https://github.com/tacotexmex/), [hybriddog](https://github.com/hybriddog/).
 
-## Description
-A highly configurable mod providing item magnet and in-world node drops
-
 ## Licensing
 LGPLv2.1/CC BY-SA 3.0. Particle code from WCILA mod by Aurailus, originally licensed MIT.
 
 ## Notes
-item_drop can be played with Minetest 0.4.16 or above. It was originally
+`item_drop` can be played with Minetest 0.4.16 or above. It was originally
 developed by [PilzAdam](https://github.com/PilzAdam/item_drop).
 
 ## List of features

init.lua

@@ -93,34 +93,46 @@ if legacy_setting_getbool("item_drop.enable_item_pickup",
 		flowingliquid = true,
 	}
 
+	-- Get an image string from a tile definition
+	local function tile_to_image(tile, fallback_image)
+		if not tile then
+			return fallback_image
+		end
+		local tile_type = type(tile)
+		if tile_type == "string" then
+			return tile
+		end
+		assert(tile_type == "table", "Tile definition is not a string or table")
+		local image = tile.name or tile.image
+		assert(image, "Tile definition has no image file specified")
+		if tile.color then
+			local colorstr = minetest.colorspec_to_colorstring(tile.color)
+			if colorstr then
+				return image .. "^[multiply:" .. colorstr
+			end
+		end
+		return image
+	end
+
 	-- adds the item to the inventory and removes the object
 	local function collect_item(ent, pos, player)
 		item_drop.before_collect(ent, pos, player)
 		minetest.sound_play("item_drop_pickup", {
 			pos = pos,
 			gain = pickup_gain,
-		})
+		}, true)
 		if pickup_particle then
 			local item = minetest.registered_nodes[
 				ent.itemstring:gsub("(.*)%s.*$", "%1")]
-			local image = ""
+			local image
 			if item and item.tiles and item.tiles[1] then
 				if inventorycube_drawtypes[item.drawtype] then
 					local tiles = item.tiles
-
-					local top = tiles[1]
-					if type(top) == "table" then
-						top = top.name
-					end
-					local left = tiles[3] or top
-					if type(left) == "table" then
-						left = left.name
-					end
-					local right = tiles[5] or left
-					if type(right) == "table" then
-						right = right.name
-					end
-
+					-- color in the tile definition is handled by tile_to_image.
+					-- color in the node definition is not yet supported here.
+					local top = tile_to_image(tiles[1])
+					local left = tile_to_image(tiles[3], top)
+					local right = tile_to_image(tiles[5], left)
 					image = minetest.inventorycube(top, left, right)
 				else
 					image = item.inventory_image or item.tiles[1]
@@ -201,9 +213,9 @@ if legacy_setting_getbool("item_drop.enable_item_pickup",
 			local itemdef = minetest.registered_entities["__builtin:item"]
 			local old_on_step = itemdef.on_step
 			local function do_nothing() end
-			function itemdef.on_step(self, dtime)
+			function itemdef.on_step(self, ...)
 				if not self.is_magnet_item then
-					return old_on_step(self, dtime)
+					return old_on_step(self, ...)
 				end
 				ObjectRef = ObjectRef or getmetatable(self.object)
 				local old_funcs = {}
@@ -212,7 +224,7 @@ if legacy_setting_getbool("item_drop.enable_item_pickup",
 					old_funcs[method] = ObjectRef[method]
 					ObjectRef[method] = do_nothing
 				end
-				old_on_step(self, dtime)
+				old_on_step(self, ...)
 				for i = 1, #blocked_methods do
 					local method = blocked_methods[i]
 					ObjectRef[method] = old_funcs[method]
@@ -252,6 +264,13 @@ if legacy_setting_getbool("item_drop.enable_item_pickup",
 		return keys_pressed ~= key_invert
 	end
 
+	local function is_inside_map(pos)
+		local bound = 31000
+		return -bound < pos.x and pos.x < bound
+			and -bound < pos.y and pos.y < bound
+			and -bound < pos.z and pos.z < bound
+	end
+
 	-- called for each player to possibly collect an item, returns true if so
 	local function pickupfunc(player)
 		if not has_keys_pressed(player)
@@ -260,9 +279,13 @@ if legacy_setting_getbool("item_drop.enable_item_pickup",
 			return
 		end
 
-		local pos = player:getpos()
+		local pos = player:get_pos()
+		if not is_inside_map(pos) then
+			-- get_objects_inside_radius crashes for too far positions
+			return
+		end
 		pos.y = pos.y+0.5
-		local inv
+		local inv = player:get_inventory()
 
 		local objectlist = minetest.get_objects_inside_radius(pos,
 			magnet_mode and magnet_radius or pickup_radius)
@@ -271,14 +294,6 @@ if legacy_setting_getbool("item_drop.enable_item_pickup",
 			local ent = opt_get_ent(object)
 			if ent
 			and item_drop.can_pickup(ent, player) then
-				if not inv then
-					inv = player:get_inventory()
-					if not inv then
-						minetest.log("error", "[item_drop] Couldn't " ..
-							"get inventory")
-						return
-					end
-				end
 				local item = ItemStack(ent.itemstring)
 				if inv:room_for_item("main", item) then
 					local flying_item
@@ -330,17 +345,65 @@ end
 
 if legacy_setting_getbool("item_drop.enable_item_drop", "enable_item_drop", true)
 and not minetest.settings:get_bool("creative_mode") then
-	function minetest.handle_node_drops(pos, drops)
+	-- Workaround to test if an item metadata (ItemStackMetaRef) is empty
+	local function itemmeta_is_empty(meta)
+		local t = meta:to_table()
+		for k, v in pairs(t) do
+			if k ~= "fields" then
+				return false
+			end
+			assert(type(v) == "table")
+			if next(v) ~= nil then
+				return false
+			end
+		end
+		return true
+	end
+
+	-- Tests if the item has special information such as metadata
+	local function can_split_item(item)
+		return item:get_wear() == 0 and itemmeta_is_empty(item:get_meta())
+	end
+
+	local function spawn_items(pos, items_to_spawn)
+		for i = 1,#items_to_spawn do
+			local obj = minetest.add_item(pos, items_to_spawn[i])
+			if not obj then
+				error("Couldn't spawn item " .. name .. ", drops: "
+					.. dump(drops))
+			end
+
+			local vel = obj:get_velocity()
+			local x = math.random(-5, 4)
+			if x >= 0 then
+				x = x+1
+			end
+			vel.x = 1 / x
+			local z = math.random(-5, 4)
+			if z >= 0 then
+				z = z+1
+			end
+			vel.z = 1 / z
+			obj:set_velocity(vel)
+		end
+	end
+
+	local old_handle_node_drops = minetest.handle_node_drops
+	function minetest.handle_node_drops(pos, drops, player)
+		if not player or player.is_fake_player then
+			-- Node Breaker or similar machines should receive items in the
+			-- inventory
+			return old_handle_node_drops(pos, drops, player)
+		end
 		for i = 1,#drops do
 			local item = drops[i]
-			local count, name
 			if type(item) == "string" then
-				count = 1
-				name = item
-			else
-				count = item:get_count()
-				name = item:get_name()
+				-- The string is not necessarily only the item name,
+				-- so always convert it to ItemStack
+				item = ItemStack(item)
 			end
+			local count = item:get_count()
+			local name = item:get_name()
 
 			-- Sometimes nothing should be dropped
 			if name == ""
@@ -348,24 +411,16 @@ and not minetest.settings:get_bool("creative_mode") then
 				count = 0
 			end
 
-			for _ = 1,count do
-				local obj = minetest.add_item(pos, name)
-				if not obj then
-					error("Couldn't spawn item " .. name .. ", drops: " .. dump(drops))
+			if count > 0 then
+				-- Split items if possible
+				local items_to_spawn = {item}
+				if can_split_item(item) then
+					for i = 1,count do
+						items_to_spawn[i] = name
+					end
 				end
 
-				local vel = obj:get_velocity()
-				local x = math.random(-5, 4)
-				if x >= 0 then
-					x = x+1
-				end
-				vel.x = 1 / x
-				local z = math.random(-5, 4)
-				if z >= 0 then
-					z = z+1
-				end
-				vel.z = 1 / z
-				obj:set_velocity(vel)
+				spawn_items(pos, items_to_spawn)
 			end
 		end
 	end