calvinmetcalf / shapefile-js

Convert a Shapefile to GeoJSON. Not many caveats.
http://calvinmetcalf.github.io/shapefile-js/
715 stars 228 forks source link

Issue with importing shapefile containing polygon with hole #97

Closed rajkishan16 closed 2 years ago

rajkishan16 commented 6 years ago

Hi,

I've been using the shapefile-js library for importing zipped shapefiles. Thanks a lot for your work on this.

Recently I've noticed that this is not working properly for me for shapefiles that contain polygons with holes. When I try to upload a shapefile, which has one polygon that also has a hole, when it loads onto the map, it is actually adding two separate polygons. You can see that in this jsfiddle.

I've attached the zipped shapefile that I tried it with. TestShapefileWithHole.zip

But when I tried uploading it to ArcGIS Online and a couple of other websites, it did load fine, displaying only one polygon that has a hole.

So then I checked out the geojson that's returned by the shp function, and it seems that the 'type' of features returned by it is a 'MultiPolygon', and that is causing two polygons to be loaded onto the map. But when I tried using this site to convert the same shapefile to a GeoJSON, the resultant 'type' was a 'Polygon', and that gets loaded properly with just one polygon with a hole.

I was just wondering what I'm doing wrong, and if there's anything I can do to make this work correctly.

Thanks, Rajkishan

calvinmetcalf commented 6 years ago

this is probably a bug here, for background shapefiles don't distinguish between multipolygons and polygons or between exterior and interior rings the same way geojson does so there looks to be a bug in conversion maybe/.

calvinmetcalf commented 6 years ago

ok so you have an invalid shapefile as it turns out, basically they way they distinguish between interior and exterior rings is by having exterior rings be clockwise and interior rings be counter clockwise, your shapefile has it backwards, which is causing a bit of a bug on our part as we are assuming the first ring will always be an exterior one.

it works in most of the other ones because they are happily parsing it as a polygon where the hole is outside of the exterior ring which leaflet at least will display regularly even though its somewhat nonsensical but if you try to do more with the file you may have some issue.

that being said we should probable handle cases where the first ring is a hole better...

rajkishan16 commented 6 years ago

That is indeed the case! When I created a shapefile with the polygon being the right way, and then loaded it onto the map, it did load properly with the hole. This was happening because I wasn't checking the polygon orientation before the shapefile is exported from my application, and I'll make sure that the orientation is fixed before the export is done. Thanks for your help!!

P.S. Should I close this issue now, or just leave it as it is for now?

calvinmetcalf commented 6 years ago

leave it open, there is a bug here that could theoretically screw up a valid shapefile

On Tue, Jun 19, 2018 at 3:40 PM rajkishan16 notifications@github.com wrote:

That is indeed the case! When I created a shapefile with the polygon being the right way, and then loaded it onto the map, it did load properly with the hole. This was happening because I wasn't checking the polygon orientation before the shapefile is exported from my application, and I'll make sure that the orientation is fixed before the export is done. Thanks for your help!!

P.S. Should I close this issue now, or just leave it as it is for now?

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/calvinmetcalf/shapefile-js/issues/97#issuecomment-398520524, or mute the thread https://github.com/notifications/unsubscribe-auth/ABE4n26egx0e9mvadwMStcuurD3Z12KUks5t-VO0gaJpZM4UtvGp .

vmwherez commented 5 years ago

We are using shapefile.js to upload *.shp created in a farmer's field. At the moment, I can read and upload all the subsections of a field, and I'm trying to write an algorithm to 'trace' only the perimeter. Interestingly this could be a feature if we wanted the interior pieces, but at the moment we only need the whole. Am I missing anything?

screen shot 2018-11-03 at 8 22 25 pm

Do we need to go this far:

https://www.nayuki.io/page/convex-hull-algorithm

vmwherez commented 5 years ago

For anyone else who might find themselves here, try this:

https://github.com/brian3kb/graham_scan_js

maxwell-oroark commented 4 years ago

I believe I am seeing a similar issue. When using shpjs on a shapefile with a hole in it I get a resulting geojson that when linted produces these errors:

Line 9071: a LinearRing of coordinates needs to have four or more positions
Line 9071: the first and last positions in a LinearRing of coordinates must be the same
Line 9087: the first and last positions in a LinearRing of coordinates must be the same

Would this be the expected type of errors you would see from the inner ring outer ring bug this thread desribes?

vmwherez commented 4 years ago

@maxwell-oroark I can't fully recall, this was 2 years ago, but basically you need to 'grab' that last pair of coords and make sure it's the same as the first. I remember this error, the solution is that simple.

calvinmetcalf commented 4 years ago

@maxwell-oroark your bug might be a slightly different type of malformation, i'd need to take a look at the shp in question

maxwell-oroark commented 4 years ago

@calvinmetcalf thanks for taking time to respond to this old thread! find shapefile in question attached: Charlotte_NC.zip

maxwell-oroark commented 4 years ago

I'm a little confused on whether I would be validating the shapefile before converting and saying 'oh this isn't a valid shapefile because of the inner outer ring problem' or doing that validation on the resulting geojson?

RandomFractals commented 4 years ago

I just tried loading those shapefiles in my GeoDataViewer vscode extension which uses shapefile-js and keplergl for maps display.

Is this what you expected to see? What's missing?

geo-data-viewer-Charlotte-NC

maxwell-oroark commented 4 years ago

That looks correct. The error occurs after using shpjs to convert to geojson. The resulting geojson file is invalid and has the following errors:

Line 9071: a LinearRing of coordinates needs to have four or more positions
Line 9071: the first and last positions in a LinearRing of coordinates must be the same
Line 9087: the first and last positions in a LinearRing of coordinates must be the same
RandomFractals commented 4 years ago

interesting. I am actually using shpjs to convert those shapefiles to geojson for keplergl to render those polygons on the map.

maxwell-oroark commented 4 years ago

Your viewer might be more flexible than the server where I am submitting the geojson. what happens when you run the geojson file through http://geojsonlint.com?

calvinmetcalf commented 4 years ago

the ring validity criteria for shapefiles and geojson are broadly similar, they must be 'closed' as in the first and list point must be the same and they must have at least 3 distinct points (so 4 points total since the last must be the same as the first) this library deson't check that (and keplergl might not either) but likely the issue is a faulty ring somewhere, you should be able to run a validity check in qgis on your shapefile

RandomFractals commented 4 years ago

yeah, I'd run it through qgis too.

maxwell-oroark commented 4 years ago

Appears to be valid shapefile according to qgis:

Screen Shot 2020-04-30 at 9 34 28 AM
RandomFractals commented 4 years ago

well, I never used geojsonlint, but here is the geojson I got created with shpjs from running those shapefiles ...

Charlotte_NC.geojson.txt

calvinmetcalf commented 4 years ago

looks like the winding order is backwards

On Thu, Apr 30, 2020 at 10:38 AM Taras Novak notifications@github.com wrote:

well, I never used geojsonlint, but here is the geojson I got created with shpjs from running those shapefiles ...

Charlotte_NC.geojson.txt https://github.com/calvinmetcalf/shapefile-js/files/4558956/Charlotte_NC.geojson.txt

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/calvinmetcalf/shapefile-js/issues/97#issuecomment-621894947, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAITRH7OBHBQTZKB7OJ7EZDRPGENNANCNFSM4FFW6GUQ .

-- -Calvin W. Metcalf

maxwell-oroark commented 4 years ago

Oh v. interesting. okay the app I'm working on is using the rewind library... but it seems like it using it pretty naively by not passing any arguments. I'm not sure I understand how this library is supposed to function, should I always pass it 'clockwise' to it? or should I parse the geojson and determine if it needs 'rewinding' or not