switch from many parameters in constructor to a single Options struct

[mikemorris]

semver major, fixes #20

/cc @mourner @jfirebaugh @incanus

[incanus]

👍, especially the abstraction of extent and buffer.

[mourner]

Looks great.

I'd suggest mirroring the JS version API though, with features as the first flat required argument and optional object for all the rest options.

BTW what's the reason to keep the API 2-step with first doing convertFeatures on GeoJSON data and then passing the converted features to constructor, instead of one step like in the JS version?

[mikemorris]

I'd suggest mirroring the JS version API though, with features as the first flat required argument and optional object for all the rest options.

Sounds good, will change this. 👍

BTW what's the reason to keep the API 2-step with first doing convertFeatures on GeoJSON data and then passing the converted features to constructor, instead of one step like in the JS version?

Not sure, I'm not too familiar with either version to be honest.

[mourner]

@incanus do you know?

[mikemorris]

@jfirebaugh Is there an easy fix for initializer lists in GCC?

../test/test_full.cpp:21:55: error: no matching function for call to ‘mapbox::util::geojsonvt::GeoJSONVT::GeoJSONVT(<brace-enclosed initializer list>)’
                         .indexMaxPoints = maxPoints } };
                                                       ^
../test/test_full.cpp:21:55: note: candidate is:
In file included from ../test/test_full.cpp:4:0:
../include/mapbox/geojsonvt/geojsonvt.hpp:33:5: note: mapbox::util::geojsonvt::GeoJSONVT::GeoJSONVT(mapbox::util::geojsonvt::Options)
     GeoJSONVT(Options);
     ^
../include/mapbox/geojsonvt/geojsonvt.hpp:33:5: note:   no known conversion for argument 1 from ‘<brace-enclosed initializer list>’ to ‘mapbox::util::geojsonvt::Options’

[jfirebaugh]

That's designated initializer syntax, and it's a C99 feature. As far as I know there's no portable way to use it in C++. You'll have to write it out manually:

    Options options;
    options.features = features;
    options.maxZoom = 14;
    options.indexMaxZoom = maxZoom;
    options.indexMaxPoints = maxPoints;
    GeoJSONVT index(options);

[incanus]

BTW what's the reason to keep the API 2-step with first doing convertFeatures on GeoJSON data and then passing the converted features to constructor, instead of one step like in the JS version?

Hmm, interesting, and I didn't do it in the intermediary Swift port:

https://github.com/mapbox/geojsonvt.swift/blob/bc44007a3baf7d26734e108e5d05079d037f39cd/geojsonvt/geojsonvt/geojsonvt.swift

I think this was related to something in the original shape annotations implementation (https://github.com/mapbox/mapbox-gl-native/pull/1655/files) like needing to convert first, but not necessarily tile until a later point.

[mourner]

I think this was related to something in the original shape annotations implementation

@incanus can we ditch the step now so that it mirrors the JS API?

[incanus]

Fine by me, particularly since this is now a separate project.

[kkaefer]

Why not store an Options object directly into the GeoJSONVT object rather than exploding it into its parts?

[mikemorris]

Hmm that seems like it would work well.

[mikemorris]

@mourner @kkaefer Is it valuable to keep convertFeatures as a public static method for performance testing?

[mourner]

@mikemorris doesn't matter much, I think this is good to merge.

[kkaefer]

@mikemorris let's make it private; we should add more detailed benchmarking to the internals of geojson-vt-cpp anyway if the need arises to optimize.

[mikemorris]

Rebased and squashed, this should be good to merge (and tag v3.0 - is breaking from JS versions an issue here @mourner?) once Travis passes.

[mourner]

and tag v3.0 - is breaking from JS versions an issue here @mourner?

Not sure what's the best practice is, but worth noting that we diverged the API anyway even though the version was mirrored, so we probably have to break the version mirroring here, even if it's not wanted. @jfirebaugh what do you think?

[jfirebaugh]

I think it's fine to version js and cpp independently.