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

Response as GeoJSON #1651

Merged
merged 10 commits into from Jul 20, 2019

Conversation

@manueltimita
Copy link
Contributor

manueltimita commented Jun 28, 2019

As discussed in #1591, I have added the possibility to make a request to the isochrone endpoint specifying the response type as GeoJSON with type=geojson. Using type=json or no type parameter returns the output as usual.

@manueltimita manueltimita changed the title Response as geojson Response as GeoJSON Jun 28, 2019
@karussell

This comment has been minimized.

Copy link
Member

karussell commented Jul 1, 2019

Can you update this to the latest master? (there seems to be some conflicts)

@manueltimita

This comment has been minimized.

Copy link
Contributor Author

manueltimita commented Jul 2, 2019

Can you update this to the latest master? (there seems to be some conflicts)

Fixed

Copy link
Member

karussell left a comment

Thanks, this looks good. Have one change request.

@karussell

This comment has been minimized.

Copy link
Member

karussell commented Jul 17, 2019

Nice, thanks!

@manueltimita did you test this for leaflet or some different client?

@nilsnolde would you have time to try this for your use case? Before providing a new format in the public JSON API I would like to make sure it works for at least two different use cases (i.e. clients). I mean GeoJSON is standardized and everything but just to make sure it works...

@manueltimita

This comment has been minimized.

Copy link
Contributor Author

manueltimita commented Jul 17, 2019

@manueltimita did you test this for leaflet or some different client?

I don't have a graphopper installation, but I did check the GeoJSON tests output against the standard and saw no problems. Here's an example of how to check for yourself if you need to:

Test 1: Run https://github.com/manueltimita/graphhopper/blob/response_as_geojson/web/src/test/java/com/graphhopper/http/isochrone/IsochroneResourceTest.java#L152-L172 and copy/paste the console output (System.out.println(json);) into http://geojson.io or http://geojsonlint.com/. They are both using https://github.com/mapbox/mapbox.js/ but if it works there it will definitely work with Leaflet GeoJSON. Here's the output using Leaflet: https://jsfiddle.net/e2zuqhtp/

Test 2 (multiple buckets):
https://github.com/manueltimita/graphhopper/blob/response_as_geojson/web/src/test/java/com/graphhopper/http/isochrone/IsochroneResourceTest.java#L175-L189 and here's the fiddle with the output in Leaflet 1.0.3: https://jsfiddle.net/t1bsLqu8/1/ (in this one, you can click on the buckets)

@karussell

This comment has been minimized.

Copy link
Member

karussell commented Jul 19, 2019

Thanks! I think we should test it with openlayers as a second way, just to make sure. Feel free to do this or I'll try later (not sure yet when though)

@manueltimita

This comment has been minimized.

Copy link
Contributor Author

manueltimita commented Jul 19, 2019

Here is what requestIsochrone("/isochrone?point=42.531073,1.573792&time_limit=130&type=geojson&buckets=3") looks like on OL v4: https://jsfiddle.net/6gp1v2cn/

If you like, and have an environment in which you can give me temporary access to a live endpoint that has this branch, I'm happy to run some more extensive tests.

@nilsnolde

This comment has been minimized.

Copy link

nilsnolde commented Jul 19, 2019

Oops, sorry @karussell I just saw this.. Great stuff @manueltimita! Seems like you tested already, output looks good.

I'll try to find time to try with routing-py.

@karussell karussell added this to the 0.13 milestone Jul 20, 2019
@karussell

This comment has been minimized.

Copy link
Member

karussell commented Jul 20, 2019

Thanks a lot @manueltimita - this looks good! Did also a quick test. Will merge.

@nilsnolde would be cool, if you can test this before 0.13 release (in ~6 weeks).

@karussell karussell merged commit 405ea08 into graphhopper:master Jul 20, 2019
4 checks passed
4 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
security/snyk - pom.xml (karussell) No new issues
Details
security/snyk - web/package.json (karussell) No manifest changes detected
@nilsnolde

This comment has been minimized.

Copy link

nilsnolde commented Aug 1, 2019

Tested with routingpy, all good with folium maps, eats the GeoJSON without any parsing 👍

Will release soon on our side.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.