Afforess / Factorio-Stdlib

Factorio Standard Library Project
ISC License
165 stars 49 forks source link

trains.lua - unusable in MP/other bugs #68

Closed Nexela closed 7 years ago

Nexela commented 7 years ago

First: And most important is the potential for desyncs Event.core_events.init, function() Trains._registry = create_train_registry() end

Trains._registry is not saved into the global table leading to desyncs. Player 1 creates game, init is called and scans trains now the Trains variable contains extra data Player 2 joins and since this is MP init is not triggered for player 2 and he does not have the extra data script does something with trains. 2 different things happen. game goes kaboom.

Haven't tested yet but replacing Trains._register with global._trains seems like the easiest approach

Second: function local function create_train_registry() Only scans surface 1
Should be changed to loop through all surfaces in case multiple surface trains are present.

Nexela commented 7 years ago

Third the events look like they are using the name and not the type. if that is the case this wouldn't work for mod added trains.

Afforess commented 7 years ago

@Nexela I moved the train registry to global and updated the events to filter on the locomotive type rather than diesel-locomotive entity name.

I think a bit more robust logic for surfaces is needed beyond what you've proposed, as surfaces could be created (or destroyed) mid-game. I need to look into this further.

However at a minimum, the critical issues should be resolved, if you want to test with the latest codebase.

Nexela commented 7 years ago

I wouldn't spend too much time on this until .15 It has recently been confirmed base game will have more train related events https://forums.factorio.com/viewtopic.php?f=28&t=44280&p=255850&hilit=train+event#p255850

Nexela commented 7 years ago

I think a bit more robust logic for surfaces is needed beyond what you've proposed, as surfaces could be created (or destroyed) mid-game. I need to look into this further.

That part is fine, factorio handles cleanup as long as .valid is checked before doing anything. The train will cease to exists if the surface does. Of course storing per surface would also add the overhead that now you might as well store per force, and open up a whole nother can of worms.

Nexela commented 7 years ago

global._registry seems too ambiguous As no other modules seem to use this global._trains would be a better choice

Afforess commented 7 years ago

@Nexela Fair point about the naming.

Afforess commented 7 years ago

I updated stdlib to register trains on any surfaces. I think that covers everything you've raised - if you find more, open another issue.