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

Some clusters do not transform into markers when clustering condition is based on map zoom level #774

Closed
tomsaju opened this issue Aug 13, 2020 · 7 comments · Fixed by #996
Assignees
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. released type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

Comments

@tomsaju
Copy link

tomsaju commented Aug 13, 2020

Summary

I used this library for clustering a set of points in google map. The requirement is such that clustering is based only on zoom level. On max zoom in , all items are unclustered and markers should be shown. But in some cases, the markers are not visible even on maximum zoom in.(Please note that on max zoom out even single markers are displayed as cluster with text 1)

Expected behavior

On tapping cluster(or zooming in manually) with single item, it should zoom in and turn into marker

Observed behavior

Map zoomed in, but cluster remained same. did not showed marker

Environment details

  1. Device make and model, or emulator details - Vivo 1723 (and multiple other devices)
  2. Android version - 8.1.0
  3. Library version and other environment information - Library version 2.0.3

Steps to reproduce

This can be reproduced using the demo app by making minor changes to CustomMarkerClusteringDemoActivity.
(I am minimizing the customization here)

  1. Add a Cluster Renderer to the activity with zoom based clustering condition and also a camera listener to get map zoom level
private class ZoomBasedRenderer extends DefaultClusterRenderer<Person> implements GoogleMap.OnCameraMoveListener{
        private Float mapZoomLevel = 15f;
        public ZoomBasedRenderer(Context context, GoogleMap map, ClusterManager<Person> clusterManager) {
            super(context, map, clusterManager);
        }

        @Override
        public void onCameraMove() {
            //map zoom level gets updated here on camera change
            mapZoomLevel =  getMap().getCameraPosition().zoom;
        }

        @Override
        protected boolean shouldRenderAsCluster(@NonNull Cluster<Person> cluster) {
         //cluster when mapZoomLevel is less than 12f. Else show as marker    
         return mapZoomLevel<12f;
        }
    }
  1. Initialise cluster manager by changing the startDemo(boolean isRestore)
@Override
    protected void startDemo(boolean isRestore) {
       //point camera to desired location 
       if (!isRestore) {
            getMap().moveCamera(CameraUpdateFactory.newLatLngZoom(new LatLng(18.550931, 74.115642), 9.5f));
        }

        mClusterManager = new ClusterManager<>(this, getMap());
        //set max distance to form a single cluster country wise on max zoom out
        mClusterManager.getAlgorithm().setMaxDistanceBetweenClusteredItems(300);
        
        //initialise renderer
        ZoomBasedRenderer renderer = new ZoomBasedRenderer(this,getMap(),mClusterManager);
        mClusterManager.setRenderer(renderer);
        getMap().setOnCameraIdleListener(mClusterManager);
        getMap().setOnMarkerClickListener(mClusterManager);
        getMap().setOnInfoWindowClickListener(mClusterManager);
        
        //set camera listener inside renderer
        getMap().setOnCameraMoveListener(renderer);
        mClusterManager.setOnClusterClickListener(this);
        mClusterManager.setOnClusterInfoWindowClickListener(this);
        mClusterManager.setOnClusterItemClickListener(this);
        mClusterManager.setOnClusterItemInfoWindowClickListener(this);

        addItems();
        mClusterManager.cluster();
    }

3)Modify AddItems() to return only two points. (I am returning two example points that this issue occur)

     private void addItems() {
        mClusterManager.addItem(new Person(new LatLng(18.528146, 73.797726), "Loc1", R.drawable.walter));

       
        mClusterManager.addItem(new Person(new LatLng(18.545723, 73.917202), "Loc2", R.drawable.gran));

    }
@tomsaju tomsaju added triage me I really want to be triaged. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. labels Aug 13, 2020
@nenick
Copy link

nenick commented Dec 21, 2020

I have the same issue. I found that the GridBasedAlgorithm and the default new ScreenBasedAlgorithmAdapter<>(new PreCachingAlgorithmDecorator<>(new NonHierarchicalDistanceBasedAlgorithm<T>())); behave differently.

With default NonHierarchicalDistanceBasedAlgorithm it stops calling shouldRenderAsCluster() after a certain zoom level (~11) when it was already called once. For zoom levels 1-11 this method is regularly called when you perform zoom actions. Above the zoom level 11 this method will be called once and then never again, until you zoom level is again below 11.
When there are two marks nearby, the behaviour does change and clustering works fine.

With the GridBasedAlgorithm the shouldRenderAsCluster() gets also called regularly when zoom level is above 11.

So the question is, how can we adjust that behaviour from the NonHierarchicalDistanceBasedAlgorithm?

Edit:
I found a dirty quick fix. Looks like the algorithm switch changed the behaviour but wasn't the root cause. The issue is an optimization in the DefaultClusterRenderer class to avoid unnecessary rendering if the clusters didn't change.

For the fix you have to reimplement (copy/paste) the DefaultClusterRenderer and disable the first lines from the run method in the inner RenderTask class.

public class DefaultClusterRenderer <T extends ClusterItem> implements ClusterRenderer<T> {
    ...
    private class RenderTask implements Runnable {private class RenderTask implements Runnable {
        ...
        public void run() {
            // as dirty quick fix disable these lines
            if (clusters.equals(DefaultClusterRenderer.this.mClusters)) {
                 mCallback.run();
                 return;
            }

@Program8
Copy link

I am also facing same issue . Library version is 2.2.0

@amatkivskiy
Copy link

Also having the same issue 😞

@barbeau barbeau added priority: p2 Moderately-important priority. Fix may not be included in next release. and removed triage me I really want to be triaged. labels Sep 22, 2021
barbeau added a commit that referenced this issue Sep 22, 2021
@barbeau
Copy link
Collaborator

barbeau commented Sep 22, 2021

I've confirmed that I can reproduce this issue in this branch:
https://github.com/googlemaps/android-maps-utils/tree/sean/test-774-zoom-cluster

I can also confirm that commenting out the below lines in DefaultClusterRenderer.RenderTask.run() as discussed in #774 (comment) does force correct behavior:

        @SuppressLint("NewApi")
        public void run() {
            // Commenting out these lines forces correct behavior
//            if (clusters.equals(DefaultClusterRenderer.this.mClusters)) {
//                mCallback.run();
//                return;
//            }

            final MarkerModifier markerModifier = new MarkerModifier();

@NizarETH
Copy link

Also having the same issue shouldRenderAsCluster() not working in all zoom levels.

@barbeau
Copy link
Collaborator

barbeau commented Sep 30, 2021

Hi all! I've opened a pull request here that should hopefully address this use case - #996.

barbeau added a commit that referenced this issue Oct 19, 2021
… when cluster doesn't change (#996)

In the current version of DefaultClusterRenderer, there is a built-in optimization to avoid re-rendering items when there isn't a change in the clusters.

However, as discussed in #774, this prevents use cases where the clusters/markers should be re-rendered even if the cluster contents don't change.

For example, if you want a single item to render as a cluster above a certain zoom level and as a marker below a certain zoom level, this isn't possible - the item will never be updated from the cluster to the marker because that rendering pass is skipped (because the cluster contents didn't change).

This PR adds a new DefaultClusterRenderer.shouldRender() method that apps can override to control this optimization when necessary. This allows the above use case as the app can track the previous and current zoom levels and force the render to occur when the transition over the zoom threshold is made.

A new demo activity ZoomClusteringDemoActivity (shown as "Clustering: Force on Zoom" in the demo app UI) (gms and v3 flavors) is added as part of this PR to clearly illustrate how shouldRender() is used. This demo also illustrates what I believe is an undocumented feature of the library, which is that the ClusterManager will call the onCameraIdle() implementation of any Renderer that implements GoogleMap.OnCameraIdleListener before clustering and rendering takes place. This allows us to capture the zoom level that can then be used in shouldRender().

This feature is backwards-compatible, as the current behavior of apps will not change - the default implementation of shouldRender() is the same as the current version of the library.

Closes #774 🦕
googlemaps-bot pushed a commit that referenced this issue Oct 20, 2021
# [2.3.0](v2.2.6...v2.3.0) (2021-10-20)

### Features

* Add overridable shouldRender() method for use case of rendering when cluster doesn't change ([#996](#996)) ([165534c](165534c)), closes [#774](#774) [#774](#774)
@googlemaps-bot
Copy link
Contributor

🎉 This issue has been resolved in version 2.3.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. released type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
7 participants