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

@perezdidac perezdidac commented Dec 27, 2020

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

@James103 James103 commented Dec 27, 2020

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

@perezdidac perezdidac commented Dec 27, 2020

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

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

@James103 James103 commented Dec 27, 2020

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

@2TallTyler 2TallTyler commented Jan 1, 2021

@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
Copy link
Member

@TrueBrain TrueBrain commented Jan 5, 2021

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

@TrueBrain TrueBrain force-pushed the perezdidac:perezdidac-patch-1 branch from 87b2869 to a8fdf1d Jan 6, 2021
perezdidac and others added 2 commits Dec 27, 2020
Having 4 rails is a pretty common design, and towns now couldn't
bridge out of this common design.
@TrueBrain TrueBrain force-pushed the perezdidac:perezdidac-patch-1 branch from a8fdf1d to 46ff278 Jan 6, 2021
@TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented Jan 6, 2021

@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 requested a review from michicc Jan 6, 2021
@TrueBrain TrueBrain dismissed michicc’s stale review Jan 6, 2021

Issues resolved

@TrueBrain TrueBrain removed the request for review from michicc Jan 6, 2021
@michicc
michicc approved these changes Jan 6, 2021
@TrueBrain TrueBrain merged commit a4e34e8 into OpenTTD:master Jan 6, 2021
8 checks passed
8 checks passed
Emscripten
Details
Commit checker
Details
Check preview needs update Check preview needs update
Details
Linux (clang, clang++)
Details
Linux (gcc, g++)
Details
Mac OS (x64, x86_64)
Details
Windows (x86) Windows (x86)
Details
Windows (x64) Windows (x64)
Details
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

5 participants