[ios, macos] possibility to set custom images to style #6637

rmnblm · 2016-10-09T07:33:15Z

With the changes to the core in #6375 we can now add custom images to the sprite atlas. This PR resolves #6436 to add the possibility to add custom images from bundles in iOS and macOS.

As proposed from @1ec5 in #6436, I've first named the method setImage:forName: but to stay consistent with all other methods of MGLStyle.h I've decided to name the method addImage:forName:.

Usage:

let imageURL = Bundle.main.url(forResource: "animal-dog", withExtension: "png")
mapView.style().addImage(imageURL!.path, forName: "dog")

let styleLayer  = MGLSymbolStyleLayer(identifier: "restaurants", source: geoJSONSource)
styleLayer.predicate = NSPredicate(format: "amenity == 'restaurant'")
styleLayer.iconImage = "dog" as MGLStyleAttributeValue!
mapView.style().add(styleLayer)

Remarks:
This being my first PR and I'm relatively new to the codebase of Mapbox GL Native I hope I did everything correctly following the code and contribution guidelines.

During development I had to decide what is the argument type for the image being passed to
As we all know, to use images from a .xcassets folder, you can use NSImage on macOS or UIImage on iOS but there is no base class for both to use as an argument type in

- (void)addImage:(  -->  NSImage/UIImage  <--   *)image forName:(NSString *)name

That's why I've decided to use a NSString * which is the absolute path of the image. This solution implies that you only can use images part of the bundle and not part of a .xcassets folder.

- (void)addImage:(NSString *)imagePath forName:(NSString *)name

/cc @nitrag

mention-bot · 2016-10-09T07:33:16Z

@rmnblm, thanks for your PR! By analyzing the history of the files in this pull request, we identified @1ec5, @frederoni and @incanus to be potential reviewers.

1ec5 · 2016-10-09T08:52:59Z

As we all know, to use images from a .xcassets folder, you can use NSImage on macOS or UIImage on iOS but there is no base class for both to use as an argument type

MGLTypes.h declares MGLColor as a typedef of NSColor on macOS and UIColor on iOS. Alternatively, you can use conditional compilation (#if) to declare two variations of the method, one per platform. It would be better for the method to take an image object than a path: after all, an NSColor or UIColor can be generated programmatically, without involving any file.

1ec5 · 2016-10-09T09:02:53Z

I've first named the method setImage:forName: but to stay consistent with all other methods of MGLStyle.h I've decided to name the method addImage:forName:.

Methods beginning with “add” typically append an object to an ordered collection, as in the case of -[NSMutableArray addObject:] or indeed -[MGLStyle addLayer:], or add an object to an unordered collection, as in the case of -[MGLMutableSet addObject:] or -[MGLStyle addSource:]. In these cases, either the objects lack unique identifiers or the unique identifier is part of the object being added (MGLStyleLayer, MGLSource). By contrast, the style image API you’re implementing is adding an image to an unordered collection whose keys are external to the images. In that sense, it’s analogous to NSMutableDictionary and NSUserDefaults, which have -setObject:forKey: methods.

A -setImage:forKey: method would accept nil to remove an image, so a separate -removeImage: method would be unnecessary.

rmnblm · 2016-10-09T09:30:05Z

Good point about the "add" in front of methods and I totally agree with out but then again, it's inconsistent with the map.hpp/map.cpp names

void addImage(const std::string&, std::unique_ptr<const SpriteImage>);
void removeImage(const std::string&);

while the implementation of addImage does indeed set the sprite in the atlas

impl->style->spriteAtlas->setSprite(name, std::move(image));

A -setImage:forKey: method would accept nil to remove an image, so a separate -removeImage: method would be unnecessary.

I can do this but in my opinion this would be misleading. Wouldn't it be better to also follow the API convention of a NSMutableDictionary removeObjectForKey:

- (void)removeImageForName:(NSString *)name;

rmnblm · 2016-10-09T15:52:11Z

I just pushed commit 762f0a39574606f29d670f688ed5fba6c7bee83a with the following changes:

I added a new type inside MGLTypes.h called MGLImage which is either an UIImage on iOS or a NSImage on macOS

I renamed the method as you proposed, now there are

- (void)setImage:(MGLImage *)image forName:(NSString *)name;
- (void)removeImageForName:(NSString *)name;

I reused the code from MGLMapView.mm to get a mbgl::SpriteImage from a UIImage/NSImage. This was done by adding two new source files for iOS (UIImage+MGLAdditions) and macOS (NSImage+MGLAdditions) each.
I refactored MGLMapView.mm in both iOS and macOS to use the new implementations to avoid duplicate code

Usage:

let image = UIImage(named: "animal-dog")!
mapView.style().setImage(image, forName: "dog")

let styleLayer = MGLSymbolStyleLayer(identifier: "restaurants", source: geoJSONSource)
styleLayer.predicate = NSPredicate(format: "amenity == 'restaurant'")
styleLayer.iconImage = "dog" as MGLStyleAttributeValue!
mapView.style().add(styleLayer)

1ec5 · 2016-10-10T04:19:26Z

platform/darwin/src/MGLStyle.h

@@ -258,6 +258,21 @@ static const NSInteger MGLStyleDefaultVersion = 9;
 */
 - (void)removeStyleClass:(NSString *)styleClass;

+/**
+ Sets an image to the sprite atlas with a unique identifier.


“Sprite atlas” is jargon that most developers would find unintuitive. It’s probably sufficient to describe the image being added to the style (since it lasts until the style URL changes), then mention that the developer can use the image by setting the MGLSymbolStyleLayer.iconImage property to the name passed in here.

1ec5 · 2016-10-10T04:19:42Z

platform/darwin/src/MGLStyle.h

+/**
+ Sets an image to the sprite atlas with a unique identifier.
+
+ @param image


Need to document what image is.

1ec5 · 2016-10-10T04:21:30Z

platform/darwin/src/MGLStyle.mm

@@ -254,5 +257,21 @@ - (void)removeStyleClass:(NSString *)styleClass
    }
 }

+- (void)setImage:(MGLImage *)image forName:(NSString *)name
+{
+    if (image && name)


Assert instead of failing gracefully. These two parameters are implied to be nonnull in the header.

You're right, I did not see that. Would it be sufficient to remove the if-Statement?
As a side note: Isn't this if-Statement also unnecessary?

By default, passing in nil for a nonnull parameter is only a warning rather than an error, so NSAssert() is for the benefit of developers who ignore their warnings. (Tsk, tsk!) On the other hand, we haven’t been disciplined about asserting in similar cases – -addStyleClass: and -removeStyleClass: are more examples of this – so it’s a bit unfair of me to nitpick about this detail. 😊

I'm glad you're taking it seriously and don't merge everything without some code review and I'm thankful for your nitpicking because I can only benefit from it. ☺️ Going to push all changes in a few minutes.

By the way: Do I have to NSAssert() now inside both methods because (MGLImage *)image and (NSString *)name can't be null anyway and as a side quesstion: What is your recommended way to assert for null?

NSAssert([name isEqual:[NSNull null]], "Name cannot be null"); or NSAssert(name == NULL, "Name cannot be null");

The first form will crash unless name is [NSNull null] specifically. Since name is of type NSString * rather than id, there is no way to avoid the crash.

The second form will crash unless name is NULL, which is the opposite of what you’d do in this case. Moreover, in Objective-C, nil is conventionally used for Objective-C objects, NULL is used for C pointers and double-pointers, and Nil is used for Objective-C Classes. I recommend reading this NSHipster article to learn about the various ways to say “nothing” in Objective-C.

In this case, you can simply NSAssert(name).

Thanks for the article! If I can follow, you're expecting this:

NSAssert(image); NSAssert(name);

Just pushed it. I had to add a second argument with description otherwise I got error too few arguments provided to function-like macro invocation.

1ec5 · 2016-10-10T04:23:18Z

platform/darwin/src/MGLStyle.mm

+
+- (void)removeImageForName:(NSString *)name
+{
+    if (name)


Similarly, assert here.

1ec5 · 2016-10-10T04:25:05Z

platform/darwin/src/MGLTypes.h

@@ -3,6 +3,14 @@
 #pragma once

 #if TARGET_OS_IPHONE
+@class UIImage;


👍

(Sorry, for some reason I thought we had already defined MGLImage, but in fact it was MGLColor that we implemented.)

1ec5 · 2016-10-10T04:29:17Z

platform/darwin/src/MGLStyle_Private.h

@@ -5,6 +5,12 @@
 #import <mbgl/util/default_styles.hpp>
 #include <mbgl/mbgl.hpp>

+#if TARGET_OS_IPHONE


Can we move this over to MGLStyle.mm?

1ec5 · 2016-10-10T04:30:35Z

platform/darwin/src/MGLStyle.mm

@@ -1,5 +1,7 @@
 #import "MGLStyle.h"

+#import "MGLTypes.h"


This should be unnecessary as long as MGLStyle.h imports MGLTypes.h.

1ec5 · 2016-10-10T04:34:19Z

platform/macos/macos.xcodeproj/project.pbxproj

@@ -7,6 +7,8 @@
 	objects = {

 /* Begin PBXBuildFile section */
+		30E5781B1DAA857E0050F07E /* NSImage+MGLAdditions.h in Headers */ = {isa = PBXBuildFile; fileRef = 30E578141DAA7D920050F07E /* NSImage+MGLAdditions.h */; settings = {ATTRIBUTES = (Public, ); }; };


This header’s visibility should be Project, not Public. Making it public would expose C++ syntax to pure Objective-C projects using the SDK.

I first had UIImage+MGLAdditions/NSImage+MGLAdditions public but then realized it wasn't necessary, might have missed to set it back to Project here.

1ec5

Thanks for the revisions. You’re right about having a separate -removeImageForName:, and putting the SpriteImage conversion in a category is a great idea.

1ec5 · 2016-10-10T04:40:31Z

platform/ios/src/UIImage+MGLAdditions.h

+
+@interface UIImage (MGLAdditions)
+
+ (std::unique_ptr<mbgl::SpriteImage>)mbgl_spriteImageFromImage:(UIImage *)image;


It would be more natural to have an instance method in this case: -mgl_spriteImage. (Also, for consistency’s sake, the class prefix is mgl rather than mbgl.)

rmnblm · 2016-10-10T06:51:56Z

I just noticed that I wasn't consistent with my last two commit messages (delete/remove). 😑

1ec5

Almost there!

1ec5 · 2016-10-10T07:12:58Z

platform/darwin/src/MGLStyle.h

- Sets an image to the sprite atlas with a unique identifier.
+ Sets the name and image to the style.
+ The image can be used by setting the MGLSymbolStyleLayer.iconImage
+ property to the name passed in here


This method can be used to both add a new image or override an existing one, so we should make that clear. Also, when jazzy generates the HTML documentation, it automatically links symbols enclosed in backticks, to help readers navigate the multi-page documentation. This wording would be more verbose, but it leaves nothing open to interpretation:

Adds or overrides an image used by the style’s layers.

To use an image in a style layer, give it a unique name using this method, then set the iconImage property of an MGLSymbolStyleLayer object to that name.

1ec5 · 2016-10-10T07:14:59Z

platform/ios/src/UIImage+MGLAdditions.h

@@ -4,6 +4,6 @@

 @interface UIImage (MGLAdditions)

-+ (std::unique_ptr<mbgl::SpriteImage>)mbgl_spriteImageFromImage:(UIImage *)image;
+ (std::unique_ptr<mbgl::SpriteImage>)mgl_spriteImage:(UIImage *)image;


What I meant was that this would become a parameter-less instance method, like this:

- (std::unique_ptr<mbgl::SpriteImage>)mgl_spriteImage;

Within the implementation of an instance method, we have access to all the members of self, such as self.CGImage. Then you can simply say annotationImage.image.mgl_spriteImage to get the unique_ptr.

Oops, now I know what you meant. Needed more coffee to understand it. ☕️

1ec5 · 2016-10-10T07:19:09Z

I just noticed that I wasn't consistent with my last two commit messages (delete/remove). 😑

No worries. Normally we use GitHub’s “Squash and merge” feature, so the title of this PR will become the first line of the squashed commit message. If it bothers you, though, you can use git rebase -i to revise your commit messages, then git push --force-with-lease to override what you’ve previously pushed to this branch.

1ec5 · 2016-10-10T07:22:28Z

It’s a bit unusual to have a setter and remover without an -imageForName: getter. mbgl::Map doesn’t currently expose a way to get an installed image, so that’ll be a more invasive change. This getter wouldn’t be a high priority, so it’s fine to punt on it for now.

rmnblm · 2016-10-10T07:28:25Z

I also thought about a getter like -imageForName but knew that there wasn't such an implementation in mbgl::Map. Though it's unusual to have a setter without a getter, I can't think of a use case where I'd need such getter. But somebody might need it in the future, who knows... 🌝

1ec5 · 2016-10-11T21:49:43Z

Sorry, this ended up conflicting with #6636, which just got merged. Can you resolve the (minor) conflict or check the “Allow edits from maintainers” box so I push the resolution before merging? Thanks!

rmnblm · 2016-10-12T08:39:57Z

I enabled edits from maintainers, the stage is yours 🏟

…geForName:

1ec5 · 2016-10-12T19:30:33Z

Thanks! 🎉

friedbunny · 2016-10-13T00:54:15Z

This commit to master failed to compile on CI:

 Compiling NSImage+MGLAdditions.mm

❌  /Users/vagrant/git/platform/macos/src/NSImage+MGLAdditions.mm:10:6: use of undeclared identifier 'image'

    [image lockFocus];
     ^



❌  /Users/vagrant/git/platform/macos/src/NSImage+MGLAdditions.mm:12:6: use of undeclared identifier 'image'

    [image unlockFocus];
     ^

Followup to last-minute refactoring in #6637.

1ec5 · 2016-10-13T01:56:38Z

Thanks @friedbunny. Bitrise CI doesn’t run on third-party PRs, unfortunately. Incoming fix in #6690.

Followup to last-minute refactoring in #6637.

1ec5 suggested changes Oct 10, 2016

View reviewed changes

1ec5 added feature iOS Mapbox Maps SDK for iOS macOS Mapbox Maps SDK for macOS runtime styling labels Oct 11, 2016

1ec5 added this to the ios-v3.4.0 milestone Oct 11, 2016

1ec5 approved these changes Oct 11, 2016

View reviewed changes

Roman Blum added 10 commits October 12, 2016 12:04

[ios, macos] possibility to set custom images to style

0e9306e

[ios, macos] setImage:forName now supports MGLImage (UIImage/NSImage)

d6ba206

[ios, macos] update documentation for setImage:forName: and removeIma…

c77a376

…geForName:

[ios, macos] rename method and fix prefix

4605382

[ios, macos] change header visibility to project

e61c2a1

[ios, macos] update header imports

f177ccb

[ios, macos] delete unnecessary whitespaces

e18a8db

[ios, macos] remove unnecessary nil checks

91d7dd3

[ios, macos] delete unnecessary whitespaces

bf3546b

[ios, macos] update documentation

0c0032e

Roman Blum added 2 commits October 12, 2016 12:04

[ios, macos] make mgl_spriteImage a parameter-less instance method

08033c0

[ios, macos] add asserts

01e82db

1ec5 merged commit a24e559 into mapbox:master Oct 12, 2016

rmnblm deleted the custom_style_images branch October 12, 2016 19:32

1ec5 added a commit that referenced this pull request Oct 13, 2016

[macos] Fixed compilation error

e8be169

Followup to last-minute refactoring in #6637.

1ec5 mentioned this pull request Oct 13, 2016

Fix macOS compiler error — -[NSImage(MGLAdditions) mgl_spriteImage] #6690

Merged

1ec5 added a commit that referenced this pull request Oct 13, 2016

[macos] Fixed compilation error (#6690)

f064744

Followup to last-minute refactoring in #6637.

incanus mentioned this pull request Oct 19, 2016

reduce public header code for shared color type #6763

Closed

1ec5 mentioned this pull request Feb 13, 2017

Reimplement MGLAnnotation atop MGLSource and MGLStyleLayer #6181

Closed

6 tasks


		@interface UIImage (MGLAdditions)

		+ (std::unique_ptr<mbgl::SpriteImage>)mbgl_spriteImageFromImage:(UIImage *)image;

[ios, macos] possibility to set custom images to style #6637

[ios, macos] possibility to set custom images to style #6637

Conversation

rmnblm commented Oct 9, 2016 • edited Loading

mention-bot commented Oct 9, 2016

1ec5 commented Oct 9, 2016 • edited Loading

1ec5 commented Oct 9, 2016 • edited Loading

rmnblm commented Oct 9, 2016 • edited Loading

rmnblm commented Oct 9, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmnblm Oct 10, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmnblm Oct 10, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1ec5 Oct 10, 2016 • edited Loading

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

Choose a reason for hiding this comment

1ec5 Oct 10, 2016 • edited Loading

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

Choose a reason for hiding this comment

1ec5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmnblm commented Oct 10, 2016

1ec5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1ec5 Oct 10, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1ec5 commented Oct 10, 2016

1ec5 commented Oct 10, 2016

rmnblm commented Oct 10, 2016

1ec5 commented Oct 11, 2016

rmnblm commented Oct 12, 2016

1ec5 commented Oct 12, 2016

friedbunny commented Oct 13, 2016 • edited Loading

1ec5 commented Oct 13, 2016

rmnblm commented Oct 9, 2016 •

edited

Loading

1ec5 commented Oct 9, 2016 •

edited

Loading

1ec5 commented Oct 9, 2016 •

edited

Loading

rmnblm commented Oct 9, 2016 •

edited

Loading

rmnblm commented Oct 9, 2016 •

edited

Loading

rmnblm Oct 10, 2016 •

edited

Loading

rmnblm Oct 10, 2016 •

edited

Loading

1ec5 Oct 10, 2016 •

edited

Loading

1ec5 Oct 10, 2016 •

edited

Loading

1ec5 Oct 10, 2016 •

edited

Loading

friedbunny commented Oct 13, 2016 •

edited

Loading