geo icon indicating copy to clipboard operation
geo copied to clipboard

Add `DensifyGeodesic` trait

Open grevgeny opened this issue 1 year ago • 5 comments

  • [x] I agree to follow the project's code of conduct.
  • [x] I added an entry to CHANGES.md if knowledge of this change could be valuable to users.

This PR adds DensifyGeodesic trait that follows logic similar to DensifyHaversine. However, the trait is only implemented for T = f64 as underlying Geodesic traits are implemented only for this type.

grevgeny avatar Aug 10 '24 19:08 grevgeny

@michaelkirk thanks for your review! I have added proposed changes

grevgeny avatar Aug 13 '24 18:08 grevgeny

I think you'll need to run cargo fmt, otherwise LGTM

I'll leave it open for a couple days before merging in case anyone else wants to weigh in.

michaelkirk avatar Aug 13 '24 18:08 michaelkirk

@michaelkirk

I've just realised that there is already the following method: geodesic_intermediate_fill. It expresses pretty much the same logic.

Do we still need this another implementation?

grevgeny avatar Aug 17 '24 14:08 grevgeny

Darn it. I'm sorry for not realizing this earlier. This seems related to https://github.com/georust/geo/issues/1181

michaelkirk avatar Aug 20 '24 01:08 michaelkirk

Assuming #1216 gets merged, I've got a different approach that should supersede this, generically implementing Densify for all line measures (Haversine, Geodesic, Euclidean, etc.)

You can see a WIP here: https://github.com/georust/geo/compare/mkirk/unify-densify?expand=1

michaelkirk avatar Sep 26 '24 18:09 michaelkirk

My alternative to this was merged in #1231.

With that you can call:

use geo::{Geodesic, Densify, Euclidean};
line_string.densify::<Geodesic>();
line_string.densify::<Euclidean>();

Let me know if that works for your purposes @grevgeny.

michaelkirk avatar Oct 23 '24 00:10 michaelkirk

With #1298 (not yet released) this changed to:

use geo::{Geodesic, Densify, Euclidean};
Geodesic.densify(&line_string);
Euclidean.densify(&line_string);

michaelkirk avatar Feb 05 '25 22:02 michaelkirk