[common] Update to georust/geo v0.29.0 #328
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
A lot of the line measure traits were re-worked. Mostly this is just moving code around in hopes of similar methods across metric spaces being easier to find now that they are uniform. It also enabled replacing some mostly copy/pasted implementations in geo with generic implementations, which enabled some new functionality - e.g. we can now `.densify::<Geodesic>` One notable behavioral change: the output of the new `Bearing` trait is now uniformly 0...360. Previously GeodesicBearing and HaversineBearing returned -180..180 while RhumbBearing returned 0...360. georust/geo#1210 This actually uncovered a bug in ferrostar, reflected in the new output of ferrostar/src/snapshots/ferrostar__simulation__tests__state_from_polyline.snap Since previously we were casting a potentially negative number to u16 — now it's always positive (0...360).
michaelkirk
commented
Oct 30, 2024
| // Compute total segment length in meters | ||
| let segment_length = segment_linestring.haversine_length(); | ||
| let segment_length = segment_linestring.length::<Haversine>(); |
There was a problem hiding this comment.
For example, you could double down on the more accurate but slower segment_linestring.length::<Geodesic>()
There was a problem hiding this comment.
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 😂
common/ferrostar/src/snapshots/ferrostar__simulation__tests__state_from_polyline.snap
Show resolved
Hide resolved
ianthetechie
approved these changes
Oct 31, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
A lot of the line measure traits were re-worked: https://docs.rs/geo/latest/geo/#measures
Mostly this is just moving code around, consolidating in hopes of similar methods across metric spaces being easier to find now that they are uniform.
It also enabled replacing some mostly copy/pasted implementations in geo with generic implementations, which enabled some new functionality - e.g. we can now
.densify::<Geodesic>One notable behavioral change: the output of the new
Bearingtrait is now uniformly 0...360. Previously GeodesicBearing and HaversineBearing returned -180..180 while RhumbBearing returned 0...360. (see georust/geo#1210 for more)This actually uncovered a bug in ferrostar, reflected in the new output of
ferrostar/src/snapshots/ferrostar__simulation__tests__state_from_polyline.snap
Since previously we were casting a potentially negative number to u16 — now it's always positive (0...360).
There are some other changes I'd like to make, but this represents a mostly drop in replacement for the current behavior.