Change digiline chest to send message on insertion, rather than in can_insert about digilines

[Hawk777]

Hi! I’m interested in working on this. It seems that a number of other PRs and tickets reference this as reasons for not doing things, and honestly, I’d like to see a more sane API for Digilines chests than what we have right now.

Current behaviour

This is what I’ve concluded is the current behaviour from a mix of inspecting code and experimenting in-game (all tests performed on Minetest 0.4.16, Minetest_game 0.4.16, and Digilines fceb4bb), but please correct me if I’m wrong on any of this or if I misunderstand any intent.

Players

When a player successfully adds a stack to a chest, we get:

1. A “put” of the stack the user places, from allow_metadata_inventory_put → can_insert.
2. A “put” (if there’s room for any more items of this type) or “lost” (if not, despite the fact that nothing was actually lost) of a stack of exactly one of the same item, from on_metadata_inventory_put → derpstack → can_insert.
3. A “uput” or “ufull” of the stack the user places, from on_metadata_inventory_put, depending on whether any more of the same item can be put in the chest.

When a player tries to add to too many items to an existing stack that wasn’t originally full, the chest acts precisely as if the player had tried to insert the smaller amount that does actually fit, and the residue ends up still attached to the mouse pointer.

When a player tries to add items to an existing full stack, some weird things happen due to minetest/minetest#6516, but IMO that’s not up to us to fix. However, even when weird things don’t happen, we don’t provide any information that the existing stack has been taken out (in fact it looks like Minetest doesn’t tell us this at all, so we can’t turn it into a Digilines message easily; we would have to explicitly check stack index numbers and whether the stack being dropped on was full or not).

I’m not entirely sure what we’re doing with allow_metadata_inventory_put, nor how it’s possible to generate a “uoverflow” message; it seems that the core engine handles not creating stacks larger than maximum size, and the Digilines chest doesn’t limit stacks to smaller than their natural maximum sizes, so that case doesn’t seem possible. Maybe it is though.

In any case, a player taking a stack out is pretty simple: a “utake” is issued, with an “empty” preceding it if doing so resulted in nothing left in the chest.

Tubes

When a tube adds a stack successfully (it all fits), we get a “put N” from tube.can_insert → can_insert for the added stack.

When a tube adds a stack successfully (it all fits) but there’s no space for any more of the item afterwards, we get a “put N” from tube.can_insert → can_insert for the added stack, followed by a full 0 from tube.insert_object.

When a tube tries to add a stack that doesn’t fit (either partially or completely—no attempt is made to split the stack), we get a “lost N” from tube.can_insert → can_insert for the added stack and then the stack takes an alternate path (or falls on the floor if there isn’t one). The contents of the chest don’t change.

When two tubes try to add stacks at exactly the same time, both of which would fit on their own but which don’t both fit, we get a “put N” from each tube.can_insert → can_insert for each of the two added stacks, followed by an “overflow X Y” from tube.insert_object for the stack that failed to completely insert, where X is the stack that tried to be inserted and Y is, after splitting the stack, how many were rejected and sent to an alternate path or onto the floor.

When a tube filter takes something out, it acts exactly like a player doing the same thing.

Observations

This is really confusing. I have no idea what the difference between the “u”-prefixed and non-“u”-prefixed messages is supposed to be, other than that maybe “u” stands for “user” indicating that a player is doing the action rather than a tube? In any case getting so many “put” messages, especially for derpstack, is pretty nasty and makes it nearly impossible to reliably figure out what’s going on in a chest.

Proposal

What the user of a Digilines chest cares about is:

• When items are added, how many were added of what type, and perhaps by what kind of agent?
• When items are removed, how many were removed of what type, and perhaps by what kind of agent?
• When the inventory becomes empty.
• MAYBE, when the inventory can no longer hold any more of a particular type of item.
• MAYBE, when a tube network tried to add items, but the chest was full, what items were rejected?

I don’t think the user cares about the excruciatingly detailed difference between “a stack didn’t go in because there wasn’t enough room for it when doing routing calculations” versus “a stack didn’t go in (or didn’t go fully in) because it arrived at the same time as another stack which filled up the remaining space.”

Let’s do this:

• When the user puts items in the chest, we send “uput N” from on_metadata_inventory_put.
• When the user takes items from the chest, we send “utake N” from on_metadata_inventory_take.
• Once the bug in minetest/minetest#6516 is fixed, we figure out if any rework is needed to handle the fallout such that accumulating “uput” and “utake” messages over time gives a perfect record of what’s in the chest.
• allow_metadata_inventory_put seems to be unnecessary, since Minetest itself never allows >max stacks to be built, so “uoverflow” can never happen and the entire function can be deleted.
• When a tube puts items in the chest, we send “tput D N” from tube.insert_object, where N is the portion of the stack that actually inserted (excluding any that are rejected due to simultaneous arrivals), and D is the side it came from (IMO this would be useful information to provide for some consumers).
• If we care about reporting items that tried but failed to be inserted, then we can both (1) send “toverflow D N” with N the full stack size from tube.can_insert if it’s going to return false, and also (2) send “toverflow D N” from tube.insert_object with N the leftover size if there are leftovers; IMO there is no reason to differentiate between these cases (though maybe tube.can_insert should start returning true if there’s room for any items at all, and split the stacks—perhaps that’s a topic for another day though).
• Implement tube.remove_items and send “ttake D N” from it.
• If we care about “full” messages, don’t split them into “ufull” and “tfull”. Just have “full itemname”, indicating that you can’t hold any more of that name. IMO neither stack size nor user-vs-tube are interesting for this message; the information that you can’t hold any more of an item type is independent of the question of where the items that happened to fill up the chest came from, and if you care about that, well that’s what “uput” and “tput” are for.
• After every removal (whether via tube or via user), if there’s nothing left in the chest, send “empty” with no parameters at all.

Thoughts?

[Desour]

@Hawk777

Pipes

You mean tubes. Pipeworks also has pipes but those transport water. Calling tubes pipes is like calling nodes blocks.

I have no idea what the difference between the “u”-prefixed and non-“u”-prefixed messages is supposed to be, other than that maybe “u” stands for “user” indicating that a player is doing the action rather than a tube?

I thought, "u" stands for "you". But “user” makes more sense… So, yes, always when the item is put or taken or whatever by a player, the "u"-prefix is used.

Actually this issue is just about moving the put part from can_insert to insert_object which shouldn't be too hard. It's not needed to change the chest entirely, but if you really want to since there could be much more informations, you shouldn't uses string as message type, instead use tables in which you eg. set action to "take", itemstring to "default:dirt 3", item to an item made to table and side to the input side (maybe as vector).

[Hawk777]

You mean tubes. Pipeworks also has pipes but those transport water. Calling tubes pipes is like calling nodes blocks.

Yes, I do. My apologies. Original comment modified accordingly to avoid future confusion.

Actually this issue is just about moving the put part from can_insert to insert_object which shouldn't be too hard.

I guess I saw there being three issues here:

1. Technical debt (i.e. not related to visible behaviour): can_insert is a silly place to send messages.
2. Bug (i.e. visible behaviour is incorrect): an extra “put” message is sent on player adding item to chest, with the same item type but stack size of 1.
3. Enhancement (i.e. visible behaviour could be improved): there’s some information in the messages that isn’t really interesting, and some information that could be interesting but isn’t included.

So I figured it makes no sense to fix 1 but not 2, fixing 2 means changing visible behaviour (and thus breaking API compatibility with anyone using Digilines chests already), so might as well fix 3 at the same time so there’s only one API break.

It's not needed to change the chest entirely, but if you really want to since there could be much more informations, you shouldn't uses string as message type, instead use tables

This sounds like a really good idea, though AFAICT we must be careful to avoid a couple of security holes:

1. It looks like none of digilines.receptor_send, digilines.transmit, nor the Luacontroller code make a copy of the message object. So we must be sure to pass a copy of the table, not the original, else we’ve just handed a live itemstack to untrusted code.
2. We must probably filter the table to remove functions. I’ve seen at least one case where an itemstack has a function in it. That particular one was harmless, but if some mod decided to put a dangerous function in its itemstack table, we wouldn’t want untrusted code to get a reference to that function.

What do you think?

[Desour]

I think…

• Of course don't send itemstacks via digiline.
• Tables can easily be copied with table.copy.
• Functions can't be dangerous, the luacontroller has a very secure environment.
• The function made out of a string (the code string in the textarea of lc) is called in protected mode (and also with a debug hook). Ergo I'm not sure if changing the event would affect other luacontrollers, but I don't think so. (Should maybe be tested.) If so, the event table can be table.copied here.
• The "idea" is not new, it's already done in eg. digiline_memory or pipeworks lua_tube.

[raymoo]

I think a function takes the global environment that it was constructed in, so it would still be able to access minetest stuff.

[Desour]

You are right. >_<
So, simply don't send functions.

[Hawk777]

Yeah, while getfenv isn’t in the whitelist (which means it’s not a problem to send some functions), my worry was that the function itself might do something bad, particularly when called with parameters it never expected. We can’t expect every mod in the world to be written as if its itemstacks are part of the attack surface. So let’s remove functions.

As for copying the table, I realize now I don’t think we actually need to do that, because the things we get handed as parameters to tube.insert_object and on_metadata_inventory_put are actually ItemStack C++ objects, so if we call stack:to_table(), we already have a fresh table which nobody else cares about, so it should be safe for that table to be handed as is to a Luacontroller. The only thing not copying the table means is that, if two Luacontrollers receive the table because they’re on the same network, AFAICT one could modify it and the other would see the changes, but that’s an inherent problem in handing tables to Luacontrollers which can only really sanely be fixed in Mesecons and IMO is lower priority to fix because it doesn’t represent a security hole.

I’ll work up a PR when I get some time, maybe tonight.

[Hawk777]

According to all the documentation I can find, an ItemStack object has the following things in it:

• meta, which is an ItemStackMetaRef, which is a MetaDataRef, which is a key-value container like a table, but its keys and values are all strings, so no harm can be done.
• metadata, which is always a string.
• count, always an integer.
• name, always a string.
• wear, always an integer.

So none of these are harmful. What I thought was a big problem is because I was looking at the Technic blue energy crystal. That doesn’t actually have a function in its itemstack table; rather, its metadata is a serialization of a table, which is a string that looks a bit like a function if you don’t look too closely. So it’s safe to just send the table directly, as long as it’s not a live table that anybody else is using.

[TangentFoxy]

MAYBE, when the inventory can no longer hold any more of a particular type of item.

I disagree very much on this being a maybe. IT IS NEEDED. Furthermore, a full on types should be added as well.

I thought, "u" stands for "you". But “user” makes more sense… So, yes, always when the item is put or taken or whatever by a player, the "u"-prefix is used.

Tubes are firing these events as well. :\

[Hawk777]

MAYBE, when the inventory can no longer hold any more of a particular type of item.

I disagree very much on this being a maybe. IT IS NEEDED. Furthermore, a full on types should be added as well.

Not sure what you mean by “a full on types should be added as well.” I thought that’s what I said: a message when the inventory can no longer hold any more of a particular type of item. Did you mean something different?

I thought, "u" stands for "you". But “user” makes more sense… So, yes, always when the item is put or taken or whatever by a player, the "u"-prefix is used.

Tubes are firing these events as well. :\

Per my proposed behaviour, they would not. u-prefixed events would be sent for user actions while t-prefixed events would be sent for tube actions.

[TangentFoxy]

Sorry, I mean when there are no more available slots, meaning if an item of a type differing from those existing in the chest already arrived, it would be rejected. I personally don't see the need to differentiate between player and tube actions, and especially don't see the need to create sperate events for this. Why not just have a property that defines initiator of the event?

…

On Fri, May 3, 2019, 7:38 PM Christopher Head ***@***.***> wrote: MAYBE, when the inventory can no longer hold any more of a particular type of item. I disagree very much on this being a maybe. IT IS NEEDED. Furthermore, a full on types should be added as well. Not sure what you mean by “a full on types should be added as well.” I thought that’s what I said: a message when the inventory can no longer hold any more *of a particular type of item*. Did you mean something different? I thought, "u" stands for "you". But “user” makes more sense… So, yes, always when the item is put or taken or whatever by a player, the "u"-prefix is used. Tubes are firing these events as well. :\ Per my proposed behaviour, they would not. u-prefixed events would be sent for user actions while t-prefixed events would be sent for tube actions. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#34 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADTE6P3PJ26ITHKQPSUO4WLPTTZLFANCNFSM4CLC2FLA> .

[Hawk777]

To begin with, GH-49 was supposed to close this issue, but I guess that didn’t happen for some reason. I think this issue should be closed, because most of what you want is already in master due to GH-49.

Sorry, I mean when there are no more available slots, meaning if an item of a type differing from those existing in the chest already arrived, it would be rejected.

Ah, OK, I get it. That could potentially be useful. Put more clearly: is there a completely empty slot somewhere? Then I think you need to know two things: you need to know when the last empty slot becomes nonempty (this is when items of types not already in the chest stop being accepted) and when some slot becomes empty (this is when items of types not already in the chest start being accepted again). To generalize, how about if each message also carried a value along with it which is how many empty slots there are? IMO that should be discussed as a separate issue, since, as I said, the original behaviour change for this issue was done in GH-49.

I personally don't see the need to differentiate between player and tube actions, and especially don't see the need to create sperate events for this. Why not just have a property that defines initiator of the event?

Well, if you’re suggesting to have a table property for the initiator, then you clearly do think it’s useful to differentiate, you just disagree on how that information should be communicated. Given that the u and t prefixes are already implemented, I’m not sure it makes sense to go back and rethink that now, ~two months after GH-49 was merged. Again, I think that deserves to be a separate issue if you want it changed.

[Hawk777]

@DS-Minetest do you have the ability to close this issue? I don’t actually know how to find out who has what permissions on a Github repo.

[Desour]

No, I don't.
@sofar Does.

[raymoo]

I can close it because I'm the OP. It looks like the OP issue has been implemented so I'll close.