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 #7043, Fix #7274: Delete town owned bridge based on adjacent tile ownership, not nearest town. #7284

Merged
merged 1 commit into from Mar 3, 2019

Conversation

@PeterN
Copy link
Member

PeterN commented Feb 26, 2019

The existing test performed badly with a large number of towns due to having to calculate the
nearest town, and also by its nature assumed a bridge was built by the nearest town, leading
to bridges of nearby large towns be removed incorrectly.

Two birds with one stone...

This is an alternative solution to #7282.

… ownership, not nearest town.

The existing test performed badly with a large number of towns due to having to calculate the
nearest town, and also by its nature assumed a bridge was built by the nearest town, leading
to bridges of nearby large towns be removed incorrectly.
@PeterN

This comment has been minimized.

Copy link
Member Author

PeterN commented Feb 26, 2019

Due to no longer using CalcClosestTownFromTile(), worldgen performance on large maps is increased substantially. A quick worldgen test with time bin/openttd -v null g -G 1138 came back with:

  • master: 5m9.612s
  • this pr: 1m23.780s

Worldgen settings: 4096x4096, TGP, High, Mountainous, None, High, Improved, Manual, 31, Jan 1950, Funding only, Very Rough, Many, Freeform.

@PeterN PeterN force-pushed the PeterN:bridge-ownership-test branch from b6dab5a to 4ab4d7d Feb 26, 2019
@PeterN PeterN changed the title Fix #7043, #7274: Delete town owned bridge based on adjacent tile ownership, not nearest town. Fix #7043, Fix #7274: Delete town owned bridge based on adjacent tile ownership, not nearest town. Feb 26, 2019
@nielsmh

This comment has been minimized.

Copy link
Contributor

nielsmh commented Feb 27, 2019

I think I would still prefer a solution that stores the owning town in the map array, also to avoid the double map array traversal.

@PeterN

This comment has been minimized.

Copy link
Member Author

PeterN commented Feb 27, 2019

PR #7282 now does exactly that. I was going for the agreed semi-KISS approach, however.

The ultimate KISS approach is of course to not build bridges with zero-pop, which is what #7282 originally did. That method avoids the double map scan too.

@PeterN PeterN added this to the 1.9.0 milestone Feb 27, 2019
@TrueBrain TrueBrain modified the milestones: 1.9.0, 1.10.0 Mar 3, 2019
@TrueBrain

This comment has been minimized.

Copy link
Member

TrueBrain commented Mar 3, 2019

I think this is a very nice stepping stone towards an even better solution (like #7282). For now I suggest we reschedule this to 1.10, as that allows us to accept this PR as first step to improve performance, and work on another before 1.10 is released.

@PeterN

This comment has been minimized.

Copy link
Member Author

PeterN commented Mar 3, 2019

Note that town bridges are only deleted by a town on map generation when a town fails to build. This is normally quite rare but some settings can cause it to happen enough that it's noticeably slow. During the game, towns will never delete their own bridges, and even if they did it would not be an intensive loop, just a one-off.

@nielsmh
nielsmh approved these changes Mar 3, 2019
@PeterN PeterN merged commit ebc3934 into OpenTTD:master Mar 3, 2019
8 checks passed
8 checks passed
OpenTTD CI Build #20190226.13 succeeded
Details
OpenTTD CI (Linux commit-checker) Linux commit-checker succeeded
Details
OpenTTD CI (Linux linux-amd64-clang-3.8) Linux linux-amd64-clang-3.8 succeeded
Details
OpenTTD CI (Linux linux-amd64-gcc-6) Linux linux-amd64-gcc-6 succeeded
Details
OpenTTD CI (Linux linux-i386-gcc-6) Linux linux-i386-gcc-6 succeeded
Details
OpenTTD CI (MacOS) MacOS succeeded
Details
OpenTTD CI (Windows Win32) Windows Win32 succeeded
Details
OpenTTD CI (Windows Win64) Windows Win64 succeeded
Details
@PeterN PeterN deleted the PeterN:bridge-ownership-test branch Mar 3, 2019
nielsmh added a commit to nielsmh/OpenTTD that referenced this pull request Mar 11, 2019
… adjacent tile ownership, not nearest town. (OpenTTD#7284)

This only affects failed town generation, as towns do not delete bridges under any other circumstances.

The existing test performed badly with a large number of towns due to having to calculate the
nearest town, and also by its nature assumed a bridge was built by the nearest town, leading
to bridges of nearby large towns be removed incorrectly.

If we gain the ability to quickly retrieve the correct town (which is _not_ the nearest town) from the bridge, this change should be reviewed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.