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

Feature: Make town bridge max length a function of its population #8439

Merged
merged 2 commits into from Jan 6, 2021

Conversation

perezdidac
Copy link
Contributor

Make new town bridge max length a function of its population.

Motivation / Problem

Really small towns can build disproportionately long bridges, all the way up to 11 tiles. To make it more realistic, the max number of tiles for a built bridge should be a function of its population.

Description

In order to prevent small towns from building disproportionately long bridges, this change makes the max allowed bridge length a function of its population, while letting every town build at least a town as long as 4 tiles. The max length is defined as a min of 4 plus 1 for each 1000 population, rounded down.

  • town of 200 population -> max bridge length of 4
  • town of 2000 population -> max bridge length of 6
  • town of 10000 population -> max bridge length of 14
  • town of 20000 population -> max bridge length of 24

Limitations

None.

Checklist for review

This PR is safe, and backwards compatible, since saved games won't be affected other than towns will build bridges with a variable max length as opposed as the current fixed hard coded max length.

@perezdidac perezdidac changed the title Make town bridge max length a function of its population Feature: Make town bridge max length a function of its population Dec 27, 2020
@James103
Copy link
Contributor

Suggested commit message: "Feature: Make town bridge max length a function of its population"

Also, would it be a good idea to hard cap the town max bridge length at the max bridge length defined in the Advanced Settings?

@perezdidac
Copy link
Contributor Author

James103: Yes that is a good idea. We could hard code a limit of 20 probably. I tried different town sizes and bridge lengths and 20 seems too much regardless of the city size. Is there a max bridge length in settings? Any code pointer?

Copy link
Member

@2TallTyler 2TallTyler left a comment

Choose a reason for hiding this comment

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

I think 20 tiles is a bit excessive and would lead to large cities building ridiculous bridges. What about keeping the current 11 tile length?

The max bridge length mentioned by James103 is for player-built bridges, which defaults to 64 tiles. Far too long for a town-built bridge, IMHO.

src/town_cmd.cpp Outdated Show resolved Hide resolved
src/town_cmd.cpp Outdated Show resolved Hide resolved
@michicc
Copy link
Member

michicc commented Dec 27, 2020

I like this idea. One idea to consider is to make the length not depend on the population but on the house count as some kind of proxy for "town area".

@James103
Copy link
Contributor

But what if the "max bridge length" setting was set to some really low value like 7 tiles (down from the default of 64)?

src/town_cmd.cpp Outdated Show resolved Hide resolved
@2TallTyler
Copy link
Member

@michicc: I like population over number of houses because a small island of skyscrapers may have more population (and thus need for a bridge) than a sprawling town of cottages. This is probably more meaningful with a house NewGRF like Improved Town Layouts where population doesn't necessarily grow linearly with number of houses.

@James103: The max bridge length chosen in the settings is checked by the DoCommand which actually builds the bridge.

@TrueBrain TrueBrain added candidate: yes This Pull Request is a candidate for being merged size: small This Pull Request is small, and should be relative easy to process work: minor details This Pull Request has some minor details left to do labels Jan 5, 2021
@TrueBrain TrueBrain added this to the 1.11.0 milestone Jan 5, 2021
@TrueBrain
Copy link
Member

As a small update, we would like to include this PR for our 1.11 release, but there is some small work from our side to do first before we can include it. Mostly, we want to align it with #8473 , and make sure we like the values etc. No work required from your side just yet, but just so you know this is in the works :)

perezdidac and others added 2 commits January 6, 2021 14:23
Having 4 rails is a pretty common design, and towns now couldn't
bridge out of this common design.
@TrueBrain
Copy link
Member

@perezdidac : I made some minor changes to your code: renamed min_max to base, remove the floating-point operation, and some minor coding style. Nothing fancy.

I kept it depending on population rather than houses; I think this is a good first iteration to make towns a bit more playful.

@TrueBrain TrueBrain removed the work: minor details This Pull Request has some minor details left to do label Jan 6, 2021
@TrueBrain TrueBrain requested a review from michicc January 6, 2021 15:35
@TrueBrain TrueBrain removed the request for review from michicc January 6, 2021 15:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
candidate: yes This Pull Request is a candidate for being merged size: small This Pull Request is small, and should be relative easy to process
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants