Skip to content
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

[common] Update to georust/geo v0.29.0 #328

Merged
merged 5 commits into from
Oct 31, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
94 changes: 92 additions & 2 deletions common/Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion common/ferrostar/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ wasm_js = [
]

[dependencies]
geo = "0.28.0"
geo = "0.29.0"
polyline = "0.11.0"
serde = { version = "1.0.210", features = ["derive"] }
serde_json = { version = "1.0.128", default-features = false }
Expand Down
77 changes: 26 additions & 51 deletions common/ferrostar/src/algorithms.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,8 @@ use crate::{
navigation_controller::models::TripProgress,
};
use geo::{
Closest, ClosestPoint, Coord, EuclideanDistance, GeodesicBearing, HaversineDistance,
HaversineLength, LineLocatePoint, LineString, Point,
Bearing, Closest, Coord, Distance, Euclidean, Geodesic, Haversine, HaversineClosestPoint,
Length, LineLocatePoint, LineString, Point,
};

#[cfg(test)]
Expand All @@ -28,23 +28,6 @@ use std::time::SystemTime;
#[cfg(all(test, feature = "web-time"))]
use web_time::SystemTime;

/// Normalizes a bearing returned from several `geo` crate functions,
/// which may be negative, into a positive unsigned integer.
///
/// NOTE: This function assumes that the input values are in the range -360 to +360,
/// and does not check the inputs for validity.
pub(crate) fn normalize_bearing(degrees: f64) -> u16 {
let rounded = degrees.round();
let normalized = if rounded < 0.0 {
rounded + 360.0
} else if rounded >= 360.0 {
rounded - 360.0
} else {
rounded
};
normalized.round() as u16
}

/// Get the index of the closest *segment* to the user's location within a [`LineString`].
///
/// A [`LineString`] is a set of points (ex: representing the geometry of a maneuver),
Expand All @@ -64,10 +47,13 @@ pub fn index_of_closest_segment_origin(location: UserLocation, line: &LineString
// Iterate through all segments of the line
.enumerate()
// Find the line segment closest to the user's location
.min_by(|(_, line1), (_, line2)| {
.min_by(|(_, line_segment_1), (_, line_segment_2)| {
// Note: lines don't implement haversine distances
ianthetechie marked this conversation as resolved.
Show resolved Hide resolved
let dist1 = line1.euclidean_distance(&point);
let dist2 = line2.euclidean_distance(&point);
// In case you're tempted to say that this looks like cross track distance,
// note that the Line type here is actually a line *segment*,
// and we actually want to find the closest segment, not the closest mathematical line.
let dist1 = Euclidean::distance(line_segment_1, &point);
let dist2 = Euclidean::distance(line_segment_2, &point);
dist1.total_cmp(&dist2)
})
.map(|(index, _)| index as u64)
Expand All @@ -85,13 +71,8 @@ fn get_bearing_to_next_point(
let current = points.next()?;
let next = points.next()?;

// This function may return negative bearing values, but we want to always normalize to [0, 360)
let degrees = normalize_bearing(current.geodesic_bearing(next));

Some(CourseOverGround {
degrees,
accuracy: None,
})
let degrees = Geodesic::bearing(current, next);
ianthetechie marked this conversation as resolved.
Show resolved Hide resolved
Some(CourseOverGround::new(degrees, None))
}

/// Apply a snapped course to a user location.
Expand Down Expand Up @@ -161,7 +142,7 @@ fn snap_point_to_line(point: &Point, line: &LineString) -> Option<Point> {
// Bail early when we have two essentially identical points.
// This can cause some issues with edge cases (captured in proptest regressions)
// with the underlying libraries.
if line.euclidean_distance(point) < 0.000_001 {
if Euclidean::distance(line, point) < 0.000_001 {
return Some(*point);
}

Expand All @@ -170,8 +151,7 @@ fn snap_point_to_line(point: &Point, line: &LineString) -> Option<Point> {
return None;
}

// TODO: Use haversine_closest_point once a new release is cut which doesn't panic on intersections
match line.closest_point(point) {
match line.haversine_closest_point(point) {
Closest::Intersection(snapped) | Closest::SinglePoint(snapped) => {
let (x, y) = (snapped.x(), snapped.y());
if is_valid_float(x) && is_valid_float(y) {
Expand Down Expand Up @@ -210,7 +190,7 @@ fn snap_point_to_line(point: &Point, line: &LineString) -> Option<Point> {
/// };
/// let off_line = point! {
/// x: 1.0,
/// y: 0.5,
/// y: 0.5,
/// };
///
/// // The origin is directly on the line
Expand All @@ -220,13 +200,14 @@ fn snap_point_to_line(point: &Point, line: &LineString) -> Option<Point> {
/// assert_eq!(deviation_from_line(&midpoint, &linestring), Some(0.0));
///
/// // This point, however is off the line.
/// // That's a huge number, because we're dealing with degrees ;)
/// // That's a huge number, because we're dealing with points jumping by degrees ;)
/// println!("{:?}", deviation_from_line(&off_line, &linestring));
/// assert!(deviation_from_line(&off_line, &linestring)
/// .map_or(false, |deviation| deviation - 39312.21257675703 < f64::EPSILON));
/// .map_or(false, |deviation| deviation - 39316.14208341989 < f64::EPSILON));
/// ```
pub fn deviation_from_line(point: &Point, line: &LineString) -> Option<f64> {
snap_point_to_line(point, line).and_then(|snapped| {
let distance = snapped.haversine_distance(point);
let distance = Haversine::distance(snapped, *point);

if distance.is_nan() || distance.is_infinite() {
None
Expand All @@ -243,7 +224,7 @@ fn is_close_enough_to_end_of_linestring(
) -> bool {
if let Some(end_coord) = current_step_linestring.coords().last() {
let end_point = Point::from(*end_coord);
let distance_to_end = end_point.haversine_distance(current_position);
let distance_to_end = Haversine::distance(end_point, *current_position);

distance_to_end <= threshold
} else {
Expand Down Expand Up @@ -310,8 +291,8 @@ pub fn should_advance_to_next_step(
// If the user's distance to the snapped location on the *next* step is <=
// the user's distance to the snapped location on the *current* step,
// advance to the next step
current_position.haversine_distance(&next_step_closest_point)
<= current_position.haversine_distance(&current_step_closest_point)
Haversine::distance(current_position, next_step_closest_point)
<= Haversine::distance(current_position, current_step_closest_point)
} else {
// The user's location couldn't be mapped to a single point on both the current and next step.
// Fall back to the distance to end of step mode, which has some graceful fallbacks.
Expand Down Expand Up @@ -366,7 +347,7 @@ pub(crate) fn advance_step(remaining_steps: &[RouteStep]) -> StepAdvanceStatus {
/// The result is given in meters.
/// The result may be [`None`] in case of invalid input such as infinite floats.
fn distance_along(point: &Point, linestring: &LineString) -> Option<f64> {
let total_length = linestring.haversine_length();
let total_length = linestring.length::<Haversine>();
if total_length == 0.0 {
return Some(0.0);
}
Expand All @@ -379,9 +360,9 @@ fn distance_along(point: &Point, linestring: &LineString) -> Option<f64> {

// Compute distance to the line (sadly Euclidean only; no haversine_distance in GeoRust
// but this is probably OK for now)
let segment_distance_to_point = segment.euclidean_distance(point);
let segment_distance_to_point = Euclidean::distance(&segment, point);
// Compute total segment length in meters
let segment_length = segment_linestring.haversine_length();
let segment_length = segment_linestring.length::<Haversine>();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For example, you could double down on the more accurate but slower segment_linestring.length::<Geodesic>()

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, exactly. That's probably worth evaluating properly. I've opened #329 to follow up on this. This is probably a case where Geodesics could make sense. On the other hand, I am pretty sure Valhalla uses haversine exclusively and is generally fine 😂


if segment_distance_to_point < closest_dist_to_point {
let segment_fraction = segment.line_locate_point(point)?;
Expand Down Expand Up @@ -410,7 +391,7 @@ fn distance_to_end_of_step(
snapped_location: &Point,
current_step_linestring: &LineString,
) -> Option<f64> {
let step_length = current_step_linestring.haversine_length();
let step_length = current_step_linestring.length::<Haversine>();
distance_along(snapped_location, current_step_linestring)
.map(|traversed| step_length - traversed)
}
Expand Down Expand Up @@ -536,7 +517,7 @@ proptest! {
prop_assert!(is_valid_float(x) || (!is_valid_float(x1) && x == x1));
prop_assert!(is_valid_float(y) || (!is_valid_float(y1) && y == y1));

prop_assert!(line.euclidean_distance(&snapped) < 0.000001);
prop_assert!(Euclidean::distance(&line, &snapped) < 0.000001);
} else {
// Edge case 1: extremely small differences in values
let is_miniscule_difference = (x1 - x2).abs() < 0.00000001 || (y1 - y2).abs() < 0.00000001;
Expand Down Expand Up @@ -635,7 +616,7 @@ proptest! {
speed: None
};
let user_location_point = Point::from(user_location);
let distance_from_end_of_current_step = user_location_point.haversine_distance(&end_of_step.into());
let distance_from_end_of_current_step = Haversine::distance(user_location_point, end_of_step.into());

// Never advance to the next step when StepAdvanceMode is Manual
prop_assert!(!should_advance_to_next_step(&current_route_step.get_linestring(), next_route_step.as_ref(), &user_location, StepAdvanceMode::Manual));
Expand Down Expand Up @@ -730,12 +711,6 @@ proptest! {
prop_assert!(index < (coord_len - 1) as u64);
}

#[test]
fn test_bearing_correction_valid_range(bearing in -360f64..360f64) {
let result = normalize_bearing(bearing);
prop_assert!(result < 360);
}

#[test]
fn test_bearing_fuzz(coords in vec(arb_coord(), 2..500), index in 0usize..1_000usize) {
let line = LineString::new(coords);
Expand Down
14 changes: 12 additions & 2 deletions common/ferrostar/src/models.rs
Original file line number Diff line number Diff line change
Expand Up @@ -171,8 +171,18 @@ pub struct CourseOverGround {
}

impl CourseOverGround {
pub fn new(degrees: u16, accuracy: Option<u16>) -> Self {
Self { degrees, accuracy }
/// # Arguments
///
/// - degrees: The direction in which the user's device is traveling, measured in clockwise degrees from
/// true north (N = 0, E = 90, S = 180, W = 270).
/// NOTE: Input values must lie in the range [0, 360).
/// - accuracy: the accuracy of the course value, measured in degrees.
pub fn new(degrees: f64, accuracy: Option<u16>) -> Self {
ianthetechie marked this conversation as resolved.
Show resolved Hide resolved
debug_assert!(degrees >= 0.0 && degrees < 360.0);
Self {
degrees: degrees.round() as u16,
accuracy,
}
}
}

Expand Down
7 changes: 5 additions & 2 deletions common/ferrostar/src/navigation_controller/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,10 @@ use crate::{
},
models::{Route, UserLocation},
};
use geo::{HaversineDistance, LineString, Point};
use geo::{
algorithm::{Distance, Haversine},
geometry::{LineString, Point},
};
use models::{NavigationControllerConfig, StepAdvanceStatus, TripState};
use std::clone::Clone;

Expand Down Expand Up @@ -125,7 +128,7 @@ impl NavigationController {
let next_waypoint: Point = waypoint.coordinate.into();
// TODO: This is just a hard-coded threshold for the time being.
// More sophisticated behavior will take some time and use cases, so punting on this for now.
current_location.haversine_distance(&next_waypoint) < 100.0
Haversine::distance(current_location, next_waypoint) < 100.0
} else {
false
};
Expand Down
4 changes: 2 additions & 2 deletions common/ferrostar/src/navigation_controller/test_helpers.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use crate::models::{BoundingBox, GeographicCoordinate, Route, RouteStep, Waypoint, WaypointKind};
#[cfg(feature = "alloc")]
use alloc::string::ToString;
use geo::{line_string, BoundingRect, HaversineLength, LineString, Point};
use geo::{line_string, BoundingRect, Haversine, Length, LineString, Point};

pub fn gen_dummy_route_step(
start_lng: f64,
Expand All @@ -24,7 +24,7 @@ pub fn gen_dummy_route_step(
(x: start_lng, y: start_lat),
(x: end_lng, y: end_lat)
]
.haversine_length(),
.length::<Haversine>(),
duration: 0.0,
road_name: None,
instruction: "".to_string(),
Expand Down
Loading
Loading