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

Add: Kdtree for AirportGetNearestTown #7424

Merged
merged 2 commits into from Dec 24, 2019

Conversation

@SamuXarick
Copy link
Contributor

SamuXarick commented Mar 27, 2019

Add: Kdtree for AirportGetNearestTown …
For increased performance, it only iterates over airport perimeter tiles (assuming rectangular shape layout).

REQUIRES #7429 or else it fails.

@PeterN
Copy link
Member

PeterN commented Mar 27, 2019

Hmm, looking through this, there are further simplification/optimizations to be made.

src/station_cmd.cpp Outdated Show resolved Hide resolved
src/station_cmd.cpp Outdated Show resolved Hide resolved
src/station_cmd.cpp Outdated Show resolved Hide resolved
@SamuXarick SamuXarick force-pushed the SamuXarick:kdtree-for-AirportGetNearestTown branch 2 times, most recently from dca2aa5 to 1180ac2 Mar 27, 2019
@SamuXarick
Copy link
Contributor Author

SamuXarick commented Mar 27, 2019

https://imgur.com/a/GZ1W0dV

Most relevant gains only seen past around 3000 towns :(

@nielsmh
Copy link
Contributor

nielsmh commented Mar 27, 2019

I wonder if the algorithm for this even needs to be that complicated at all. Couldn't you just calculate the middle tile of the airport and use the town nearest to that tile?

@SamuXarick
Copy link
Contributor Author

SamuXarick commented Mar 27, 2019

Looping over perimeter tiles:
Unnamed, 2051-01-21

Previously:
XxHWxXC

@SamuXarick SamuXarick force-pushed the SamuXarick:kdtree-for-AirportGetNearestTown branch from 4f48e56 to 6a58290 Mar 28, 2019
@SamuXarick
Copy link
Contributor Author

SamuXarick commented Mar 29, 2019

Just finished testing all (vanilla) airport types on all tiles of a 4096x4096 map (~12k-13k towns), combined with #7429.

Test consisted of comparing the results of the old code to the new code, via an assert(forall == kdtree && kdtree == kdtper);

Ran this code in an AI:

//		local alltiles = AIMap.GetMapSize();
//		for (local type = 0; type <= 8; type++) {
//			AILog.Info("type: " + WrightAI.GetAirportTypeName(type));
//			local percent = -1;
//			for (local tile = 0; tile < alltiles; tile++) {
//				AIAirport.GetNearestTown(tile, type);
//				local newpercent = (tile + 1) * 100 / alltiles;
//				if (percent != newpercent) AILog.Info(newpercent + "%");
//				percent = newpercent;
//			}
//		}
//		AIController.Break("Iterated all types and tiles");
@SamuXarick SamuXarick force-pushed the SamuXarick:kdtree-for-AirportGetNearestTown branch from 6a58290 to 8c5b13d Mar 29, 2019
@SamuXarick
Copy link
Contributor Author

SamuXarick commented Mar 31, 2019

I wonder if the algorithm for this even needs to be that complicated at all. Couldn't you just calculate the middle tile of the airport and use the town nearest to that tile?

The results of my researchs so far regarding this and other tests I went through:

  • 4 corners of the outer rectangle (via as->size_x/y): asserts vs original code
  • 1 center tile, computed via as->size_x/y sizes: asserts vs original code
  • 1, 2 or 4 center tiles, computed via as->size_x/y sizes: asserts vs original code
  • all it layout tiles: successful vs original code
  • the perimeter (computed via as-size_x/y) it layout tiles (this PR): successful vs original code

Maybe pointless to report this, but just so that the already known information is in one place.

@SamuXarick
Copy link
Contributor Author

SamuXarick commented Mar 31, 2019

@SamuXarick SamuXarick force-pushed the SamuXarick:kdtree-for-AirportGetNearestTown branch from 8c5b13d to 278d25b Mar 31, 2019
@SamuXarick SamuXarick force-pushed the SamuXarick:kdtree-for-AirportGetNearestTown branch 2 times, most recently from 14cb269 to 3cefb24 Apr 10, 2019
uint it_x = TileX(it);
uint it_y = TileY(it);
uint max_it_x = it_x + as->size_x - 1;
uint max_it_y = it_y + as->size_y - 1;

This comment has been minimized.

@PeterN

PeterN Apr 29, 2019 Member

Don't like this naming. perimeter_min_x etc, maybe?

uint max_it_x = it_x + as->size_x - 1;
uint max_it_y = it_y + as->size_y - 1;

mindist = UINT_MAX - 1; // prevent overflow

This comment has been minimized.

@PeterN

PeterN Apr 29, 2019 Member

Again, comment why you need the you need this. (If two airports are the same distance, you want the one with the lowest index)

This comment has been minimized.

@SamuXarick

SamuXarick May 7, 2019 Author Contributor

It is in the description.

@SamuXarick SamuXarick force-pushed the SamuXarick:kdtree-for-AirportGetNearestTown branch from 3cefb24 to d90481f May 7, 2019
@SamuXarick SamuXarick force-pushed the SamuXarick:kdtree-for-AirportGetNearestTown branch from d90481f to 88d843a Jul 18, 2019
@LordAro LordAro requested a review from PeterN Nov 23, 2019
For increased performance, it only iterates over airport perimeter tiles (assuming rectangular shape layout).
@SamuXarick SamuXarick force-pushed the SamuXarick:kdtree-for-AirportGetNearestTown branch from 88d843a to a7f5631 Dec 22, 2019
@LordAro LordAro dismissed PeterN’s stale review Dec 23, 2019

old review

src/station_cmd.cpp Outdated Show resolved Hide resolved
@LordAro LordAro merged commit 40605ef into OpenTTD:master Dec 24, 2019
8 checks passed
8 checks passed
OpenTTD CI Build #20191224.1 succeeded
Details
OpenTTD CI (Linux commit-checker) Linux commit-checker succeeded
Details
OpenTTD CI (Linux linux-amd64-clang-3.9) Linux linux-amd64-clang-3.9 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
@SamuXarick SamuXarick deleted the SamuXarick:kdtree-for-AirportGetNearestTown branch Dec 28, 2019
douiwby added a commit to douiwby/OpenTTD that referenced this pull request Apr 16, 2020
spnda added a commit to spnda/OpenTTD that referenced this pull request Jun 17, 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.