Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements directed graph generation for OvertureMaps segments by creating separate SegmentSplit records for forward and backward headings based on access restrictions. The PR addresses issue #75 by adding directionality to segments that are implicitly bidirectional in the OvertureMaps schema unless restricted.
Changes:
- Added heading-aware logic to generate directed edges from segments by evaluating access restrictions against user-defined travel modes
- Extended SegmentSplit to include heading information and implemented logic to reverse src/dst vertices and coordinates for backward edges
- Refactored SegmentDestination and related structs to properly model the OvertureMaps schema with required fields and public visibility
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| rust/bambam-omf/src/graph/segment_ops.rs | Implements core logic for determining valid headings based on access restrictions with comprehensive test coverage |
| rust/bambam-omf/src/graph/segment_split.rs | Extends SegmentSplit with heading field and implements vertex/coordinate reversal for backward edges |
| rust/bambam-omf/src/graph/omf_graph.rs | Modifies graph construction to generate splits for both headings separately |
| rust/bambam-omf/src/graph/serialize_ops.rs | Updates split finding to accept optional SegmentAccessRestrictionWhen parameter |
| rust/bambam-omf/src/collection/record/transportation_segment.rs | Refactors SegmentDestination and related types to match OvertureMaps schema |
| rust/bambam-omf/src/collection/record/geometry_wkb_codec.rs | Reorders enum variants for serde deserialization optimization |
| rust/bambam-omf/src/collection/record/common.rs | Adds Default derives for common types used in testing |
| rust/bambam-omf/src/collection/record/mod.rs | Exports additional types needed for graph construction |
| rust/bambam-omf/src/collection/mod.rs | Exports SegmentAccessRestrictionWhen publicly |
| rust/bambam-omf/src/collection/filter/travel_mode_filter.rs | Renames ignore_unset to allow_unset for clarity |
| rust/bambam-omf/src/app/network.rs | Implements conversion from NetworkEdgeListConfiguration to SegmentAccessRestrictionWhen |
| rust/bambam-omf/Cargo.toml | Adds serde_bytes dependency and alphabetizes dependencies |
| rust/bambam/Cargo.toml | Alphabetizes dependencies |
| rust/bambam-py/Cargo.toml | Alphabetizes dependencies |
| rust/bambam-osm/Cargo.toml | Alphabetizes dependencies |
| rust/bambam-gbfs/Cargo.toml | Alphabetizes dependencies |
| rust/Cargo.toml | Alphabetizes workspace members and dependencies |
| configuration/bambam-omf/travel-mode-filter.json | Updates field name from ignore_unset to allow_unset |
Comments suppressed due to low confidence (1)
rust/bambam-omf/src/collection/record/transportation_segment.rs:1
- The SegmentDestination struct changes three fields from
Option<String>to requiredStringfields. This is a breaking change that may cause deserialization failures if the OvertureMaps data contains null values for these fields. According to the OvertureMaps schema documentation, these fields should be optional if they may not always be present in the source data. Verify that these fields are always present in the actual OvertureMaps data, or revert toOption<String>to handle missing values gracefully.
use geo::{Coord, Geometry, Haversine, InterpolatableLine, Length, LineString};
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
yamilbknsu
left a comment
There was a problem hiding this comment.
This makes sense to me. Just for the future: is it correct to say that this relies on the assumption that any directionality will be given as an access restriction and thus it is defined at the segment level (and not at the split level under any circumstances)?
I think that is a sensible assumption, just wanted to make sure its here in case we are looking for bugs later.
that's the understanding i developed during the task. when i started, i thought other fields like destinations might also contribute to directionality but after working through it and looking at examples, i left with the impression that directionality is 1) default bidrectional, 2) if specified, defined via access_restriction |
this PR closes #75 by implementing the logic to generate directed SegmentSplit records from a Segment based on the user-defined target SegmentModes and the segment-specific values for access_restriction. in the process, the basic SegmentSplit variant was extended to include a Heading. this is determined by comparing a user-provided
Option<SegmentAccessRestrictionWhen>instance to the optional instance stored on the segment. by applying a few rules, in particular about heading-specific restrictions vs broader restrictions, and tracking "deny-by-default" patterns, we can determine what headings are valid.handling the deny-by-default, permit by exception logic requires making a decision about ambiguity. for example, from a real segment:
we read this as
but this could also be interpreted as "no one can travel backward except bikes who have a reverse travel lane".
ensuring we are correctly handling restriction logic should remain an open discussion.