-
Notifications
You must be signed in to change notification settings - Fork 36
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
Change digiline chest to send message on insertion, rather than in can_insert #34
Comments
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 behaviourThis 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. PlayersWhen a player successfully adds a stack to a chest, we get:
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 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. TubesWhen a tube adds a stack successfully (it all fits), we get a “put N” from 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 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 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 When a tube filter takes something out, it acts exactly like a player doing the same thing. ObservationsThis 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 ProposalWhat the user of a Digilines chest cares about is:
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:
Thoughts? |
You mean tubes. Pipeworks also has pipes but those transport water. Calling tubes pipes is like calling nodes blocks.
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 |
Yes, I do. My apologies. Original comment modified accordingly to avoid future confusion.
I guess I saw there being three issues here:
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.
This sounds like a really good idea, though AFAICT we must be careful to avoid a couple of security holes:
What do you think? |
I think…
|
I think a function takes the global environment that it was constructed in, so it would still be able to access minetest stuff. |
You are right. >_< |
Yeah, while 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 I’ll work up a PR when I get some time, maybe tonight. |
According to all the documentation I can find, an
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 |
I disagree very much on this being a maybe. IT IS NEEDED. Furthermore, a full on types should be added as well.
Tubes are firing these events 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?
Per my proposed behaviour, they would not. |
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>
.
|
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.
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.
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 |
@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. |
No, I don't. |
I can close it because I'm the OP. It looks like the OP issue has been implemented so I'll close. |
Because the item is recorded as "put" over digiline when can_insert is called, it is impossible to tell after receiving the message when the item is actually in the chest. This is a problem when using with pipeworks if you need to activate an injector that removes things from the chest, because it may fire before the item is actually in there.
The message should be sent in insert_object rather than can_insert.
The text was updated successfully, but these errors were encountered: