[core] Refactor core annotations API #5200

jfirebaugh · 2016-05-31T23:30:32Z

Use geometry.hpp types (fixes mbgl::AnnotationManager should use mapbox::geometry types #5158).
Rename annotation types based on the underlying style layer type they use: Symbol, Line, or Fill.
Introduce a unified variant Annotation type; eliminate type-specific add/update methods.
Eliminate separate array-oriented methods. They are not significantly faster than simply calling the singleton method in a loop.
@1ec5, can you please review the osx and ios changes?
@tobrun, can you please review the android changes?
@brunoabinader or @tmpsantos, can you please make the necessary qt changes? I'm not sure what the best approach is for this platform.

1ec5 · 2016-05-31T23:45:53Z

platform/darwin/src/MGLMultiPoint.mm

-    return { segment };
-}
-
- (mbgl::ShapeAnnotation::Properties)shapeAnnotationPropertiesObjectWithDelegate:(__unused id <MGLMultiPointDelegate>)delegate {


Currently, if you call -[MGLMapView visibleFeaturesAtPoint:] at the location of a multipoint (in geometry.hpp terms), you get an MGLMultiPointFeature object back. MGLMultiPointFeature is a subclass of MGLMultiPoint that conforms to MGLAnnotation, so you can add this MGLMultiPoint object to the map using -[MGLMapView addAnnotations:]. With these changes, MGLMapView will end up calling -shapeAnnotationObjectWithDelegate: on an MGLMultiPoint that has no implementation of -shapeAnnotationObjectWithDelegate:, resulting in an Objective-C exception.

If it isn’t straightforward to create an mbgl::ShapeAnnotation representing a multipoint geometry at this time, I think it’d be reasonable to implement a -shapeAnnotationObjectWithDelegate: that asserts for now.

MGLMultiPoint is trying to do too many things. It should represent the geometric "multi point" concept, and not also try to be a base class for MGLPolygon and MGLPolyline. (Our Java classes make a similar mistake.)

Agreed. Unfortunately, I think we’re stuck with MapKit’s concept of a multipoint (as any shape with multiple points) for backwards compatibility. What could we call the class that represents a standard GIS multipoint? Or should we bite the bullet and bump the major version number for simplicity?

MGLSparseMultiPoint? 😬

If I make 5698978 part of this PR, how hard would it be to refactor things so that -annotationObjectWithDelegate: is implemented by every class that implements MGLAnnotation?

MGLAnnotation is a public protocol, so we can’t use any C++ types or expect the developer to implement this method themselves for their custom MGLAnnotation classes.

The base model class for things that get turned into mbgl::ShapeAnnotations is currently MGLMultiPoint. There’s no base model class for things that get turned into mbgl::PointAnnotations, because any Objective-C object can conform to the MGLAnnotation protocol and thus be added as a point annotation.

We could centralize this shape conversion logic to avoid any surprises, since it isn’t like the developer can really come up with their own novel geometry class. MGLMultiPoint is a natural place to host a centralized conversion method, but the most flexible approach would be to implement a standalone C function similar to MGLFeaturesFromMBGLFeatures().

tmpsantos · 2016-06-01T08:14:49Z

On it.

tmpsantos · 2016-06-01T08:36:18Z

@jfirebaugh made it so it will build, but we are not really using shape annotations for Qt ATM and we are likely to rewrite it completely anyway.

tobrun · 2016-06-01T14:06:45Z

@jfirebaugh will review later today

jfirebaugh · 2016-06-01T15:27:59Z

Merged the larger refactor into this PR and updated the description.

1ec5 · 2016-06-01T15:53:20Z

platform/darwin/src/MGLGeometry_Private.h

@@ -6,6 +6,7 @@
 #endif

 #import <mbgl/util/geo.hpp>
+#import <mbgl/util/geometry.hpp>


This is causing the Darwin unit tests to fail to compile. Those tests need only a couple of the methods in this header. We should split this header into MGLGeometry_Private.h (private visibility, so it's visible to test target) and MGLGeometry_Internal.h (project visibility, so it's visible only to the dynamic and static targets).

Alternatively, you can put MGLPointFromLocationCoordinate2D() in MGLFeature_Private.h, which already houses some geometry type conversion code.

The issue was that geometry.hpp was still in src rather than include.

tobrun · 2016-06-01T18:40:02Z

@jfirebaugh

I tried building this locally with:

git submodule update
git make clean
git make android-lib-arm-v8

I have no issues with building but at runtime I'm running into:

Caused by: java.lang.UnsatisfiedLinkError: dlopen failed: cannot locate symbol "_ZN6mapbox9geojsonvt9GeoJSONVTC1ENSt6__ndk16vectorINS0_16ProjectedFeatureENS2_9allocatorIS4_EEEENS0_7OptionsE" referenced by "libmapbox-gl.so"...

I validated that this is not happening on master.

The code itself LGTM and reading the C++ was really useful/insightful for me.

LMK if I can help out or need to retest.

jfirebaugh · 2016-06-01T22:45:22Z

@tobrun Looks to me like this crash does happen on master too.

jfirebaugh · 2016-06-01T23:42:20Z

It's #5108 again. 😠

Instead of providing both singleton and List-based implementations: * Define everything in terms of arrays * Do singleton ⇢ array and List ⇢ array conversions in Java

…legate:]

1ec5 reviewed May 31, 2016
View reviewed changes

1ec5 added refactor Core The cross-platform C++ core, aka mbgl annotations Annotations on iOS and macOS or markers on Android labels May 31, 2016

jfirebaugh mentioned this pull request Jun 1, 2016

MGLMultiPoint is both an abstract superclass and a concrete class for multipoints #5201

Closed

jfirebaugh force-pushed the 5158-minimal branch from b2bab79 to 7cf0379 Compare June 1, 2016 15:20

jfirebaugh changed the title ~~[core] Use geometry.hpp types for shape annotations~~ [core] Refactor core annotations API Jun 1, 2016

1ec5 reviewed Jun 1, 2016
View reviewed changes

jfirebaugh force-pushed the 5158-minimal branch 3 times, most recently from f1161dc to ac4ccdd Compare June 1, 2016 17:12

jfirebaugh added the ✓ ready for review label Jun 1, 2016

jfirebaugh force-pushed the 5158-minimal branch from 22bbc87 to 498b483 Compare June 2, 2016 00:06

jfirebaugh and others added 9 commits June 1, 2016 17:36

[android] Simplify the JNI definitions for annotation methods

555c9ed

Instead of providing both singleton and List-based implementations: * Define everything in terms of arrays * Do singleton ⇢ array and List ⇢ array conversions in Java

[android] Basic cleanups in annotation JNI code

efaf646

[core] Do annotation longitude wrapping together with latitude clamping

aa1a54c

[core] Use geometry.hpp types for shape annotations

8985b13

[Qt] Use geometry.hpp internally for shape annotations

cfd6757

[all] Rationalize annotation API

0fba70d

[ios, osx] Add an assertion to -[MGLMultiPoint annotationObjectWithDe…

574dd14

…legate:]

[build] Add android-* make targets for specific ABIs

8068424

[core] Constrain annotation API to the supported geometry types

3689f52

jfirebaugh force-pushed the 5158-minimal branch from a159990 to 3689f52 Compare June 2, 2016 00:37

jfirebaugh merged commit 3689f52 into master Jun 2, 2016

jfirebaugh deleted the 5158-minimal branch June 2, 2016 00:55

jfirebaugh mentioned this pull request Jun 2, 2016

add support for multipolygons and polygon interior holes #1729

Closed

1ec5 mentioned this pull request Jun 20, 2016

Multipoint annotations should be accessible to VoiceOver #4838

Closed

1ec5 mentioned this pull request Jul 14, 2016

Shape annotation z-order is reversed #5691

Closed

1ec5 removed the ✓ ready for review label Aug 17, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[core] Refactor core annotations API #5200

[core] Refactor core annotations API #5200

jfirebaugh commented May 31, 2016 •

edited

Loading

1ec5 May 31, 2016 •

edited

Loading

jfirebaugh May 31, 2016

1ec5 Jun 1, 2016 •

edited

Loading

1ec5 Jun 1, 2016

jfirebaugh Jun 1, 2016

1ec5 Jun 1, 2016 •

edited

Loading

tmpsantos commented Jun 1, 2016

tmpsantos commented Jun 1, 2016

tobrun commented Jun 1, 2016

jfirebaugh commented Jun 1, 2016

1ec5 Jun 1, 2016

1ec5 Jun 1, 2016

jfirebaugh Jun 1, 2016

tobrun commented Jun 1, 2016

jfirebaugh commented Jun 1, 2016

jfirebaugh commented Jun 1, 2016

[core] Refactor core annotations API #5200

[core] Refactor core annotations API #5200

Conversation

jfirebaugh commented May 31, 2016 • edited Loading

1ec5 May 31, 2016 • edited Loading

Choose a reason for hiding this comment

jfirebaugh May 31, 2016

Choose a reason for hiding this comment

1ec5 Jun 1, 2016 • edited Loading

Choose a reason for hiding this comment

1ec5 Jun 1, 2016

Choose a reason for hiding this comment

jfirebaugh Jun 1, 2016

Choose a reason for hiding this comment

1ec5 Jun 1, 2016 • edited Loading

Choose a reason for hiding this comment

tmpsantos commented Jun 1, 2016

tmpsantos commented Jun 1, 2016

tobrun commented Jun 1, 2016

jfirebaugh commented Jun 1, 2016

1ec5 Jun 1, 2016

Choose a reason for hiding this comment

1ec5 Jun 1, 2016

Choose a reason for hiding this comment

jfirebaugh Jun 1, 2016

Choose a reason for hiding this comment

tobrun commented Jun 1, 2016

jfirebaugh commented Jun 1, 2016

jfirebaugh commented Jun 1, 2016

jfirebaugh commented May 31, 2016 •

edited

Loading

1ec5 May 31, 2016 •

edited

Loading

1ec5 Jun 1, 2016 •

edited

Loading

1ec5 Jun 1, 2016 •

edited

Loading