Elevation handling improvements #4033
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
StreetVertex used a lambda to pass String parameters to LocalizedString(), which meant that toString() wasn't overridden. By using I18NString parameters in LocalizedString the need for the lambda is removed, while generalising the code.
This allows testing the elevation related functionality on an actual graph.
WalkStep also contains the elevation, but having it explicitly on Leg simplifies its usage. An explicit NaN value is added to differentiate when there is no elevation data.
Due to the elevation data it is possible to have Infinity / NaN bicycleSafetyFactor or edge traversal weight. To avoid errors latter on, check for invalid values when setting updated values. ElevationProfileFailure items are added to the issue store.
NoDataGridCoverage is added which throws a PointOutsideCoverageException() if the value at a given point is the NO_DATA value for the loaded file.
This simplifies edge splitting since less cases need to be handled, while also allowing for cleaner code.
When the from/to vertices are along the same edge a partial TemporaryPartialStreetEdge was created without copying the properties from the original StreetEdge. This remedies the oversight by: * moving the responsibility from SameEdgeAdjuster to StreetEdge * and reusing StreetEdge#copyPropertiesToSplitEdge()
All elevation profiles were compacted, which caused the uncompacted profile to have differing distances. By compacting only "computed" elevations this is avoided.
The existing logic is modified so that is deterministic and tested. The new logic interpolates the elevation for each vertex that is within maxElevationPropagationDistance on the street network using the two closest vertices with elevation.
hannesj
previously approved these changes
Mar 28, 2022
| * The elevation profile as a comma-separated list of x,y values. x is the distance from the start of the leg, y is the elevation at this | ||
| * distance. | ||
| */ | ||
| public String legElevation; |
There was a problem hiding this comment.
Should we use some more efficient data structure for this?
There was a problem hiding this comment.
Isn't that what the WalkStep also uses?
There was a problem hiding this comment.
It is, but for example the same encoded polylines could be used as for the legGeometry.
There was a problem hiding this comment.
Encoded polyline can't be used due to the different ranges the numbers have, so the existing string seems like the best solution, and so can stay the same.
|
Why '[changelog skip]' ? This seems relevant to include to me. |
src/main/java/org/opentripplanner/inspector/ElevationEdgeRenderer.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opentripplanner/routing/edgetype/StreetEdge.java
Outdated
Show resolved
Hide resolved
I'm not sure which parts should be in the changelog, and which parts shouldn't. I can rename the PR to better reflect what should be in the changelog, or create separate PRs for the relevant parts. |
hannesj
approved these changes
Mar 29, 2022
leonardehrenfried
approved these changes
Mar 29, 2022
flaktack
changed the title
t2gran
pushed a commit
that referenced
this pull request
Mar 30, 2022
8 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This is a set of related changes to improve elevation handling. It fixes several bugs, along with adding new features.
Bugs
MissingElevationHandler) and refactored so that it is deterministic and testable. A new build parameters is added to limit the propagation distance (maxElevationPropagationMeters).StreetEdgeandStreetWithElevationEdge, along with moving the partial edge splitting toStreetEdge.StreetEdges was compacted. For split edges and propagated elevations the profile doesn't have uniform distances, which is required for compaction.StreetElevationExtensioncan store both the compacted and uncompacted profile.Features
NO_DATAtag to mark areas without elevation data. This was ignored, resulting in areas with unrealistic elevation. ANoDataGridCoverageclass is added, which treatsNO_DATAareas the same as areas outside the grid's bounds.DataImportIssueStore.Leg#legElevationis added, which contains the elevation profile for the wholeLeg. If the elevation is missing in some segments, thanNaNsegments are added.Graph#minElevationandGraph#maxElevationto have a consistent coloring between tiles.Refactors
StreetWithElevationEdgeis merged intoStreetEdge, with the elevation specific parts moved intoStreetElevationExtension. This was needed to simplify edge splitting, which previously discarded the elevation details.Issue
closes #3862
Unit tests
Unit tests are added for:
A snapshot test is added for planning trips with elevation data.
Code style
☑️
Documentation
A single new
build-config.jsonparameter is added (maxElevationPropagationMeters), for which the documentation is updated.