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

Make geojson optional, use geo-types for geometry representation. #6

Closed michaelkirk closed 1 year ago

michaelkirk commented 1 year ago

Fixes #5

Note this is a breaking change.

Rather than author up geometry types from scratch, I opted to use the existing geo-types for Coordinate/Polygon/MultiPolygon. If you'd prefer we could implement them directly in this crate to get rid of the dependency, but geo-types:

before:

test bench_build_geojson_contour                                       
        ... bench:       3,337 ns/iter (+/- 182)

test bench_build_geojson_contour_no_smoothing                          
        ... bench:       3,252 ns/iter (+/- 328)

test bench_build_geojson_contours_multiple_thresholds                  
        ... bench:      16,838 ns/iter (+/- 1,371)

test
bench_build_geojson_contours_multiple_thresholds_and_x_y_steps_and_origins
... bench:      16,934 ns/iter (+/- 1,325)

test bench_build_isoring                                               
        ... bench:       2,890 ns/iter (+/- 135)

test bench_build_isoring_values2                                       
        ... bench:       5,103 ns/iter (+/- 395)

after:

test bench_build_geojson_contour                                       
        ... bench:       1,917 ns/iter (+/- 29)

test bench_build_geojson_contour_no_smoothing                          
        ... bench:       1,833 ns/iter (+/- 83)

test bench_build_geojson_contours_multiple_thresholds                  
        ... bench:       9,414 ns/iter (+/- 206)

test
bench_build_geojson_contours_multiple_thresholds_and_x_y_steps_and_origins
... bench:       9,490 ns/iter (+/- 539)

test bench_build_isoring                                               
        ... bench:       1,639 ns/iter (+/- 72)

test bench_build_isoring_values2                                       
        ... bench:       2,893 ns/iter (+/- 102)
mthh commented 1 year ago

Thanks ! I see no special reason not to use geo-types so let's go for it.

I glanced at the abstreet code you linked to, I'm glad it makes using this crate easier and avoids the unnecessary passage through the geojson representation !

mthh commented 1 year ago

In fact I may be AFK the next few days, so in order not to let this PR linger I will merge it now, fix the small typo and change the "value" field to "threshold" in the geojson representation in subsequent commits, update changelog and readme and make a new release today.

Thanks again for the very useful contribution !