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

Point should not be "coordinates": [[lon, lat]], but [lon, lat] when setting "geometry=geojson" #3425

Closed
kkdd opened this issue Dec 8, 2016 · 6 comments
Milestone

Comments

@kkdd
Copy link

kkdd commented Dec 8, 2016

Hello,
At present, a query withgeometry=geojson results in:

$ curl "http://localhost:5000/route/v1/driving/133.540336,33.558187;133.540937,33.55841?steps=true&geometries=geojson" | jq -c '.routes[0].legs[].steps[].geometry | select(.type == "Point")'

{"type":"Point","coordinates":[[133.540906,33.558277]]}

But a correct geojson should be the following:

{"type": "Point", "coordinates": [133.540906, 33.558277]}
(or {"type": "MultiPoint", "coordinates": [[133.540906, 33.558277]]}).
@daniel-j-h
Copy link
Member

You're right, quoting the spec:

2.1.2. Point
For type "Point", the "coordinates" member must be a single position.

2.1.3. MultiPoint
For type "MultiPoint", the "coordinates" member must be an array of positions.

and for Position:

2.1.1. Positions

A position is the fundamental geometry construct. The "coordinates" member of a geometry object is composed of one position (in the case of a Point geometry), an array of positions (LineString or MultiPoint geometries), an array of arrays of positions (Polygons, MultiLineStrings), or a multidimensional array of positions (MultiPolygon).

A position is represented by an array of numbers. There must be at least two elements, and may be more. The order of elements must follow x, y, z order (easting, northing, altitude for coordinates in a projected coordinate reference system, or longitude, latitude, altitude for coordinates in a geographic coordinate reference system). Any number of additional elements are allowed -- interpretation and meaning of additional elements is beyond the scope of this specification.

@TheMarex
Copy link
Member

TheMarex commented Dec 8, 2016

Urg yeah this is an API bug, fixing this would be a breaking API change though.

What would work is not not emit Point at all anymore but only LineString, but maybe with the same coordinate twice. Client code already needs to handle both cases, we would just not emit one of them anymore. Always emitting LineString might also be more consistent with the idea of step geometries.

@daniel-j-h
Copy link
Member

For the record: #3426 fixed this by no longer emitting a Point in the single coordinate case. Instead we now always emit a LineString with the single coordinate duplicated. Backwards compatible.

Thanks for reporting and sorry for not noticing this edge case earlier.

@daniel-j-h daniel-j-h added this to the 5.5.0 milestone Dec 9, 2016
@kkdd
Copy link
Author

kkdd commented Dec 9, 2016

In a geojson, "LineString" should have two or more coordinates. See a pull request of "Adhere to GeoJSON spec by not return invalid LineStrings #2286" (Project-OSRM/osrm-backend).

@daniel-j-h
Copy link
Member

Yes now a LineString has two coordinates if the geometry would have been a Point.

Before: Point coord
After:  LineString [coord, coord]

@TheMarex
Copy link
Member

TheMarex commented Dec 9, 2016

Yeah the original "fix" was a bad call on my side. LineString makes much more sense, as for these segments the start and endpoint coincide (zero length segments, such as the last segment of a route that marks the Arrive instruction.)

raymond0 added a commit to raymond0/osrm-backend that referenced this issue Jan 10, 2017
* master: (1951 commits)
  Log some memory usage statistics after preprocessing tasks.
  Adds failing tests for directional access overrides, discovered in Project-OSRM#3345
  Works around Unreachable Warning for Debug Build
  fix invalid assertion in coordinate_extractor
  Update node weights if traffic data is applied.
  Fix path to node binaries
  nvm -> install_node
  Try to use mapbox node mirror
  assert that there is an open logger file when trying to log geojson output (Project-OSRM#3417)
  Fix removing shared memory segments in a multi-process setup
  Changes Single Coordinate Geoms from Point to LineString, closes Project-OSRM#3425.
  don't assign unused name to exception
  Fixes Compiler Crashes on Windows
  Log helpful error message if mmap fails.
  Refactors and improves the Sliproad Handler, resolves Project-OSRM#3109
  clean-up guidance code/code in general
  fix errors in coordinate extractor due to duplicated coordinates fix offset calculation in curve detection
  Revert "Add simple code review checklist to PR template."
  Refactor logging, improve error handling workflow, clang-format. (Project-OSRM#3385)
  Refactor logging, improve error handling workflow, clang-format. (Project-OSRM#3385)
  ...

# Conflicts:
#	extractor/extraction_containers.cpp
#	extractor/extraction_containers.hpp
#	extractor/extraction_way.hpp
#	extractor/extractor.cpp
#	extractor/extractor_callbacks.cpp
#	extractor/extractor_options.cpp
#	extractor/internal_extractor_edge.hpp
#	extractor/scripting_environment.cpp
#	include/engine/engine_config.hpp
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants