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

Fix #9407: desync when founding a town nearby a station #9526

Merged
merged 1 commit into from Aug 31, 2021

Conversation

TrueBrain
Copy link
Member

Motivation / Problem

When a town is founded during a game, we misuse _generating_world to bypass a bunch of validation that normally should happen when towns grow etc.
This is also done to skip updating stations_near, as it is a pretty expensive operation, and when generating a new map there is of course never a station around a town to start with. So this is safe to skip in that scenario. Not when founding a new town.

As I wrote in the commit message:

"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.

This was reported in #9407.

Description

JGR also made a solution for this (JGRennison/OpenTTD-patches@b23ba0c), and although effective, it felt a bit like bringing a gun to a pillow fight.
I was also mostly wondering why _generating_world was misused like this, and why it would skip the stations_near update.

Long story short: the check on _generating_world is an optimization, to avoid wasting a lot of CPU on something that is never going to happen during map generation. So I looked for the next-best-thing that does the same but differently, and settled on checking if there is any station at all. During map generation of course there isn't.

In result, after this PR the code does the same for new map generation (skipping the expensive station check), but fixes the bug in #9407.

JGR's solution is also a fine solution, but it introduces several globals and more complexity that can break over time. On the other hand, his solution might fix issues we were not even aware of. I leave it to the reviewer to balance this.

Limitations

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, gs_changelog.hpp need updating.
    • The compatibility wrappers (compat_*.nut) need updating.
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')

@JGRennison
Copy link
Contributor

The Station::GetNumItems() != 0 check could be moved inside ForAllStationsAroundTiles perhaps?
The other call sites of ForAllStationsAroundTiles can also be called when there are no stations. (PopulateStationsNearby for industries also has a similar _generating_world check).

The reason for the slightly more complicated implementation in my branch it to avoid repeating the station tile scan for every new house in the founded town, and the catchment area scan for each found station x each house which found it. The scans only need to be done once after the town is founded. I did not make any measurements at the time but it may be worth bearing in mind.

"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 merged commit 2c05412 into OpenTTD:master Aug 31, 2021
@TrueBrain TrueBrain deleted the fix-create-town branch August 31, 2021 12:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants