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

Turf polygonize #767

Merged
merged 37 commits into from
Jun 1, 2017
Merged

Turf polygonize #767

merged 37 commits into from
Jun 1, 2017

Conversation

NickCis
Copy link
Contributor

@NickCis NickCis commented May 28, 2017

New Module @turf/polygonize

Polygonizes a set of Geometrys which contain linework that represents the edges of a planar graph. It's basically an implementation of GEOS's Polygonizer.

I ended up porting this to javascript because I wanted the same functionality as Shapely's polygonize function. This python framework uses GEOS'S C api as backend.

JSDoc

/** Implementation of GEOSPolygonize function (`geos::operation::polygonize::Polygonizer`).
 *
 * Polygonizes a set of lines that represents edges in a planar graph. Edges must be correctly
 * noded, i.e., they must only meet at their endpoints. LineStrings must only have two coordinate
 * points.
 *
 * The implementation correctly handles:
 *
 * - Dangles: edges which have one or both ends which are not incident on another edge endpoint.
 * - Cut Edges (bridges): edges that are connected at both ends but which do not form part
 *     of a polygon.
 *
 * @param {FeatureCollection<LineString>} geoJson - Lines in order to polygonize
 * @returns {FeatureCollection<Polygon>} - Polygons created
 */

Example

This example is the test found in test/in/complex.geojson

const polygonize = require('@turf/polygonize'),
    fs = require('fs'),
    input = JSON.parse(fs.readFileSync('./test/in/complex.geojson'));

console.log(JSON.stringify(polygonize(input)));

An example of input's LineString:

captura de pantalla de 2017-05-27 23-39-11

Turned into polygons:

captura de pantalla de 2017-05-27 23-42-17

@DenisCarriere
Copy link
Member

@NickCis 👍 This is a great new module! I'll send over a review

@DenisCarriere
Copy link
Member

DenisCarriere commented May 28, 2017

👎 All source code should be written in ES5 (with the exceptions of tests could be written in ES6).

All TurfJS modules should be very minimal with only a single index.js & test.js file.

You should consider publishing polygonize as it's own module on npm and TurfJS could simply include it as a dependency.

  • Convert code to ES5
  • Only include a single index.js
  • Only include single test.js
  • Convert /src to external dependency (ex: polygonize)
  • Add @private to other utils functions

Copy link
Member

@DenisCarriere DenisCarriere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Convert to ES5
  • Publish Polygonize as separate module
  • Only include single test.js & index.js files as source code

* - Dangles: edges which have one or both ends which are not incident on another edge endpoint.
* - Cut Edges (bridges): edges that are connected at both ends but which do not form part
* of a polygon.
*
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add @name polygonize

@NickCis
Copy link
Contributor Author

NickCis commented May 31, 2017

Should be removed, you can use @turf/line-segment to easily solve this.

I've used @turf/meta :: coordReduce in order to iterate over the coordinates and segment the LineString.

I'm working on the MultiLineString support. Sorry for the delay.

@DenisCarriere
Copy link
Member

@NickCis I've got MultiLineString support by adding @turf/line-segment at the top level. Converts all the Geometry into 2 vertex LineStrings.

module.exports = function (lines) {
    return polygonize(lineSegment(lines));
};

@NickCis
Copy link
Contributor Author

NickCis commented May 31, 2017

I'm working some issues in the polygonize module, i'll update the yarn.lock file when finished

@DenisCarriere
Copy link
Member

Awesome stuff! So far everything looks good to me.

@NickCis
Copy link
Contributor Author

NickCis commented May 31, 2017

Finished update of polygonize package :)

module.exports = function (geojson) {
return polygonize(lineSegment(geojson));
};
module.exports = polygonize;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Going to keep the basic function syntax in case anyone wants to expand the TurfJS method they can easily do so.

module.exports = function (geojson) {
  return polygonize(geojson);
}

@DenisCarriere DenisCarriere merged commit 792f2f9 into Turfjs:master Jun 1, 2017
@DenisCarriere DenisCarriere added this to the 4.4.0 milestone Jun 1, 2017
@DenisCarriere
Copy link
Member

Noticed you're making Polygon fail, any reason why this should fail? I can already see this error being very annoying, example if you have a collection of LineStrings and you polygonize them you then get a FeatureCollection of Polygons, if you keep adding more LineStrings to that FeatureCollection and re-run polygonize then it would fail since the FeatureCollection will have mixed contents.

It seems like throwing an Error against Polygons would make this module less practical.

@NickCis
Copy link
Contributor Author

NickCis commented Jun 1, 2017

@DenisCarriere I thought it would be more coherent to throw an error than just ignore another input that isn't a LineString ([Collection]Feature/Geometry<[Multi]LineString>). The exception is a good way to tell the user "Hey, are you sure that you want to send me all these things that i do not understand?".

But, as you are a maintainer of the library, i think that you have a better understanding of the target audience. If you explain me what will the expected behavior of the user, i'll happily make the necessary changes.

@DenisCarriere
Copy link
Member

DenisCarriere commented Jun 1, 2017

@NickCis As for Polygon Features, you can simply just return itself and not need to polygonize Polygons.

if (type === 'Polygon') return feature

The only thing that really need to be strict is the output, as long as the output is always the same type.

As for Inputs, the more geometry types the better, you could even attempt to properly support GeometryCollection (I don't know if that works with your library yet).

@DenisCarriere
Copy link
Member

DenisCarriere commented Jun 1, 2017

Would be nice to polygonize with mixed contents (polygons & lines).

Just shooting it out there, maybe it's not a valid request.

Red: LineString
Blue: Polygon
image

@DenisCarriere
Copy link
Member

Come to think of it, polygons with holes might get problematic... 😕 Might be best to leave out Polygons from the supported input Geometry types.

@NickCis Keep throwing that Error 👍

@NickCis
Copy link
Contributor Author

NickCis commented Jun 1, 2017

The library correctly supports GeometryCollections (there is also a test for this, i think that i've forgotten to add it in turf).

As regards (Multi)Polygons as input, is a valid request, in a way i'll have to suppose they are a especial case of a LineString which starts and ends in the same point, i could transform them using @turf/polygon-to-linestring. I cannot store it and just returned, because, if there are is a line which creates a polygon inside the polygon, the starting polygon should not be returned.

And example is the following:

captura de pantalla de 2017-06-01 12-25-20
Red: MultiLineString
Blue: Polygon

captura de pantalla de 2017-06-01 12-29-09
The valid result is the 4 inner polygons, and not the one that contains them.

The problem with this approach is that the algorithm still assumes that lines are correctly nodded (i.e.: they only met at their endpoints). So, if the polygon doesn't have a vertex where the line starts, it won't work correctly. Perhaps, it could be useful to add a non default option to find all intersections and divide lines and polygons. I think that it will have a huge impact on performance. Does turf have a module that given polygons and LineStrings, it finds all intersections and add them as new vertex? (if not, well, i think that i'll be starting to write a new module, haha).

@NickCis
Copy link
Contributor Author

NickCis commented Jun 1, 2017

@DenisCarriere I didn't read you answer before posting, sorry!

@DenisCarriere
Copy link
Member

DenisCarriere commented Jun 1, 2017

@NickCis This looks awesome, maybe that can be an entirely other module, I had attempted this before but it was getting too complex for the basic implementation I had.

I never finished it but this would be ideal for @turf/polygon-slice.

More info here on this issue #580

Here's what I started which was never finished. I like your graph approach.

@NickCis NickCis deleted the turf-polygonize branch June 3, 2017 19:29
@NickCis
Copy link
Contributor Author

NickCis commented Jun 3, 2017

Ok!, I'll look the issue and the branch! We'll follow the topic there!. Thanks!

@maphew
Copy link

maphew commented Jun 25, 2017

I'm not getting expected results from polygonize. Where is the appropriate place to ask questions about it? I'm a raw newbie to Turf and barely familiar with Node.

@DenisCarriere
Copy link
Member

@maphew this is the right place to ask questions, or you can submit an issue with a sample code. Use this issue #702 as an example.

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

Successfully merging this pull request may close these issues.

3 participants