More ergonomic FeatureCollection with properties creation

[urschrei]

I was looking at the new ZoneBuilder functionality, and this comment from @dabreegster caught my eye, and the RStar GeomWithData method addresses a similar need.

Ideally, we'd have some FeatureCollection::from impls that accept e.g. a vec (or any impl iterator) of structs with a property field (anything that can be used to build a serde_json::Map) and a Into<geo::Geometry> field, or a vec-of-vecs, vec-of-tuples, tuple-of-vecs &c.

[rmanoka]

One approach is to split the concern into two:

1. Implement the std Fromiterator. This allows conversion via the collect() method:

   let _: FeatureCollection = into_iter_of_features.collect();

   This is a std. trait and should be impl by all collection types. One neat feature is that it automatically also provides collection into a Result<FeatureCollection, E> from iterator of Result<Feature, E> due to this impl.

2. Implement ergonomic conversion from appropriate types to Feature. Along with above, it allows:

   let _: FeatureCollection = into_iter.map( /* convert to feature */ ).collect();

   The crate already seems to already have some such conversions. I don't have too much opinion on how / if we should implement this for tuple types.

[michaelkirk]

This is a fun case study in API design!

For context:

Originally, the zonebuilder geojson output code looked like this: https://github.com/zonebuilders/zonebuilder-rust/commit/0ec72093eb21e9b6f74a0c3a162a8a186511fc32

1

let mut features: Vec<Feature> = polygons
    .iter()
    .map(|poly| Feature {
        bbox: None,
        geometry: Some(Geometry::from(poly)),
        id: None,
        properties: None,
        foreign_members: None,
    })
    .collect();

let fc = FeatureCollection {
    bbox: None,
    features,
    foreign_members: None,
};

let gj = GeoJson::from(fc);
gj

IIRC, there was a little consternation at this point - I believe the thought was along the lines of, "I already have a bunch of polygons, I just want to turn them into geojson, this should be easier".

So, my internal monologue was something like:

Sometimes people are only using geojson as a way to serialize geometeries, and have no need to output property-data — a use case more like WKT. We should offer a streamlined API to do this...

In response to that, I opened https://github.com/georust/geojson/issues/163 (implemented in https://github.com/georust/geojson/pull/165).

Then in https://github.com/zonebuilders/zonebuilder-rust/commit/853b9fd25e23b72755eba0d42583240c5082285f, the zonebuilder code adopted it, and was made to look like this:

2

let gc = geo::GeometryCollection::from_iter(polygons);
let fc = geojson::FeatureCollection::from(&gc);
GeoJson::from(fc)

However, when @dabreegster added labeling to zonebuilder, which indeed required passing properties along with the geometric data. Reverting the code to something a little more like the original code.

3

let geom_collection =
    geo::GeometryCollection::from_iter(zones.iter().map(|(_, poly)| poly.clone()));
    let mut feature_collection = geojson::FeatureCollection::from(&geom_collection);
    for (feature, (name, _)) in feature_collection.features.iter_mut().zip(zones) {
        let mut properties = serde_json::Map::new();
        properties.insert("name".to_string(), name.into());
        feature.properties = Some(properties);
    }
GeoJson::from(feature_collection)

[michaelkirk]

FWIW, I like @rmanoka's proposal.

[urschrei]

FWIW, I like @rmanoka's proposal.

Yes, me too!

[rmanoka]

Thanks for providing more context @michaelkirk . The FromIterator impl. is definitely useful; in your snippet (3) above, it should help remove the double iteration, and hopefully also the extra clone:

let fc: FeatureCollection = zones.iter().map(|name, poly| {
    // Produce a feature
}).collect();

fc.into() // Assuming function output type is declared as GeoJson

This leaves the following couple pts. to discuss.

Feature from geo_types::*

It looks like we have a From<&geo::GeometryCollection> for FeatureCollection but no equivalent for a Feature itself. That should help circumvent doing the somewhat roundabout geo::* -> geo::GeomCollection -> FeatureCollection and the more intuitive geo::* -> Feature -> FeatureCollection. It'll also streamline the adding of properties, and hopefully the extra clone above. I think the implementation could basically piggy-back on the existing From<> for Value conversions.

BBox calculation

This might be more bikeshedding at this point, but is it common to just leave the bbox as None? Should we use the geo algo traits to compute a bbox on the fly when converting? I believe the algo. cost is almost negligible as it's just computing max/min of the coords. The user could always opt out by converting to Value and creating Feature themselves.

The same also applies for FromIterator for FeatureCollection: combine the bbox of the individual Feature. I'm not an expert on the semantics of this property; might be safe to leave it None unless all the items have it.

---

With all these, snippet 3 would look fairly ergonomic IMO:

let fc: FeatureCollection = zones.iter().map(|name, poly| {
    let feat: Feature = poly.into();
    feat.set_property("name", name);
    feat
}).collect();

fc.into() // Assuming function output type is declared as GeoJson

Implementation Checklist

To summarize, this is the impl. checklist I'm suggesting:

1. [ ] FromIterator for FeatureCollection
2. [ ] From<&'a T> for Feature where From<&'a T> for Value, T: BoundingRect
3. [ ] Ensure correct bbox calculations in 1 and 2 above

[michaelkirk]

To summarize, this is the impl. checklist I'm suggesting:

That sounds great, thanks for laying it out @rmanoka!