Fix: only count distance traveled in vehicles for cargo payment

[TrueBrain]

Motivation / Problem

Given this map:

Where most stations are like this:

One can abuse the fact that cargo in stations are instantly (and free of charge) moved between tiles. What happens here is this:

• Cargo is picked up from the coal mine
• It is delivered in the first station. The first station is station-spread connected to the second.
• The second station is right next to the first station. So the truck goes from the first to the second station by driving 4 tiles (2 station tiles + 2 road tiles)
• This continues all the way down to close by the destination
• There it is picked up and delivered

Now something really funny happens here: the incoming per vehiclechain is the same as having a single truck drive all the way. As the loading/unloading takes about as long the travel time. But: the throughput increases by a lot. So you can make a lot more money this way .. about ten times as much in this setup.

This PR sets out to address that, and is basically #11274 implemented on top of #11281. It only contains a lot more documentation and description, to make much more clear what is going on. Also, I picked a few things differently. The end result is the same: after this PR, you can no longer misuse the free labour given to you by station workers.

teleport-profit.zip

Closes #11274.

Description

To combat this misuse, #11274 found a clever approach: create a vector of distance moved in vehicles, and use that for payment. This boils down to:

• Get the vector of each leg the cargo traveled
• Add those vectors up
• Get the Manhattan distance of this total vector
• This is your distance traveled.

This works really well to resolve this teleportation. However, there is an edgecase: big networks might take the cargo the wrong way first to bring it back in, using a lot of this station teleportation along the way. It can be that the vector ends up much larger than the actual distance between source and destination. For that reason, the smallest of these two is used: either the (Manhattan) size of the vector, or the Manhattan distance between source and destination.

To cheap out on variables (and struct-size), cargo_movement is a vector in station, but it is a virtual point when in a vehicle. As we have to calculate the vector of a leg, which is A - B, we add A to the vector when leaving a station, and remove B when entering a station. That way, cargo_movement is only a vector when in a station. When it is in a vehicle, it doesn't actually have a meaning.

The variable in_vehicle is introduced purely to run assert on. As such, it is only compiled in when WITH_ASSERT is enabled. It is not meant to be used otherwise. It is just there to ensure cargo_movement is in the right state.

For loading older savegames, choices have to be made. I picked one, but you sure can have another. I am open for that dialog.

Although 5 bytes are added to CargoPacket, the actual size (on a 64-bit machine) isn't changed, as there were 5 bytes free to be used: 27 used. Now 32 used. As we still need access to source_xy, so we can calculate the Manhattan distance between source and destination, cargo_movement is added as extra field.

Limitations

None that I know of, although this could use some extra testing etc. I did a lot of testing with all kind of different savegames, and all seem to be good. The attached savegame also shows the teleport problem is solved.

Checklist for review

Some things are not automated, and forgotten often. This list is a reminder for the reviewers.

• The bug fix is important enough to be backported? (label: 'backport requested')
• This PR touches english.txt or translations? Check the guidelines
• This PR affects the save game format? (label 'savegame upgrade')
• This PR affects the GS/AI API? (label 'needs review: Script API')
  • ai_changelog.hpp, game_changelog.hpp need updating.
  • The compatibility wrappers (compat_*.nut) need updating.
• This PR affects the NewGRF API? (label 'needs review: NewGRF')
  • newgrf_debug_data.h may need updating.
  • PR must be added to API tracker

[ldpl]

While I'm still not convinced that extending travel distance with spreading has any practical exploits as you still need to run vehicle the whole way and having transfers makes it much worse that running directly it's still have some niche uses in theory. So fixing it preemptively is probably fine as that doesn't seem to add that much overhead. On the other hand, typical big mp game seems to have 10-30k cargo packets, adding 40-120k of savegame data (pre commression) is not critical but still somewhat significant, so may be worth postponing this addition until it's clear how impactful this theoretical exploit actually is.

[TrueBrain]

While I'm still not convinced that extending travel distance with spreading has any practical exploits as you still need to run vehicle the whole way and having transfers makes it much worse that running directly it's still have some niche uses in theory. So fixing it preemptively is probably fine as that doesn't seem to add that much overhead. On the other hand, typical big mp game seems to have 10-30k cargo packets, adding 40-120k of savegame data (pre commression) is not critical but still somewhat significant, so may be worth postponing this addition until it's clear how impactful this theoretical exploit actually is.

That was a bit rambling, but I think I get what you mean. 120KiB on a savegame with 30k cargopackets really is not significant in any terms. And it is not as niche as you make it out. It happens really easily if you have a few proper transfer stations of max size around your map, which you use to navigate cargo (and especially passengers).

So yeah, given it is a very low impact (to address the issue), and keeps things fair both ways, let's go with it. No need to only have fairness work one way :)

(further more, I noticed it really helped with debugging, so it is also pretty nice for that).

[PeterN]

I suppose you can 'exploit' this by making further-than-necessary stops/platforms, but that's not much different than the current version of making the station signs as far apart as possible...

[frosch123]

int16_t too small?

If I read this right, CargoPacket::travelled accumulates the number of tiles travelled (signed). It uses int16_t for that.
This will at least not work for the huge-map patches.

How about inverting the accumulation? Sum the distance travelled within stations: tile_for_loading - tile_of_last_unload.

• The result should be the same: destination - source = travel_vector_in_train + travel_vector_in_station
• Just the numbers are smaller.

Train reversing

I am not quite sure how reversing trains affect this. Do long trains have to travel less distance than short trains, if they reverse at terminal stations?
Should the "travel distance" exclude distance "travelled" by reversing trains?

Off-topic

Out of scope for this PR, but I'll write it anyway:

• One could also account for "distance(producer, first-loading-tile)" and "distance(final-unloading-tile, accepter)".
• This would allow adding fees / negative payment / ..., if you build stations far from industries.
• Though town producers/accepters have no specific tile.

[TrueBrain]

I suppose you can 'exploit' this by making further-than-necessary stops/platforms, but that's not much different than the current version of making the station signs as far apart as possible...

Yup.

[TrueBrain]

int16_t too small?

If I read this right, CargoPacket::travelled accumulates the number of tiles travelled (signed). It uses int16_t for that. This will at least not work for the huge-map patches.

How about inverting the accumulation? Sum the distance travelled within stations: tile_for_loading - tile_of_last_unload.

* The result should be the same: `destination - source = travel_vector_in_train + travel_vector_in_station`

* Just the numbers are smaller.

Train reversing

I am not quite sure how reversing trains affect this. Do long trains have to travel less distance than short trains, if they reverse at terminal stations? Should the "travel distance" exclude distance "travelled" by reversing trains?

Off-topic

Out of scope for this PR, but I'll write it anyway:

* One could also account for "distance(producer, first-loading-tile)" and "distance(final-unloading-tile, accepter)".

* This would allow adding fees / negative payment / ..., if you build stations far from industries.

* Though town producers/accepters have no specific tile.

int16_t: I did look at this, but can't remember my conclusion. Is the length of the vector in stations actually the invert of the length of the vector travelled. Will look into this again (and write down the conclusion :P)
PS: it is a vector, not a distance traveled. It is distance and direction.

Considering reversing a teleport would be the next step I guess. But I would say, not for this PR.

As for your offtopic, yes, I went through the same motions. Also it isn't fully trivial which Station accepted your cargo in the current code .. requires getting some values in other layers then they life now. But I do agree with the idea, and 2TallTyler had similar remarks. But yeah, not this PR :)

[ldpl]

If I read this right, CargoPacket::travelled accumulates the number of tiles travelled (signed). It uses int16_t for that. This will at least not work for the huge-map patches.

How about inverting the accumulation? Sum the distance travelled within stations: tile_for_loading - tile_of_last_unload.

As transfers aren't limited it seems possible to overflow int16 in both travelled and teleported vector cases. But overflowing travelled vector is pointless as you just decrease the distance you get paid for. Not that it matters as it requires transporting stuff far beoyond the map size and it will be capped anyway. Overflowing teleported vector is actually more promising for the exploit as you can get 64 teleported tiles for 1 tile of movement. Though after 512 transfers it probably doesn't matter either. And considering how much effort it would require to overflow I don't think any additional checks are even necessary here.
Patches that allow more than int16 map side would need to use larger type but don't think that's something vanilla should worry about.

I am not quite sure how reversing trains affect this. Do long trains have to travel less distance than short trains, if they reverse at terminal stations? Should the "travel distance" exclude distance "travelled" by reversing trains?

As I mentioned in #11274 reversing still gives some "unfair" money but it's less that could be done prior to this with sign manipulation. And as endpoint check prevents accumulating reversing movement endlessly anyway it doesn't seem like any significant exploiting is possible here, there are better ways to use long trains.

* One could also account for "distance(producer, first-loading-tile)" and "distance(final-unloading-tile, accepter)".

* This would allow adding fees / negative payment / ..., if you build stations far from industries.

Not counting distance between industries and loading tile is a good thing imo, as allows more flexibility with station placement. Gaining some distance with extended station placement is of no concern as usually players aren't forced to use specific industries and can just chose ones that have required distance in between. Same with teleport fees. I just don't see how forcing players into a smaller area would add anything to the game. Station catchment already is just a few tiles anyway.

[TrueBrain]

Did some more testing for the different vehicle-types (RV, Train, Ship and Aircraft). This now all works how I expect it, although I didn't test really old savegames.

What you see is for vehicles that are already on their way, that their payout is identical to before this PR (which is expected). Vehicles that are still loading, have their payout altered. Especially noticable when using transfer stations, ofc.

So all seems to work as advertised :)