ashare80 / TSClusterMapView

MKMapView with animated clustering for iOS and OSX
MIT License
149 stars 21 forks source link

Not zooming enough on a cluster to divide it into pins #7

Closed kevin-hirsch closed 9 years ago

kevin-hirsch commented 9 years ago

Hello @ashare80,

I noticed a small bug. When you tap on a cluster, the map is zooming on this cluster, then dividing it into several pins/sub-clusters. But when a cluster is englobing pins that are too close, tapping on this cluster is not zooming anymore/at all and is not dividing into pins. I have to manually zoom by hand to see the cluster dividing into pins.

You can test that bug on your demo app. I made a video to show you. Look at the "10" cluster bubble in the center of the screen, when tapping on it, it zooms and I see a "7" cluster bubble appears. But then, when I tap on this "7" cluster bubble, it stays at the same zoom level like it was the maximum zoom level and so I cannot see the pins contained by this "7" cluster bubble. The video is here: http://fr.tinypic.com/r/2dhs8y0/8

Could you take a look at this problem?

Thanks.

ctaskin commented 9 years ago

I am having the same problem, hope you fix it soon

ashare80 commented 9 years ago

This bug will be fixed in the next update. For some reason if you tell the map view to zoom to a map rect that is past the maximum internal zoom level it decides to just not zoom at all instead of zooming to the maximum level.

kevin-hirsch commented 9 years ago

Thanks!

ctaskin commented 9 years ago

Thanks @ashare80, Do you have an ETA on the next update?

ashare80 commented 9 years ago

@bigtigercahit Most likely this weekend

ctaskin commented 9 years ago

Great! Thanks...

ashare80 commented 9 years ago

@bigtigercahit @kevin-hirsch Pushed updates to the develop branch to fix this issue and others. See if it helps and I'll go ahead and merge into master and update the pod as well.

ctaskin commented 9 years ago

Hi @ashare80 the zooming issue is better now but there are some new weird stuff happening. When I distribute the cluster, sometimes moving the map without changing the zoom regroups them. Another issue is sometimes clicking on the last cluster (it has multiple annotations with same coordinates) makes the map zoom out and zoom in instantly, which causes re-clustering and weird annotation actions. I will keep testing and let you know if there is anything else.. Thanks

kevin-hirsch commented 9 years ago

It works better now but tapping on a cluster with pins that are close is not zooming enough to split them the first time. Each time you tap on the cluster, it zooms until the cluster is able to split into different pins. Video here: http://fr.tinypic.com/r/28r38n5/8

ashare80 commented 9 years ago

@kevin-hirsch Just pushed an update that should help. I converted the mapRect size to map camera altitude to more accurately zoom without using the setRegion or setVisibleMapRect, both of which get stuck at at a mapRect width of 3000.

ashare80 commented 9 years ago

@bigtigercahit "sometimes moving the map without changing the zoom regroups them"

There are two ways to minimize this and you'll have to find a good middle ground between performance (smaller values) and accuracy (larger values).

  1. Increase the edgeBufferSize.
  2. Increase clusterPreferredVisibleCount (probably what you need)

Here's some info on how it all works so you can determine what is best for your use case. I'm looking into making this a bit more dynamic but for the time being you'll have to set these values manually for your use case.

edgeBufferSize: (UISegmentedControl in demo) In order to make the library more efficient the clustering operation only takes into account the visible region of the map. If you change the visible region even without zooming the clustering operation will need to run again to take into account the new visible area. One of the new features I added was the clusterEdgeBufferSize property. Setting this to a larger size will increase the mapRect passed to the cluster operation outside of the visibleRect thus giving a smoother result when panning the map.

clusterPreferredVisibleCount: (UISlider in demo) This limits the number of annotations visible on screen at one time to speed up clustering and makes sure the map isn't too cluttered. This number is automatically increased when the edgeBufferSize is increased to allow extra annotations off screen. Clustering will still take into account the clusterAnnotationViewSize property to minimize overlap so you'll usually end up with less on screen than the amount set.

ashare80 commented 9 years ago

@bigtigercahit Just pushed a fix for the "clicking on the last cluster" part.

kevin-hirsch commented 9 years ago

Thank you @ashare80, I will test the new commits on develop tomorrow morning (GMT+1)

kevin-hirsch commented 9 years ago

Hello @ashare80, I have tested you latest commits, it improves a lot the zooming on small clusters but I notice 2 things:

  1. When I tap on a small cluster (= cluster with pins very near each other), it is zooming to the correct zoom level but not dividing into pins. If I tap again, there is no more zoom but the cluster just divides itself into the pins. Video is better than words.
  2. When zooming on any cluster, the map is shifting a little bit after the zoom animation. It is like if the velocity of the zoom that is stopped transform itself into a map shift. I don't know if I express myself correctly on this. It is not that important but here's the video if you want to take a look at it.
ashare80 commented 9 years ago

@kevin-hirsch

1: Yeah that's kind of a work around I've built in since the zooming on a cluster and the actual clustering are two independent parts. The clustering operation does no know that you tapped on a cluster it just knows that the visible region has changed and should update accordingly. Once you've hit or come close to the max zoom level of the map view I just make the next tap force a split.

I'm going to work on creating a better integration between the two parts so that on a cluster tap, I'll pass into the clustering operation the cluster that needs to be forced to split but it will take some time.

You may be able to remedy this by increasing the clusterPreferredVisibleCount

2: The zoom animations are all standard mapkit animations (setRegion:animated: and setCamera:animated:) so I'm not sure if I can do anything about that.

kevin-hirsch commented 9 years ago

@ashare80

Thanks! I'll be testing clusterPreferredVisibleCount and tell you if it changes something ;)

No problem for the little shift when zooming, it is not very disturbing so it is okay. But I have found this thread on stackoverflow, maybe trying this could solve the problem:

[UIView animateWithDuration:3.0 animations:^{
    self.mapView.camera = nextCam;
}];
ashare80 commented 9 years ago

@kevin-hirsch Ah so I did some testing and that doesn't fix it but I see the problem. That reaction you are seeing is what the mapView does when it hits the "brick wall" of the max zoom level. I'll just limit the altitude of the zoom animation so that it never goes past the around 300 meters maximum.

kevin-hirsch commented 9 years ago

@ashare80: Okay, so any cluster will divide into pins at the altitude of 300 meters no matter the pins approximation?

ashare80 commented 9 years ago

You can check out what I'm doing here: https://github.com/ashare80/TSClusterMapView/blob/develop/Pod/Classes/TSClusterMapView.m#L729-L764

If you are currently < 500 meters altitude I'm not zooming and just divide the individual cluster

If you are currently > 500 meters and the zoom level of the cluster annotation map rect puts you at an altitude < 280 meters (around max zoom level, it varies by region) I just zoom as close as possible to make sure the animation doesn't hit the bottom and do that "little shift when zooming"

kevin-hirsch commented 9 years ago

@ashare80: Okay perfect! I'll test it tomorrow morning (GMT+1) ;)

kevin-hirsch commented 9 years ago

@ashare80, Sorry but, there's still a problem: I have to tap twice on a cluster to divide it into pins.

  1. First tap = zoom
  2. Second tap = dividing

Video here.

ashare80 commented 9 years ago

@kevin-hirsch Yes. That's as good as it will get with the current implementation. Will work on fixing that but it will take some time because it requires changes to how the clustering algorithm works.

The clustering happens on regionDidChange so once you tap and the zoom occurs, everything is reclustered and at this point the clustering does not take into account that you previously selected an annotation. Since the annotations are so close they end up staying clustered. The work around at this point is what you are seeing, that once you can't zoom anymore the tap forces a cluster to split.

I'll keep you posted on when that get's fixed.

kevin-hirsch commented 9 years ago

@ashare80: Okay, I understand. Keep me posted. Thanks!

ashare80 commented 9 years ago

@kevin-hirsch pushed code that should improve the issue

kevin-hirsch commented 9 years ago

@ashare80: Great! It seems to work well :) Thank you :+1:

Should I close this issue or do you still want to improve something for this issue?

ashare80 commented 9 years ago

@kevin-hirsch I'm going to close this for now if it's working well