Skip to content

Conversation

binh-dam-ibigroup
Copy link
Contributor

@binh-dam-ibigroup binh-dam-ibigroup commented Sep 30, 2025

Summary

This PR adds a CrosswalkNamer class to assign names to crosswalks, such as "crossing crosswalk over 10th Street" (see screenshot), so that they have better names than just "path" for turn-by-turn directions. As of writing, only crosswalks that have 3 or more nodes in OSM and a shared node with a street are covered. The PR also apply the crosswalk name to short sidewalk segments if they are the only adjacent way at either end of the crosswalk.

The CrosswalkNamer is copied over from from SidewalkNamer (both classes refactored for common logic), and a few supporting helper methods are added to OsmWay for determining footways and adjacent ways.

Screenshot:
screenshot of OTP Debug showing paths with street names and street crossings

Issue

Closes #6864

Things to discuss/resolve

Unit tests

Unit tests are added for the process for finding and applying names to various types of crosswalks (over a named street, over a service road, over a turn lane).

Documentation

Added.

Bumping the serialization version id

Not necessary, I think

Copy link

codecov bot commented Oct 2, 2025

Codecov Report

❌ Patch coverage is 83.58209% with 33 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.20%. Comparing base (9966fd4) to head (7d52252).

Files with missing lines Patch % Lines
...raph_builder/module/osm/naming/CrosswalkNamer.java 85.07% 4 Missing and 6 partials ⚠️
...lder/module/osm/naming/SidewalkCrosswalkNamer.java 0.00% 10 Missing ⚠️
...uilder/module/osm/naming/PreciseBufferFactory.java 73.33% 4 Missing ⚠️
...graph_builder/module/osm/naming/SidewalkNamer.java 87.50% 0 Missing and 3 partials ⚠️
...ain/java/org/opentripplanner/osm/model/OsmWay.java 86.66% 0 Missing and 2 partials ⚠️
...ilder/module/osm/naming/BufferedEdgeProcessor.java 96.87% 0 Missing and 1 partial ⚠️
...aph_builder/module/osm/naming/StreetEdgeIndex.java 90.00% 0 Missing and 1 partial ⚠️
...pplanner/graph_builder/services/osm/EdgeNamer.java 50.00% 1 Missing ⚠️
...ing/algorithm/mapping/StatesToWalkStepsMapper.java 93.33% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@              Coverage Diff              @@
##             dev-2.x    #6918      +/-   ##
=============================================
+ Coverage      72.18%   72.20%   +0.01%     
- Complexity     19838    19895      +57     
=============================================
  Files           2155     2161       +6     
  Lines          80048    80185     +137     
  Branches        8082     8104      +22     
=============================================
+ Hits           57786    57896     +110     
- Misses         19415    19430      +15     
- Partials        2847     2859      +12     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@leonardehrenfried
Copy link
Member

We are getting there. I took the liberty to not use to many static methods: ibi-group@ee9d9bb

If you agree with this, can you cherry-pick it into this branch.

Also, there are conflicts now.

@binh-dam-ibigroup
Copy link
Contributor Author

We are getting there. I took the liberty to not use to many static methods: ibi-group@ee9d9bb

Good idea!

Copy link
Contributor

@VillePihlava VillePihlava left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job refactoring and extracting stuff out of classes. Recently I looked at similar OsmEntity way logic related to levels and it was really confusing. Can there be a good reason that an EdgeNamer can have OsmEntity way as parameter or should they always be OsmWay way? I think this should be changed if possible.

Comment on lines +51 to +53
public I18NString name(OsmEntity way) {
return way.getAssumedName();
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is really a way, we should use the OsmWay type. If it has to be an OsmEntity there might be a logic error somewhere because relations and nodes can also be given as parameter in theory.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also personally prefer OsmWay and I also got confused because a lot of way/edge code is in OsmEntity. Does a StreetEdge always come from an OsmWay? Happy to discuss how to proceed in an upcoming call.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think another option is to maybe do it in a separate PR. Lets discuss in Thursday's meeting to decide how to proceed

Comment on lines +56 to +57
public void recordEdges(OsmEntity way, StreetEdgePair pair, OsmDatabase osmdb) {
if (way instanceof OsmWay osmWay) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you check if all of the namer-related OsmEntity way uses can be changed to OsmWay way?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

!Improvement A functional improvement or micro feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Crosswalk naming

4 participants