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 #8137: New clients can't join (desync) after funding an industry #8140

Merged
merged 2 commits into from May 13, 2020

Conversation

ldpl
Copy link
Contributor

@ldpl ldpl commented May 11, 2020

Ended up copying half of FindStationsAroundTiles (which is a mess anyway) in favor of reducing duplication with Station::RecomputeCatchment.

Closes #8141

@ldpl ldpl force-pushed the fund-industry-desync branch 2 times, most recently from 0fd333a to e47384b Compare May 11, 2020 22:44
@LordAro
Copy link
Member

LordAro commented May 11, 2020

I have to wonder whether just returning a list/vector/whatever of stations then operating on the return value would be nicer than the lambda & capture variables...

@ldpl
Copy link
Contributor Author

ldpl commented May 11, 2020

@LordAro it may be nicer but I highly doubt it will be as performant.
Though StationFinder already returns a vector and is still a mess imo.

@LordAro
Copy link
Member

LordAro commented May 11, 2020

You'd be surprised what optimisers can do with vectors. Especially when compared with variable-capturing lambdas...

Copy link
Member

@LordAro LordAro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the meantime...

src/industry_cmd.cpp Outdated Show resolved Hide resolved
src/station_base.h Outdated Show resolved Hide resolved
src/station_base.h Show resolved Hide resolved
src/station_base.h Show resolved Hide resolved
src/station_base.h Show resolved Hide resolved
Comment on lines -4026 to +3985
FindStationsAroundTiles(*this, &this->stations);
if (IsTileType(this->tile, MP_HOUSE)) {
/* Town nearby stations need to be filtered per tile. */
assert(this->w == 1 && this->h == 1);
AddNearbyStationsByCatchment(this->tile, &this->stations, Town::GetByTile(this->tile)->stations_near);
} else {
ForAllStationsAroundTiles(*this, [this](Station *st, TileIndex tile) {
this->stations.insert(st);
return true;
});
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing MP_INDUSTRY case ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@glx22 not missing, it just makes no sense whatsoever if you see how it is called

Copy link
Contributor

@glx22 glx22 May 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see, it's never called for industries. But I think the industry case was future proof.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@glx22 it's a weird way to future proof as it was not handled correctly and likely broke newgrf houses.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps an assert/NOT_REACHED to make it clear?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@LordAro Not reached what? industries can be reached, just shouldn't work as it was before.

Copy link
Member

@LordAro LordAro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pretty sure this is fine now

@LordAro LordAro added the backport requested This PR should be backport to current release (RC / stable) label May 13, 2020
@LordAro LordAro merged commit f2a9a1e into OpenTTD:master May 13, 2020
@ldpl ldpl deleted the fund-industry-desync branch July 14, 2020 08:27
@LordAro LordAro added backported This PR is backported to a current release (RC / stable) and removed backport requested This PR should be backport to current release (RC / stable) labels Jul 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backported This PR is backported to a current release (RC / stable)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants