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

Add GeoJSON export ... #8

Merged
merged 10 commits into from
Jun 27, 2013
Merged

Add GeoJSON export ... #8

merged 10 commits into from
Jun 27, 2013

Conversation

scooterw
Copy link
Contributor

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 .

@scooterw
Copy link
Contributor Author

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.

@stolstov
Copy link
Member

Thanks for the contribution! The changes look good. We're clarifying our process for accepting the external contributions yet.

@tlpinney
Copy link
Contributor

tlpinney commented Jun 8, 2013

Looks like the travis build for that pull request is failing.

@scooterw
Copy link
Contributor Author

scooterw commented Jun 8, 2013

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.

@randallwhitman
Copy link
Contributor

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

@scooterw
Copy link
Contributor Author

@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.

@randallwhitman
Copy link
Contributor

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.
Thanks again.

@scooterw
Copy link
Contributor Author

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.

@randallwhitman
Copy link
Contributor

Yes, I had pulled it before the latest commit - that explains it :)

@stolstov
Copy link
Member

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.

@scooterw
Copy link
Contributor Author

Thanks. I will take a look at it today. I am traveling so I may not be able to get it fixed right away.

@scooterw
Copy link
Contributor Author

@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.

@jcardonadcdev
Copy link
Member

@scooterw : Here’s an esriJSON polygon with multiple rings and the same one in GeoJSON:

esriJSON: {
"rings":[
[ [102.0, 2.0], [103.0, 2.0], [103.0, 3.0], [102.0, 3.0], [102.0, 2.0] ],
[ [100.0, 0.0], [101.0, 0.0], [101.0, 1.0], [100.0, 1.0], [100.0, 0.0] ]
],
"spatialReference": {
"wkid":4326
}
}
geoJSON: {
"type": "MultiPolygon",
"coordinates": [
[
[ [102.0, 2.0], [103.0, 2.0], [103.0, 3.0], [102.0, 3.0], [102.0, 2.0] ]
],
[
[ [100.0, 0.0], [101.0, 0.0], [101.0, 1.0], [100.0, 1.0], [100.0, 0.0] ]
]
]
}

@stolstov
Copy link
Member

Another one, that converts to WKT.
It consist of an exterior ring, a hole, and exterior ring inside of the hole.
String restJson = "{"rings": [[[-100, -100], [-100, 100], [100, 100], [100, -100], [-100, -100]], [[-90, -90], [90, 90], [-90, 90], [90, -90], [-90, -90]], [[-10, -10], [-10, 10], [10, 10], [10, -10], [-10, -10]]]}";
g = OperatorImportFromJson.local().execute(Geometry.Type.Unknown, restJson);
String wkt = OperatorExportToWkt.local().execute(0, g.getGeometry(), null);
assertTrue(wkt.equals("MULTIPOLYGON (((-100 -100, 100 -100, 100 100, -100 100, -100 -100), (-90 -90, 90 -90, -90 90, 90 90, -90 -90)), ((-10 -10, 10 -10, 10 10, -10 10, -10 -10)))"));

@scooterw
Copy link
Contributor Author

Thanks @jcardonadcdev and @stolstov. I will take a look at these while I do my testing.

@scooterw
Copy link
Contributor Author

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.

@stolstov
Copy link
Member

@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?

@scooterw
Copy link
Contributor Author

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?

@JerrySievert
Copy link

@stolstov we shouldn't need to add an Esri copyright.

@stolstov
Copy link
Member

@scooterw Actually, we'd like you to have a reference to the Apache license at the top of the file like that:
/* Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

   http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.*/

@stolstov
Copy link
Member

@scooterw For the new files you contributed.

@JerrySievert
Copy link

@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.

@aaronpk
Copy link

aaronpk commented Jun 27, 2013

@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.

@scooterw
Copy link
Contributor Author

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.

@stolstov
Copy link
Member

Thank you!

stolstov added a commit that referenced this pull request Jun 27, 2013
@stolstov stolstov merged commit 10632fd into Esri:master Jun 27, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants