0ptera / Logistic-Train-Network

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

Move `train.schedule =` to below dispatcher table changes #319

Closed Quezler closed 1 year ago

Quezler commented 1 year ago

Before 1.18.x one had to rely on the on_delivery_created event to know when a train has been sent out, it was not possible to rely just on the basegame on_train_schedule_changed event since that gets called right when train.schedule changed and the event wouldn't be sent out until after this large function was done.

Now with the removal of that event since 1.18.x it seems logical (to me) that if you wish to know if a train that had just had its schedule set was done so by LTN to call this within the on_train_schedule_changed event: script.on_event(remote.call("logistic-train-network", "get_next_logistic_stop"), handler)

Without the change this pull request suggests, any mod would have to store the train from the on_train_schedule_changed event in a table with save/load support for just 1 tick since the remote call wouldn't have the most recent changes to global.Dispatcher.Deliveries yet.

Setting the schedule at the end of this function (and thus below global.Dispatcher.Deliveries[selectedTrain.id] =) would remove a lot of complexity from mods that depend on knowing ASAP if a train is working for LTN or not.

Waiting on the next on_dispatcher_updated is not an option since the time in between grows larger and larger for megabases (unless of course those players finetune the map option), but in terms of ASAP it simply is not.

So in short, please move train.schedule = to below the global.Dispatcher[ changes, wouldn't mind if its above or below the set lamps 🙂

0ptera commented 1 year ago

Setting the schedule after updating internal storage of the train makes no sense.

LTN only exposes data through remote calls on_dispatcher_updated and on_stops_updated raised at least 1 tick after all deliveries are created. If you want to know what schedules LTN created use on_dispatcher_updated.new_deliveries.

Quezler commented 1 year ago

As i mentioned, on_dispatcher_updated allows too much time between actual dispatching and receiving the event.

Thank you for reviewing this, but it looks like i'll just be making a small library mod that does on_delivery_created emulation and has its own tick delayed global tasks table, a little unfortunate that this line couldn't just be moved down.

0ptera commented 1 year ago

on_dispatcher_updated allows too much time between actual dispatching and receiving the event.

In a normal setup LTN events are raised well within 60 ticks (1s) after setting the schedule. Most times even within 10 ticks.

What are you trying to do that requires real time updates? Announcers or thought bubbles certainly don't need real time updates. Remember every circuit component has a 1 tick delay between input and output.

PS: This is not Reddit or TickTock, reacting with downvote emotes to my post only effects how negatively I see this request.

Quezler commented 1 year ago

Announcers or thought bubbles certainly don't need real time updates.

Strictly speaking: no they don't, but it looks really bad if they're not in sync with when the lamp's color changes.

0ptera commented 1 year ago

Sorry, but my priority is preventing lag spikes.

If that means some visuals are updated a few ticks later that's a very small price to pay.

Quezler commented 1 year ago

I mean, moving down the train.schedule = part a little lower within the same function doesn't impact anyone by default, only mods that specifically request the next logistic stop via remote.call(, its not like it fires an unused event again. 🤔

0ptera commented 1 year ago

Again you moved assigning train.schedule below storing train in global.dispatcher.

Quezler commented 1 year ago

that's like my entire point? so a script.call( called from within on_train_schedule_changed returns accurate data:

function handler.on_train_schedule_changed(event)
  print('on_train_schedule_changed')
  print(remote.call("logistic-train-network", "get_next_logistic_stop", event.train))
end

+

  ...
  schedule.records[#schedule.records + 1] = NewScheduleRecord(to, "item_count", "=", loadingList, 0)

  -- log("DEBUG: schedule = "..serpent.block(schedule))
  selectedTrain.schedule = schedule
  ...

=

on_train_schedule_changed
nil

whilst

function handler.on_train_schedule_changed(event)
  print('on_train_schedule_changed')
  print(remote.call("logistic-train-network", "get_next_logistic_stop", event.train))
end

+

  ...
  global.Dispatcher.availableTrains[selectedTrain.id] = nil
  selectedTrain.schedule = schedule
  ...

=

on_train_schedule_changed
3   80582   provider
0ptera commented 1 year ago

Thanks for the code example. That is indeed a bug where GetNextLogisticStop requires global.Dispatcher.Deliveries set before on_train_schedule_changed is raised.

Filling globals before setting the schedule shouldn't cause issues as train is an object reference. (I still have a hard time figuring out when lua uses pointers instead of values.)