Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Additional hooks for inventory manipulations callbacks. #4035

Closed
wants to merge 1 commit into from
Closed

Additional hooks for inventory manipulations callbacks. #4035

wants to merge 1 commit into from

Conversation

Foghrye4
Copy link
Contributor

Well, i screwed a little when i tried to remove 'minetest.clear_craft' from old master branch, so here is a new pull request.

For those, who does not saw #3962 this pull will add modding API function, which allow modders to trace any inventory changes (detached, nodemeta or player).

@@ -510,7 +510,15 @@ core.registered_craft_predicts, core.register_craft_predict = make_registration(
core.registered_on_protection_violation, core.register_on_protection_violation = make_registration()
core.registered_on_item_eats, core.register_on_item_eat = make_registration()
core.registered_on_punchplayers, core.register_on_punchplayer = make_registration()

core.registered_on_player_inventory_remove_item, core.register_on_player_inventory_remove_item = make_registration()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mind the conventional plural-s on most other callback tables.

@t4im
Copy link
Contributor

t4im commented Jul 7, 2016

If you transfer an item from your creative inventory to a chest this will trigger at least two callbacks for essentially the same action.
Having everything in 9 different callbacks likely forces people to register something multiple times.

Can't you just pass the InvRef as first argument? Its get_location gives you type (player, nodemeta, detached) and position/name. You'd have to look it up anyway, first thing in a callback.

You could even combine all 9 into a single callback:

--- passed to core.register_on_inventory_change
function(from_inv, from_list, from_slot, to_inv, to_list, to_slot, stack, player)
  -- maybe we can allow to interrupt the inventory action? Or maybe even change it. 
  return
end

The parameters might seem a bit long, but if you account for the additional player, it is only 2 arguments longer than your current suggestion. And it is less verbose than having to create multiple callbacks.

Coming to think of it, why don't node callbacks do something similar?

@Foghrye4
Copy link
Contributor Author

Foghrye4 commented Jul 7, 2016

@t4im There is a reason for this. If you look up C++ code, you will see, that every change made passed by "InventoryList" object functions. Currently you can directly access lists and change their contents without using inventory object. As well, not only player can do so, but any script. As well you can get inventory reference from first argument of callback function (detached name or player or position).
Making list "know" where from comes new itemstack is complex. Currently inventory manager just delete itemstack in an old place and re-create it on a new place. Both source and destination list have they own listeners (which could be a same) and differ this movement case from any other cases would be very difficult and bring unnecessary complexity to code.

You are free to fork my work and modify it a way you want to.

@paramat
Copy link
Contributor

paramat commented Oct 9, 2016

Please squash commits to make it easier to review.

@paramat paramat added the Rebase needed The PR needs to be rebased by its author. label Oct 9, 2016
@Foghrye4
Copy link
Contributor Author

Foghrye4 commented Oct 9, 2016

@paramat Pull is rewritten on fresh version. Commits are squashed.

Geting rid of another old branch

fixing bugs
@paramat paramat removed Rebase needed The PR needs to be rebased by its author. Enhancement labels Oct 23, 2016
@sfan5 sfan5 added Feature ✨ PRs that add or enhance a feature Rebase needed The PR needs to be rebased by its author. labels Nov 27, 2016
@sfan5
Copy link
Member

sfan5 commented Nov 27, 2016

What's the intended usage for register_on_nodemeta_inventory_* and register_on_detached_inventory_*?

@Foghrye4
Copy link
Contributor Author

@sfan5 there is no such. Because callbacks implemented on server side inside InventoryList class, InventoryChangesReciever class, as Rubenwardy suggest, implemented in any inventory type, including detached and nodemeta. It could be used to trace all item movements for all players and mod actions. But I absolutely don't care about such movements.

@paramat paramat added Adoption needed The pull request needs someone to adopt it. Adoption welcomed! Action / change needed Code still needs changes (PR) / more information requested (Issues) labels Apr 15, 2017
@SmallJoker
Copy link
Member

Replaced with #5647

@SmallJoker SmallJoker closed this Apr 23, 2017
@Zughy Zughy removed the Adoption needed The pull request needs someone to adopt it. Adoption welcomed! label May 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Action / change needed Code still needs changes (PR) / more information requested (Issues) Feature ✨ PRs that add or enhance a feature Rebase needed The PR needs to be rebased by its author. @ Script API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants