changing marker icon color causes full rebuild of marker clusterer due to ungraceful watch ?

[furiousOyster]

Apologies in advance for a not very helpful issue report....

Using markercluster plugin ( https://github.com/Leaflet/Leaflet.markercluster ) if I have 1000 markers, 500 clustered and 500 not clustered, using ExtraMarkers, then simply changing the color of a NON clustered marker causes the full rebuild of all the clustered markers.

I am still tracing down the exact cause of this, but it appears to be due to the way ui-leaflet watches markers - it seems to watch the ENTIRE marker array for any change whatsoever, then dumps the entire array.

I will post more info as I discover it.

[nmccready]

Yes this code needs to be cleaned up for sure. The data structure is oriented around iterating through everything for all markers in a hash or array. The problem is this is ripe for race conditions.

This comes down to a possible design flaw in how ui-leaflet is laid out to begin with. IE the markers directive is not scope based and each marker set belongs to one global blob. Contrast this to angular-google-maps where each markers set gets it's own directive and is independent in managing it's state. This boils down to all markers and all data for that matter is stored in a giant promised base hash leafletData.

Again this is awesome for the API user and for a lot of convenience factors in the API. But we need to break down leaflet data and this global data tracking down to the individual data set like markers1, and markers2 .

The big issue here is that we are relying on angular $watches to tells us when stuff is updated and take a decent guess on how to update the struct. $Watch in my opinion is a mess and should not be used. Using directiveControls to specifically update the data in how you see fit is currently the safest way.

I am totally open for re-design suggestion on how to better handle this data structure mess. A better change tracking system avoiding angular.$watch would be a great start.

[nmccready]

Related #79 #163 #61

[furiousOyster]

Thanks ! This is clearly too heavy a fix for me to do. Any suggestions for something cheap and nasty to get me around the problem ?

It works perfectly fine so long as I don't load the markercluster plugin.

[wpanas]

For sure markers should be handled separately by each instance of markers directive. Global access to data causes much more problems than there is advantages from its usage.

I don't really see dumping $watches completely. Could you explain, what you mean by directiveControls and especially how it would affect the usage of markers directive?

[nmccready]

This entire example is around not using watches to update the data.

https://github.com/angular-ui/ui-leaflet/blob/master/examples/0605-mixed-overlays-markers-nested-no-watch-example.html#L30-L31

[wpanas]

Ok, I checked it out. I get the idea with moving a trigger from $watch to user, but it seems odd. I have never seen similar solution in other angular libraries.

I my opinion, the main change that has to be done is to quit using leafletData. After this change, it won't be necessary to reset groups. Plainly with a lot of markers that causes problems.

[furiousOyster]

For reference, using the example with no watchers still produces this bug - change an icons color, even if it is not included in the clustering, and all clustered markers will rebuild causing massive GC and time lag.

[nmccready]

Depends on how you use use it. Did you try using $q.all? You need to only update all your rending in one step.

[nmccready]

@wpanas it is odd, but this is mapping and many things can happen at high load. This is where angular falls flat on it face. IE avoid ng-repeat, and avoid watches. This is documented everywhere these days. Anyway give me some advice on other ways.

At the end of the day if your map is updating frequently you can not compare mapping to the domain of other basic directives. I could see the case for some advanced features for (if they exist) in ui-grid to handle frequent updates. IE do they do anything special to avoid ng-repeat and watches.

Again, just because I am part of angular-ui does not mean I am an expert in every angular-ui library. So please forgive me for any ignorance.

Bottom line i'm looking for help here to have a go-path.

[nmccready]

Also keep in mind right now this bug is not an urgent need at my work and we have been working around it. So right now I can not support this fix unless it is on my time. My time is very limited these days, and I do not have time to brainstorm half baked ideas. So lets all brainstorm together here.

[wpanas]

@nmccready I got you. I was thinking last night what we can do better. As @furiousOyster mentioned, it does not matter whether he used promises or watches. I think that currently the most expensive operation is rebuilding. My last pull request made it even more expensive, so the problems began.

Resetting all groups cause that every time something is changed, groups must be rebuild. I came to conclusion that first of all I should fix this.

I want to make another pull request. This time I will change resetting all groups to deleting only those groups whose DOM element no longer exists.

I want to change also this line. Simple change of order and when isEqual is false, angular.equals never runs. We all know that it is very expensive operation.

If this issue isn't solved after those changes, we will think about dumping watches. If you agree with me, I will start coding :)

[nmccready]

Sure should skipping angular.equals be a choice? In watchOptions, or add markerOptions?

[wpanas]

This change doesn't change logic of the directive. It only avoids unnessesery angular.equals calls. When isEqual is false, whole line is always false, so why bother and check the equality? It's just a good practice. What you say, may I start?

[nmccready]

Ok yeah sounds good.

[nmccready]

Only time you need to check for equality is when you have deeply nested markers and you need to detect the change on the same reference. Otherwise we could just force immutability here and not worry about reference changes and always expect changes to come from new objects. This would need to be documented.

[wpanas]

@furiousOyster let us know if problem still occurs

[rustywebguy]

@nmccready on using this leafletData.getDirectiveControls is there any way to create or destroy one marker only?