minetest-mods / digilines

Digilines is a mod for minetest which adds data bus wires
Other
40 stars 38 forks source link

Implement batched messages for Digiline Chests #87

Open andriyndev opened 3 months ago

andriyndev commented 3 months ago

This patch implements wrapping signals, which were send in a very short interval (<= 0.1 seconds), into batches. This is needed to prevent Lua Controller from burning when using batch move (performed by selecting an item, and clicking on an item of the same type with Shift pressed). This causes the chest to send Digiline signals for each added stack within milliseconds that will cause the connected Lua controller to burn, and since Digiline chests can be accessed by any player, this gives any player possibility to burn any Lua controller connected to a Digiline chest. This patch implements sending several Digiline chest events in a single Digiline signal if the previous Digiline signal was sent or included into the pending batch less than 0.1 seconds ago. Thus, in case of using batch move, the first chest event is sent as a separate Digiline signal (since we cannot know in advance about the upcoming events), and the next events are included into a batch.

andriyndev commented 3 months ago

I didn't edit README.inventory since it's already outdated

SmallJoker commented 3 months ago
  1. How exactly can I reproduce the issue in current master? Would you please be so nice to share a screenshot, and Lua Controller code if there any any necessary?
  2. Would you please be so nice to at least update the README a little bit? I think there's barely anyone who is currently familiar with the code, let alone with what you just implemented.
andriyndev commented 3 months ago

How exactly can I reproduce the issue in current master? Would you please be so nice to share a screenshot, and Lua Controller code if there any any necessary?

I described how to reproduce the issue in comment. But here is also a video with demonstration of the bug: https://youtu.be/lDgaIYBr0ZA

Would you please be so nice to at least update the README a little bit? I think there's barely anyone who is currently familiar with the code, let alone with what you just implemented.

This patch adds a new digiline signal for the chest in the format {action = "batch", signals = {<signals in the batch>}}. I used the name action for uniformity with other signals, and <signals in the batch> is just an array of the batched signals in their format. However the final format of the batch signal is a matter of discussion as well as other ways of fixing the bug. Regarding updating README a little bit, I see no sense in it because it currently doesn't mention Digiline chests at all, while README.inventory, which describes the Digiline chests, already contains outdated information about the format of the signals, which doesn't correspond to the current reality. As an option - suspend the patch until README.inventory is updated, and then I'll happily update README.inventory according to my changes.

andriyndev commented 3 months ago

I think I also need to restrict the maximum size of a batch to avoid its growth up to infinity under particular conditions.

UPD. Done

andriyndev commented 3 months ago

To do: handle the situation of multiple channels UPD. Decided to leave as is for now because multiple channel means that the channel of the chest itself was changed which is extremely unlikely, and sending all the signals to the new channel is likely not an error. As another option - send the batch right on changing the channel name if it's not empty.

andriyndev commented 1 month ago

We use last_message_time_for_chest to store time, at which the last message was sent, for each chest. A possible problem is that it's cleared only when its batch is sent, and if a chest doesn't send batched signals but sends other ones, its related value will always be present in last_message_time_for_chest. I don't know whether it's bad but in any case creating a separate timer for this purpose would be an overhead (IMHO), and I don't see other elegant solution except of iterating over the whole table on some regular events. At the same time, clearing it right after sending a batch might be also a bad idea since in this case if we need to send another message right after sending the batch, it won't be included into the next one.

SmallJoker commented 1 month ago

At the same time, clearing [the time] right after sending a batch might be also a bad idea since in this case if we need to send another message right after sending the batch, it won't be included into the next one.

I see. You'd generally want to send single messages immediately and batch afterwards to avoid delays. I currently do not know of any solution that wouldn't involve a periodic "garbage collector" by iterating. You could add a TODO comment in the code for now if you think that it does not pose an issue. Unfortunately I do not know either how much of a problem this can be.

andriyndev commented 1 month ago

I see. You'd generally want to send single messages immediately and batch afterwards to avoid delays. I currently do not know of any solution that wouldn't involve a periodic "garbage collector" by iterating. You could add a TODO comment in the code for now if you think that it does not pose an issue. Unfortunately I do not know either how much of a problem this can be.

I think I'll try to add a timer which will perform garbage collection in some periods of time such as every 1 minute or 1 hour, and it will start on insertion if there are no active timers, or restart if the table is not empty at the moment of expiration.

andriyndev commented 1 month ago

By the way, I call minetest.load_area(pos) if the area at pos is unloaded. But I have doubts regarding whether calling it once will be reliable.