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 May 11, 2020
@LordAro
Copy link
Member

@LordAro 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 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 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

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

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

missing MP_INDUSTRY case ?

Copy link
Contributor Author

@ldpl ldpl May 11, 2020

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

@ldpl ldpl May 11, 2020

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

@LordAro LordAro May 12, 2020

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

@ldpl ldpl May 12, 2020

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.

@ldpl ldpl force-pushed the fund-industry-desync branch from e47384b to 6ea41e7 May 11, 2020
@ldpl ldpl force-pushed the fund-industry-desync branch from 6ea41e7 to c6aa290 May 11, 2020
Copy link
Member

@LordAro LordAro left a comment

Pretty sure this is fine now

@LordAro LordAro merged commit f2a9a1e into OpenTTD:master May 13, 2020
8 checks passed
@ldpl ldpl deleted the fund-industry-desync branch Jul 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants