-
Notifications
You must be signed in to change notification settings - Fork 155
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
Added ClusterMethodDistanceBased #83
base: master
Are you sure you want to change the base?
Conversation
BOOL respondsToSelector = [_clusterControllerDelegate respondsToSelector:@selector(mapClusterController:willReuseMapClusterAnnotation:)]; | ||
|
||
double zoomLevel = CCHMapClusterControllerZoomLevelForRegion(self.mapViewRegion.center.longitude, self.mapViewRegion.span.longitudeDelta, self.mapViewWidth); | ||
BOOL disableClustering = (zoomLevel > self.maxZoomLevelForClustering); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is never read in your code. We should not ignore this property.
When trying out the code I got an exception:
|
@amishjake I'd like to work on this algorithm, too. Do you have more resources to read about this algorithm in detail? Thx. |
In general I am wondering if it makes sense to offer different cluster algorithms to chose from. Why would you want to use the grid based algorithm when it obviously comes with some issues? I guess the library should just provide the best clustering possible in terms of performance and UX. |
This is a work in progress commit with a first working version of a distance based clustering algorithm. It’s based on ideas from choefele#83
Hey @amishjake I went through your code in more detail and I like the idea of the algorithm. As I've said before, I guess it doesn't make too much sense to ship the library with two algorithms (especially if one of them provides bad UX). Based on your PR I've changed the existing algorithm. I've changed everything to make the test suit pass. It's still work in progress but it's already usable. The only thing the is still missing is considering the As we have an urgent deadline in our project, I will first go with this solution and create an actual PR later on when the I have more time to add some performance tests and the missing support for You can have a look at the current state in my fork: https://github.com/mikrobi/CCHMapClusterController/tree/distance_clustering |
This is a drop-in replacement for CCHMapClusterOperation that addresses issue #82, the limitations of the grid-based algorithm. Now, when the operation is created in the controller, you can specify:
operation.clusterMethod = ClusterMethodDistanceBased; (defaults to grid based)
The algorithm is mostly translated from Google Maps Utilities, that I use in an Android app that handles map clustering nicely.
It appears to work correctly here in iOS with some amount of performance hit with larger 100+ datasets. I think it's worth it to not see the pins jump around illogically when making small incremental zoom motions.
I must admit I don't really understand the "unique locations" part of the original method, and I may have gotten some of the "reusing" existing annotations wrong, so let me know.