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 cluster radius scaling in setClusterOptions #5055

Merged
merged 4 commits into from
Nov 17, 2024

Conversation

ciscorn
Copy link
Contributor

@ciscorn ciscorn commented Nov 17, 2024

The calculation for superclusterOptions.radius in GeoJSONSource.setClusterOptions is missing the necessary scaling, resulting in incorrect values.

For reference, the relevant initialization code can be found here:

const scale = EXTENT / this.tileSize;

radius: (options.clusterRadius || 50) * scale,

Note: setClusterOptions was introduced in #1998.

Launch Checklist

  • Confirm your changes do not include backports from Mapbox projects (unless with compliant license) - if you are not sure about this, please ask!
  • Briefly describe the changes in this PR.
  • Link to related issues.

@ciscorn ciscorn force-pushed the fix-change-cluster-options branch from 8c725b6 to 8d68acd Compare November 17, 2024 05:57
Copy link

codecov bot commented Nov 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.06%. Comparing base (bc43205) to head (0afac35).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5055      +/-   ##
==========================================
- Coverage   89.06%   89.06%   -0.01%     
==========================================
  Files         269      269              
  Lines       38318    38320       +2     
  Branches     2362     2366       +4     
==========================================
+ Hits        34129    34130       +1     
+ Misses       3187     3183       -4     
- Partials     1002     1007       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@HarelM
Copy link
Collaborator

HarelM commented Nov 17, 2024

Thanks! I would advise to add a test to this case or modify an existing one.
Also consider making a small method out of this if it is used both for initialization and update.

@ciscorn ciscorn force-pushed the fix-change-cluster-options branch from 8d68acd to f99007d Compare November 17, 2024 06:04
@ciscorn
Copy link
Contributor Author

ciscorn commented Nov 17, 2024

@HarelM
Thanks. I've updated the expected values in the existing test and refactored the logic into a separate method.

@HarelM
Copy link
Collaborator

HarelM commented Nov 17, 2024

Thanks!
Can you add a changelog item?

extent: 8192,
radius: 1600,
extent: EXTENT,
radius: 100 * EXTENT / 512,
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the hard coded 512?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I’ve replaced the hardcoded 512 with source.tileSize.

@@ -182,7 +182,7 @@ export class GeoJSONSource extends Evented implements Source {
maxZoom: options.clusterMaxZoom !== undefined ? options.clusterMaxZoom : this.maxzoom - 1,
minPoints: Math.max(2, options.clusterMinPoints || 2),
extent: EXTENT,
radius: (options.clusterRadius || 50) * scale,
radius: this._scaleClusterRadius(options.clusterRadius || 50),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we use the same method to scale the buffer input field, and rename it accordingly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I renamed the method to _pixelsToTileUnits and applied it to scale those fields as well.

@HarelM
Copy link
Collaborator

HarelM commented Nov 17, 2024

Added a few nitpicking, looks good otherwise, THANKS!

@ciscorn ciscorn requested a review from HarelM November 17, 2024 12:05
Copy link
Collaborator

@HarelM HarelM left a comment

Choose a reason for hiding this comment

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

Thanks!

@HarelM HarelM merged commit b9e001d into maplibre:main Nov 17, 2024
17 checks passed
@ciscorn ciscorn deleted the fix-change-cluster-options branch November 17, 2024 13:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants