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

[ios][android] SDK Bindings for Image Sources #9110

Merged
merged 5 commits into from
Jun 19, 2017

Conversation

asheemmamoowala
Copy link
Contributor

@asheemmamoowala asheemmamoowala commented May 25, 2017

This PR includes iOS/macOS and Android bindings for Image sources.

As per style spec, the Image Source accepts:

[longitude, latitude] pairs for the image corners listed in clockwise order: top left, top right, bottom right, bottom left.

In GL Native, this is represented as an std::vector<LatLng> for simplicity. For the iOS and Android APIs, we need to use a better structure/class to allow developers to easily and correctly provide the coordinates for their Image source.

The existing LatLngBounds objects in native, Obj-C, and Java are all aligned LatLng rects, where as ImageSource allows non-aligned rects. To support the non-aligned rects, this PR introduces LatLngQuad to represent four distinct LatLng pairs.

//cc @1ec5 @boundsj @zugaldia @ivovandongen @incanus

@asheemmamoowala asheemmamoowala added Android Mapbox Maps SDK for Android iOS Mapbox Maps SDK for iOS macOS Mapbox Maps SDK for macOS ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold labels May 25, 2017
Copy link
Contributor

@1ec5 1ec5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To finish the iOS/macOS frontend for this feature:

  • Implement MGLImageSource.mm. There are helper methods in NSImage+MGLAdditions.h and UIImage+MGLAdditions.h for converting between Cocoa and mbgl image types. Note that these methods also treat template images as SDF icons.
  • Follow these instructions to ensure that MGLImageSource.h is integrated correctly into the module and documentation.
  • -[MGLStyle sourceFromMBGLSource:] needs an additional case to convert an ImageSource into an MGLImageSource.

@@ -45,6 +45,18 @@ typedef struct MGLCoordinateBounds {
CLLocationCoordinate2D ne;
} MGLCoordinateBounds;

typedef struct MGLCoordinateQuad {
/** Coordinate at the northwest corner. */
CLLocationCoordinate2D tl;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's write out these field names. It was a mistake to abbreviate the MGLCoordinateBounds fields.

/** Coordinate at the northwest corner. */
CLLocationCoordinate2D tl;
/** Coordinate at the northeast corner. */
CLLocationCoordinate2D tr;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it top-right or northeast? Can the developer line up the top-right corner with the southwest geographic extent to rotate the image?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be top-right, not cardinal directions.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, the documentation for these fields is a bit confusing in that case.

@@ -45,6 +45,18 @@ typedef struct MGLCoordinateBounds {
CLLocationCoordinate2D ne;
} MGLCoordinateBounds;

typedef struct MGLCoordinateQuad {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This struct needs a documentation comment in order for the struct to appear in jazzy documentation.

/// Returns the smallest rectangle that contains both the given rectangle and
/// the given point.
CGRect MGLExtendRect(CGRect rect, CGPoint point);

mbgl::LatLng MGLLatLngFromLocationCoordinate2D(CLLocationCoordinate2D coordinate);

NS_INLINE std::vector<mbgl::LatLng> MGLLatLngFromLocationQuad(MGLCoordinateQuad quad) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A more accurate name would be MGLalatLngsFromCoordinateQuad.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excuse my phone keyboard… MGLatLngsFromCoordinateQuad


NS_ASSUME_NONNULL_BEGIN

MGL_EXPORT
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly, we'll need some documentation here too. See the other source classes' documentation for examples. In particular, we should emphasize that this source is for georeferenced images; the SDK already has several other ways to put images on the map (MGLAnnotationImage, MGLAnnotationView with UIImageView, MGLStyle images).

@param coordinates the NW, NE, SW, and SE coordiantes for the image.
@return An initialized image source.
*/
- (instancetype)initWithIdentifier:(NSString *)identifier coordinates:(MGLCoordinateQuad)coordinates NS_DESIGNATED_INITIALIZER;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's call the second parameter "coordinateQuad", since "coordinates" is usually the name of a C array of CLLocationCoordinate2Ds.

#pragma mark Initializing a Source

/**
Returns an image source with an identifier and coordinates
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it valid to create an image source without a URL or image, or to use it with a raster layer? What does it look like?

- (instancetype)initWithIdentifier:(NSString *)identifier coordinates:(MGLCoordinateQuad)coordinates NS_DESIGNATED_INITIALIZER;

/**
Returns an image source with an identifier, coordinates and a URL
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What image formats does this API accept?

*/
@property (nonatomic, copy, nullable)NSURL *URL;

@property (nonatomic, nullable, setter=setImage:)MGLImage *image;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-setImage: is the default setter name, so no need to specify it explicitly.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this property follow copy semantics, or does it retain the image strongly?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That brings up a good question, should there even be a getter for this property. The setImage: implementation will copy the data into an UnpremultipliedImage which gets moved to mbgl::style::ImageSource where there isn't a getImage() method.

Maybe the property should be replaced by a setImage: method?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Write-only properties are virtually unheard of in Objective-C and Swift. We would definitely want a getter if we have a setter. Ideally, there would be an mbgl::style::ImageSource::getImage(). If that isn’t possible, consider keeping parallel state in MGLImageSource (but see #7375).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would be the compelling reason to maintain parallel state?
From #7376 (comment) this is only needed if developers are continuously modifying the source data. I can surely image that to be true for images, but that might mean we should implement and support video or some other means of showing that data - custom source/layer perhaps.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only reason for maintaining parallel state would be as a last resort for implementing an image getter. mbgl::style::ImageSource::getImage() would make that unnecessary.

Having an image getter would make MGLImageSource consistent with -[MGLStyle imageForName:] for non-georeferenced images. It would also enable some of the same use cases as that method. Off the top of my head, an application could get the image from the source, manipulate it, and pass the manipulated image back to the source. Or the user could tap on an image source and the application would display the “selected” source’s image in an image view on the side. The application doesn’t have to keep track of the image itself; mbgl remains the source of truth.

Continuously modifying the source data could in some cases be better served by video sources; on the other hand, it’s much more feasible for an application to programmatically generate an image than a video.

Are there technical reasons against adding a getImage() in mbgl?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider the following situations

// A
id  imgSource = [[ImageSource alloc] initiWithIdentifier:@"foo" coordinates:c imageURL: url];
UIImage * img = imgSource.image;  //no image data is available 

// B
id  imgSource = [[ImageSource alloc] initiWithIdentifier:@"foo" coordinates:c image: myImage];
imgSource.URL = "http://host/myimage.png";

UIImage * img = imgSource.image;  //no image data is available 

Under scenario A mbgl::ImageSource will resolve the url asynchronously and hold on to an UnassociatedImage until the next render pass. The image data is then moved to the bucket and a copy is not held in mbgl::ImageSource

Conceptually MGLImageSource.image is the image being used by the source, but given that it could be set via URL or directly breaks my mental model of being able to access the resolved image data when using a url.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it’s fine for the property to have a getter with the caveat that it remains unset if the source is created from a URL. For precedent, MGLTileSource.configurationURL is unset if the raster or vector source was created using tile URL templates instead of a TileJSON URL.


@property (nonatomic, nullable, setter=setImage:)MGLImage *image;

@property (nonatomic, nonnull)CLLocationCoordinate2D * coordinates;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the initializers take an MGLCoordinateQuad, this property should be typed as an MGLCoordinateQuad too.

@tobrun tobrun requested a review from ivovandongen May 26, 2017 19:33
Copy link
Contributor

@ivovandongen ivovandongen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@asheemmamoowala The interfaces for the Android API look good to me. Some small remarks.

*/
public class LatLngQuad implements Parcelable {

private final LatLng mTopLeft;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can drop the Hungarian notation. We are not using that anymore since #6109

* This class does not wrap valies to the world bounds
* </p>
*/
public class LatLngQuad implements Parcelable {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Naming wise; NonAlignedLatLongBounds? Makes it very clear what the relation to LatLngBounds is. cc @tobrun

Copy link
Contributor Author

@asheemmamoowala asheemmamoowala Jun 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In iOS, this is MGLCoordinateQuad, so I wanted to try and use similar terminology.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding the Objective-C struct’s name, it may be worth seeing if there’s precedent for a concept like this in SceneKit or SpriteKit and naming the struct accordingly. The Objective-C struct’s name doesn’t have to match the Java class’s name – after all, the Android SDK has LatLng and the iOS/macOS SDK has CLLocationCoordinate2D.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@1ec5 I looked through SpriteKit, SceneKit, and the Maps APIs for HERE, Tangram, Google, Amazon and Yandex. None of those have a concept of a non-aligned rect. Beyond rects, they all support polygons or arrays of points.

Im going to add convenience methods to support LatLngBounds and MGLCoordinateBounds for the SDKs. This avoids the awkwardness of the *Quad/NonAlignedBounds names for those who have regular aligned images

* Construct a new LatLngQuad based on its corners,
* in order top left, top right, bottom left, bottom right
*/
LatLngQuad(final LatLng tl, final LatLng tr, final LatLng br, final LatLng bl) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please spell out the variable names, eg topLeft, etc

@1ec5
Copy link
Contributor

1ec5 commented Jun 1, 2017

This PR depends on #8968.

@1ec5 1ec5 mentioned this pull request Jun 1, 2017
@jfirebaugh
Copy link
Contributor

There's a warning in the darwin builds that we should make sure is fixed in this PR:

⚠️  /Users/john/Development/mapbox-gl-native/platform/darwin/src/MGLOfflineStorage.mm:83:21: enumeration value 'Image' not handled in switch [-Wswitch]

            switch (kind_) {

@asheemmamoowala asheemmamoowala force-pushed the image-source-bindings branch 4 times, most recently from 38e8dbe to 579aeae Compare June 13, 2017 08:17
@1ec5
Copy link
Contributor

1ec5 commented Jun 13, 2017

One more thing: this feature is worth mentioning in the Android, iOS, and macOS changelogs.

@asheemmamoowala asheemmamoowala force-pushed the image-source-bindings branch 2 times, most recently from 2c25274 to bbf972b Compare June 13, 2017 21:08
Copy link
Contributor

@1ec5 1ec5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is getting a lot closer. Most of the comments below are relatively minor, but there are still some rough edges around MGLImageSource’s initializers and image property.

@@ -2,6 +2,10 @@

Mapbox welcomes participation and contributions from everyone. Please read [CONTRIBUTING.md](../../CONTRIBUTING.md) to get started.

## master
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The “3.6.0” section below is mistitled: it should be “master”. There’s a second “3.6.0” section below that corresponds to the actual 3.6.0 release.

@@ -2,6 +2,10 @@

Mapbox welcomes participation and contributions from everyone. Please read [CONTRIBUTING.md](../../CONTRIBUTING.md) to get started.

## master
* Add support for ImageSource [#9110](https://github.com/mapbox/mapbox-gl-native/pull/9110)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the iOS and macOS changelogs, please use a complete sentence that explains the purpose of the addition (and uses the correct class name). Something along these lines:

Added the MGLImageSource class for displaying a georeferenced image. (#9110)

@@ -1,5 +1,8 @@
# Changelog for Mapbox macOS SDK

## master
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly, the “3.6.0” section below should say “master”. I believe this was a botched merge conflict; the macOS SDK’s next release will be v0.6.0, not v3.6.0.

@param coordinateQuad The top left, top right, bottom right, and bottom left coordinates for the image.
@return An initialized image source.
*/
- (instancetype)initWithIdentifier:(NSString *)identifier coordinates:(MGLCoordinateQuad)coordinateQuad NS_DESIGNATED_INITIALIZER;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let’s call the second parameter coordinateQuad. I should’ve been clearer in #9110 that I was referring to the parameter’s label as well as the local parameter name.

@@ -104,6 +104,20 @@ class MGLDocumentationExampleTests: XCTestCase, MGLMapViewDelegate {
XCTAssertNotNil(mapView.style?.source(withIdentifier: "pois"))
}

func testMGLImageSource() {
//#-example-code
let coordinates: MGLCoordinateQuad = MGLCoordinateQuad(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It’s unnecessary to specify the type as MGLCoordinateQuad here. Swift can infer the type, similar to how the auto keyword works in C++.

@@ -154,6 +154,8 @@ typedef NS_ENUM(NSUInteger, MGLResourceKind) {
/** JSON part of a sprite sheet. It is constructed of the prefix in
https://www.mapbox.com/mapbox-gl-js/style-spec/#root-sprite and a JSON file extension. */
MGLResourceKindSpriteJSON,
/** Image **/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should clarify that it’s specifically an image source’s image (as opposed to an icon image or annotation image, aka sprite).

@@ -1954,7 +1973,6 @@
TargetAttributes = {
DA1DC9491CB6C1C2006E619F = {
CreatedOnToolsVersion = 7.3;
DevelopmentTeam = GJZR2MEM28;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How’d that sneak in there in the first place? 😅

/**
`MGLImageSource` is a map content source that supplies a georeferenced raster
image to be shown on the map. The geographic location of the raster image content,
supplied with `MGLCoordinateQuad`, can be non-axis aligned.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per #9110, we should point out that we have different ways to handle non-georeferenced images:

An image source is used for a georeferenced image – that is, an image in which the contents correspond to physical locations on the map. The georeferenced image scales and rotates as the user zooms and rotates the map. Images may also be used as icons or patterns in a style layer. To register an image for use as an icon or pattern, use the -[MGLStyle setImage:forName:] method. To configure a point annotation’s image, use the MGLAnnotationImage class.

#pragma mark Initializing a Source

/**
Returns a georeferenced image source with an identifier and coordinates
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does it look like when you add an image source without content and use it in a raster layer? Does an error occur, or does it succeed silently with no visual effect?

Copy link
Contributor Author

@asheemmamoowala asheemmamoowala Jun 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The more I think about this, it doesn't make sense to partially initialize this type of source. Will just remove this initializer. That swill also reduce our initializers down to 4 which seems more reasonable

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the other hand, we do make it possible to create an MGLShapeSource with no shape and fill in the shape later. Developers have found that pattern to be very useful. If it isn’t too much trouble to support that pattern for image sources as well (without throwing errors and the like), that would be very nice, but not a blocker for landing this feature.

@@ -10,6 +10,8 @@ NS_ASSUME_NONNULL_BEGIN

- (std::unique_ptr<mbgl::style::Image>)mgl_styleImageWithIdentifier:(NSString *)identifier;

- (mbgl::PremultipliedImage)mgl_PremultipliedImage;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: mgl_premultipliedImage would be a more natural spelling for this method name.

@1ec5
Copy link
Contributor

1ec5 commented Jun 14, 2017

This guide needs to be updated with the new source type.

@asheemmamoowala
Copy link
Contributor Author

@ivovandongen @tobrun can one of you take a look at the updated implementation

@asheemmamoowala asheemmamoowala force-pushed the image-source-bindings branch from bbf972b to 8ffbd43 Compare June 15, 2017 01:59
Copy link
Contributor

@ivovandongen ivovandongen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Few minor nits.



/**
* Image source
*
* * @see <a href="https://www.mapbox.com/mapbox-gl-style-spec/#sources-image">the style specification</a>
* <p>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT; I believe empty <p> elements will break javadoc generation still


import com.mapbox.mapboxsdk.testapp.R;

public class AnimatedImageSourceActivity extends AppCompatActivity implements OnMapReadyCallback {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT; a short description of what the class is for (even though it is fairly obvious :))


template <>
optional<std::array<LatLng, 4>> Converter<std::array<LatLng, 4>>::operator()(const mbgl::android::Value& value, Error& error) const {
if(value.isNull() || !value.isArray()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit; space after if

@@ -16,7 +19,7 @@ class ImageSource : public Source {

static void registerNative(jni::JNIEnv&);

ImageSource(jni::JNIEnv&, jni::String, jni::Object<>);
ImageSource(jni::JNIEnv&, jni::String, jni::Object<LatLngQuad> coordinatesObject);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can leave out the param name here

@asheemmamoowala asheemmamoowala force-pushed the image-source-bindings branch from 8ffbd43 to eba51de Compare June 15, 2017 21:13
@asheemmamoowala asheemmamoowala removed the ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold label Jun 15, 2017
@asheemmamoowala asheemmamoowala changed the title [ios][android] Draft API interfaces for ImageSource [ios][android] SDK Bindings for Image Sources Jun 15, 2017
@1ec5 1ec5 added GL JS parity For feature parity with Mapbox GL JS runtime styling labels Jun 16, 2017
Copy link
Contributor

@1ec5 1ec5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more round of feedback, nothing major.

For better or worse, we’ve also hard-coded lists of subclasses in abstract superclass documentation, with the goal of providing navigational landmarks to developers (see #9095 for example). The MGLSource documentation will need to be updated to note the existence of MGLImageSource. (Perhaps in the future we’ll autogenerate these source headers as well, but the headers differ so much from the style specification’s structure that we’re maintaining these subclass lists by hand for now.)


### Styles

* Added suuport for diplaying georeferenced images via the `MGLImageSource`. [#9110](https://github.com/mapbox/mapbox-gl-native/pull/9\
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A stray backslash snuck into this URL.

@@ -178,8 +178,9 @@ In style JSON | In the SDK
`geojson` | `MGLShapeSource`
`raster` | `MGLRasterSource`
`vector` | `MGLVectorSource`
`image` | `MGLImageSource`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that this is an .ejs file, which means it’s a template for the runtime styling codegen tool. After running make darwin-style-code, you should see these changes propagate to the iOS- and macOS-specific Markdown files.

current application’s resource bundle.
@return An initialized shape source.
*/
- (instancetype)initWithIdentifier:(NSString *)identifier coordinateQuad:(MGLCoordinateQuad)coordinateQuad imageURL:(NSURL *)url;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will bridge to Swift as init(identifier:coordinateQuad:image:), which is a bit misleading. (Since Objective-C conventionally describes the type in argument labels where Swift omits them, the Swift compiler tries to remove such words from argument labels when bridging Objective-C methods.) We can prevent this situation by replacing imageURL: with URL:, which is also consistent with -[MGLShapeSource initWithIdentifier:URL:options:].

@property (nonatomic, retain, nullable)MGLImage *image;

/**
The coordinates at which the source image will be placed.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: these are the coordinates at which the corners of the source image will be placed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incidentally, this suggests another possible name for MGLCoordinateQuad: MGLCoordinateCorners. Indeed, there’s already a bitmask on iOS called UIRectCorner. But I’m comfortable with “coordinate quad” since it does have precedent in graphics programming.

Creates a new `MGLCoordinateQuad` structure from the given top left, top right,
bottom right, and bottom left coordinates .
*/
NS_INLINE MGLCoordinateQuad MGLCoordinateQuadMake(CLLocationCoordinate2D topLeft, CLLocationCoordinate2D topRight, CLLocationCoordinate2D bottomRight, CLLocationCoordinate2D bottomLeft) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These parameters should go in the same order as the fields are declared above, to ensure consistency between Objective-C and Swift.


- (void)testMGLImageSourceWithImageURL {

MGLCoordinateQuad quad = { { 80, 37}, { 80, 39}, { 81, 39}, { 81, 37}};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These coordinates appear to be listed in BL/BR/TL/TR order, whereas the struct’s fields are declared in TL/BL/BR/TR order. (×2) Using the designated initializer syntax would head off this mistake, but I wonder if we should introduce a runtime check as well. (A runtime check can be tail work, but we should correct this test before merging.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this test we dont really care about how an image is placed, so the coordinate order can be fixed for someone else to understand.
In general though the corner coordinates are relative to the image, and can be used to rotate an image for placement. The top left corner does not have to be the north eastern coordinate.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That’s reasonable, although in this case the order would result in a non-quadrilateral, wouldn’t it?

@@ -271,6 +274,10 @@
/* End PBXCopyFilesBuildPhase section */

/* Begin PBXFileReference section */
07A019EA1ED662D800ACD43E /* MGLImageSource_Private.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = MGLImageSource_Private.h; sourceTree = "<group>"; };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file no longer exists.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😕 I deleted it from the IDE

Copy link
Contributor

@1ec5 1ec5 Jun 16, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That’s the correct (only) way to do it; however, the macOS project is currently separate from the iOS project, even though they share many files. So you need to also open up make xproj and delete the (now broken) reference to the file from that project as well. #5942 would merge the two projects, avoiding this complication in the future. (Also, the new build system in Xcode 9 more heavily emphasizes the file system, so it’s possible that Apple will solve this problem for us.)

@asheemmamoowala asheemmamoowala force-pushed the image-source-bindings branch from eba51de to 37fa8ad Compare June 16, 2017 21:19
Copy link
Contributor

@1ec5 1ec5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All set!

@asheemmamoowala
Copy link
Contributor Author

Closes #1350 and #9295

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Android Mapbox Maps SDK for Android GL JS parity For feature parity with Mapbox GL JS iOS Mapbox Maps SDK for iOS macOS Mapbox Maps SDK for macOS runtime styling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants