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: Don't terraform during town generation #8483

Closed
wants to merge 1 commit into from

Conversation

SamuXarick
Copy link
Contributor

@SamuXarick SamuXarick commented Jan 2, 2021

Prevent rare occurrences of roads and houses being flooded, resulting in disconnected towns.

Motivation / Problem

road and house flooded
During world generation, town growth algorithm is speed up, allowing the town to perform multiple sequential growth actions, like building houses, roads, and terraforming. If it happens to lower terrain adjacent to sea and then in one of those sequences it builds a road or a house there, it will end up flooded during the "run tile loop" phase of world generation.
The result could be similar to what the screenshot shows: disconnected roads and/or houses.

Description

Bug is solved by preventing towns to terraform during world generation. There is already code preventing town terraforming during world generation like shown below, but this one went missing.
Line 930:

OpenTTD/src/town_cmd.cpp

Lines 930 to 934 in 126f40e

if (!_generating_world && Chance16(1, 10)) {
/* Note: Do not replace "^ SLOPE_ELEVATED" with ComplementSlope(). The slope might be steep. */
res = DoCommand(tile, Chance16(1, 16) ? cur_slope : cur_slope ^ SLOPE_ELEVATED, 0,
DC_EXEC | DC_AUTO | DC_NO_WATER, CMD_TERRAFORM_LAND);
}

Limitations

This happens very rarely. I tried 12k towns 4096x4096 map, and detected no more than 3 flooded road or house tiles.

Checklist for review

Some things are not automated, and forgotten often. This list is a reminder for the reviewers.

  • The bug fix is important enough to be backported? (label: 'backport requested')
  • This PR affects the save game format? (label 'savegame upgrade')
  • This PR affects the GS/AI API? (label 'needs review: Script API')
    • ai_changelog.hpp, gs_changelog.hpp need updating.
    • The compatibility wrappers (compat_*.nut) need updating.
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')

Prevent rare occurrences of roads and houses being flooded, resulting in disconnected towns.
@stormcone
Copy link
Contributor

Wouldn't be better just don't allow to town to build on sea level? I don't know how it affects the growing of town, but for me it doesn't feel like the right approach to disable the whole terraforming because it can occurs 3 times in the case of 12k towns.

@FLHerne
Copy link
Contributor

FLHerne commented Jan 3, 2021

I believe this is probably the wrong fix. A town that has elements at sea level can still flood itself by lowering terrain into the ocean outside of world-generation, so it doesn't fully fix the reported issue, while also being massive overkill.

Prevent terraforming of tiles only at sea level, but regardless of worldgen?

@TrueBrain
Copy link
Member

A few observations:

  • The coded piece of code is only used for "natural" road-layout generator of towns. Not for the 2x2 and 3x3 grid layout. So it is a bit deceiving to use it as "argument" for this PR
  • After town generation, towns can still flood their own town
  • If someone has build_on_slopes set to false, his towns out of map generation will be horrible. This because of this line of code:
if (!_settings_game.construction.build_on_slopes || Chance16(1, 6)) LevelTownLand(tile);

All in all, you managed to fix your use-case, but disregarded any other use-case out there ;) I am pretty sure this PR makes things worse, not better. To put it in numbers: it made 12k out of 12k towns look worse, while fixing 3 out of 12k towns. Pretty sure that is the wrong balance ;)

I think the suggestions given in this PR are solid: prevent towns from terraforming below sea-level sounds like a more robust and "this doesn't brake most cases" kind of solution :)

I am going to close this PR now, just to close this chapter. Please feel free to create a new PR with a more robust solution; and please take the time to test other use-cases :) Tnx!

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

5 participants