fix: fixed marker flickering caused by creating new instances on each render

[nora-soderlund]

This PR aims to resolve #439 by reusing the previous markers where possible, by comparing exact coordinates on clusters with a "processed" marker (Cluster.marker).

This also fixes part of #384. This will keep the bounce animation while zooming in and out, but it will stop when the marker is turned into a cluster and won't bounce when turned back into a marker again, explained here.

HtbxIet.mp4

This is how it was previously, taken from #439:

192104133-b37be700-7e72-44f8-898c-b9b8bb91f1cb.webm

[daun]

Can confirm this helps a lot to reduce flicker on zoom! Would love to see this merged or as an optional setting.

[amuramoto]

@nora-soderlund apologies for the slow reply - this looks great. Can you add a test to cover it then I can merge?

[nora-soderlund]

Will do next week.

[vicb]

I am seeing flicker with advanced markers (on v2.1.3).

Would fixing this issue also impacts advanced markers?

Let's try to move this forward!

[nora-soderlund]

Will do next week.

oops, looking into this now!

[vicb]

Thanks for all the updates!

I think it might be worth trying the map solution I proposed in a comment, something like:

function clusterKey(cluster: Cluster) {
  const position = cluster.getPosition();
  return `${position.lat()}#${position.lng()}`;
}

// ...
        const clusterMap = new Map<string, Marker>();
        this.clusters.forEach(cluster => {
          if (cluster.marker) {
            clusterMap.set(MarkerUtils.clusterKey(cluster), cluster);
          }        
        });  
    
        clusters.forEach((cluster, index) => {
          const clusterKey = clusterKey(cluster);          
          const duplicateCluster = clusterMap.get(clusterKey);

          if(duplicateCluster) {
            // we'll reset the Cluster.markers here and only add the existing item
            // down below in renderClusters, it will assign this one item to Cluster.marker
            clusters[index].markers.length = 0;
            clusters[index].markers.push(duplicateCluster.marker);
            duplicateCluster.marker = null;
            clusterMap.delete(clusterKey);
          }
        });
// ...

At least time it vs find with a large nb of markers and see what's more efficient

[nora-soderlund]

Thanks for all the updates!

I think it might be worth trying the map solution I proposed in a comment, something like:

// ...

At least time it vs find with a large nb of markers and see what's more efficient

At the very least in my browser, string comparisons took up to 27 times longer:

Edit: invalid test

[nora-soderlund]

@amuramoto ready for review, added advanced marker support and a proper test case.

[vicb]

@nora-soderlund thanks for trying. That was my fear but we can do better, switching from:

const clusterMap = new Map<string, Marker>();

to

const clusterMap = new Map<number, Map<number, Marker>>();

The idea would be lat -> lng -> Marker.

It should be both way faster and more memory efficient.

[vicb]

Also I am not sure what you tested, can't see a Map there?

[amuramoto]

@vicb Since this PR resolves the flickering issue as described, how about we merge and open a new issue on the potential optimizations and continue this there?

[vicb]

@amuramoto I saw a clear lag during my tests - o(n^2) can become slow with big ns.

[nora-soderlund]

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Map works differently than I thought, it is indeed quicker. Updated.

[vicb]

I think #536 (comment) is worth considering.

It will probably be a few more LOCs but should be faster (do not create string keys, comparing number vs long string) and more memory efficient (numbers take way less memory than this string key).

[vicb]

And also you need to record.delete() to only re-use a cluster once - see my original snippet

[nora-soderlund]

I will leave it as it is now.

[vicb]

@nora-soderlund I can take over to implement new Map<number, Map<number, Marker>>() if you prefer. Will need to find some time over the next couple days. Let me know.

[vicb]

I was not keen on merging that PR earlier also because of a corner case.

Imagine you have markers aligned on the equator at lon -10, -1, 1, and 10 and you zoom out.

There will first be a cluster at 0 of -1 and 1
Then zooming out more will also aggregate -10 and 10 but the cluster position will still be 0, only the count changes.

I would like to see a test for that.

The fix I think will be needed is to use Map<number, Map<number, Map<number, Marker>>> which would be lat -> lng -> count.

Sorry for my fragmented comments it only reflect my fragmented spare time

[vicb]

I had some time to give this CL more thoughts.

What the original code does:

In MarkerClusterer.render when the Clusters have changed they are all removed from the map at once (via this.reset()) and then re-added to the map one by one in renderClusters via MarkerUtils.setMap(cluster.marker, map).

The flicker is caused by removing + re-adding the Clusters.

Note that a Cluster (i.e. the class name) could either be an individual maker or a "Cluster" (=group) of marker represented by a a marker created by the renderer.

What this CL changes:

This CL do no remove all the markers from the map.

It would only remove a marker is there was no marker at the exact same (lat, lon) in thr previous render iteration.

This is fine when the Cluster contains a single marker.
However it has corner cases when the Cluster was a group of multiple marker.
I added one example in my previous message: the Cluster can be at the same position but have a different number of markers.

But more generally a group icon depends on more things: this.renderer.render(cluster, stats, map). stats here should also be taken into account. Let's imagine you are zooming in or out but the grouping does not change. It is still likely that the stats would change.

In conclusion I think that the assumption of a group position not changing meaning that the rendering does not change is flawed. The count can change, the stats can change.

I think this can be solved by adding a Cluster.shouldRerender(stats) method. This could be an optional method to keep backward compatibility. If the method is not implemented by the renderer we should keep a conservative approach and re-render groups every time.

Thoughts?

[amuramoto]

@vicb I think there may be an easy solution here that relates to your comment 'This is fine when the Cluster contains a single marker.' Assuming two things:

1. The goal here is to get the individual markers to not constantly re-render which causes flickering
2. Actual 'clusters', i.e. clusters where Cluster.markers > 1 likely should always re-render anyway since their position is influenced by zoom-level change, and/or the addition/subtraction when the number of markers in the cluster changes

So, can't we just adjust what's in this PR to only do the duplicateCluster check if Cluster.markers === 1? This would apply the logic to only individual markers and not clusters of multiple markers, which I think avoids the edge case you're describing.

On a side note, I think there is one additional edge case in this PR. Since records is a Map, we can't track duplication of multiple markers that are at the same lat/lng. Should be fairly easy to solve that tho.

[vicb]

Some quick comments

Actual 'clusters', i.e. clusters where Cluster.markers > 1 likely should always re-render anyway since their position is influenced by zoom-level change, and/or the addition/subtraction when the number of markers in the cluster changes

A marker is re-used only if there was a marker at the exact same location in the previous iteration.
Depending on your data it could be fairly common that you don't have to re-render (zoom in/out without changing the markers in a cluster).

So, can't we just adjust what's in this PR to only do the duplicateCluster check if Cluster.markers === 1?

That sounds like a reasonable start to me.
Tackle non-grouped Clusters first and create a bug/PR for the grouped Clusters.

Since records is a Map, we can't track duplication of multiple markers that are at the same lat/lng

I quickly thought about it and concluded that it would never happened because those would be grouped.
But yeah it can probably happen:

• With the noop algo,
• At high enough zoom where no clustering is done.

This one should be easy to solve by using a Map of Marker[] instead of Marker.
Might need more thinking because we're not guaranteed that all those those markers are the same (i.e. they could use a different icon). In such a case we do not want to swap them. Probably not hard to check but need to check. (edit: a clearer case is 2 markers at the same location, one gets removed - we should make sure it would not re-appear).

I think the next steps are:

• Update this PR
  • to only affect Clusters of length 1
  • to use `Map<number, Map<number, Marker[]>>
  • add test for multiple markers at the same location
• Create a new bug for
  • cover the grouped Clusters (see shouldRerender() above)
  • add tests for corner cases

[amuramoto]

Sounds like a plan to me!

@nora-soderlund Any thoughts on this approach? Me and @vicb would be happy to contribute the changes to this PR

[nora-soderlund]

Feel free to push changes. :)

[vicb]

It took some discussion but I really like the solution we are shaping together.

Starting with Clusters with only one marker was a really great idea.

I think the implementation could be very simple.
There is no need to check the marker position.
The only question we need to answer is: was this marker already rendered?

When it was then there is nothing to do. That is no need to remove/add it back (via setMap(null)/ setMap(map)) which means no flicker.

When it was not, we only need to add it to the map (via setMap(map)).

Checking the position would still be useful for cluster with > 1 markers.

I probably won't have time to work on that until next week.

[nora-soderlund]

Closing in favour of #660.

[amuramoto]

Thank you for your work on this @nora-soderlund !