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

Elevation handling improvements #3862

Closed
5 tasks
flaktack opened this issue Feb 4, 2022 · 3 comments · Fixed by #4033
Closed
5 tasks

Elevation handling improvements #3862

flaktack opened this issue Feb 4, 2022 · 3 comments · Fixed by #4033
Labels
Improvement A functional improvement New GraphQL API Label issues we should consider when making a new GraphQL API for OTP2.
Milestone

Comments

@flaktack
Copy link
Contributor

flaktack commented Feb 4, 2022

There are several improvements to elevation handling which could be done:

  • move elevation data from leg.walkStep.elevation to leg.legElevation similar to leg.legGeometry, which would make it easier to use / verify
  • return edges/sections without elevation in the API (pathways and if there were problems with elevation handling)
  • support the NODATA tag in tiff files to mark areas without elevation coverage. Currently this may lead to Infinity/NaN calculations
  • copy elevation when splitting StreetEdges destructively (currently it is dropped)
  • add an elevation graph to the debug client
@t2gran t2gran added New GraphQL API Label issues we should consider when making a new GraphQL API for OTP2. Improvement A functional improvement labels Feb 4, 2022
@t2gran t2gran added this to the 2.1 milestone Feb 4, 2022
@optionsome
Copy link
Member

FYI I fixed some issues here HSLdevcom#314 but that pr got stale because I didn't have time to finish it and some issue was fixed in a different way in upstream otp1 if I remember correctly. I'm not planning to upstream these changes any time soon nor do I know if any of them are relevant after your suggested changes.

@flaktack
Copy link
Contributor Author

flaktack commented Feb 8, 2022

Thanks, I took a quick look at the PRs and the walkStep related fixes wouldn't be needed if the elevation is stored at the leg level. This seems like a useful addition:

  • Remove elevation points within steps that are too close to each other (distance between them is <5m)

The main question here is if having a StreetEdge and StreetEdgeWithElevation is needed, or if the classes may be merged into one? Merging the two would simplify the edge splitting logic.

@t2gran
Copy link
Member

t2gran commented Feb 8, 2022

The main question here is if having a StreetEdge and StreetEdgeWithElevation is needed

The concussion after discussing this on todays OTP developer meeting is that we should merge these two classes. It slightly increases the memory consumptions for those who are not using elevation, but most major deployments do use it and benefit from merging it. We, also discussed if it was possible to make an extension point like the StreetEdge#StreetEdgeCostExtension, but the conclusion was that this is probably not worth it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Improvement A functional improvement New GraphQL API Label issues we should consider when making a new GraphQL API for OTP2.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants