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
Conversation
0fd333a
to
e47384b
Compare
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... |
@LordAro it may be nicer but I highly doubt it will be as performant. |
You'd be surprised what optimisers can do with vectors. Especially when compared with variable-capturing lambdas... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the meantime...
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; | ||
}); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing MP_INDUSTRY
case ?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
e47384b
to
6ea41e7
Compare
6ea41e7
to
c6aa290
Compare
There was a problem hiding this 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
Ended up copying half of FindStationsAroundTiles (which is a mess anyway) in favor of reducing duplication with Station::RecomputeCatchment.
Closes #8141