android-maps-utils icon indicating copy to clipboard operation
android-maps-utils copied to clipboard

Missing animation when overridden shouldRenderAsCluster uses zoom level to decide

Open blurpy opened this issue 8 years ago • 7 comments

Summary:

Using zoom level to decide if a cluster should be rendered makes the animation of markers moving from the clusters towards their final position stop working. The markers just "appear".

Steps to reproduce:

Use a custom renderer like this to disable clusters on zoom level 15 and closer: https://github.com/blurpy/android-maps-utils/blob/60398d9190028c82d231db69ba8404cfcb3274c3/demo/src/com/google/maps/android/utils/demo/CustomAnimationNotWorkingClusterRenderer.java

Zoom to you reach level 15.

I have a fork of the code with a modified demo app using this renderer: https://github.com/blurpy/android-maps-utils/tree/zoom-animation-bug

Expected behavior:

All zoom levels should display markers animating in and out of clusters.

Observed behavior:

Animations work on all zoom levels except the last one. The renderer has specified that no clusters should be rendered at zoom level 15 or closer. When reaching level 15 the markers just pop up instead of animating out of clusters.

Solution?:

I don't understand all the details of how this framework works, but I noticed that having two different callbacks to decide if a cluster should be rendered made animations work. Seems like the framework expects shouldRenderAsCluster to be "stable", as in returning the same result based on the same input. Using zoom level in the decision breaks that assumption.

See https://github.com/blurpy/android-maps-utils/commit/60398d9190028c82d231db69ba8404cfcb3274c3

This commit contains a custom renderer enabled by default that overrides the old shouldRenderAsCluster, and one that uses the new shouldRenderAsClusterForAnimation. Both tries to disable clusters at zoom level 15. Switch between them in the ClusteringDemoActivity to see the difference in behavior.

The change I made to make this work seems a bit hacky to me, so I didn't submit a pull request. Also don't know if it breaks something, but I'm using this in an app at the moment and haven't noticed any issues.

Note: this exact same issue (and workaround) happens in the iOS version of the framework (https://github.com/googlemaps/google-maps-ios-utils) as well.

blurpy avatar Jul 17 '17 11:07 blurpy

They fixed this properly in the iOS version (referenced above). Could a similar fix be applied here as well perhaps?

blurpy avatar Jul 29 '18 13:07 blurpy

This is still unresolved on Android :/

dimgrav avatar Aug 04 '18 12:08 dimgrav

+1. Would love if this could be fixed upstream; I'm using @blurpy 's fix for now. Thanks @blurpy!

aphexcx avatar Mar 07 '19 23:03 aphexcx

This issue has been automatically marked as stale because it has not had recent activity. Please comment here if it is still valid so that we can reprioritize. Thank you!

stale[bot] avatar Oct 03 '19 07:10 stale[bot]

Closing this. Please reopen if you believe it should be addressed. Thank you for your contribution.

stale[bot] avatar Nov 02 '19 07:11 stale[bot]

Fix from blurpy does not work for me. My dirty quick fix is also to reimplement (copy/paste) the DefaultClusterRenderer and remove one shouldRenderAsCluster check. It produced no issues yet.

public class DefaultClusterRenderer<T extends ClusterItem> implements ClusterRenderer<T> {
    ...
    private class RenderTask implements Runnable {
        ...
        public void run() {
            ...
                    // Remove that shouldRenderAsCluster check and animation will work.
                    if (shouldRenderAsCluster(c) && visibleBounds.contains(c.getPosition())) {
                        Point point = mSphericalMercatorProjection.toPoint(c.getPosition());
                        existingClustersOnScreen.add(point);
                    }

nenick avatar Dec 28 '20 11:12 nenick

I just upgraded this library in my app from v0.5 that I have been using since posting this issue, to v3.4.0.

I was hoping the issue had been fixed since it's been 6 years and it's been closed, but with the stock DefaultClusterRenderer the same issue is still present.

Is there some officially supported way to disable clusters on certain zoom levels in the newer versions? There is a new method called shouldRender() that mentions zoom level, but the only thing I was able to accomplish with it is not getting anything rendered below a certain zoom level, which is not quite what I'm looking for.

Unlike @nenick I was able to get my original workaround to function exactly as before by patching DefaultClusterRenderer, so the workaround is still valid.

Unless there is a new way to handle this issue, then it should be reopened.

blurpy avatar Apr 16 '23 11:04 blurpy