Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

support stations with multiple stops with the same name #164

Closed
wants to merge 7 commits into from

Conversation

Projects
None yet
3 participants
@mrvn
Copy link
Contributor

commented Mar 29, 2019

This patch allows stations with the same name to be used if one of the following is true:

  1. all stations are depots
  2. all stations are connected by a green wire
  3. all stations are connected by a red wire

That means the following still gives the purple error light:

  1. mixing depots and non-depots with the same name
  2. naming 2 random stations the same without connecting them with a wire
  3. blueprinting stations with names (unless you place them so they are connected by wire)

What is allowed:

  1. Single stations without any wires.
  2. Stations connected by green wire but red wire does something else (or vice versa)
  3. Depots with the same name without any wires.

The patch should not change any correct LTN setup. And it's unlikely that a LTN setup with broken (same name) stations accidentally becomes valid.
LTN-multiple.zip

Show resolved Hide resolved script/stop-update.lua Outdated
Show resolved Hide resolved script/stop-update.lua Outdated
Show resolved Hide resolved script/stop-update.lua Outdated
Show resolved Hide resolved script/stop-update.lua Outdated
Show resolved Hide resolved script/stop-update.lua Outdated
@mrvn

This comment has been minimized.

Copy link
Contributor Author

commented Apr 2, 2019

Didn't know about ipairs being bad, will change that and the other issues.

For setLamp there are two cases: errors need to go to all stops, train events should go to one stop. Will have to look into that.

I didn't benchmark the signals. The code only runs for stations with the same name and now ignores wires on depots. So there shouldn't be a performance loss for exiting setups at least. I see that you loop over all stations so maybe getting the signals once per loop would be better. If you have many stations with the same name getting the signals every time is O(n^2) work. My usual use case is n=2 though so not noticeable for me. I will have to make a test case with say 50 stations with the same name. That should stress the code.

@mrvn mrvn force-pushed the mrvn:feature-multi-stop-station branch from ea8e25d to 70457b4 Apr 6, 2019

@Yousei9

This comment has been minimized.

Copy link
Owner

commented Apr 6, 2019

Quite a lot of changes, even a whole new module.
Thinking through all your changes to my logic might take me longer than writing it myself.

@mrvn mrvn force-pushed the mrvn:feature-multi-stop-station branch from 70457b4 to 9922dc0 Apr 6, 2019

@mrvn mrvn force-pushed the mrvn:feature-multi-stop-station branch 2 times, most recently from ee415ba to d5e6da9 Apr 6, 2019

@mrvn mrvn force-pushed the mrvn:feature-multi-stop-station branch from d5e6da9 to 9372548 Apr 6, 2019

@mrvn

This comment has been minimized.

Copy link
Contributor Author

commented Apr 6, 2019

The Station module contains all the stuff that works on the station name instead of individual stops. Every stop has a station and a station can have many stops. Deliveries are from station to station. This used to be in stop.activeDeliveries.

The biggest change is probably that I used sets (tbl[k] = true) instead of an arrays (table.insert(tbl, k)) for a few things. This allows faster insert, remove and lookup.

There are some new cases for lamp colors + count:

  1. 2 new errors, still pink but the count is different. Should they have new colors?
  2. A blue LED gives the count of the stopped train + any trains on the way to the station
  3. A yellow LED gives the number of trains on the way to the station
  4. A stop can be green if all trains for that station are parked at other stops of that station

This should only affect new stations with multiple stops. I tried to not change anything for existing setups with one stop per station.

The code now passed all my test setups. So I'm going to use this in a real game to see if any other errors show up.

@Yousei9

This comment has been minimized.

Copy link
Owner

commented Apr 6, 2019

Thanks for explanation, could you comment your functions and changes. So I can have a rough sense of what that part is doing and why.
I know I don't do that in my own code most of the time since i tend to remember it, but you'd make life for future me a lot easier.

  1. that's fine as is, base factorio only has so many sensible colors to use for error states. pink for name error and code for specifics is what I'd do myself
  2. it's only blue on the stop with the train and yellow on other stops of a station?
  3. same as before i guess?
  4. makes sense, nothing is being delivered and the train is parked somewhere else
@Yousei9

Yousei9 approved these changes Apr 6, 2019

@mrvn

This comment has been minimized.

Copy link
Contributor Author

commented Apr 6, 2019

concerning 2: it's blue only for a stop with a parked train. For the other stops it's either 3 (yellow) or 4 (green). Overall it can be blue/green or blue/yellow.

@Yousei9

This comment has been minimized.

Copy link
Owner

commented Apr 6, 2019

On very busy stations I guess it's also possible to have multiple blues with a couple of yellow or green ones.

@Yousei9

This comment has been minimized.

Copy link
Owner

commented Apr 7, 2019

Please update this pull to current dev branch.

@eduran84

This comment has been minimized.

Copy link
Contributor

commented Apr 9, 2019

Once you have finalized the changes, please give me an advance warning before releasing it. I suspect that having multiple stations sharing a name will require quite a few adjustments to LTNT.

@Yousei9

This comment has been minimized.

Copy link
Owner

commented Apr 9, 2019

Certainly.
This is a massive change to how LTN works.

@Yousei9 Yousei9 added this to the 1.11.0 milestone Apr 9, 2019

@Yousei9 Yousei9 changed the base branch from master to dev Apr 16, 2019

@Yousei9 Yousei9 closed this Apr 21, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.