0ptera / Logistic-Train-Network

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

Add remote interfaces and corresponding event data to support cross surface deliveries #308

Closed harag-on-steam closed 1 year ago

harag-on-steam commented 1 year ago

Mods like Space Exploration 0.6 allow trains to travel between surfaces. This is done by copy/deleting carriages from one surface to another while a train travels through a space elevator. This concept could be equally applied to other scenarios like sub-surface tunnels or train stations inside Factorissimo buildings.

Goals & Non-Goals

This PR is my attempt to add a minimal set of features to make LTN aware of connected surfaces. I have also added these thoughts as documentation in interface.lua.

  1. Allow to register bi-directional, non-transitive surface connections through a remote interface
  2. Consider providers from connected surfaces in the dispatcher
  3. Add the stop entities and surface connection data relevant to a delivery to its on_delivery_created on_dispatcher_updated event to allow interface users to modify the train schedule to actually make the surface transition.
  4. Provide a remote interface to notify LTN that a train was replaced by another train, possibly re-assigning the delivery in LTN. This will also add missing requester/provider temp-stops because the Factorio engine doesn't allow to add temp-stops on another surface to a train's schedule.

There are also some non-goals:

  1. No deep integration with any particular mod's form of cross-surface travel. No knowledge about the behavior of that mod's entities.
  2. Transitive connections are out-of-scope. This would require graphs and path-finding which seems way more effort than it's worth. Still, interface users can handle this themselves if they also register a connection A->C if there are connections A->B and B->C.
  3. Trains are only sourced from the provider- not the requester-surface. That avoids changes to getFreeTrain and its closest-train-to-provider algorithm and is in line with (2). The caveat is that interface users need to return trains to their origin surface to avoid creating an imbalance in the train pools on each surface.
  4. Additional surface connection data is only provided in on_delivery_created, nowhere else. That might make it harder for interface users to add their own temp stops but keeps the additional data that is passed around to a minimum.

Surface Connections

I have chosen to represent surface connections as tuples

(entity1 : LuaEntity, entity2 : LuaEntity, network_id : uint)

because I think tying the connection to two entities is a robust way to do cleanup: If the mod owning the entities and making the connection is uninstalled, normal Factorio save-game migration will clean up the entities and LTN can drop the corresponding surface-connection on-the-fly. The same goes for unfriendly (editor, etc.) removal of entities. Also, the entity owner is forced to deal with this in the same way, keeping assumptions about existing connections in-sync.

This also means there can be more than one connection between a single pair of surfaces and it is convenient to do that. For example, SE allows to have several elevators connecting a planet to its orbit and the natural thing to do to sever a singular connection is to simply deconstruct the elevator, no additional maintenance (=source of errors) needed.

I added the network_id as a player convenience to control which LTN train can go trough where and it's so cheap to apply there's no good reason not to. If a mod decides that it won't allow players to control this, it can simply register every connection with network_id = -1

Performance

I strove to keep the impact this has on games that never add a surface connection to a minimum. That's why I re-ordered the surface check to the end of the provider match in ProcessDelivery and the only additional work it does without any surface connections is the lookup of the surface-pair key.

Adding entities to on_delivery_created is simply a necessity to allow the interface user to do distance-, surface- and force-checks in order to complete the train schedule. And the additional processing in ReassignDelivery is the price to pay for how Factorio requires us to move trains from one surface to another.

I''ve been play-testing this for a while now in combination with se-ltn-glue and other mods that interface with LTN like LTN manager and LTN TCS refuel. The savegame has two large bases on the ground and in orbit and am quite happy with the performance. I also did some corner case-testing on a small map with two elevators to see if there are any issues regarding elevator removal, differing network_ids, temp-stop creation or delivery loss while switching surfaces.

I'm considering this PR as a (working) starting point for discussion. For example, this could probably use some more debug logs like the rest of LTN has them and I'm unsure if some form of last-resort, full removal of all surface connections should be available to players. And of course if anything of the above seems like a bad idea I'd like to talk about it.

I should also mention @robot256 who I had a lot of discussions with and who added a lot of ideas.

Copyright Waiver

As the LTN license requests I hereby transfer ownership of these contributions to Optera.

0ptera commented 1 year ago

I stopped playing Factorio 2 years ago and only maintain my mods with bugfixes and minor features. To be honest I'm not sure I'll find the motivation to test and implement those huge changes to LTN logic.

One problem I see with this implementation is stutter from using on_delivery_created as hook for other mods. on_delivery_created is raised on the same tick as dispatcher and should be reserved for light weight mods with minimal footprint as it runs in the same tick as the already calculation heavy dispatcher. I'd force mods into using on_dispatcher_updated as that event has it's own tick where LTN does no processing.

harag-on-steam commented 1 year ago

test and implement those huge changes to LTN logic.

Is it really that much of a change? I specifically trimmed the feature set so that the core delivery processing basically changed from surface == surface to a function call. The rest happens on demand to react to changes in the world. And temp-stop handling just needs to change because the engine has restrictions there.

One problem I see with this implementation is stutter from using on_delivery_created as hook for other mods.

So you'd just put a flag is_cross_surface in on_delivery_created to notify subscribers that the next on_dispatcher_updated will have the necessary surface connection data to modify the schedule? I can certainly try that in a second branch.

I'm a little worried that mods need to subscribe to a data dump that has quite some deserialization overhead to get information about the single delivery they know they need to do something with. It would solve non-goal (4), though. I'm also a little worried that all dependent mods then do their work in on_dispatcher_updated. I think LTN manager already does a lot of processing here.

I also thought about how that would affect the train pathfinder, if schedules changed a few ticks later, but I think cross-surface deliveries are infrequent enough that this is probably just noise among the normal re-pathing.

0ptera commented 1 year ago

Is it really that much of a change?

I dread any change to dispatcher that requires testing functions and edge cases....

I'm a little worried that mods need to subscribe to a data dump that has quite some deserialization overhead to get information

I'm more worried about mods running only from on_delivery_created making all dispatcher ticks even more process intensive.

I sent you a discord friend request, would be quicker than a conversation in a github pull request.

0ptera commented 1 year ago

Some questions I had while rewriting API documentation: Are connectors normal train stops? -- Both entities must be from the same force as the requester and the provider for deliveries to go through the connection. Do the connectors also have to be of the same force as depot, provider, requester and train? -- Trains will be sourced from the provider surface only and should return to it after leaving the requester. I read that as Depots must be on Provider surface, correct?

please test the SE side of my changes.

harag-on-steam commented 1 year ago

Are connectors normal train stops?

No, the idea is to allow anything that is a LuaEntity and has a force. LTN will not be responsible for the actual surface transition so it doesn't need to make any more assumptions.

Both entities must be from the same force as the requester and the provider for deliveries to go through the connection. Do the connectors also have to be of the same force as depot, provider, requester and train?

Yes, LTN is strict here which is why I saw no benefit in being more lenient.

Trains will be sourced from the provider surface only and should return to it after leaving the requester. I read that as Depots must be on Provider surface, correct?

Yes, non-goal 3) is the reason for this.

DrParanoia commented 1 year ago

Wow, guys, you are making great progress! Just wanted to say, that I really appreciate the effort @Yousei9 despite not playing anymore, thanks!

Also, @harag-on-steam, huge thanks for taking the initiative for creating this possibility!