mapbox / leaflet-pip

point in polygon intersections for leaflet
https://mapbox.github.io/leaflet-pip/
BSD 2-Clause "Simplified" License
199 stars 46 forks source link

Update to current leaflet. #8

Closed tmcw closed 8 years ago

nikolauskrismer commented 9 years ago

Oh... that would be great. I just tries your plugin with leaflet from today's master brunch. I get a "invalid instanceof operand" error...

BTW: Thanks for that great leaflet plugin :-)

r-barnes commented 9 years ago

I am getting Uncaught TypeError: Expecting a function in instanceof check, but got undefined when using leaflet-pip with 1.0-dev and strict JS. This may be related, but I'll start a new thread.

tmcw commented 9 years ago

Patches accepted, as always, if someone wants to help with this update.

r-barnes commented 9 years ago

I do like to contribute patches! But this is somewhat outside my expertise. Fortunately, I was able to get things running by disabling the MultiPolygon check. Thanks again for building this!

Cheers, Richard

On 07/16/2015 12:19 PM, Tom MacWright wrote:

Patches accepted, as always, if someone wants to help with this update.

— Reply to this email directly or view it on GitHub https://github.com/mapbox/leaflet-pip/issues/8#issuecomment-122026471.

nikolauskrismer commented 9 years ago

Ok... the API of leaflet for 1.0 changes. At the moment (talking of beta1) there is no L.MultiPolygon anymore. Since leaflet-pip performs a

if (l instanceof L.MultiPolygon || l instanceof L.Polygon)

check, this is a problem.

For now the solution would be to only check for L.Polygon (since now MultiPolygons are handled also in this class), but the leaflet API might change again (see https://github.com/Leaflet/Leaflet/issues/3498#3498)

nikolauskrismer commented 9 years ago

How about opening a separate branch for leaflet-1.0-dev?

r-barnes commented 9 years ago

This is the only breaking change I experienced in using leaflet-pip with Leaflet 1.0, so the fix seems easy enough: just remove the line. Except then things don't work for earlier versions of Leaflet. To make matters worse, the discussion on Leaflet regarding MultiPolygon is on-going

A branch would work, or a comment could be appended to the source code referencing the relevant threads so users can confidently repair the issue themselves until the API stabilizes.

MuellerMatthew commented 9 years ago

Why not just amend the code so it checks if L.MultiPolygon exists, and if it does, it uses it, otherwise it wont. That way when the code changes, it will still work. perhaps something like the following:

add a new function to handle both situations:

instanceCheck: function() {
     if (L.MultiPolygon) {
          return (l instanceof L.MultiPolygon || l instanceof L.Polygon);
     } else {
          return(l instanceof L.Polygon);
     }
}

and then replace

if (l instanceof L.MultiPolygon || l instanceof L.Polygon)

with

if (instanceCheck())

fyi, I havn't verified whether this works, but in theory, it should.

nikolauskrismer commented 8 years ago

Why use instanceof here? Wouldn't the following thing make more sense (especially since we need l.toGeoJSON().geometry anyway):

var leafletPip = {
    bassackwards: false,
    getGeometry: function(l) {
        var geom = null,
            type = null;

        if (l.toGeoJSON) {
            geom = l.toGeoJSON().geometry;
            type = (geom) ? geom.type : null;

            // checking for supported types (only MultiPolygon and Polygon)
            if (type === "MultiPolygon" || type === "Polygon") {
                return geom;
            }
        }

        return null;
    },

    pointInLayer: function(p, layer, first) {
        'use strict';
        if (p instanceof L.LatLng) p = [p.lng, p.lat];
        else if (leafletPip.bassackwards) p = p.concat().reverse();

        var results = [];
        layer.eachLayer(function(l) {
            if (first && results.length) return;
            var geom = leafletPip.getGeometry(l);
            if (geom && gju.pointInPolygon({ type: 'Point', coordinates: p }, geom)) {
                results.push(l);
            }
        });

        return results;
    }
};
smitchell commented 8 years ago

I applied the code from nikolauskrismer, and it got me up and running with Leaflet 1.0 Beta 2... mostly. The states in my demo, http://exploringspatial.com/#demo/8, won't unhighlight any more. I'm going to see if that is an easy fix (at this point, I'm working on a local copy).

tmcw commented 8 years ago

Fixed in #20