-
Notifications
You must be signed in to change notification settings - Fork 403
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
feat: Add topojson graph export rebase #1926
base: main
Are you sure you want to change the base?
Conversation
…port This implements the first rough TopoJson objects to construct the coming TopoJson export endpoint.
includes pingtests in various permutations of faulty and correct input
Let's not add Lombok to existing classes. Only new ones for now. Hence, this fiel gets a typical setter.
Before the "coordinates" keyword was still serialized wrongly into the topojson structure creating a wrong arcs structure.
Reduce unnecessary duplication and streamline the export serialization with e.g. JsonExportResponse. The only difference is that it doesn't extent the ExportResponse. This is also not necessary. ExportResponse holds no value for the TopoJson export.
Layers is not necessary. It can be represented by a HashMap of layers where the key is the layer name. In our case "network".
807ea6d
to
aff6244
Compare
… properties replacing weight with speed/distance, fallback for cases where OSM ids are not available, using OSM way geometries (with option to turn off and get only node coordinates (beeline graph) instead)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
e60e4aa
to
9b7530f
Compare
Quality Gate passedIssues Measures |
ors-api/src/main/java/org/heigit/ors/api/controllers/ExportAPI.java
Outdated
Show resolved
Hide resolved
request.setProfileName(profile); | ||
request.setResponseType(APIEnums.ExportResponseType.TOPOJSON); | ||
|
||
ExportResult result = exportService.generateExportFromRequest(request); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it feels a bit weird that this is using the same function as the JSON endpoint, as they are returning vastly different data?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in both cases that function gets data from the graph for a specific bbox, the function below does different things with that result, I don't think that's so confusing...
public void setEdgeWeigths(Map<Pair<Integer, Integer>, Double> edgeWeigths) { | ||
this.edgeWeigths = edgeWeigths; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this setter not needed anymore?
Lombok annotation is only there for ExportWarning
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nope
if (osmIdsAvailable) { | ||
extra.put("osm_id", osmIdGraphStorage.getEdgeValue(iter.getEdge())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is duplicating information that is already present as the key in topoGeometries.
We shouldn't add it twice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
look at my refactored ExportRequest class, should be betternow
|
||
if (debug()) { | ||
Map<String, Object> extra = new HashMap<>(); | ||
if (debug) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an API flag that should be adequately named - and not "debug" which sounds like a service-wide debug flag.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
…eter "debug" to "additional_info"
2b1cfa4
to
3f8ad0d
Compare
Pull Request Checklist
have been resolved.
[Unreleased] heading.
along with a short description of what it is for, and documented this in the Pull Request (below).
(at least Germany), and the graphs build without problems (i.e. no out-of-memory errors).
importer etc.), I have generated longer distance routes for the affected profiles with different options
(avoid features, max weight etc.) and compared these with the routes of the same parameters and start/end
points generated from the current live ORS.
If there are differences then the reasoning for these MUST be documented in the pull request.
and why the change was needed.
Information about the changes
TopoJSON response type on the export endpoint. Returns a TopoJSON representation of the graph within a given bounding box containing the ORS edges and nodes, OSM ways with ID, a flag is passable both ways, speeds and distances.