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: Remove some redundant steps during world generation #7880

Closed

Conversation

@SamuXarick
Copy link
Contributor

SamuXarick commented Dec 27, 2019

  • FixSlopes was being done twice on heightmaps when one suffice. This reduces one step for loading a heightmap. Also removes redundant call to MarkWholeScreenDirty.
  • FixSlopes doesn't need to be run on flat empty world, except when freeform_edges is off and tile_height is higher than 1. Also removes redundant call to MarkWholeScreenDirty.
  • FixSlopes apparently isn't required for TerraGenesis. This reduces one step for TerraGenesis generator.
  • ConvertGroundTilesIntoWaterTiles is only required when freeform_edges is off or when se_flat_world_height is zero.
@LordAro
Copy link
Member

LordAro commented Dec 28, 2019

Where is the first call?

@nielsmh
Copy link
Contributor

nielsmh commented Dec 28, 2019

Tracing from entry of GenerateLandscape in Heightmap mode:

OpenTTD/src/landscape.cpp

Lines 1293 to 1309 in 69f9529

void GenerateLandscape(byte mode)
{
/** Number of steps of landscape generation */
enum GenLandscapeSteps {
GLS_HEIGHTMAP = 3, ///< Loading a heightmap
GLS_TERRAGENESIS = 5, ///< Terragenesis generator
GLS_ORIGINAL = 2, ///< Original generator
GLS_TROPIC = 12, ///< Extra steps needed for tropic landscape
GLS_OTHER = 0, ///< Extra steps for other landscapes
};
uint steps = (_settings_game.game_creation.landscape == LT_TROPIC) ? GLS_TROPIC : GLS_OTHER;
if (mode == GWM_HEIGHTMAP) {
SetGeneratingWorldProgress(GWP_LANDSCAPE, steps + GLS_HEIGHTMAP);
LoadHeightmap(_file_to_saveload.detail_ftype, _file_to_saveload.name);
IncreaseGeneratingWorldProgress(GWP_LANDSCAPE);
} else if (_settings_game.game_creation.land_generator == LG_TERRAGENESIS) {

This calls LoadHeightmap, which has the first FixSlopes on line 502:

OpenTTD/src/heightmap.cpp

Lines 489 to 504 in 69f9529

void LoadHeightmap(DetailedFileType dft, const char *filename)
{
uint x, y;
byte *map = nullptr;
if (!ReadHeightMap(dft, filename, &x, &y, &map)) {
free(map);
return;
}
GrayscaleToMapHeights(x, y, map);
free(map);
FixSlopes();
MarkWholeScreenDirty();
}

After return to GenerateLandscape, the second call to FixSlopes is on line 1369:

OpenTTD/src/landscape.cpp

Lines 1367 to 1377 in 69f9529

/* Do not call IncreaseGeneratingWorldProgress() before FixSlopes(),
* it allows screen redraw. Drawing of broken slopes crashes the game */
FixSlopes();
IncreaseGeneratingWorldProgress(GWP_LANDSCAPE);
ConvertGroundTilesIntoWaterTiles();
IncreaseGeneratingWorldProgress(GWP_LANDSCAPE);
if (_settings_game.game_creation.landscape == LT_TROPIC) CreateDesertOrRainForest();
CreateRivers();
}

@LordAro
Copy link
Member

LordAro commented Dec 28, 2019

The original one in LoadHeightmap seems to date from 10b842b (r5944) which, guess what, is when TGP was merged.

The later one in GenerateLandscape comes from 14f52cf (where it was moved shortly after 3d78060)

I notice there's another call to FixSlopes just below in FlatEmptyWorld - which might be just as redundant

@SamuXarick SamuXarick force-pushed the SamuXarick:FixSlopes-was-being-done-twice branch from 54febf5 to 1a40cb3 Dec 28, 2019
@SamuXarick SamuXarick changed the title Fix: Fixing the slopes was being done twice on heightmaps when one suffices Fix: Remove some redundant steps during world generation Dec 28, 2019
@SamuXarick
Copy link
Contributor Author

SamuXarick commented Dec 30, 2019

FixSlopes apparently isn't required for Original land generator. This reduces one step for original generator.

I was wrong. Just getting a crash when trying to generate town roads on some of the "unfixed" slopes. Sorry, will fix asap.

EDIT: fixed

@SamuXarick SamuXarick force-pushed the SamuXarick:FixSlopes-was-being-done-twice branch from 1a40cb3 to ac37c17 Dec 30, 2019
@SamuXarick
Copy link
Contributor Author

SamuXarick commented Dec 30, 2019

Just fixed the crash. It was TerraGenesis that didn't require FixSlopes, I must have confused them.

@James103
Copy link
Contributor

James103 commented Dec 30, 2019

How much of a time improvement does this have on world generation (especially on larger maps)?

@LordAro LordAro changed the title Fix: Remove some redundant steps during world generation Fix: Remove some redundant steps during world generation Dec 30, 2019
@SamuXarick
Copy link
Contributor Author

SamuXarick commented Dec 30, 2019

4096x4096 original mountainous
dbg: [misc] [FixSlopes] 161798 us [avg: 161798.0 us]
dbg: [misc] [ConvertGroundTilesIntoWaterTiles] 457768 us [avg: 457768.0 us]

@LordAro
Copy link
Member

LordAro commented Jan 1, 2020

...those numbers are useless on their own

- FixSlopes was being done twice on heightmaps when one suffice. This reduces one step for loading a heightmap. Also removes redundant call to
MarkWholeScreenDirty.
- FixSlopes doesn't need to be run on flat empty world, except when freeform_edges is off and tile_height is higher than 1. Also removes
redundant call to MarkWholeScreenDirty.
- FixSlopes apparently isn't required for TerraGenesis. This reduces one step for TerraGenesis generator.
- ConvertGroundTilesIntoWaterTiles is only required when freeform_edges is off or when se_flat_world_height is zero.
@SamuXarick SamuXarick force-pushed the SamuXarick:FixSlopes-was-being-done-twice branch from ac37c17 to a84668c Feb 9, 2020
@LordAro
Copy link
Member

LordAro commented Apr 13, 2020

Going to close this. It doesn't offer any real benefit (not that any numbers have been provided) and only serves to increase complexity in the code. Potential for extra worldgen bugs being introduced is high.

@LordAro LordAro closed this Apr 13, 2020
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

4 participants
You can’t perform that action at this time.