-
Notifications
You must be signed in to change notification settings - Fork 260
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
Add GeoJSON export ... #8
Conversation
For discussion: Setting the CRS on the resulting GeoJSON output is not supported in this PR. The current GeoJSON spec calls for a relatively complex designation involving crsURN / name like 'EPSG:4326'. The SpatialReference object only carries the WKID in many cases, and the authority that maintains the WKID is not known without out of band information. This makes meeting the GeoJSON spec for CRS in its current form difficult. There is also discussion of limiting / removing CRS information from the spec: see geojson/draft-geojson#6 for more information. |
Thanks for the contribution! The changes look good. We're clarifying our process for accepting the external contributions yet. |
Looks like the travis build for that pull request is failing. |
The failure stemmed from not having a .travis.yml file on this branch, which results in a failure. See https://travis-ci.org/scooterw/geometry-api-java/builds/7523005. I merged master, which has the .travis.yml file, and it is now passing. |
Should there also be a method in GeometryEngine and/or ogc.OGCGeometry? That would be the preferred way for implementing ST_AsGeoJSON in spatial-framework-for-hadoop: https://github.com/Esri/spatial-framework-for-hadoop/blob/master/hive/src/com/esri/hadoop/hive/ST_AsGeoJson.java |
@randallwhitman See commit 67b08c4. This implements GeometryEngine#geometryToGeoJson(Geometry geometry) and OGCGeometry#asGeoJson() . Looking at the spatial-framework-for-hadoop code linked in your comment, this should work, whichever way you decide to implement it. |
Yes, I see it in the commit, thanks! After another git-pull, I now have it in my working directory (not sure why I did not get it first time I checked out the branch, but fortunately that is moot now). Now, with this enhancement, ST_AsGeoJSON outputs GeoJSON rather than NULL. |
Glad it works for you! I didn't commit it until ~1am Eastern, so if you pulled before then, it would not have been available. |
Yes, I had pulled it before the latest commit - that explains it :) |
Hi, I've realized, that this GeoJSON export produces only Polygon and never MultiPolygon. That should be corrected, because our polygons can contain exterior rings. See OperatorExportToWktLocal.exportPolygonToWkt how this is done. I think the export should follow similar pattern. |
Thanks. I will take a look at it today. I am traveling so I may not be able to get it fixed right away. |
@stolstov : Out of curiosity, can you send me an example of a geometry (Esri JSON is fine) that you have tested with that would result in a MultiPolygon? It will help, speed wise, if I don't have to create one. |
@scooterw : Here’s an esriJSON polygon with multiple rings and the same one in GeoJSON: esriJSON: { |
Another one, that converts to WKT. |
Thanks @jcardonadcdev and @stolstov. I will take a look at these while I do my testing. |
The resulting MultiPolygon from the test lints at http://geojsonlint.com. There is a parsing issue with the JSON from @jcardonadcdev in that it is not read in as multiple polygons by the library. If you reverse the direction of the coordinates, it does read as multiple polygons. I haven't had time to dive in and figure out what is going on. For this reason, in this initial run through, I used the polygons from @stolstov in the unit test that was added to cover the change in functionality. |
@scooterw Are you ok with us adding the Esri copyright header we have in other geometry files to the files you contributed in this install? |
What I am not clear on is whether I cede copyright to my contributed code, or whether the copyright line would have my name. Any idea how this works? |
@stolstov we shouldn't need to add an Esri copyright. |
@scooterw Actually, we'd like you to have a reference to the Apache license at the top of the file like that:
Unless required by applicable law or agreed to in writing, software |
@scooterw For the new files you contributed. |
@stolstov we already have that in the repo, he's contributing under that license unless it is licensed in a different manner. Just having the apache license in the repo should be sufficient unless he is contributing code that is licensed with something like the bsd or mit licenses. |
@scooterw Esri does not require you to assign the copyright, you retain the copyright of your contributions. Esri does require that you make your contributions available under the Apache license in order to be included in the main repo. You can optionally include the Apache license along with your name in the copyright line in every file, but that is not strictly necessary. |
Okay. Ultimately copyright doesn't mean a whole lot to me in this case. I was just a bit confused by the original request. The confusion I have now concerns whether the license header must be present on every contributed file or if contributions assume the license of the library to which they are contributed. My assumption was that contributions would assume the license of the library unless otherwise stated, but I will apply the license header(s) to avoid further confusion by all parties. |
Thank you! |
This pull request adds support for exporting the core Esri geometry types to GeoJSON. There are unit tests included in unittest/com/esri/core/geometry/TestGeomToGeoJson.java .