georust / geo

Geospatial primitives and algorithms for Rust
https://crates.io/crates/geo
Other
1.49k stars 194 forks source link

Add line segmentize trait #1055

Closed JosiahParry closed 10 months ago

JosiahParry commented 11 months ago

This PR adds a new trait LineStringSegmentize which provides a method line_segmentize(n: usize) to split LineStrings inton equal length LineStrings which are returned as an Option<MultiLineString> . This is inspired by the lwgeom R package which wraps liblwgeom from PostGIS.

JosiahParry commented 11 months ago

The almighty clippy should be satisfied now.

urschrei commented 11 months ago

Looks like Clippy still isn't happy. Do you see those failures when you run it locally?

JosiahParry commented 11 months ago

@urschrei I do not get them locally, I get different ones! These ones seem simple enough that I should be able to handle them. I just don't want to consume all of the GHA hours

image
JosiahParry commented 11 months ago

I pulled the most recent changes from main and CI is failing unrelated to my changes.

  failed to query replaced source registry `crates-io`
michaelkirk commented 11 months ago

A couple lines further up in the failed action output shows:

   Updating crates.io index
warning: spurious network error (3 tries remaining): [28] Timeout was reached (Operation too slow. Less than 10 bytes/sec transferred the last 30 seconds)
warning: spurious network error (2 tries remaining): [28] Timeout was reached (Operation too slow. Less than 10 bytes/sec transferred the last 30 seconds)
warning: spurious network error (1 tries remaining): [28] Timeout was reached (Operation too slow. Less than 10 bytes/sec transferred the last 30 seconds)

So, presumably this is some kind of transient network failure. I've restarted the jobs. 🤞

JosiahParry commented 11 months ago

thanks @michaelkirk! Looks good now.

michaelkirk commented 11 months ago

BTW have you seen https://github.com/georust/geo/pull/1050?

It's not quite the same thing, but it seems like maybe you could leverage the functionality within. Though, I'm not sure if you'd want to, I haven't thought about it very hard.

JosiahParry commented 11 months ago

1050 is interesting and likely a bit more challenging than what i endeavored for here! I could give it a further look after this.

JosiahParry commented 11 months ago

Recent commits allow for n to be greater than the number of Lines that compose a LineString which is done using the Densify trait to ensure that the distance between Coords is x.euclidean_length() / n.

JosiahParry commented 11 months ago

I've updated the branch with main. Please let me know what more is needed for this PR. Thanks!

JosiahParry commented 10 months ago

Just following up here. Please let me know what more is needed on this PR. Thanks!

JosiahParry commented 10 months ago

@michaelkirk / @urschrei, please let me know what tasks are remaining for this PR. Thanks! R bindings to geo are based on my fork with this functionality so a downstream package can improve their peformance by 4 orders of magnitude. I'd love to be able to point to main and keep up with new PRs :)

JosiahParry commented 10 months ago

All comments are resolved. Praying to the CI overlords 🙏🏼

urschrei commented 10 months ago

Guessing that was an inadvertent close.

JosiahParry commented 10 months ago

oops! Correct. Thank you @urschrei. CI caught an example issue.

JosiahParry commented 10 months ago

Changes should be resolved and CI is satisfied. @urschrei when you get a chance, let me know if I've missed anything! Thanks again for the feedback

JosiahParry commented 10 months ago

Sounds good! Enjoy you're weekend

urschrei commented 10 months ago

[tolling bell] Last call!