Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

mbgl::AnnotationManager should use mapbox::geometry types #5158

Closed
1ec5 opened this issue May 26, 2016 · 7 comments · Fixed by #5200
Closed

mbgl::AnnotationManager should use mapbox::geometry types #5158

1ec5 opened this issue May 26, 2016 · 7 comments · Fixed by #5200
Assignees
Labels
annotations Annotations on iOS and macOS or markers on Android Core The cross-platform C++ core, aka mbgl refactor

Comments

@1ec5
Copy link
Contributor

1ec5 commented May 26, 2016

mbgl::AnnotationManager should use the mapbox::geometry types that are being used e.g. as return values for mbgl::Map::queryRenderedFeatures() (#5066). This would give us the ability to add any kind of geometry to the annotation layer, unblocking #1729.

/cc @jfirebaugh

@1ec5 1ec5 added refactor Core The cross-platform C++ core, aka mbgl annotations Annotations on iOS and macOS or markers on Android labels May 26, 2016
@jfirebaugh
Copy link
Contributor

In 3f17b63 and 30ec9c6 I sketched out a couple potential redesigns that accomplish this. I think I slightly prefer the latter.

Both designs will be significantly easier to implement once mapbox/geojson-vt-cpp#45 lands. Currently a bunch of conversion code would be required.

@1ec5
Copy link
Contributor Author

1ec5 commented May 26, 2016

I think I’d prefer the latter option too. It wouldn’t feel right to restrict this API to just the three geometry types (point, line, polygon) in core, given that core can actually handle any of the geometry types.

@jfirebaugh jfirebaugh self-assigned this May 26, 2016
@jfirebaugh
Copy link
Contributor

The above approach expands into much more work than I'm comfortable biting off right now, given the release schedule and certainty of introducing conflicts with #4839.

We can switch the interface to geometry.hpp types with less effort: https://github.com/mapbox/mapbox-gl-native/compare/5158-minimal. Note that this approach does not itself fix #1729 or even expand the actual range of supported geometry types. Unsupported types become a runtime error, rather than not being expressible in the API.

@1ec5 WDYT?

@jfirebaugh
Copy link
Contributor

Actually, it shouldn't be hard to expand the geometry support to polygon holes, MultiLineString, and MultiPolygon. What looks difficult is Point, MultiPoint, and GeometryCollection.

@1ec5
Copy link
Contributor Author

1ec5 commented May 27, 2016

Is Point difficult because point annotations and shape annotations have different tile and layer implementations?

@jfirebaugh
Copy link
Contributor

  • A line or fill layer -- the only options for ShapeAnnotation -- displaying a single Point isn't a useful combination. It won't render anything.
  • A line or fill layer displaying a MultiPoint isn't particularly sensical. I think it will render the same as a LineString with the same coordinates, so you might as well just use that.
  • The version of geojson-vt-cpp currently in use privatizes the method we'd want to use for conversion.

@jfirebaugh
Copy link
Contributor

We could however, switch PointAnnotation from LatLng to Point<double>, if that would help the SDKs with consistency.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
annotations Annotations on iOS and macOS or markers on Android Core The cross-platform C++ core, aka mbgl refactor
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants