Add an item pick up callback (2)

[Desour]

This PR adds a callback for when items are picked up via punch.

There's also an item in devtest for testing.

(Old PR was: #6359)

API:

• An on_pickup callback in the item definition.
• A register_on_item_pickup function with that you can register callback functions for all items being picked up.
• For more details look into lua_api.txt.

Use-cases:

• A mod adds an item and wants that when someone who is too weak to pick it up there's an explosion instead.
• An item should look different when dropped. (eg. a bow that should have another size as item entity) The item has an on_drop to drop an other item. Currently there's no easy way to give the in-inventory item on pick up, the mod has to change the item name on another callback, the player has the wrong item for a time. With this PR, the mod can easily add an on_pickup callback to item definition that sets self.itemstring to another itemstring and returns true.
• A mod wants to add experience pearls as items that fill a bar when picked up.
• more (Eg. see Wuzzys comment.)

[raymoo]

I think it would be useful to factor out the "call callbacks and give item to player" code into a separate function that is part of the modding API, similar to node_dig and other functions. Then mods that let players pick up items by walking over them could reuse the code, including callback invocations.

[Desour]

separate function

They can punch the item entity object.

[raymoo]

That's true

[raymoo]

What would be an example use for this feature? I'm worried that in many cases you would want the same callbacks to run if a player gets an item by digging a node (for example).

[Desour]

Some examples:

• A mod adds an item and wants that when someone who is too weak to pick it up there's an explosion instead.
• An item should look different when dropped. (eg. a bow that should have another size as item entity) The item has an on_drop to drop an other item. Currently there's no easy way to give the in-inventory item on pick up, the mod has to change the item name on another callback, the player has the wrong item for a time. With this PR, the mod can easily add an on_pickup callback to item definition that sets self.itemstring to another itemstring and returns true.
• A mod wants to add experience pearls as items that fill a bar when picked up.

[Wuzzy2]

Looks good. I have tested it and it works as promised. Test Lua code:

minetest.register_on_item_pickup(function(luaentity, picker, time_from_last_punch, tool_capabilities, dir)
	minetest.chat_send_all("Picked up an item!")
	minetest.chat_send_all("--- START OF ITEM INFO ---")
	minetest.chat_send_all(">>> luaentity == "..tostring(dump(luaentity)))
	minetest.chat_send_all(">>> picker:get_player_name() == "..tostring(picker:get_player_name()))
	minetest.chat_send_all(">>> time_from_last_punch == "..tostring(time_from_last_punch))
	minetest.chat_send_all(">>> tool_capabilities == "..tostring(dump(tool_capabilities)))
	minetest.chat_send_all(">>> dir == "..tostring(dump(dir)))
	minetest.chat_send_all("--- END OF ITEM INFO ---")
end)

The only thing I'm missing is that you should add the registered_ table to the “Global tables” section of lua_api.txt. That's all!

[Wuzzy2]

For those who want to overwrite the item entity, all you need to do is to call the callback in your custom item definition in the event of a pickup.

[Wuzzy2]

More use cases:

• Achievements
• Updating internal item state or game state the moment it got picked up, like binoculars/mapping kit in MTG

[Desour]

The only thing I'm missing is that you should add the registered_ table to the “Global tables” section of lua_api.txt. That's all!

None of these are here.

[Wuzzy2]

The existing crappiness of lua_api.txt is not an excuse to do it properly in your PR. ;-)

[Desour]

• You are wrong, it is.
• Where should I add it? it would be lonely and feel sad. Such an inconsistency doesn't make sense.
• Adding all these to lua_api.txt would be out of scope.
• These probably aren't meant to be used by mods.

[Wuzzy2]

Let's wait what the core devs have to say.

[ClobberXD]

Actually, an implementation similar to the one for item drop would be nice to have. There could be a minetest.item_pickup() function (called when on_pickup isn't defined) which implements the default item pickup behaviour.

[rubenwardy]

This needs to be implemented in a such a way that a Minecraft-like itemdrop mod can reuse these callbacks

In MTG, this callback should be called when the player digs a node and it goes into the inventory, and when the player punches a dropped item. The luaentity being nil should be enough to distinguish between these

[Desour]

This needs to be implemented in a such a way that a Minecraft-like itemdrop mod can reuse these callbacks

A Minecraft-like itemdrop mod can call item_obj:punch(...).

In MTG, this callback should be called when the player digs a node and it goes into the inventory, and when the player punches a dropped item. The luaentity being nil should be enough to distinguish between these

No. That should be a separate callback. It would need very different parameters (such as itemstack). Maybe something like on_item_into_player_inventory(player, item, inv, source, ...).

[Desour]

Does someone want to approve or request changes? @minetest

This PR adds a useful callback. Even minetest_game would profit from it.

[Desour]

After some testing I found out that I made a mistake: the itemstring is not always exactly the itemname.
I will fix this.
Edit: Done.

[Desour]

I did tests. Everything works as in doc described.
@ClobberXD Thanks for commenting.
Again I'm waiting for approvals or change requests. : )

[raymoo]

A Minecraft-like itemdrop mod can call item_obj:punch(...).

What about mods that add vacuuming nodes? Shouldn't they also be able to get the correct picked-up item? Such mods have no way to tell the punch where to put the items (unless you use the forbidden hack of temporarily using a player's inventory for storage, and then transferring the item to the node's inventory).

[Desour]

What about mods that add vacuuming nodes?

They can either use a fake player or nil as puncher in obj:punch(puncher, ...).
They could also do it like they did it before ignoring the callbacks. In most cases this won't be deadly, I guess.
Edit: Or another hack: Set eg. luaent.foobar_vacuum = true and add a minetest.register_on_item_pickup callback that puts the item into another inventory if luaent.foobar_vacuum, then do the punch with any player.

(Actually there's a missing check for hitter being nil (which is possible) (before and with this PR).

[raymoo]

fake player may work but should not be the intended solution because it's a hack. nil won't work because there is no inventory to place the item in.

Since this is a new feature, it can be designed not to need hacks.

[Desour]

Yes, but that would probably make the change over complex which prevents a merge. :/

And it's not that hacky if a vacuuming mod runs the callbacks itself with nil (which has to be handled by the callbacks).

Of course I'm open to suggestions btw.

[ClobberXD]

#7712 (comment), is it possible to follow this approach? raymoo also suggested this, IIRC.

[Desour]

What would be the benefit of that approach?

For on_drop this makes sense because it has to be called from cpp:
https://github.com/minetest/minetest/blob/325bf680410e8012394e5f3ba5ba947c69034899/src/script/cpp_api/s_item.cpp#L40
But is it needed for on_pickup?

And on_drop doesn't need an inventory to take the item from.
I doubt that this would be easier for vacuuming nodes.

[raymoo]

A function implementing default pickup behavior could be useful for use in on_pickup callbacks because if you wanted to conditionally do normal pickup behavior. E.g. if the item is in a protected area do nothing if non-owners of the area try to pick it up, but do normal pickup behavior otherwise.

I think what I was suggesting is different though. It would be some kind of function item_pickup(item_object, inventory) that will do pickup behavior (including calling relevant callbacks). It's possible to determine if an inventory is a player inventory by checking its location, but to make it easier an object parameter could be passed too. Just if you do that, you can have the strange case where the given object is an entity, or where the object is a player but the passed inventory is not the player's inventory.

[Desour]

Resolved comments.
I found out that core.run_callbacks mode 3 (RUN_CALLBACKS_MODE_AND_SC) was completely broken. Other modes also differed a bit from spec, i.e. mode 5 (RUN_CALLBACKS_MODE_OR_SC) returned nil instead of the first callback's (false-ish) return value. So, I've fixed that.
Some mods might be affected by this. (Though this is an engine internal.)

[SmallJoker]

What would you think of making this similar to minetest.item_eat? If added to the itemdef metatable, the function could serve as a default for all picked up items.

Just thinking out loud because these two look very similar, and having less different concepts would be great to have. Like this, you could use on_pickup = minetest.item_eat(2), in an ItemDef to eat by picking up.

[Desour]

I've changed the obj parameter to a pointed_thing, so on_pickup = minetest.item_eat(2), should work now.

In that sense, it is now more similar to on_use.
Idk. how I should make it more similar to item_eat.

[SmallJoker]

API proposal: https://0x0.st/o4v6.patch

• Callback execution the same as item eat
• Return value as "leftover item" (i.e. kept item stack)
• the item on_pickup may override the default callback behaviour

[Desour]

The callbacks are now pretty similar to item_place.

Now, the question arises whether we even need the registered_on_item_pickups, as one can achieve the same by overwriting core.item_pickup. (The same applies for registered_on_item_eats.)

[SmallJoker]

Now, the question arises whether we even need the registered_on_item_pickups

I don't mind. It's up to you whether or not to keep this callback.
Tested the PR again. Works as expected.

[TurkeyMcMac]

Just a few things.

[Desour]

Now, the question arises whether we even need the registered_on_item_pickups

I don't mind. It's up to you whether or not to keep this callback.

I don't mind much either.
But there's this comment, so I'll keep it:
https://github.com/minetest/minetest/blob/525fc3833c40dcca478719533a87bc7b5e285e4e/builtin/game/item.lua#L559-L562

---

Comments addressed.

[TurkeyMcMac]

Looks good.