Skip to content

Enhanced street graph processing of OSM areas - #6463

Merged
vesameskanen merged 46 commits into
opentripplanner:dev-2.xfrom
HSLdevcom:fix-merged-area-platforms
Feb 18, 2025
Merged

vesameskanen merged 46 commits into
opentripplanner:dev-2.xfrom
HSLdevcom:fix-merged-area-platforms

Conversation

@vesameskanen

@vesameskanen vesameskanen commented Feb 14, 2025

Copy link
Copy Markdown
Contributor

Summary

Street graph generation code, which processes OSM areas, refactorized and optimized.

  • Boarding location reference list, which is needed only during graph building, moved away from graph's area group class
  • Stop linking with boarding areas is now more accurate, because it uses original platform centroid instead of the centroid of merged adjacent areas. This used to caused strange teleportation hundreds of meters away from platforms.
  • Walkable area building optimized: unnecessary self-intersection tests skipped and buggy bookkeeping of existing connections fixed. The slowest phase of street graph building is now over two times faster.
  • Visibility edges, which cross multiple areas, are no longer recursively cut into segments. Instead, area properties are accumulated into a single edge.
  • Removing duplicate area edges makes street routing faster and more efficient
  • Several poorly named classes (and some of their fields) renamed

The images below show stop linking in Kerava railway station. After the changes, a railway stop gets linked to a platform. In the past, a stop link teleported travellers far away from the platforms.

Before:
before

After:
after

Issue

Closes #6103

Unit tests

Some existing tests updated to reflect new improvements.

Documentation

Lots of outdated code comments removed and updated.

vesameskanen and others added 29 commits January 20, 2025 10:54
Points along a straight line are  often considered convex
One fake visibility point leading to nowhere was removed by convexity optimization.
Code deals with regular graph vertices. Nothing ends in the mentioned points.
- Refactor related code
- Do not split the edge to many segments. Instead, merge properties of crossed areas.
Code added every visibility edge two times.
@vesameskanen
vesameskanen requested a review from a team as a code owner February 14, 2025 12:22
implements OsmInfoGraphBuildRepository, Serializable {

private final Map<Edge, Platform> platforms = new HashMap<>();
private final Map<Area, Platform> areaPlatforms = new HashMap<>();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Area doesn't seem to have equals or hashcode. Are the maps using idenity in that case?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I did not even realize this issue, just trusted Java magic. I have to check this.

@vesameskanen vesameskanen Feb 18, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I guess java uses object address as hash or something like that. I do not see any problem in this.

.map(VertexLabel::osm)
.collect(Collectors.toSet())
),
Arguments.of(true, Set.of(VertexLabel.osm(3223067049L), VertexLabel.osm(768590748)))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I believe this is intended, but can you explain what is going on?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

One of the optimizations was adding a threshold for convexity. 3 points along a straight line were randomly interpreted to define a convex or concave angle, because of inaccuracy of computations. Only true concave points matter in visibility computations. Herrenberg test platform lost one unnecessary fake visibility point which is good.

@leonardehrenfried

Copy link
Copy Markdown
Member

Generally this looks very good and I can confirm that it resolves an issue in one of my data sets.

I understand that you radically simplified the WalkableAreaBuilder. I would love to understand what you did and what you removed. Are you available for a quick meeting about this?

vesameskanen and others added 2 commits February 18, 2025 08:38
One of the platforms in now concave (L shape). Average of vertices is outside the area of the platform.
Test that walkable area building generates a boarding location point which is inside the area.
optionsome
optionsome previously approved these changes Feb 18, 2025
@leonardehrenfried

Copy link
Copy Markdown
Member

Can you pull this commit so that we have a bit of documentation on equals/hashcode leonardehrenfried@f0a94d2

?

@t2gran

t2gran commented Feb 18, 2025

Copy link
Copy Markdown
Member

Area doesn't seem to have equals or hashcode. Are the maps using idenity in that case?

OTP 1.x relayed on Object#equals() and Object#hashCode(). This is in some situations much faster than overriding these methods and base the implementation on comparing fields. I recommend to do it explicit so it is clear why you do it:

  /**
   * Object identity is used for eq/hc. In this case this is safe since instances are 
   * created during graph build and is guaranteed to be unique.
   */
  @Override
  public final boolean equals(Object other) {
    return super.equals(other);
  }

  /**
   * See {@link #equals(Object)}
   */
  @Override
  public final int hashCode() {
    return super.hashCode();
  }

Note! Both methods are final.

Comment thread application/src/main/java/org/opentripplanner/street/model/edge/Area.java Outdated
Comment thread application/src/main/java/org/opentripplanner/street/model/edge/Area.java Outdated
…e/Area.java

Co-authored-by: Leonard Ehrenfried <mail@leonard.io>
…e/Area.java

Co-authored-by: Leonard Ehrenfried <mail@leonard.io>
@vesameskanen
vesameskanen merged commit 37f17c3 into opentripplanner:dev-2.x Feb 18, 2025
@vesameskanen
vesameskanen deleted the fix-merged-area-platforms branch February 18, 2025 13:28
t2gran pushed a commit that referenced this pull request Feb 18, 2025
t2gran pushed a commit that referenced this pull request Feb 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

+Bump Serialization Id Add this label if you want the serialization id automatically bumped after merging the PR !Optimization The feature is to improve performance.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Neighbouring platforms are merged when calculating centroid for boarding location links

4 participants