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

[Bug]: CmdFoundTown may cause multiplayer desyncs due to not initialising Town::stations_near #9407

Closed
JGRennison opened this issue Jun 30, 2021 · 2 comments
Labels
Milestone

Comments

@JGRennison
Copy link
Contributor

@JGRennison JGRennison commented Jun 30, 2021

Version of OpenTTD

master

Expected result

No multiplayer desyncs or incorrect caches

Actual result

CmdFoundTown may be called by the player in game, in the scenario editor, or by a GS.

In the event that a town is founded in range of existing stations (i.e. where one or more new town house tiles are within the station's catchment area), the town's stations_near will be incorrectly empty, instead of containing the stations within range.
In the event that this occurs in multiplayer, players which join after the town founding event will not have an incorrect cache and this may result in a state divergence. Most notably this is used by StationFinder::GetStations.

Steps to reproduce

  1. Found a town near to an existing station
  2. Note that Town::stations_near is incorrect, CheckCaches can be used to verify this
JGRennison added a commit to JGRennison/OpenTTD-patches that referenced this issue Jun 30, 2021
@TrueBrain TrueBrain added the bug label Jun 30, 2021
@glx22
Copy link
Contributor

@glx22 glx22 commented Jul 7, 2021

The issue is

OpenTTD/src/town_cmd.cpp

Lines 2313 to 2318 in a7fabe4

if (!_generating_world) {
ForAllStationsAroundTiles(TileArea(t, (size & BUILDING_2_TILES_X) ? 2 : 1, (size & BUILDING_2_TILES_Y) ? 2 : 1), [town](Station *st, TileIndex tile) {
town->stations_near.insert(st);
return true;
});
}
in MakeTownHouse() (added with 8b1b3fd) being skipped because
Backup<bool> old_generating_world(_generating_world, true, FILE_LINE);
in CmdFoundTown() (added with ffec9b4 present since at least r1)

Loading

@LordAro LordAro added this to the 12.0 milestone Aug 14, 2021
TrueBrain added a commit to TrueBrain/OpenTTD that referenced this issue Aug 31, 2021
"stations_near" wasn't updated when founding a town near
a station. As this variable is not saved, any client joining
after the town is founded has a different value for
"stations_near", potentially causing desyncs.
TrueBrain added a commit to TrueBrain/OpenTTD that referenced this issue Aug 31, 2021
"stations_near" wasn't updated when founding a town near
a station. As this variable is not saved, any client joining
after the town is founded has a different value for
"stations_near", potentially causing desyncs.
@TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented Aug 31, 2021

Tnx for the analysis @glx22 , that really helped as lot :) @JGRennison : I want for a slightly different solution than your commit, as I explain in #9526. Let me know if you foresee any issue with that.

Loading

TrueBrain added a commit to TrueBrain/OpenTTD that referenced this issue Aug 31, 2021
"stations_near" wasn't updated when founding a town near
a station. As this variable is not saved, any client joining
after the town is founded has a different value for
"stations_near", potentially causing desyncs.
TrueBrain added a commit to TrueBrain/OpenTTD that referenced this issue Aug 31, 2021
"stations_near" wasn't updated when founding a town near
a station. As this variable is not saved, any client joining
after the town is founded has a different value for
"stations_near", potentially causing desyncs.

As the intention of this if() statement was to skip an expensive
calculation when there are clearly no stations, better to move
that check inside the function, so other places also enjoy
the speedup.
@TrueBrain TrueBrain closed this in 2c05412 Aug 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants