Support maxZoom >= 31 without corrupting cluster ids - #262
Open
spokodev wants to merge 1 commit into
Open
Conversation
The cluster id packs the origin zoom into the low 5 bits (`<< 5`, `>> 5`, `% 32`), which can only hold `zoom + 1` up to 31 — i.e. maxZoom <= 30. Two coincident (or Float32-coincident) points always cluster at zoom-pass = maxZoom, so with `maxZoom >= 31` the `zoom + 1` value (>= 32) overflows the zoom field into the origin-index bits. Decoding then yields the wrong origin id and zoom, and `getChildren` / `getLeaves` / `getClusterExpansionZoom` throw "No cluster with the specified id." on input the library silently accepts (there is no documented upper bound on maxZoom). Size the zoom field to the configured maxZoom (`zoomBase`, a power of two >= 32) and pack/unpack with `*` / `/` / `%` instead of the fixed 5-bit shifts. `zoomBase === 32` for every maxZoom <= 30, so existing cluster ids are unchanged; using arithmetic instead of the 32-bit bitwise operators also removes a latent overflow for very large point counts. Fixes mapbox#221, mapbox#243.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The cluster id packs the origin zoom into the low 5 bits:
Five bits hold
zoom + 1only up to 31, i.e.maxZoom <= 30. Two coincident(or Float32-coincident) points always cluster at zoom-pass =
maxZoom, so withmaxZoom >= 31thezoom + 1value (>= 32) overflows the zoom field into theorigin-index bits. Decoding then returns the wrong origin id/zoom, and
getChildren/getLeaves/getClusterExpansionZoomthrow"No cluster with the specified id." on input the library silently accepts —
maxZoomhas no documented upper bound (README: default 16).This is the symptom in #221 (
maxZoom: 999→ "originId is too high") and #243("behavior strange when zoom above 20").
Fix
Size the zoom field to the configured
maxZoom—zoomBase, the smallest powerof two
>= 32that can holdmaxZoom + 1— and pack/unpack with*///%instead of the fixed 5-bit shifts.
zoomBase === 32for everymaxZoom <= 30,so existing cluster ids are byte-identical (the current tests, which assert
hardcoded ids 164/196/581/2504, still pass unchanged). Using arithmetic instead
of the 32-bit bitwise operators also removes a latent
>>overflow for verylarge point counts.
Test
Added a case asserting
getLeaves/getChildren/getClusterExpansionZoomwork for
maxZoomof 31, 32 and 40. Fails onmain("No cluster with thespecified id."), passes with the fix; the existing 15 tests are unchanged.
Fixes #221, #243.