stujones11 / minetest-3d_armor

Visible player armor & wielded items for minetest
Other
56 stars 98 forks source link

Allow replacing similar armor in the same slot #142

Closed SmallJoker closed 6 years ago

SmallJoker commented 6 years ago

Rather trivial change which also takes care of that #141 doesn't happen.

EDIT: Are there more problems left over from that issue after the recent fixes? I could try to fix those too.

stujones11 commented 6 years ago

@SmallJoker Thank you for the PR, the first part clearly fixes a redeclaration of stack but I am struggling to understand the purpose of the second part. Do you mean to by using the 'swap trick'?

From what I found, neither allow_putnor on_put get called when you swap in an item like that, however, on_take strangely does. This is what triggered the inventory save where I have reluctantly had to put in additional validation.

One remaining problem is that the armor api's (un)equip callbacks are mixed up, however, I doubt there is an easy fix for that. It would be helpful if you could verify that there is indeed a bug with MT detached inventories missing callbacks with this 'swap trick'. Other mods could also be vulnerable.

SmallJoker commented 6 years ago

When you put an item into any slot of the inventory, allow_put is called at the remote inventory. If you take an item from the remote inventory, allow_take is called there. It is called regardless on whether it replaces another item or not - the swap is performed automatically as soon allow_put (first inventory) and allow_take (second inventory) return any non-zero value. Triggering the save function (may it be direct or delayed with a "dirty" flag) in on_put and on_take is totally correct. It happens when the last stack movement was moved successfully (i.e. allowed movement).

This PR only allows swapping the slot if the replacement stack has the same armor type (element) as the one which it will replace.

However, unrelated to detached inventories: There was a fix for a very rare used item move from one node inventory to another node inventory - however, the other callbacks seem to be fine and are called consistently. EDIT: What do you mean by "(un)equip callbacks are mixed up"? The model always seems to be updated correctly.

stujones11 commented 6 years ago

When you put an item into any slot of the inventory, allow_put is called at the remote inventory.

Thank you for the explanation but problem for me is that it does not get called when you mess around with things bit. Here is a description I once gave on the forums of how to reproduce this using an older version of the mod. https://forum.minetest.net/viewtopic.php?f=11&t=4654&start=700#p307701

We are not talking about regular item replacement here, perhaps that is what this PR fixes?

stujones11 commented 6 years ago

This PR only allows swapping the slot if the replacement stack has the same armor type (element) as the one which it will replace.

I guess that answers the question and the PR looks good, however, I still believe there are some issues with detached inventories. I will investigate further when I have time.

What do you mean by "(un)equip callbacks are mixed up"? The model always seems to be updated correctly.

I mean that the exposed api callaback armor.register_on_unequip gets called because of the erroneous on_take likewise the on_equip is missed due to on_put being bypassed. The armor is now displayed correctly because it gets 'sanitized' again prior to being saved and subsequently updated, irregardless of which inventory callback triggered the process.

https://github.com/stujones11/minetest-3d_armor/blob/7d30bc25a3a3e765dcc67048a6b87c9ac3a43122/3d_armor/api.lua#L486-L497 Try out-commenting that section and do as I described on the forums, add some print() lines to the detached inventory callback handlers and you should be able see what I mean :)