0ptera / Logistic-Train-Network

Factorio mod adding logistic network for trains.
Other
155 stars 55 forks source link

Extra remote calls and/or events related to dispatching #304

Closed Quezler closed 1 year ago

Quezler commented 1 year ago

TL;DR Currently the dispatcher table is only accessible at the end of each tick cycle, this is not enough for more specialized mods.

What ? Extra ways of accessing deliveries, on_dispatcher_updated_event only updates at the end of each tick cycle, so depending on the updated-per-tick settings & size of the megabase there could easily be 20 or more seconds in between each call, fine for mods like ltn gui, but not so much for mods that need to react in the same tick (or the one after) a train is dispatched.

WHAT do you suggest? What is your idea that should be changed? Explain in detail your idea.

and also asking about/for these 3 for an acquaintance:

Why ? It will make it easier for other mods to tightly integrate with ltn, currently one would have to do tons of hacks like reverse-engineering/reading the train's schedule when it's state changes or long polling the dispatcher_updated_event for delayed news.

Who ? i am not asking you to implement these yourself right now, i am more than willing to make a pull request for mine & the other 3 ones, but before i put any effort into writing a pull request i'd like to know of any of those 4 have a chance at succeeding or not.

0ptera commented 1 year ago

Events are broadcasts other mods can subscribe to. As far as I know requesting details for one specific train is not possible, since LTN can't receive data from subscribers.

on_delivery_pickup_complete, on_delivery_completed, on_delivery_failed already exist, see Interface documentation: https://forums.factorio.com/viewtopic.php?p=397844#p397844 or directly in https://github.com/Yousei9/Logistic-Train-Network/blob/master/script/interface.lua in case I forget to update the forum post.

on_delivery_created was removed as it served no purpose. LTN Manager had to subscribe to on_train_changed_state and compare train id to trains from on_dispatcher_updated.available_trains anyway.

I could add on_delivery_created back in to complete the set, but it's safer and faster to use base events and lookup tables wherever possible.

Quezler commented 1 year ago

As far as I know requesting details for one specific train is not possible, since LTN can't receive data from subscribers.

i was thinking more along the lines of: (specifically an interface call, not an event)

script.on_event(defines.events.on_train_schedule_changed, function(event)
  local delivery = remote.call("logistic-train-network", "get_delivery_by_train_id", event.train.id)
  if delivery ~= nil then -- is this train currently doing an LTN delivery, or is this a normal/manual train?
  ...
end)

(this code example would function along the lines of on_delivery_created, but this would allow you to check the delivery state (if any) of a specific train at any point in time)

lookup tables wherever possible

in one of my mods i tried re-caching the deliveries table each time the dispatcher updated, but it simply was too infrequent when the tick cycles had a lot to do. (since there is no bidirectional coupling in the passed table)

0ptera commented 1 year ago

I see, what I would do is add this:

on_delivery_created
Raised when dispatcher creates a delivery
-> Contains:
  event.train_id
  event.train
  event.shipment= { [item], count } }
Quezler commented 1 year ago

any chance the from & to station ids could also be included?

0ptera commented 1 year ago

Would be easy enough.

Quezler commented 1 year ago

I see, what I would do is add this:

this is a little open for interpretation, will you be implementing it yourself, or is it an invitation for me to pr it like that? 🤔

Quezler commented 1 year ago

👌

0ptera commented 1 year ago

Let me know if this works for you before I publish it on mod portal.

Dessix commented 1 year ago

Before it's finalized- I'd like to request the addition of unique identifiers (potentially just a simple incremental ID) to determine exactly which delivery was removed?

It also potentially allows for a more-granular alternative to on_dispatcher_updated which identifies only the changed entities, and prevents a lot of the clone overhead LuaRemote otherwise produces at larger scales.

0ptera commented 1 year ago

Deliveries are indexed by train.id.

Quezler commented 1 year ago

Let me know if this works for you before I publish it on mod portal.

allright, i'll have to reverse a bunch of commits own my own mod in order to re-implement support for dispatcher events & to tack on this new event in order to keep it updated, so it might take a few hours to a day. (and i wouldn't be able to immediately test it properly on my public facing server when the mod portal doesn't yet have that patch on it, but i suppose waiting makes sense from your perspective)

and also to address the elephant in the room: for my ltn addon mod i had to copy/mimic some parts, specifically:

the licence demands that i'd ask, even though this is low level code & there are only so many ways to do x 🤔

Dessix commented 1 year ago

Deliveries are indexed by train.id.

Is there an ordering guarantee that the new create event will always occur after any prior removal (completion/fail) event? If so, that will be plenty sufficient to build up a table without subscription to on_dispatcher_updated.

0ptera commented 1 year ago

the licence demands that i'd ask, even though this is low level code & there are only so many ways to do x 🤔

A good reason for only working with either very restrictive or total copyleft licenses. ;)

PS: I still don't see the point in glutenfree speaker. Buffer chests handle any lag from delivering with bots, belts or whatever in a simpler and unbeatable ups efficient manner.

Is there an ordering guarantee that the new create event will always occur after any prior removal (completion/fail) event? If so, that will be plenty sufficient to build up a table without subscription to on_dispatcher_updated.

on_stops_updated and on_dispatcher_updated are raised n ticks after dispatcher is done parsing all requests. All other events and alerts are raised within the same tick. I try to have them at the end of internal processing so other mods can't fuck things up.

Quezler commented 1 year ago

PS: I still don't see the point in glutenfree speaker. Buffer chests handle any lag from delivering with bots, belts or whatever in a simpler and unbeatable ups efficient manner.

it doesn't really have to do anything with lag, but when you have several trains back-to-back demanding speedy loading procedures it can save a few seconds of bots having to carry items from storage chests into the requester chests next to the track, that & being able to hover it and quickly see what the pending deliveries or pickups are, lamp only sums trains.

and it might also help with space exploration's space elevator, if you combine it with the content reader you could send the available items/fluids down the wire, and as soon as a station downwind expects a pickup you could use the signal from the announcer back up the space elevator to order a supply towards an intermediate train, though not designed for.

either way i do greatly appreciate that you've added the on_delivery_created event, opens up new possibilities for my mod(s) and possibly many others, even though announcer might never be a part of the ltn codebase itself. ^-^

0ptera commented 1 year ago

Not sure if you've seen my companion/API demo mod LTN Content Reader. It adds one combinator for Provided, Requested and currently delivering items/fluids per Network ID.

Quezler commented 1 year ago

if you combine it with the content reader

yes, i saw the content reader mod, and i suppose i could put the space elevator on its own network, but its still neat. ^-^

0ptera commented 1 year ago

I meant you could use it as tested guide for LTN API consumption.

Quezler commented 1 year ago

i did look at that mod's code before and it only used on_dispatcher_updated, but i needed events the moment a train started leaving the depo, hence on_delivery_created was required for my usecase, dispatcher updates were too slow :)

Quezler commented 1 year ago

Let me know if this works for you before I publish it on mod portal.

i've ran a small-scale test to test if it works (registering the event handlers, didn't re-refractor the rest of the mod yet):

Screenshot 2022-09-23 at 11 48 38 as far as i can tell it is functioning properly, is this enough evidence for 1.17.0 to be published on the mod portal?

0ptera commented 1 year ago

Let me know if this works for you before I publish it on mod portal.

i've ran a small-scale test to test if it works (registering the event handlers, didn't re-refractor the rest of the mod yet): as far as i can tell it is functioning properly, is this enough evidence for 1.17.0 to be published on the mod portal?

I was asking if the event had all information you need and if it passes valid LuaTrain and LuaEntity objects.

Quezler commented 1 year ago

I was asking if the event had all information you need and if it passes valid LuaTrain and LuaEntity objects.

oh, i kinda did the information part with the 👌 already, and train(id), from's id and to's id were the only thing i needed.

and as for trains now being passed as objects, those appear to work just fine as well: Screenshot 2022-09-23 at 12 08 43 (no nil's, and of course only one {} amongst many @'s because of the way factorio throttles duplicate messages)