-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Better escalator duration control: specific duration from OSM duration tag, default speed from build-config.json #6268
Merged
optionsome
merged 17 commits into
opentripplanner:dev-2.x
from
HSLdevcom:escalator-duration
Dec 9, 2024
Merged
Changes from 11 commits
Commits
Show all changes
17 commits
Select commit
Hold shift + click to select a range
47300f9
use duration tag from osm as escalator traversal duration
tkalvas d9ead12
make default escalator speed a configuration parameter
tkalvas 8028990
test the added duration tag in OsmWay
tkalvas aa3de3a
ignore escalator duration tags if the speed are nonsense. include dur…
tkalvas 4566194
use Optional<Duration> instead of Integer which can be null
tkalvas 3127cf3
test changes included
tkalvas 1e5aca1
use Optional upwards only
tkalvas 627b3fd
add bad duration tags to issue store, better testing of duration parsing
tkalvas dccc6d3
add tests for the osm duration tag cases where the leading unit is la…
tkalvas d345a03
explain the duration parser better, more tests
tkalvas c161378
prettier reformatting
tkalvas 21e183f
fix showing duration for escalator edges in debug client
tkalvas 6f5b5df
put escalator preferences in a sub object in walk preferences
tkalvas 1e64a99
prettier wanted less spaces
tkalvas 9c8e928
comment formatting
tkalvas 68b9c61
revert explanation of default escalator speed
tkalvas ffffb13
Update application/src/main/java/org/opentripplanner/osm/model/OsmWit…
optionsome File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,8 +1,12 @@ | ||
package org.opentripplanner.graph_builder.module.osm; | ||
|
||
import java.time.Duration; | ||
import java.util.Arrays; | ||
import java.util.List; | ||
import java.util.Map; | ||
import java.util.Optional; | ||
import org.opentripplanner.graph_builder.issue.api.DataImportIssueStore; | ||
import org.opentripplanner.graph_builder.issue.api.Issue; | ||
import org.opentripplanner.osm.model.OsmWay; | ||
import org.opentripplanner.street.model.edge.EscalatorEdge; | ||
import org.opentripplanner.street.model.vertex.IntersectionVertex; | ||
|
@@ -13,9 +17,19 @@ | |
class EscalatorProcessor { | ||
|
||
private final Map<Long, IntersectionVertex> intersectionNodes; | ||
private final DataImportIssueStore issueStore; | ||
|
||
public EscalatorProcessor(Map<Long, IntersectionVertex> intersectionNodes) { | ||
// If an escalator is tagged as moving less than 5 cm/s, or more than 5 m/s, | ||
// assume it's an error and ignore it. | ||
private static final double SLOW_ESCALATOR_ERROR_CUTOFF = 0.05; | ||
private static final double FAST_ESCALATOR_ERROR_CUTOFF = 5.0; | ||
|
||
public EscalatorProcessor( | ||
Map<Long, IntersectionVertex> intersectionNodes, | ||
DataImportIssueStore issueStore | ||
) { | ||
this.intersectionNodes = intersectionNodes; | ||
this.issueStore = issueStore; | ||
} | ||
|
||
public void buildEscalatorEdge(OsmWay escalatorWay, double length) { | ||
|
@@ -27,30 +41,58 @@ public void buildEscalatorEdge(OsmWay escalatorWay, double length) { | |
.boxed() | ||
.toList(); | ||
|
||
Optional<Duration> duration = escalatorWay.getDuration(v -> | ||
issueStore.add( | ||
Issue.issue( | ||
"InvalidDuration", | ||
"Duration for osm node %d is not a valid duration: '%s'; it's replaced with 'Optional.empty()' (unknown).", | ||
escalatorWay.getId(), | ||
v | ||
) | ||
) | ||
); | ||
if (duration.isPresent()) { | ||
double speed = length / duration.get().toSeconds(); | ||
if (speed < SLOW_ESCALATOR_ERROR_CUTOFF || speed > FAST_ESCALATOR_ERROR_CUTOFF) { | ||
duration = Optional.empty(); | ||
tkalvas marked this conversation as resolved.
Show resolved
Hide resolved
|
||
issueStore.add( | ||
Issue.issue( | ||
"InvalidDuration", | ||
"Duration for osm node {} makes implied speed {} be outside acceptable range.", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use URL here as well. |
||
escalatorWay.getId(), | ||
speed | ||
) | ||
); | ||
} | ||
} | ||
for (int i = 0; i < nodes.size() - 1; i++) { | ||
if (escalatorWay.isForwardEscalator()) { | ||
EscalatorEdge.createEscalatorEdge( | ||
intersectionNodes.get(nodes.get(i)), | ||
intersectionNodes.get(nodes.get(i + 1)), | ||
length | ||
length, | ||
duration.orElse(null) | ||
); | ||
} else if (escalatorWay.isBackwardEscalator()) { | ||
EscalatorEdge.createEscalatorEdge( | ||
intersectionNodes.get(nodes.get(i + 1)), | ||
intersectionNodes.get(nodes.get(i)), | ||
length | ||
length, | ||
duration.orElse(null) | ||
); | ||
} else { | ||
EscalatorEdge.createEscalatorEdge( | ||
intersectionNodes.get(nodes.get(i)), | ||
intersectionNodes.get(nodes.get(i + 1)), | ||
length | ||
length, | ||
duration.orElse(null) | ||
); | ||
|
||
EscalatorEdge.createEscalatorEdge( | ||
intersectionNodes.get(nodes.get(i + 1)), | ||
intersectionNodes.get(nodes.get(i)), | ||
length | ||
length, | ||
duration.orElse(null) | ||
); | ||
} | ||
} | ||
|
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
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
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
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
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
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
Oops, something went wrong.
Oops, something went wrong.
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.
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.
I think you could use a link to OSM instead of just referencing the id (nodes have
url()
method). Also, the "it's replaced with 'Optional.empty()'" is too technical for the issue report. Instead you could just say something like "the value is ignored".