offset cluster ids, add index ids to non cluster points #121
Conversation
clif-os
changed the title
| @@ -235,7 +232,7 @@ export default class Supercluster { | |||
| const clusterProperties = reduce ? this._map(p, true) : null; | |||
|
|
|||
| // encode both zoom and point index on which the cluster originated | |||
| const id = (i << 5) + (zoom + 1); | |||
| const id = (i << 5) + (zoom + 1) + this.points.length; | |||
There was a problem hiding this comment.
If we're changing the meaning of cluster id, we should also change how it's decoded in methods like getLeaves, getChildren and getClusterExpansionZoom, otherwise those will break.
There was a problem hiding this comment.
Ok, updated decoding for getChildren and getClusterExpansionZoom -- I didn't see where it was decoded in the function series starting at getLeaves, am I missing something?
There was a problem hiding this comment.
getLeaves decodes it through getChildren so should be good 👍
index.js
Outdated
| if (id !== undefined) { | ||
| f.id = id; | ||
| } | ||
| f.id = c.numPoints ? c.id : (this.points[c.index].id || c.index); |
There was a problem hiding this comment.
This will break with points that have an id of 0. Better check for undefined.
There was a problem hiding this comment.
Also, instead of always assigning an id, I would put this behind an option (e.g. generateIds), like discussed in #97.
There was a problem hiding this comment.
@mourner how are options linked between mapbox gl and supercluster? does supercluster have access to the same set?
There was a problem hiding this comment.
currently the option is actually called generateId, so I'll put it behind that unless we are going to rename it to generateIds simultaneously...
There was a problem hiding this comment.
We relay the options manually in GL JS. Let's go with generateId though to be consistent with geojson-vt which already has that option.
index.js
Outdated
| if (id !== undefined) { | ||
| f.id = id; | ||
| } | ||
| f.id = c.numPoints ? c.id : (this.points[c.index].id || c.index); |
There was a problem hiding this comment.
currently the option is actually called generateId, so I'll put it behind that unless we are going to rename it to generateIds simultaneously...
| } | ||
|
|
||
| if (id !== undefined) f.id = id; |
There was a problem hiding this comment.
@mourner ok i
- put id assignment behind an undef check
- put id generation behind the
generateIdoption (not sure if that is directly avail from the mapbox gl options) - persisted already existing ids if the
generateIdoption is false, otherwise we overwrite it -- could see that being reversed
| @@ -235,7 +232,7 @@ export default class Supercluster { | |||
| const clusterProperties = reduce ? this._map(p, true) : null; | |||
|
|
|||
| // encode both zoom and point index on which the cluster originated | |||
| const id = (i << 5) + (zoom + 1); | |||
| const id = (i << 5) + (zoom + 1) + this.points.length; | |||
There was a problem hiding this comment.
Ok, updated decoding for getChildren and getClusterExpansionZoom -- I didn't see where it was decoded in the function series starting at getLeaves, am I missing something?
| const originZoom = decremented % 32; | ||
| return { originZoom, originId }; | ||
| } | ||
|
|
There was a problem hiding this comment.
is this the correct decoding @mourner
There was a problem hiding this comment.
Reviewed — this looks good, pending removal of yarn.lock.
One more stylistic change I'd personally make is break decodeClusterId into two methods so that we don't have to allocate a temporary object on calls to it (e.g. replace with _getOriginId(clusterId) and _getOriginZoom(clusterId)), but it probably doesn't make much difference.
|
@ChiefKleef hey, would you mind doing a final pass before I merge? |
|
This is a blocker for me. I'm going to fork and apply these changes, but it'd be nice to get this merged. |
|
@mourner Are the changes suggested in #121 (review) required before this can merge? |
|
@asheemmamoowala yes, pending removal of |
|
@ChiefKleef , this is ready to merge except the removal of the |
Here is my interpretation of the discussion here
I am unsure how to regenerate test data to fix the tests, can someone point me in the right direction? Is it a manual process?
I originally opened this bug, as I was using
cluster = trueandgenerateId = truefor my source specification. In this case, only the cluster points have ids, which breaks the use of setFeatureState per usual.Closes #97.