search

Raruto / leaflet-rotate

Leaflet plugin that allows to add rotation functionality to map tiles
GNU General Public License v3.0
77 stars 21 forks source link

Fix issue where sometimes `L.Map::getBoundsZoom(bounds)` depends on the current zoom level #27

Closed rphlo closed 1 year ago

rphlo commented 1 year ago

Closes: https://github.com/Raruto/leaflet-rotate/issues/25

Raruto commented 1 year ago

@rphlo What about just trying to override the latLngToLayerPoint function as follows? (just to remove the _round call)

latLngToLayerPoint: function (latlng) {
  if (!this._rotate) {
    return mapProto.latLngToLayerPoint.apply(this, arguments);
  }
  return this.project(L.latLng(latlng))._subtract(this.getPixelOrigin());
},

Doing so I don't know what can be broken, but hopefully that patch will be integrated directly on the leaflet side (sometime in the future.. 🤞)

For further information:

rphlo commented 1 year ago

Doing so I don't know what can be broken, but hopefully that patch will be integrated directly on the leaflet side (sometime in the future.. 🤞)

I didn't feel like risking a thing that might affect other part of leaflet, but if you think it is safe, I can provide a PR that do so.

BTW, I didn't manage to run npm run build without errors, is there any trick I missed?

Raruto commented 1 year ago

I didn't feel like risking a thing that might affect other part of leaflet, but if you think it is safe, I can provide a PR that do so.

In general, given the potential intrinsic complexity of this plugin, I prefer to keep the code small and readable (fewer functions also means less manual checks within the upstream leaflet repository, especially in between various updates...).

So the ideal would be to just fix getBoundsZoom() or mapBoundsToContainerBounds() functions (I'm open to suggestions in this direction 😄).

Anyway, from what I understand the main problems you might have overriding latLngToLayerPoint() in that way could be:

  1. an overall deterioration in performance (it depends a bit on the desired performance level..)
  2. a blur effect on map overlays on Safari devices (I'm unable to test..)

As for available tests, I imagine that already now when using this plugin those present on the leaflet repository will not pass:

BTW, I didn't manage to run npm run build without errors, is there any trick I missed?

Maybe we are working with different node versions, exactly what error are you seeing?

Raruto commented 1 year ago

Patch released in v0.2.4