Additional hooks for inventory manipulations callbacks.

[Foghrye4]

With three new callbacks (on inventory itemstack add, move and drop) modders will be able to trace inventory state at any moment without need to iterate thru all players inventories. This way we can make, for example, overweight inventory effects.

[rubenwardy]

we use

if (condition) {
    code;
}

[Foghrye4]

Should i edit my patch immediately for this pull to be accepted, or this is just a note for a future?

[ObaniGemini]

You should maybe wait for other reviews, discuss about it on IRC.
Read to know more about the contributing process

[PilzAdam]

Why did you change player:get_inventory():add_item() to player:add_item()? This doesn't even work according to lua_api.txt.

[rubenwardy]

Why did you change player:get_inventory():add_item() to player:add_item()? This doesn't even work according to lua_api.txt.

It looks like player:add_item() is a new function

[paramat]

This way we can make, for example, overweight inventory effects.

Please could you explain more what this would be useful for? What is an overweight inventory effect?

[asl97]

@paramat i am guessing that it means slowing down the player movement (ie: set_physics_override) when the player carry too many stuff.

[Foghrye4]

@paramat asl97 guessed absolutely correctly. First i have idea to make a mod, which will limit player carrying capacity by weight of carried items, secondly i found out - there is not enought callbacks to do so, third - i made a nessesary callbacks and create this request.

[Foghrye4]

@paramat second option - detect whenever player take something radioactive, for example, enriched uranium fuel and add effects of radiation sickness. Or opposite - some magic device, which will heal player.

[est31]

Can this be accomplished by not adding add_item and get_item methods to the player?

Otherwise, the idea is good, however you should do only one commit for this change, and add a meaningful commit description.

[Foghrye4]

@est31 commit? You mean - change only one file? Or do not change files after this request created?
There is no get_item method in this pull request. Only add_item.
About reason of this function to be added: of course i can hook up on InvRef add_item, but if i do so callback "on_inventory_add" will be launched on any inventory interactions, including node and detached inventories, unnecessary overlapping with they own callbacks. As well, its hard to extract player reference from this function.

[rubenwardy]

Each commit can update multiple files. There should only be one entry here ideally: https://github.com/minetest/minetest/pull/3962/commits

The merger can squash your commits, though

[PilzAdam]

This is missing documentation in lua_api.txt.

I would prefer if this would work without adding player:add_item, since it would require to update some mods. I wouldn't consider it bad that the callbacks may be overlapping; we have overlapping callbacks already, e.g. minetest.register_on_dignode and nodedef.on_dig.

[sofar]

• Lots of style issues (spaces after 'if', around '()' and '{}' not on the same lines as 'if' and 'else')
• Even if squashed, there's not a single commit that has an actual commit message that is sufficient.

[Foghrye4]

@PilzAdam This could be done. But for my purposes i need player reference. And i have no idea how to extract player reverence from InvRef::l_add_item(lua_State *L).
'lua_api.txt' was updated

[PilzAdam]

I don't like that the callbacks are only called if player:add_item() is used, but not if player:get_inventory():add_item() is called.

I think that the new player:add_item() function is only a workaround, because it's not simple to add it to the existing functions. I don't like this workaround, since it requires mods to be updated so that the callbacks work for every occasion.

So, 👎 to this implementation. The general idea of this PR is good, tough.

[Foghrye4]

@PilzAdam
I don't know a single mod, that should be updated for this implementation to work. It will work perfectly with 3d_armor, for example. Modders still may use whatever method they like - there is no holes, what players can exploit in currently presented mods.

[PilzAdam]

@Foghrye4 every mod that calls player:get_inventory():add_item() will cause the callbacks to be ignored for this action. Thus, mods which want to use your new callbacks can't rely on them.

[Foghrye4]

@PilzAdam 100% of them do so on minetest.register_on_newplayer function or by server commands. A first one is easily catched by minetest.register_on_joinplayer as in my example mod. A second one - is not my case.
Well, i do see your point here. Here is what i can do:

1. Add ServerActiveObject field in InvRef class.
2. Add ServerActiveObject player parameter in InvRef::create(..)
3. Add Lua API function get_player() in InvRef.
   Should i use this method instead?

[Foghrye4]

@rubenwardy here is a realisation of your idea. I will change some things (will remove Player parameter from ScriptApiPlayer functions and InvRef from Lua call, will update lua_api.txt).
It works perfectly fine with all three types of inventory: players, nodemeta and detached.

I would like to see your opinion and opinion of others members.

[Foghrye4]

There is 'minetest.clear_craft(recipe)' in this pull request as well here now. It was properly checked in all possible uses. This function allow modders remove previously registered recipes either by output or by input and type (including "fuel" and "cooking"). Plus, removed recipes no longer visible via crafting guides, because they eliminated from registry.

[est31]

@Foghrye4 can you open a separate PR for that, its unrelated to this change.

[Foghrye4]

@est31 As you wish.