feat: add and remove points#6
Conversation
Max-Verbinnen
left a comment
Max-Verbinnen
left a comment
There was a problem hiding this comment.
awesome work! just left minor comments/suggestions
| const timerId = `prepare ${points.length} points`; | ||
| if (log) console.time(timerId); | ||
|
|
||
| this.trees = new Array(maxZoom + 2); |
There was a problem hiding this comment.
This is something that should have actually been +2 from the start, because that's the amount of space we need, but previously we just increased the length by 1 afterwards. We need space for each zoom level: 0...maxZoom and one extra: maxZoom + 1, which is a total of maxZoom + 2.
| return expansionZoom; | ||
| } | ||
|
|
||
| printClusterData() { |
There was a problem hiding this comment.
maybe _printClusterData or do we want to expose this more publicly too?
| () => new Array(), | ||
| ); | ||
| ancestorRemovals[maxZoom + 1].push(removedNode); | ||
| this._removeAncestors( |
There was a problem hiding this comment.
i will pretend to know what exactly is going on here 😅
| // encode both zoom and point index on which the cluster originated -- offset by total length of features | ||
| const id = (((i / stride) | 0) << 5) + (zoom + 1) + this.points.length; | ||
| // encode both zoom and point index on which the cluster originated | ||
| const id = -((((i / stride) | 0) << 5) + (zoom + 1)); |
There was a problem hiding this comment.
a comment to explain why we use negative ids might be useful here!
| for (let i = idx * stride; i < (idx + 1) * stride; i++) { | ||
| this.clusterData[zoom][i] = null; | ||
| } |
There was a problem hiding this comment.
maybe you can use the array fill() method instead
| } | ||
| } | ||
|
|
||
| while (notVisited.length > 0) { |
There was a problem hiding this comment.
this section looks very similar to the nonIndexedNodes check above. maybe extract into a helper? if it's too annoying, don't bother
There was a problem hiding this comment.
Extracted common part into a lambda!
| _linearSearchInPoints(id) { | ||
| const index = this.points.findIndex((p) => this.getId(p) === id); | ||
| const index = this.points.findIndex((p) => | ||
| p ? this.getId(p) === id : false, |
There was a problem hiding this comment.
We do need some way to verify p is not null before calling getId() on it, because now with point removal, null can occur in the point list. I changed it to be a bit more concise though.
| _removeAncestors(firstZoom, descendantNodes, removals) { | ||
| const { minZoom } = this.options; | ||
| for (let zoom = firstZoom; zoom >= minZoom; zoom--) { | ||
| if (descendantNodes.length === 0) break; |
There was a problem hiding this comment.
you could add this as a condition in the for-loop itself if you want
2 participants
No description provided.