mthh / contour-rs

Contour polygon creation in Rust (using marching squares algorithm)
https://crates.io/crates/contour
Apache License 2.0
50 stars 10 forks source link

Feature f32 #8

Closed hakolao closed 9 months ago

hakolao commented 9 months ago

Hi, thanks for this library.

I needed f32 version, so I thought I'd PR it also.

Let me know if this won't do :D

hakolao commented 9 months ago

I am using the contour_lines function only, and with the following use case: Turning 2d bitmaps into contour lines.

image

mthh commented 9 months ago

Hi and thank you for your contribution (as well as for showing your example use case!).

I'll take a closer look and let you know if needed, or merge it and make a new release otherwise.

mthh commented 9 months ago

I took a closer look at your code and I was wondering : do you have any particular reasons for using the "Float" crate? I suspect it's not widely used (judging by its download statistics at https://crates.io/crates/Float).

I feel that by using the "num-traits" crate we could also make the code generic on f32 and f64, without this functionality being under a specific feature flag.

In most of the code, I guess it's a matter of changing the function signatures (fn within<T: num_traits::Float>(p: T, q: T, r: T) ...) and the Structs implementations (impl<T: num_traits::Float> ...), but in some (many ?) places we will probably also have to specify that T implements the CoordNum trait (https://docs.rs/geo/latest/geo/trait.CoordNum.html) since we're using geo_type::Coord (https://docs.rs/geo/latest/geo/geometry/struct.Coord.html) as the underlying type to describe points.

In fact, the "geo_types" crate (and therefore also "geo") uses "num-traits" to manage genericity over f32/f64 types.

What do you think about it ?

Anyway, I also think it would also be a good thing to be able to use this crate indifferently with f32 or f64 !

hakolao commented 9 months ago

I didn't use any crate, just:

#[cfg(feature = "f32")]
pub type Float = f32;
#[cfg(not(feature = "f32"))]
pub type Float = f64;
hakolao commented 9 months ago

Perhaps the more generic T route would be more idiomatic, I kinda went as simple as possible :), feature-gated it and made it so it doesn't break anything.

So yes, num-traits might be better. But do let me know if you prefer that rather than this type aliasing.

mthh commented 9 months ago

I didn't use any crate

Indeed, sorry for the confusion !

Using num-traits requires quite some work (whereas your proposal is simpler and covers the use case of using f32 or f64).

I'll merge your PR and make a new release. Maybe in a future release I'll integrate num-traits.

hakolao commented 9 months ago

great! Thanks :)

mthh commented 9 months ago

Published on crates.io. Thanks for your contribution :)