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

K-d tree data structure for spatial lookups #7250

Merged
merged 4 commits into from Mar 9, 2019
Merged

Conversation

@nielsmh
Copy link
Contributor

nielsmh commented Feb 19, 2019

Implements a k-d tree data structure for spatial indexes of map objects that don't move very often. Right now this is used for stations and towns (their sign location).

The k-d tree allows fast lookup of the nearest indexed point to a given point, and fast iteration of indexed points within a rectangle (orthogonal tile area).

Currently, testing shows no particular improvement. This is expected since almost all the operations changed to use the tree currently are responses to player commands, i.e. building and removing stations.

@nielsmh

This comment has been minimized.

Copy link
Contributor Author

nielsmh commented Feb 19, 2019

Looks like this needs some more work. Regressions are failing, and letting the Wentbourne save run for a while with master and this branch side by side has this branch slowly fall behind.

@nielsmh

This comment has been minimized.

Copy link
Contributor Author

nielsmh commented Feb 19, 2019

Tried performing the same test as in the Voronoi patch and this also shows some improvement. Not quite as big, but still around 15%-20% better performance early on in a game with five AI players, set on fast-forward.

@nielsmh

This comment has been minimized.

Copy link
Contributor Author

nielsmh commented Feb 19, 2019

Another thing to be aware of is that this can cause iteration order to become effectively randomized. Any use of FindContained in game code must be sure to have the same result regardless of iteration order. This is because the indexes are built on game load, but only updated during most operations, so clients that have joined a game at different times can have differently shaped indexes.

@GabdaZM

This comment has been minimized.

Copy link
Contributor

GabdaZM commented Feb 19, 2019

Currently if two towns have equal distance from a tile, and you call CalcClosestTownFromTile, it returns the town with the lower index. Do you have/can you add this index check?

@nielsmh

This comment has been minimized.

Copy link
Contributor Author

nielsmh commented Feb 19, 2019

That's a really good point, and probably a bug. I'm not sure exactly how to address that problem.

@nielsmh

This comment has been minimized.

Copy link
Contributor Author

nielsmh commented Feb 19, 2019

This should solve the insufficiently defined results from FindNearest when multiple points had the same lowest distance. I've only tested it with the simplest case of two towns, and no formal proof, but I have a gut feeling this is correct :P

@TrueBrain

This comment has been minimized.

Copy link
Member

TrueBrain commented Feb 19, 2019

Just so I mentioned it here too: this begs for UnitTests :D (not blaming you if you don't, as we have none to start with)

src/viewport.cpp Outdated Show resolved Hide resolved
@nielsmh

This comment has been minimized.

Copy link
Contributor Author

nielsmh commented Feb 22, 2019

If I can make the viewport signs (stations, waypoints, towns, signs) indexing right and not flicker/glitch the drawing, it can offer upwards a factor 10 improvement in drawing speed, depending on number of signs.

I'm not sure why the regressions are now failing on some platforms but not all.

@GabdaZM

This comment has been minimized.

Copy link
Contributor

GabdaZM commented Feb 23, 2019

After it works well, it might be better to store town signs in a separate tree, as these signs are not added/removed during a game, and you don't have to rebuild it after a certain amount of station build/deletion.

@nielsmh

This comment has been minimized.

Copy link
Contributor Author

nielsmh commented Feb 23, 2019

Town names should probably be separated for visual purposes regardless. I think they're supposed to be drawn below any station and player signs, with this they can often end up drawn on top of those.

@PeterN

This comment has been minimized.

Copy link
Member

PeterN commented Feb 23, 2019

Towns can be founded in game, and removed in the scenario editor. It's less common, but also station signs being added/moved/removed does not happen very often, so I don't think that would be necessary.

EDIT: Of course that other reason sounds like a good reason.

@nielsmh

This comment has been minimized.

Copy link
Contributor Author

nielsmh commented Feb 24, 2019

It appears there is a case of undefined behaviour in somewhere in the code, that causes some builds to produce wrong results for tree lookups, or maybe build incorrect trees.

Enabling the CheckInvariant debugging does not trigger any asserts when the tree returns wrong results for a lookup, so it appears the error is somewhere in the recursive search algorithm.

@nielsmh nielsmh force-pushed the nielsmh:kdtree branch from 93b4819 to b0d1c52 Feb 24, 2019
@PeterN

This comment has been minimized.

Copy link
Member

PeterN commented Feb 25, 2019

This resolves #7274. According to @SamuXarick his world-gen time on a 4096^2 map with 9000+ towns went from 40 minutes down to 95 seconds.

@nielsmh

This comment has been minimized.

Copy link
Contributor Author

nielsmh commented Feb 25, 2019

Those numbers match an asymptotic improvement from Θ(n²) to Ω(n log n) quite well.

√(40 * 60) ≈ 49
49 * log(49) ≈ 83 seconds

So that matches an improvement from 2400 seconds down to below 100 seconds, apart from some constants.
It's worth keeping in mind the nearest item lookup in the k-d tree really only at best Ω(log n), but in worst case can be O(n), but should still be an improvement in nearly all non-pathological cases.

@PeterN

This comment has been minimized.

Copy link
Member

PeterN commented Feb 25, 2019

Apparently the voronoi version was about 6 seconds faster (but these tests would need repeating to give stable results), however memory usage was up by the appropriate 32MB for 4096^2 (absolute figure ws 841MB vs 875MB, which is just a 4% increase.)

@nielsmh nielsmh added this to the 1.10.0 milestone Feb 27, 2019
@nielsmh nielsmh force-pushed the nielsmh:kdtree branch from b0d1c52 to 88f6d20 Feb 27, 2019
@PeterN

This comment has been minimized.

Copy link
Member

PeterN commented Feb 27, 2019

Worldgen test with time bin/openttd -v null g -G 1138:

  • master: 5m9.612s
  • #7284 1m23.780s
  • this PR: 1m10.836s

#7284 also fixes disconnected towns which this doesn't address.

@GabdaZM

This comment has been minimized.

Copy link
Contributor

GabdaZM commented Mar 1, 2019

Searching for closest town for 2 x 256 x 256 tiles (~ the number of tiles you can see in fullHD resolution when max zoomed out) takes about 100 ms when there are ~37k towns. I've tried to add the town boundary visualization, and with (unnecessarily) heavy mouse movement (frequent MarkWholeScreenDirty calls as different town boundaries are displayed), the simulation speed dropped to 0.4.

@nielsmh

This comment has been minimized.

Copy link
Contributor Author

nielsmh commented Mar 1, 2019

So in short, for "nearest town" display across the entire screen some bitmappy caching is still necessary.
However if a much simpler variation was implemented e.g. during building stations ("which town will this station belong to?") showing just town name for the currently hovered tile, k-d tree alone would be fine.

@PeterN

This comment has been minimized.

Copy link
Member

PeterN commented Mar 1, 2019

Note the TGP map gen makes only up to around 13,000 towns on a 4096² map

@nielsmh

This comment has been minimized.

Copy link
Contributor Author

nielsmh commented Mar 1, 2019

Idea for another representation during building a station, the brown line indicates the town border (and is faked in this picture), and the current town the station would belong to is shown in the build window. The border line is intentionally not drawn outside the catchment area, a fixed 9x9 tile area or similar might also be reasonable.

image

(This is obviously not relevant for the k-d tree itself, and should probably be in a separate PR.)

@nielsmh nielsmh marked this pull request as ready for review Mar 3, 2019
@nielsmh

This comment has been minimized.

Copy link
Contributor Author

nielsmh commented Mar 3, 2019

I think this is stable enough, except for maybe the viewport sign search range.

Copy link
Contributor

GabdaZM left a comment

I think most of the times auto can be replaced with node or It from the template.

src/core/kdtree.hpp Outdated Show resolved Hide resolved
src/core/kdtree.hpp Outdated Show resolved Hide resolved
src/core/kdtree.hpp Outdated Show resolved Hide resolved
src/core/kdtree.hpp Outdated Show resolved Hide resolved
src/core/kdtree.hpp Show resolved Hide resolved
@nielsmh nielsmh force-pushed the nielsmh:kdtree branch 3 times, most recently from 618f67c to fdc9804 Mar 4, 2019
src/core/kdtree.hpp Show resolved Hide resolved
src/core/kdtree.hpp Outdated Show resolved Hide resolved
src/core/kdtree.hpp Show resolved Hide resolved
src/core/kdtree.hpp Show resolved Hide resolved
src/core/kdtree.hpp Show resolved Hide resolved
src/core/kdtree.hpp Show resolved Hide resolved
@nielsmh nielsmh force-pushed the nielsmh:kdtree branch from fdc9804 to b3a35f6 Mar 5, 2019
src/core/kdtree.hpp Show resolved Hide resolved
src/core/kdtree.hpp Show resolved Hide resolved
src/town_cmd.cpp Outdated Show resolved Hide resolved
src/town_kdtree.h Outdated Show resolved Hide resolved
src/town_cmd.cpp Show resolved Hide resolved
src/station_cmd.cpp Outdated Show resolved Hide resolved
src/station_cmd.cpp Outdated Show resolved Hide resolved
src/station_cmd.cpp Show resolved Hide resolved
src/station_cmd.cpp Outdated Show resolved Hide resolved
src/station_kdtree.h Outdated Show resolved Hide resolved
src/viewport.cpp Show resolved Hide resolved
src/viewport.cpp Outdated Show resolved Hide resolved
src/station_cmd.cpp Outdated Show resolved Hide resolved
@nielsmh nielsmh force-pushed the nielsmh:kdtree branch from 0bcba8b to cebf8c4 Mar 9, 2019
@nielsmh nielsmh force-pushed the nielsmh:kdtree branch from cebf8c4 to 71c1782 Mar 9, 2019
@nielsmh nielsmh force-pushed the nielsmh:kdtree branch from 71c1782 to db7ac9b Mar 9, 2019
@PeterN
PeterN approved these changes Mar 9, 2019
@nielsmh nielsmh merged commit e8d397e into OpenTTD:master Mar 9, 2019
8 checks passed
8 checks passed
OpenTTD CI Build #20190309.21 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
@nielsmh nielsmh deleted the nielsmh:kdtree branch Mar 9, 2019
stormcone added a commit to stormcone/OpenTTD that referenced this pull request Jul 7, 2019
nielsmh added a commit that referenced this pull request Jul 22, 2019
…moved.

Code based on the patch by JGRennison.
JGRennison/OpenTTD-patches@ac84f34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.