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

PolylineDecorator.getBounds() throws error #74

Closed
boromisp opened this issue Dec 8, 2017 · 5 comments
Closed

PolylineDecorator.getBounds() throws error #74

boromisp opened this issue Dec 8, 2017 · 5 comments
Labels

Comments

@boromisp
Copy link

boromisp commented Dec 8, 2017

I've run into an issue when trying to find the bounds of a PolylineDecorator.

Since PolylineDecorator is a FeatureGroup, it has getBounds method.
That method will try to iterate all of its layers and call getBounds or getLatLng on those.
But because the child layers are instances of LayerGroup, they do not have either method, and so a TypeError: e.getLatLng is not a function error is thrown.

Since the 1.3.0 release the bounds of the child layers would not even be the real bounds of a PolylineDecorator - the closest we could get now is the bound of its paths.

These changes work for my use-case, I could make a PR of them, if it would get merged:

L.PolylineDecorator = L.FeatureGroup.extend({
    /* ... */

    setPaths: function(paths) {
        /* ... */
        this._bounds = this._initBounds();
    },

    initialize: function(paths, options) {
        /* ... */
        this._bounds = this._initBounds();
    },

    _initBounds: function () {
        var bounds = L.latLngBounds(this._paths[0]);
        for (var i = 1; i < this._paths.length; ++i) {
            bounds.extends(L.latLngBounds(this._paths[i]))
        }
        return bounds;
    },

    getBounds: function () {
      return this._bounds;
    }
});
@bbecquet
Copy link
Owner

Hi, and sorry for waiting so long to answer.

You're right about the bug and what's causing it. The lack of a getBounds method on LayerGroup is one of the strangest things in the Leaflet class hierarchy. I remember an old issue in Leaflet about that, but it was closed by maintainers because they didn't think it was useful. Maybe it would be worth proposing it again…

Anyways, to fix the problem on PolylineDecorator, I think the simplest way is to use FeatureGroup instead of LayerGroup for grouping the pattern symbols. This reduces the need to overwrite code and it returns the real pattern bounds, not the path ones. I will publish a fix soon.

@bbecquet bbecquet added the bug label Jan 23, 2018
@boromisp
Copy link
Author

I don't think simply using FeatureGroup would work here, because off-screen symbols are not rendered. The method would not throw error, but it also would not return the correct result.

The exact bounds - including the symbols - depend on the zoom level, because the symbol size is in screen space (at least in my use-cases).

@bbecquet
Copy link
Owner

Oh yes, you're right, I underestimated things. I think your solution of using the path bounds is the right compromise. Would you like to make a PR like you proposed? It could be merged quickly.
You just need to edit the src/L.PolylineDecorator.js file with ES6 style and use the npm run build command to generate the dist file, not modifying this one directly (I should write a CONTRIBUTING.md file to explain this…)
BTW, I already pushed a commit to change LayerGroup to FeatureGroup as it fixed another bug.

@bbecquet
Copy link
Owner

I had some time so I added it myself. It should be fixed in version 1.6.0. Thanks for your help!

@transducer
Copy link

Same issue with getLatLng?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants