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

Fix invalid progress calculation #127

Merged
merged 6 commits into from
Jul 2, 2024

Conversation

ianthetechie
Copy link
Contributor

This is probably the root cause of #126.

@ianthetechie ianthetechie requested review from Archdoog and CatMe0w June 30, 2024 11:08
current_step_linestring: &LineString,
remaining_steps: &[RouteStep],
) -> TripProgress {
if remaining_steps.is_empty() {
let Some(current_step) = remaining_steps.first() else {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Passing current_step explicitly here opened the door for potentially very confusing errors caused by this ambiguity. I've just made it impossible to make that mistake now.

crate-type = ["cdylib", "staticlib", "lib"]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Linter catch: this didn't have any effect in a workspace sub-project ;)

Comment on lines +68 to +70
cargo build -p $basename --lib --release --target x86_64-apple-ios
cargo build -p $basename --lib --release --target aarch64-apple-ios-sim
cargo build -p $basename --lib --release --target aarch64-apple-ios
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Minor improvement: we don't need to build uniffi-bindgen for any other architectures.

Comment on lines +298 to +302
let pct_remaining_current_step = if current_step.distance > 0f64 {
distance_to_next_maneuver / current_step.distance
} else {
0f64
};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was the core issue. If the snapped point is exactly at the end of the step, this results in division by zero.

Comment on lines +504 to +506
// This may look wrong, but it's actually how Valhalla (and presumably others)
// represent a point geometry for the arrival step.
let current_route_step = gen_dummy_route_step(x1, y1, x1, y1);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

And this is why I noticed the issue; the last maneuver's geometry is the same point twice, so its length is zero; thus, the division by zero above, which ended up crashing one of the iOS formatters in the arrival view with a NaN.

@ianthetechie ianthetechie merged commit f8af19f into main Jul 2, 2024
11 checks passed
@ianthetechie ianthetechie deleted the fix-invalid-progress-calculation branch July 2, 2024 01:12
@Archdoog Archdoog mentioned this pull request Jul 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants