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

Exception adding layer when map has a non-integer max-zoom. #789

Closed
5 tasks done
carl-ranson opened this issue May 29, 2017 · 10 comments
Closed
5 tasks done

Exception adding layer when map has a non-integer max-zoom. #789

carl-ranson opened this issue May 29, 2017 · 10 comments

Comments

@carl-ranson
Copy link

carl-ranson commented May 29, 2017

  • I'm reporting a bug, not asking for help
  • I'm sure this is a Leaflet.MarkerCluster code issue, not an issue with my own code nor with the framework I'm using (Cordova, Ionic, Angular, React…)
  • I've searched through the issues to make sure it's not yet reported

How to reproduce

  • Leaflet version I'm using: 1.0.3
  • Leaflet.MarkerCluster version I'm using:1.0.5
  • Browser (with version) I'm using: Chrome 58.0.3029.110
  • OS/Platform (with version) I'm using: OSX Sierra 10.12.5

Create a leaflet map and specify an non-integer maxZoom in the options.
var mapOptions = {maxZoom: 16.5, zoomControl: false, attributionControl: false, constrainLon: false};
this.MapObj = leaflet.map(mapId, mapOptions);

  • step 2
    Later after adding a layer to the map, marker cluster gets into the MarkerClusterGroup._addLayer method. this fails when it tries to reference gridClusters 1.5 (note the minZoom is 2 in this case).

What behaviour I'm expecting and which behaviour I'm seeing

Exception as follows:
TypeError: Cannot read property 'addObject' of undefined
at NewClass._addLayer (leaflet.markercluster.js:1002)
at NewClass. (leaflet.markercluster.js:253)
at NewClass.addLayers (leaflet.markercluster.js:287)
at NewClass.onAdd (leaflet.markercluster.js:624)
at NewClass._layerAdd (ng-geobase-map.js:17383)
at NewClass.whenReady (ng-geobase-map.js:17025)
at NewClass.addLayer (ng-geobase-map.js:17441)
at InsightsMap.SetLayerVisibility (geobaseMap.js:466)
at addMarkers (geobaseMap.js:756)
at Scope.$digest (ionic.bundle.js:30244)
(anonymous) @ ionic.bundle.js:26799

Minimal example reproducing the issue

This seems to be a discrepency between the code in _generateInitialClusters and _addLayers. It looks as if _generateInitialClusters created the distances grids it needs, and then expects this line
this._topClusterLevel = new this._markerCluster(this, minZoom - 1);

to create the final level. In this case minZoom is 2 so it makes level 1, whereas it should have actually created level 1.5 (local var zoom -1). This is what the _addLayer code expects to see when it dereferences the array in line
gridClusters[z].addObject(lastParent, this._map.project(closest.getLatLng(), z));

  • this example is as simple as possible
  • this example does not rely on any third party code
    Sorry, haven't had time to work up an example for you, but hopefully it will be trivial given the information above. let me know if you really need me to provide a reproducer.

thanks,

@danzel
Copy link
Member

danzel commented May 29, 2017

Can we have a playground example or a unit test to reproduce this please?
http://playground-leaflet.rhcloud.com/beg/1/edit?html,output

@carl-ranson
Copy link
Author

here you go.
http://playground-leaflet.rhcloud.com/yayi/1/edit?html,output

I literally did nothing other than update the markercluster version number and add the maxZoom field to the map configuration object.

thanks,
CR

@carl-ranson
Copy link
Author

sorry, closed by accident

@carl-ranson
Copy link
Author

Do you need anything further from me on this?

@danzel
Copy link
Member

danzel commented Jun 10, 2017

https://gist.github.com/danzel/912c140d879f82518bd9f13e0648a2b3
Try this out, committing it now.

@carl-ranson
Copy link
Author

While it does fix the initial exception, I think you need to do something similar in _addLayer as well since that also accesses the gridClusters and gridUnclustered arrays. Cheers.

@danzel
Copy link
Member

danzel commented Jun 15, 2017

lol thanks, on it!

@danzel
Copy link
Member

danzel commented Jun 15, 2017

Ok changed.
https://gist.github.com/danzel/7570bd36ad29e3c40332b700e733fe81
I think this would have caused issues if you zoomed down to the bottom fractional zoom level before, we wouldn't have generated clusters for that level.

Please try that and report back! Thanks.

@carl-ranson
Copy link
Author

Thanks Danzel,
Seems to be fine now.

@danzel
Copy link
Member

danzel commented Jun 19, 2017

1.0.6 released with this included

@danzel danzel closed this as completed Jun 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants