googlemaps / android-maps-utils

Maps SDK for Android Utility Library
https://developers.google.com/maps/documentation/android-sdk/utility
Apache License 2.0
3.55k stars 1.53k forks source link

Missing animation when overridden shouldRenderAsCluster uses zoom level to decide #408

Open blurpy opened 7 years ago

blurpy commented 7 years ago

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 commented 6 years ago

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

dimgrav commented 6 years ago

This is still unresolved on Android :/

aphexcx commented 5 years ago

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

stale[bot] commented 5 years ago

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] commented 5 years ago

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

nenick commented 3 years ago

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);
                    }
blurpy commented 1 year ago

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.