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

Fix an issue with small-radius clusters and uncap clusterMaxZoom #10300

Merged
merged 2 commits into from
Jan 22, 2021

Conversation

mourner
Copy link
Member

@mourner mourner commented Jan 19, 2021

Closes #2898. Closes #6454. There were two issues here:

  • Close points sometimes not being clustered on max zooms with small cluster radius — a tricky numerical bug fixed by Fix small-radius clusters not always forming on higher zooms supercluster#173
  • clusterMaxZoom was always capped by source.maxzoom - 1, which is not always desirable. This PR changes behavior so that the option is respected without capping (although it's still maxzoom - 1 by default if not set). This means that you can set it in such a way that clusters occur at all zoom levels, including the max one, but this is no longer an issue because you can still recover original points if needed with source.getClusterLeaves, and we leave the decision whether to force the last zoom level to be unclustered to the user.

Launch Checklist

  • briefly describe the changes in this PR
  • write tests for all new functionality (covered in Supercluster)
  • document any changes to public APIs (the clusterMaxZoom behavior now matches the existing docs more closely)
  • manually test the debug page
  • tagged @zmiao to check whether the clusterMaxZoom change needs to be reflected in native
  • apply changelog label ('bug', 'feature', 'docs', etc) or use the label 'skip changelog'
  • add an entry inside this element for inclusion in the mapbox-gl-js changelog: <changelog>Fix a bug where close points where sometimes not clustered on higher zoom levels given a small clustering radius. Fix clusterMaxZoom so that it's not capped by source maxzoom.</changelog>

@mourner mourner self-assigned this Jan 19, 2021
@mourner mourner changed the title Fix an issue with small-radius clusters and uncap clusterMaxRadius Fix an issue with small-radius clusters and uncap clusterMaxZoom Jan 19, 2021
@mourner mourner requested a review from a team January 19, 2021 17:18
Copy link
Contributor

@asheemmamoowala asheemmamoowala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR changes behavior so that the option is respected without capping (although it's still maxzoom - 1 by default if not set). This means that you can set it in such a way that clusters occur at all zoom levels, including the max one, but this is no longer an issue because you can still recover original points if needed with source.getClusterLeaves

What do you think about adding a query+render test to assert this new behavior?

@zmiao
Copy link
Contributor

zmiao commented Jan 22, 2021

@mourner thanks for tagging me regarding this issue.
After checking on gl-native side, it turns out we are behaving similarly as how the fix is doing, i.e. taking the clusterMaxZoom with its original value if it is set, but with one difference from gl-js, that is if undefined, use default value uint8_t clusterMaxZoom = 17;, as the default max zoom is uint8_t maxzoom = 18; But I think we should update it to use option.maxzoom - 1 instead of 17.

Here is the source code link.

@mourner mourner force-pushed the fix-small-cluster-radius branch 2 times, most recently from f4b17a4 to 802fa78 Compare January 22, 2021 16:52
@mourner mourner force-pushed the fix-small-cluster-radius branch from 802fa78 to 7411e91 Compare January 22, 2021 16:53
@mourner mourner merged commit 1d0e3dc into main Jan 22, 2021
@mourner mourner deleted the fix-small-cluster-radius branch January 22, 2021 17:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Points are not clustered past z20 Fix bug causing clusters to disappear when "clusterRadius" is very small
3 participants