[ASMapNode] Add custom pin annotation for static maps

[michalziman]

Using delegate call for annotation view for annotation and drawing this view to static map image. This resolves #1889.

Also adds simple example. Just to demonstrate that it works.

[ghost]

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

[hannahmbanana]

@michalziman: thanks for contributing to the community by adding this functionality!

I haven't reviewed your code yet, but with regards to the sample project, could you copy an existing ASDK sample project (cmd + D on any /example folder) and put the example project in that (e.g. top level folder can be named MapCustomAnnotationTest, but inner project needs to be named Sample and have same project settings. Thanks!

[george-gw]

@michalziman That is great that you added this, I was going to do that soon as well.

@hannahmbanana my PR is in "development in progress" right now (#1792), it also includes a sample project for the ASMapNode, assuming my PR goes through, maybe these changes/samples could be merged into my PR (/its outcome) somehow?

[michalziman]

@hannahmbanana , @george-gw Thanks for commenting so quickly. :)

Should I fix the sample project in my branch? Or maybe I can just wait till the other PR gets to master. Then update my branch merging wrong sample into the correct one from George.

I am sorry I did not check other samples before to see if there is some common structure.

[hannahmbanana]

@george-gw - thanks for commenting! I believe your PR is in good standing, but the reason it hasn't been merged yet is because few of us are very familiar with ASMapNode. @maicki - any reason we couldn't merge #1792 soon?

@michalziman: thanks for the quick response! Assuming we can get @george-gw's PR merged soon, it would be nice to combine the two map sample projects into one.

[aaronschubert0]

Hey @michalziman thanks for this! I've always wanted this feature in ASMapNode eventually. In terms of approach I'd prefer if you followed the current approach to customising ASMapNode, which is to have a public property on ASMapNode (with a sensible default). So for instance you would have:

@property (nonatomic, strong) MKAnnotationView *pinView;

The only downside to this would be that you can't have multiple different type of pins on the same map but the code would be a lot cleaner and more inline with the current approach, something to think about.

[michalziman]

Thanks @aaronschubert0 for an idea. As you mention, it would make the code cleaner. However, what I try to follow is making AsyncDisplayKit classes as similar in usage as possible to their UIKit (MapKit in this case) counterparts. MKMapView uses delegate, so it makes sense, that ASMapNode would do that, too. Using delegate to create views for annotations is not a difficult concept. And it also works with multiple kinds of custom pins. Which, is necessary almost every time when using custom pins.

[george-gw]

As a user of ASMapNode, I would like the map to have the same look regardless of whether it's a live map or an image. So the downside @aaronschubert0 mentioned would actually be a deal breaker. The path @michalziman took allows for that to happen without any additional code.

However, I do see a potential problem with that approach:
When liveMap = NO, self.mapView is actually nil, and thus we would be calling mapView:viewForAnnotation: with mapView = nil. And if the developer is using the argument mapView in their implementation instead of some ivar, that could result in undesirable side effects.

[michalziman]

I was thinking about this issue with mapView:viewForAnnotation:. I did not came to any smart and simple solution. Only viable two of which I thought were holding MKMapView object in self.mapView even for static map (not live), just to send it as parameter. The other is risking sending nil from self.mapView. In the declaration of this mapView:viewForAnnotation: method, mapView parameter is assumed nonnull, so it can cause problems to users.

[george-gw]

If we are to do what @aaronschubert0 suggested, I believe that property would have to be a block that would only be used when liveMap = NO, something like:

@property (nonatomic, copy, nullable) MKAnnotationView * _Nullable (^annotationViewInStaticMap)(id<MKAnnotation> annotation);

Alternatively we could create an ASMapNodeDelegate that would add an extra method to handle that case.

Thoughts?

[michalziman]

I like the block property approach a bit more.

[aaronschubert0]

@michalziman @george-gw Thanks for this, that approach looks very clean!

[ghost]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[george-gw]

Could you please change self. to strongSelf. to be consistent with the rest of the function!

[Adlai-Holler]

This is great stuff – the pin annotation limitation of ASMapNode has been a major oversight for a while. However, rather than returning a view, how about having the block return a UIImage? That way the client can use whatever approach they like – snapshotting a view, loading from disk, etc. Plus, since the snapshotter completion is off-main it's not safe to use UIViews in that context.

The documentation for the block property should specify that the block will be called on an arbitrary serial queue.

@michalziman Thoughts?

[michalziman]

Sure @Adlai-Holler , documentation should tell that block is called on an arbitrary queue.

I am thinking about changing return type of the block. Returning view, it can carry location of pin's center. I can't think of simple way to bring this information in UIImage. And it is important for annotations. Also, with current approach, client can always use standard MKAnnotationView and just pass UIImage as .image property, just like in example. The drawback is necessity to render this view while snapshotting map.

I am hesitating which approach is better. Any suggestion?

[Adlai-Holler]

Ah good point @michalziman. How about we mirror the MKAnnotationView centerOffset property? So the block would be:

UIImage * _Nullable (^imageForStaticMapAnnotationBlock)(id<MKAnnotation> annotation, inout CGPoint *centerOffset)

Just like the BOOL * stop in enumeration methods, if the user wants to provide us a center offset they can – defaults to CGPointZero. Thoughts?

[michalziman]

I modified the block as suggested. Probably the best approach, even though it seems a bit less straightforward to use.

[Adlai-Holler]

I don't think we want to set center here, since the map view is going to place the annotation wherever it decides. Was this meant to be centerOffset?

[Adlai-Holler]

Instead of instantiating an MKPinAnnotationView every time (which can get very expensive!) let's make this a class method + (UIImage *)defaultPinImageWithCenterOffset: and wrap the body of it in a dispatch_once (see my last comment for example code).

[Adlai-Holler]

@michalziman Thanks for the updates! To fix the test failures – apologies for that – rebase this branch onto the latest master and let me know what you think of those comments.

[george-gw]

You should have used git rebase instead of merging master in, this way you don't get the "Merge branch 'master' of ..." message in the git history.

Remove previous commit
git reset --hard HEAD~1

Fetch updates from original repo
git fetch upstream

Rebase
git rebase upstream/master

Push
git push origin BRANCH_NAME --force

It seems you've done a merge at some point earlier as well, I'll leave that to the core ASDK team to decide the best course of action here.

[michalziman]

Why is not getting the "Merge branch 'master' of ..." message so important? Does it spoil something to just pull from upstream? What if there are some conflicts? It would be useful to see what was merged and when. I just can not imagine any good reason to rebase.

[george-gw]

Maybe this can be changed to:

Always get the default pin, just in case it might be used (better here than in a for loop)

CGPoint defaultPinCenterOffset = CGPointZero;
UIImage *defaultImage = [strongSelf.class defaultPinImageWithCenterOffset:&defaultPinCenterOffset];

Now that we have the default image, we can loop through the annotations, if the property is not set or if it returns nil, we will just use the default pin.

for (id<MKAnnotation> annotation in annotations) {
  UIImage *pinImage = nil;
  CGPoint pinCenterOffset = CGPointZero;

  if (strongSelf.imageForStaticMapAnnotationBlock) {
    pinImage = strongSelf.imageForStaticMapAnnotationBlock(annotation, &pinCenterOffset);
  }

  if (!pinImage) {
    pinImage = defaultImage;
    pinCenterOffset = defaultPinCenterOffset;
  }

[...]

The idea is that if we are to use the defaultImage it is most likely going to be for more than one annotation, and this way we don't have to call the defaultPin several times since it will always return the same value anyway.

What do you think?

PS: If we won't go this way, either remove the comments that you have, or change the references from 'annotation view' to 'image for pin' or something to be consistent with the new code.

[george-gw]

@michalziman with that kind of rebase you shouldn't get any merge conflicts. It creates a cleaner git log if you don't have all these merge messages, the history will be preserved in the correct order as well... In any case, perhaps someone from the core ASDK team can weigh in on this? @hannahmbanana maybe? (I'm not sure who the right person to @ for this case though hehe...)

[Adlai-Holler]

Hmm unfortunately we just cannot allocate views in here, since we're off the main thread. Since CustomMapAnnotation has an image property, let's use that. Also let's make sure to set centerOffset to 21,21

[Adlai-Holler]

After the rebase (thanks for it!) my comment is collapsed. I'm really excited to land this one so @michalziman if you get a sec to modify that would be awesome.

[michalziman]

I am sorry guys for a little delay. I was on holiday. I did the rebase as you suggested.

[Adlai-Holler]

We can't allocate views in this block, since we're off the main thread. @michalziman Since CustomMapAnnotation has an image property, can you modify this like:

CustomMapAnnotation *customAnnotation = (CustomMapAnnotation *)annotation;
return customAnnotation.image;

[Adlai-Holler]

Great! Thanks for your continued work bringing this all the way @michalziman!

[michalziman]

Thanks for helping, to all of you!