feat: change underlying geospatial index to RBush#1
Conversation
Max-Verbinnen
left a comment
Max-Verbinnen
left a comment
There was a problem hiding this comment.
awesome, lgtm! feel free to merge after addressing my small remarks
| | radius | 40 | Cluster radius, in pixels. | | ||
| | extent | 512 | (Tiles) Tile extent. Radius is calculated relative to this value. | | ||
| | nodeSize | 64 | Size of the KD-tree leaf node. Affects performance. | | ||
| | nodeSize | 9 | Size of the R-tree nodes. Affects performance. | |
There was a problem hiding this comment.
9 is the default nodeSize for the RBush library as well. Different numbers result in either higher query time or build time, and I guess this number strikes a balance.
| "name": "mutable-supercluster", | ||
| "version": "1.0.0", | ||
| "description": "A library for fast and mutable geospatial point clustering.", | ||
| "main": "dist/supercluster.js", |
There was a problem hiding this comment.
let's maybe use mutable-supercluster.js everywhere in this file instead to avoid confusion
| map: props => props, // props => ({sum: props.my_value}) | ||
|
|
||
| // a function that maps a point to its Id, which should be numerical | ||
| getId: point => point.geometry.coordinates[0] |
There was a problem hiding this comment.
let's also mention getId in the readme
There was a problem hiding this comment.
Yes, I forgot to do this as this option was only added as preparation for the new features.
| if (log) console.time(timerId); | ||
|
|
||
| this.points = points; | ||
| this.points.sort((a, b) => this.getId(a) - this.getId(b)); |
There was a problem hiding this comment.
For the remove and update features we need a way to quickly access points by their id. However, I came to the conclusion that sorting also won't really work, so something has to be found to index over the point id.
| _rbushWithin(ax, ay, zoom, radius) { | ||
| const result = []; | ||
| const r2 = radius * radius; | ||
| const pointsInSquare = this.trees[zoom].search({minX: ax - radius, minY: ay - radius, maxX: ax + radius, maxY: ay + radius}); |
| const pointsInSquare = this.trees[zoom].search({minX: ax - radius, minY: ay - radius, maxX: ax + radius, maxY: ay + radius}); | ||
| for (const [bx, by, idx] of pointsInSquare) { | ||
| if (sqDist(ax, ay, bx, by) <= r2) { | ||
| result.push(idx); |
There was a problem hiding this comment.
imo it would be more readable to do a filter on pointsInSquare here
2 participants
This PR changes the underlying geospatial index of clustering library from a KD-Tree to an R-Tree. This is done as a first step towards making the cluster mutable after it is built.
Unfortunately, using the new index comes with a cost as well. These are the results form the benchmark that is included in the project: