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

Change: Ability to convert between road types for town roads and bridges now indi… #8457

Merged

Conversation

UnsuspiciousGooball
Copy link
Contributor

…rectly depends on town ratings.

Motivation / Problem

Previously it was possible to convert town roads and bridges without restriction by the local authority.

Description

With this change the ability to convert town roads and bridges depends on the players' companies' standing with the local authority, which is determined using CheckforTownRating() with the ROAD_REMOVE and the TUNNELBRIDGE_REMOVE TownRatingCheckTypes, respectively.

Basically the player is allowed to convert town roads if and only if the player would be allowed to destroy town roads, and the same goes for town bridges.

@LordAro
Copy link
Member

LordAro commented Dec 28, 2020

Not that we particularly mind, but the commit author does not appear to match your GitHub user. Might want to fix that

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.

This appears to do exactly the same thing at the top of both if/else branches.
Instead of duplicating quite so much, I think a better solution would be to pull it out of the loop, and just use a ternary (tt == MP_TUNNELBRIDGE ? TUNNELBRIDGE_REMOVE : ROAD_REMOVE) for the only bit that actually differs. Would be able to get rid of the is_owner_town variable too.

…idge conversion to a single if statement.

Also, ClosestTownFromTile() is now only called when the tile in question actually belongs to a town.
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.

LGTM

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