Implement splitting on access restrictions #45
Conversation
brad-richardson
left a comment
brad-richardson
left a comment
There was a problem hiding this comment.
Great work! A few minor thoughts
|
|
||
| /// Compares two floating point numbers with a small tolerance | ||
| fn fuzzy_compare(a: f64, b: f64) -> bool { | ||
| // less than one centimeter at equator |
There was a problem hiding this comment.
Can you elaborate on this a bit? We're assuming everything is in our original WGS84 projection, etc right?
There was a problem hiding this comment.
Should this go in a shared utility?
| fuzzy_compare(c1.x, c2.x) && fuzzy_compare(c1.y, c2.y) | ||
| } | ||
|
|
||
| /// Removes duplicate start and end points from a LineString, while ensuring at least two points remain |
There was a problem hiding this comment.
How common is this in your testing? I think most of these duplicate coords should be fixed upstream
There was a problem hiding this comment.
but it may also be only repairing two consecutive duplicate coords, so 3+ might slip through
| for i in 1..segment.geometry.0.len() { | ||
| let p1 = &segment.geometry.0[i - 1]; | ||
| let p2 = &segment.geometry.0[i]; | ||
| let segment_length = ((p2.y - p1.y).powi(2) + (p2.x - p1.x).powi(2)).sqrt(); |
There was a problem hiding this comment.
This probably works for most cases, but at global scale, we'll need to have a more robust length calculation. We spent quite a bit of time working through issues on this for the splitter: https://github.com/OvertureMaps/transportation-splitter/blob/main/transportation_splitter.py#L608
| } | ||
|
|
||
| /// Aproximates the length of the segment by summing the distances between its points | ||
| fn calc_segment_length(segment: Segment) -> f64 |
There was a problem hiding this comment.
Same here, this probably works for most cases, but the connector at values won't always match
| road_class = Some(class.to_string()); | ||
| } | ||
| } else if column.0 == "access_restrictions" { | ||
| let restrictions = column.1; |
There was a problem hiding this comment.
Can we extract this to its own function? There's a lot going on here
| geo_types::Coord { x: 1.0, y: 0.0 }, | ||
| geo_types::Coord { x: 2.0, y: 0.0 }, | ||
| geo_types::Coord { x: 3.0, y: 0.0 }, | ||
| geo_types::Coord { x: 4.0, y: 0.0 }, |
There was a problem hiding this comment.
Nice, thanks for adding tests. May want to add one or two more that have more complex "real-world" data, ideally directly from production data for the connector LR values to test projection mismatches
|
|
||
| // Loop while there are access restrictions to process | ||
| while !remaining_segment.properties.access_restrictions.as_ref().unwrap().is_empty() { | ||
| let length = calc_segment_length(remaining_segment.clone()); |
There was a problem hiding this comment.
Is the clone necessary here?
Implement conversion of Overture access restrictions to Valhalla arcs.