[core] geometry@0.8.0 / geojsonvt@6.0.0 #5514

yhahn · 2016-06-29T23:31:16Z

geometry.hpp-based geojsonvt.

Shape annotation tests are failing. I need to dig into this
Update all platform build configs
Tests passing, IRL test with glfw-app
Remaining code adjustments

geometry-0.8.0

rebase in geometry 0.8.0
update geojson-vt dep

Code review

jfirebaugh · 2016-06-30T00:34:22Z

include/mbgl/util/geometry.hpp

@@ -12,6 +12,16 @@ enum class FeatureType : uint8_t {
 Polygon = 3
 };

+struct ToFeatureType {
+ FeatureType operator()(mapbox::geometry::point<int16_t>) const { return FeatureType::Point; }


These should all take their argument as const __ & to avoid copying, and use the CamelCase aliases defined below.

yhahn · 2016-06-30T19:53:03Z

src/mbgl/annotation/shape_annotation_impl.cpp

 mapbox::geojsonvt::Options options;
- options.maxZoom = maxZoom;
+ options.maxZoom = mbgl::util::clamp((int)maxZoom, 0, 18);


Shape annotation tests were failing without this clamp -- the maxZoom passed here is the map's mapZoom (20). I need to dig further in master to understand what's changed (was geojson-vt-cpp clamping this?)

This looks similar to the issue I am debugging in #5517. I've previously lowered the maximum zoom range for AnnotationSource from 22 to 18 alleviate the issue of disappearing annotations in z19 here.

util::clamp<uint8_t>(maxZoom, 0, 18)

mourner · 2016-06-30T20:24:28Z

@brunoabinader can you check if this fixes that maxZoom issue?

brunoabinader · 2016-06-30T21:07:21Z

src/mbgl/annotation/shape_annotation_impl.cpp

 return;

 AnnotationTileLayer& layer = *data.layers.emplace(layerID,
 std::make_unique<AnnotationTileLayer>(layerID)).first->second;

+ ToGeometryCollection toGeometryCollection;
+ ToFeatureType toFeatureType;
 for (auto& shapeFeature : shapeTile.features) {


I think we can replace this auto& with a const auto&.

brunoabinader · 2016-06-30T21:50:23Z

@brunoabinader can you check if this fixes that maxZoom issue?

Just did a rebase of my patches from #5517 on top of this branch. I removed the zoom clamping introduced by @yhahn otherwise the app crashes inside GeoJSONVT's getTile:

#0  0x00007ffff56f9418 in __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:54
#1  0x00007ffff56fb01a in __GI_abort () at abort.c:89
#2  0x00007ffff5d3284d in __gnu_cxx::__verbose_terminate_handler() () from /usr/lib/x86_64-linux-gnu/libstdc++.so.6
#3  0x00007ffff5d306b6 in ?? () from /usr/lib/x86_64-linux-gnu/libstdc++.so.6
#4  0x00007ffff5d30701 in std::terminate() () from /usr/lib/x86_64-linux-gnu/libstdc++.so.6
#5  0x00007ffff5d30919 in __cxa_throw () from /usr/lib/x86_64-linux-gnu/libstdc++.so.6
#6  0x000000000089bbf4 in mapbox::geojsonvt::GeoJSONVT::getTile (this=0x227a1b0, z=20 '\024', x_=167740, y=405294)

Although the unit test I introduce passes (all zoom levels returns non-empty features when queried with queryRenderedFeatures) - I've noticed erratic annotation behavior as shown:

This comes along with a console filled with [DEBUG] {mapbox-glfw}[OpenGL]: Buffer doesn't contain elements output. This weird behavior is also present also without my patches applied.

yhahn · 2016-07-03T21:40:51Z

Once 🍏 this PR will be ready for another review! Here are some known issues/open questions remaining:

geojsonvt maxzoom issue - ~~determine if this work should wait until @brunoabinader's annotation maxzoom PR fix lands.~~

Strictness changes

Determine what to do about two known API behavior changes:

Shape annotation API now must receive closed rings for polygons to render correctly. There's no validation/assertion in place for this, and the behavior of this has changed from master. (https://github.com/mapbox/mapbox-gl-native/blob/geojsonvt/platform/default/glfw_view.cpp#L275-L276)
geojson.cpp is now stricter and requires a properties key when parsing GeoJSON features. (Add properties value to achieve valid geojson mapbox-gl-test-suite#120)

Question: do we need to smooth these changes out for developers or should we nudge them toward more strictness?

cc @jfirebaugh @mourner @brunoabinader

mourner · 2016-07-04T08:53:23Z

Shape annotation API now must receive closed rings for polygons to render correctly.

What that the rendering issue you were seeing earlier? For the annotations API, it would be easy enough to close any unclosed polygon rings, right?

geojson.cpp is now stricter and requires a properties key when parsing GeoJSON features.

Note that features with properties: null are still a valid GeoJSON.

yhahn · 2016-07-04T16:57:22Z

What that the rendering issue you were seeing earlier? For the annotations API, it would be easy enough to close any unclosed polygon rings, right?

Yes. I started to poke at this but it looks like a slippery slope to me -- we would need to add an early closed-ring validator + corrector for fixing any polygons/multipolygons (ie. before shape annotations are passed to geojsonvt for clipping.) Do we do this elsewhere in mbgl?

Note that features with properties: null are still a valid GeoJSON.

Yep, this condition is handled properly.

yhahn · 2016-07-04T17:07:09Z

After re-reading the discussion on #5517 I think we should land this first (get fully up to date on geojson-vt) and then I can reroll @brunoabinader's testcase and test.

mourner · 2016-07-05T11:28:28Z

src/mbgl/style/sources/geojson_source_impl.cpp

+ return mapbox::geojson::feature_collection { value };
+ }
+ mapbox::geojson::feature_collection operator()(const mapbox::geojson::geometry value) const {
+ return mapbox::geojson::feature_collection { { value } };


C++ should deduct the return type so this can be just return { ... }, right?

mourner · 2016-07-05T11:30:54Z

Looked through the code and this looks great to me! Looking forward to this being merged. I'll follow-up with supercluster.hpp integration after that.

springmeyer · 2016-07-05T16:40:32Z

src/mbgl/style/sources/geojson_source_impl.cpp

@@ -18,14 +20,30 @@ using namespace mapbox::geojsonvt;
 namespace mbgl {
 namespace style {
 namespace conversion {
+
+struct ToFeatureCollection {
+ mapbox::geojson::feature_collection operator()(const mapbox::geojson::feature_collection value) const {


The value in each visitor here could be passed by const & to avoid an unneeded copy. That said, the compiler will likely optimize away the copy so its not certain my suggestion will run any faster.

jfirebaugh · 2016-07-05T16:46:05Z

Shape annotation API now must receive closed rings for polygons to render correctly.

My sense is this will be an unexpected / breaking change for users and we should retain the loose API for annotations. We've never documented the annotations API as requiring closed rings or strict GeoJSON geometry.

geojson.cpp is now stricter and requires a properties key when parsing GeoJSON features. (mapbox/mapbox-gl-test-suite#120)

In contrast this seems like an acceptable change. It's reasonable to require a GeoJSON source have valid GeoJSON data. Making the parser more conformant can be construed as a bug fix. And use of GeoJSON sources is low in any case.

jfirebaugh · 2016-07-05T16:58:25Z

src/mbgl/style/sources/geojson_source_impl.cpp

+ ToFeatureCollection toFeatureCollection;
+ const auto geojson = mapbox::geojson::convert(value);
+ const auto features = apply_visitor(toFeatureCollection, geojson);
+ return GeoJSON { std::make_unique<GeoJSONVT>(features, options) };


It's awkward to be unable to directly compose mapbox::geojson::convert and mapbox::GeoJSONVT::GeoJSONVT. We should add a GeoJSONVT constructor that accepts mapbox::util::variant<geometry, feature, feature_collection>. cc @mourner

jfirebaugh · 2016-07-05T17:01:46Z

src/mbgl/tile/geometry_tile_data.hpp

+ GeometryCoordinates coordinates;
+ coordinates.reserve(geom.size());
+ for (const auto& point : geom) {
+ coordinates.emplace_back(point.x, point.y);


s/emplace_back(point.x, point.y)/emplace_back(point)/g

1ec5 · 2016-07-05T17:34:58Z

Shape annotation API now must receive closed rings for polygons to render correctly.

This is fine from the iOS and macOS SDKs’ perspective: for compatibility with MapKit, we’ve documented a requirement that MGLPolygon be closed, and the behavior is undefined otherwise.

yhahn · 2016-07-06T17:26:44Z

@jfirebaugh ready whenever you have a chance for another 👀 Only major change besides the cleanup from notes is the addition of the ring-closer for fill annotations:

dd8ceb4

jfirebaugh · 2016-07-06T17:47:46Z

Ring closing should only apply to polygons and multipolygons, not line strings.

… and geojsonvt

Since #5514, feature IDs may be integers, unsigned integers, floating-point numbers, or strings, not just unsigned integers.

1ec5 · 2016-07-10T22:06:58Z

8ca8e2c updates the MGLFeature documentation in the iOS and macOS SDKs to reflect the fact that a feature identifier may be one of several types, not necessarily an NSNumber. We deliberately made the identifier as broadly typed as possible (id, amusingly enough) in anticipation of this change, so no backwards compatibility issues are expected.

jfirebaugh reviewed Jun 30, 2016
View reviewed changes

yhahn force-pushed the geojsonvt branch from e98342c to ec0f3fb Compare June 30, 2016 19:42

yhahn reviewed Jun 30, 2016
View reviewed changes

brunoabinader reviewed Jun 30, 2016
View reviewed changes

yhahn force-pushed the geojsonvt branch 2 times, most recently from 3bb5010 to b851c94 Compare July 3, 2016 15:36

yhahn mentioned this pull request Jul 3, 2016

Add properties value to achieve valid geojson mapbox/mapbox-gl-test-suite#120

Merged

yhahn force-pushed the geojsonvt branch 5 times, most recently from 421d0e1 to ebd29c1 Compare July 3, 2016 21:31

yhahn changed the title ~~[notready] geojsonvt @ 6.0.0~~ geometry@0.8.0 / geojsonvt@6.0.0 Jul 3, 2016

yhahn changed the title ~~geometry@0.8.0 / geojsonvt@6.0.0~~ [core] geometry@0.8.0 / geojsonvt@6.0.0 Jul 4, 2016

mourner reviewed Jul 5, 2016
View reviewed changes

springmeyer reviewed Jul 5, 2016
View reviewed changes

jfirebaugh reviewed Jul 5, 2016
View reviewed changes

yhahn mentioned this pull request Jul 6, 2016

Constructor from geojson-cpp variant mapbox/geojson-vt-cpp#50

Merged

yhahn force-pushed the geojsonvt branch from 71e9e5d to b126868 Compare July 6, 2016 17:09

jfirebaugh and others added 6 commits July 6, 2016 15:36

[core] geometry.hpp 0.8.0

6c49f35

geojsonvt @ 6.0.0

fc8bd85

Update platform deps, build scripts

ac85223

Perf optimizations/cleanup

90c4d07

Rebase in geometry@080

8fffdb8

D.R.Y. etc

2d802b0

yhahn force-pushed the geojsonvt branch from b126868 to 8caad01 Compare July 6, 2016 19:37

yhahn added 3 commits July 6, 2016 15:57

Ensure fill annotation geometries have closed rings.

3e64c5b

Optimizations

3203f87

Update to geojsonvt @ 6.1.0 for clean handoff between geojson parsing…

ca7e000

… and geojsonvt

yhahn force-pushed the geojsonvt branch from 8caad01 to ca7e000 Compare July 6, 2016 19:57

Apply close multi/poly geoms for line annotations as well

26121ce

yhahn merged commit 71a3b1d into master Jul 6, 2016

yhahn deleted the geojsonvt branch July 6, 2016 21:29

1ec5 added a commit that referenced this pull request Jul 10, 2016

[ios, macos] Updated MGLFeature ID documentation

8ca8e2c

Since #5514, feature IDs may be integers, unsigned integers, floating-point numbers, or strings, not just unsigned integers.

brunoabinader mentioned this pull request Jul 22, 2016

[core] Line annotation disappears and/or clips to tile boundaries from zoom 3 onwards #5763

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[core] geometry@0.8.0 / geojsonvt@6.0.0 #5514

[core] geometry@0.8.0 / geojsonvt@6.0.0 #5514

yhahn commented Jun 29, 2016 •

edited

Loading

jfirebaugh Jun 30, 2016

yhahn Jun 30, 2016

brunoabinader Jun 30, 2016

jfirebaugh Jul 5, 2016

mourner commented Jun 30, 2016

brunoabinader Jun 30, 2016

brunoabinader commented Jun 30, 2016

yhahn commented Jul 3, 2016 •

edited

Loading

mourner commented Jul 4, 2016

yhahn commented Jul 4, 2016

yhahn commented Jul 4, 2016

mourner Jul 5, 2016

mourner commented Jul 5, 2016

springmeyer Jul 5, 2016

jfirebaugh commented Jul 5, 2016

jfirebaugh Jul 5, 2016

jfirebaugh Jul 5, 2016

1ec5 commented Jul 5, 2016

yhahn commented Jul 6, 2016

jfirebaugh commented Jul 6, 2016

1ec5 commented Jul 10, 2016

[core] geometry@0.8.0 / geojsonvt@6.0.0 #5514

[core] geometry@0.8.0 / geojsonvt@6.0.0 #5514

Conversation

yhahn commented Jun 29, 2016 • edited Loading

geometry-0.8.0

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mourner commented Jun 30, 2016

Choose a reason for hiding this comment

brunoabinader commented Jun 30, 2016

yhahn commented Jul 3, 2016 • edited Loading

Strictness changes

mourner commented Jul 4, 2016

yhahn commented Jul 4, 2016

yhahn commented Jul 4, 2016

Choose a reason for hiding this comment

mourner commented Jul 5, 2016

Choose a reason for hiding this comment

jfirebaugh commented Jul 5, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1ec5 commented Jul 5, 2016

yhahn commented Jul 6, 2016

jfirebaugh commented Jul 6, 2016

1ec5 commented Jul 10, 2016

yhahn commented Jun 29, 2016 •

edited

Loading

yhahn commented Jul 3, 2016 •

edited

Loading