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

annotations plan #877

Closed
incanus opened this issue Feb 13, 2015 · 49 comments
Closed

annotations plan #877

incanus opened this issue Feb 13, 2015 · 49 comments
Labels
feature iOS Mapbox Maps SDK for iOS
Milestone

Comments

@incanus
Copy link
Contributor

incanus commented Feb 13, 2015

"Annotations" is iOS-speak for points and polygons on the basemap with a high-level client-side API.

  • Data — This encapsulates such things as client-side data (client-side data #507) or formats like GeoJSON, and also includes programmatic addition of points and polygons that don't exist in files or formats at all (e.g. show user location, add these stores to the map, trace this route from parsed directions API). In fact, format-less features are the MVP for this. Spatial indexing falls under this category as well — the system needs to know what's on screen right now as well as be able to translate between pixel-space and geo-space (fixes #476: pixel / projected meter / lat/long conversions #853).
  • Design — Polylines & polygons can get styling properties like width, fill color, opacity, and stroke color. This can change at runtime, so it's not part of the GL style. Markers can get a) static marker imagery, b) iOS UIView objects, or even c) animations of views. These can also all change / be provided at runtime. Markers always remain upright when the map rotates and order themselves in the z-axis from current top-to-bottom of screen, updating continuously as the map rotates. Lastly, marker annotations have callouts, which should be implemented specific to iOS (likely using SMCalloutView as our SDK does).
  • Interaction — Users can tap on poly & marker visuals and get callbacks with the associated data object referenced.
  • API — All APIs follow Apple's MapKit, so relevant classes & protocols are MKMapView, MKAnnotation, MKAnnotationView, and MKMapViewDelegate.
    • An MKAnnotation-derived data object is added (map.addAnnotation(foo)) .
    • An MKMapViewDelegate is set on a map (map.delegate = bar).
    • When the visual for an annotation is relevant/on screen, a view is requested from the delegate, which creates it on the fly (func mapView(map, viewForAnnotation: foo)).
    • The map maintains and responds to annotation selection state (func mapView(map, didSelectAnnotation:foo))

Will kick off deeper tickets for each of these buildouts as we progress.

@incanus incanus added feature iOS Mapbox Maps SDK for iOS labels Feb 13, 2015
@incanus incanus added this to the iOS Beta milestone Feb 13, 2015
@incanus
Copy link
Contributor Author

incanus commented Feb 16, 2015

Conversion routines are in and API for mocking annotations atop the GL view is available.

Next steps are @ljbade & @bleege checking out #756 on Android and I will start mocking markers (not spatially indexed, however, to start) to see how the FPS handles the compositing.

@incanus
Copy link
Contributor Author

incanus commented Feb 17, 2015

@kkaefer Here's a small sample project that does animated point markers and animated polygon rendering as an example of what an iOS developer might expect.

Video demo: https://dl.dropboxusercontent.com/u/575564/animatedannotations.mp4

screen shot 2015-02-16 at 5 57 57 pm

Markers

The ViewController delegates to the PointAnnotationView class, which uses Core Animation to rotate itself over time (keyframe animations over a few seconds) and a repeating timer to alter its background color, alpha, and border thickness (eight or so animation steps). You could typically see something like this for a game or interactive app which uses keyframe animations at points on the map (animated oil wells or realtime pie charts, for example).

Polylines

The ViewController also delegates to the AnimatedRenderer class, which uses a timer to change the size of an image that is used as a tiled pattern fill color every few seconds. This is a bit of a contrived example, but you might do an animated radar overlay or a moving pattern animation to indicate flow across a map surface.

The point is less about flashiness and more about the fact that an iOS dev will expect to author views and renderers as first-class Cocoa objects for the control of animations, colors, strokes, patterns, opacities, and timing.

@incanus incanus mentioned this issue Feb 17, 2015
5 tasks
@incanus
Copy link
Contributor Author

incanus commented Feb 17, 2015

Per voice with @1ec5, it might make sense to not necessarily adopt an either-or (core vs. per-platform) approach. For example, simple sprite-based markers (Maki or otherwise) could be done in the icon shader, but more complex views could be composited and tracked to the map per-platform.

Of note in the simple use case is the fact that z-ordering matters; markers are ordered back-to-front (z) by order of top-to-bottom (y), including changing on the fly during map rotations. On iOS, when selected, a marker comes all the way to the front temporarily, then goes back in place on deselect.

@1ec5
Copy link
Contributor

1ec5 commented Feb 17, 2015

For example, simple sprite-based markers (Maki or otherwise) could be done in the icon shader, but more complex views could be composited and tracked to the map per-platform.

And ideally the heuristic would be on our end, not something the dev has to think about.

@mb12
Copy link

mb12 commented Feb 17, 2015

@incanus and @kkaefer Is it possible to share how you would synchronize between iOS UIView and GLViews. Otherwise there would always be a lag between overlay UIView (e.g. Bubble) and underlying MGLMapView. Specifically this is the problem I am referring it.

http://stackoverflow.com/questions/12967340/how-to-synchronize-opengl-drawing-with-uikit-updates

@incanus
Copy link
Contributor Author

incanus commented Feb 17, 2015

Is it possible to share how you would synchronize between iOS UIView and GLViews

I've had good luck using CADisplayLink for this in some old tests.

https://github.com/incanus/FeaturesPlay

http://www.mobypicture.com/user/incanus771/view/17359491

I'm trying this again now in this project currently.

@incanus
Copy link
Contributor Author

incanus commented Feb 17, 2015

@1ec5 I pushed what I've been playing with at 877-display-link branch.

@incanus
Copy link
Contributor Author

incanus commented Feb 17, 2015

Per voice with @1ec5 @kkaefer, we decided the beta will not support full UIView objects passed by the delegate as annotation views, but rather only static imagery at this point.

MapKit's delegate API, which we will model after, looks like this:

- (MKAnnotationView *)mapView:(MKMapView *)mapView viewForAnnotation:(id<MKAnnotation>)annotation

When an annotation view is requested, you would return an MKAnnotationView or its subclass MKPinAnnotationView, which are all subclasses of UIView.

What we can do as a first step is to only support an MKPinAnnotationView-like class which either:

  • Refers to known parameters such as Maki icon, color, or size (similar to MKPinAnnotationView.pinColor) and keys those to GL core routines to render those with the icon shader, or
  • Accepts user-provided imagery which we pass across the boundary to the icon shader.

@mb12
Copy link

mb12 commented Feb 18, 2015

Is it possible to make the pins draggable for iOS?

@incanus
Copy link
Contributor Author

incanus commented Feb 18, 2015

I'm not sure yet if we'll hit that for the beta, but eventually, yes.

bleege added a commit that referenced this issue Feb 18, 2015
bleege added a commit that referenced this issue Feb 19, 2015
… methods that also send MapChangeRegion notifications
@bleege
Copy link
Contributor

bleege commented Feb 19, 2015

Finished building the MapRegionIsChanging notification hook on regionChanged branch. Waiting for Mr Travis to green light it and then I'll merge.

@incanus
Copy link
Contributor Author

incanus commented Feb 19, 2015

This is looking good. However, do you get multiple fires of the region changing across animation durations? I think we need that.

For example: animate the map view center over the default iOS duration (300ms). With the old callback methods, you get a ping at the start and a ping 300ms later. It seems like the same thing would happen with this added notification, too. What we should get instead is a ping per draw frame through that 300ms (roughly 0.3 seconds x 30-60 fps = 10-20 callbacks).

@incanus
Copy link
Contributor Author

incanus commented Feb 19, 2015

As for how to do this... I think it will involve looking into how the transitions are worked through on each frame, since that's where the state changing is happening. That is, _moveBy() sets up the animated change, but is not run through again; rather transitions are iterated on until complete, at which point they are removed. See around here.

@ljbade
Copy link
Contributor

ljbade commented Feb 19, 2015

Yeah we need per frame callbacks, currently my Android app does not update the marker position during animation.

@1ec5
Copy link
Contributor

1ec5 commented Feb 21, 2015

The 877-callout branch is all set with using delegate callbacks instead of CADisplayLink.

The last couple days I thought we were seeing that markers in the 877-display-link and 877-callout branches were jumping ahead of the map while panning. But it looks like the marker sometimes gets offset up to 50 meters after a drag, pinch, or rotation. That may point at a bug in the marker positioning rather than a synchronization issue.

@1ec5
Copy link
Contributor

1ec5 commented Feb 21, 2015

But see also this comment here.

@mb12
Copy link

mb12 commented Feb 21, 2015

@1ec5
If you need to something per-frame, you can put your code in MBGLView::swap.(It may slow stuff down depending on what is done in the callback).
The overlay would still jitter, but it should get to the right location, when the map pan animation finishes.

@incanus
Copy link
Contributor Author

incanus commented Feb 23, 2015

Interesting idea @mb12. I'm pretty sure Apple doesn't provide such a callback because of the potential to slow down rendering, but I think we may at least want this privately for such things as annotation sync.

Only thing with swap though is that when we build up to more style transition animations & its API, that routine will run despite likely not needing annotation changes, i.e. swap will not be limited to merely zoom/pan changes.

@kkaefer
Copy link
Member

kkaefer commented Feb 23, 2015

Note that ::swap is called in the Map rendering thread, so you'd need to dispatch to the UI thread, which would probably incur some lag as well, unless you block swap until the UI thread completed its actions.

@mb12
Copy link

mb12 commented Feb 23, 2015

Yes, that causes a really jarring jitter in the overlay view upon panning.

This is what others seems to be doing on iOS.

1.) Image pins and Polylines in opengl view.
2.) Detect tap and open either ActionSheet or some other UIView that does not need to pan with map.

Here is a link to the GMaps SDK for iOS. GMSMarker just shows the snippet in a static info window at the bottom instead of a pannable Apple Maps like Annotation View.

https://developers.google.com/maps/documentation/ios/reference/index

@incanus
Copy link
Contributor Author

incanus commented Feb 27, 2015

I'm curious if we've looked further into the per-animation-frame updates to the delegate callbacks? See #877 (comment) above. @1ec5 @bleege can one of you look into this? I can possibly start to look into it next week but I'm tied up with GL-side annotation rendering right now.

Note that ::swap is called in the Map rendering thread, so you'd need to dispatch to the UI thread, which would probably incur some lag as well, unless you block swap until the UI thread completed its actions.

Agreed, and I would say to probably at least try blocking swap to see how this fares.

bleege added a commit that referenced this issue Mar 3, 2015
@bleege
Copy link
Contributor

bleege commented Mar 3, 2015

The UI is MUCH more responsive now with the hide / visible functionality added for the pins. This is definitely getting closer to where things need to be. I'm curious as to how big an impact a few optimizations could have (namely creating an annotation class to avoid all the dictionary lookups and geo object creations). I'll try that next and see what happens.

bleege added a commit that referenced this issue Mar 3, 2015
…and converting the MBXViewController to use it instead of NSDictionaries
@incanus
Copy link
Contributor Author

incanus commented Mar 3, 2015

This is sounding good @bleege. I'm curious what Instruments profiling turns up now? Might be an approach to check out.

bleege added a commit that referenced this issue Mar 3, 2015
@bleege
Copy link
Contributor

bleege commented Mar 3, 2015

@incanus I just profiled with Activity Monitor, Allocations, and GPU Driver. Didn't turn up too much, although Allocations did hold pretty steady so I'm feeling pretty decent about that. All in all I'm not sure how much more we can get considering that the app is crossing UIKit and OpenGL. I'm curious how this behaves on actual iPhone / iPad compared to the emulator.

@incanus
Copy link
Contributor Author

incanus commented Mar 3, 2015

Give the Time Profiler a whirl and see what methods are taking up the most time — that might shed some light. It should also be a lot different on device — the simulator isn't GPU accelerated IIRC.

@bleege
Copy link
Contributor

bleege commented Mar 3, 2015

Nah.. the biggest time users are the MBGL drawing routines.

20150303-timeprofile

@incanus
Copy link
Contributor Author

incanus commented Mar 3, 2015

Right, my point more is that if you filter out these routines that are known to be heavy and just focus on the Cocoa stuff, something there will be the "new heaviest" and you can see what that is.

You never really eliminate bottlenecks, you just move them around.

@bleege
Copy link
Contributor

bleege commented Mar 3, 2015

Yay! I just ran it on my iPhone 5s with iOS 8.1.3 and it's smoooooooth. I ❤️ Apple hardware.

@bleege
Copy link
Contributor

bleege commented Mar 3, 2015

I'm going to pull the SampleLocation code out of Core GL codebase so that this branch can eventually be merged.

@incanus
Copy link
Contributor Author

incanus commented Mar 4, 2015

Sweet! This is great. Runs well for me, too. What is there to merge just yet, though @bleege? The actual driver of where the annotations should track to is coming in #893 and we have yet to architect what the annotation view classes look like, though they likely will be UIImageView like here, since we're only supporting Maki and user-inputted imagery (see #941).

@bleege
Copy link
Contributor

bleege commented Mar 4, 2015

Awesome! Thanks for testing it out @incanus.

As for what to merge the most important part is MGLMapView.h and MGLMapView.mm from 4ea6b25 where the GLKView is made accessible. That said maybe the easiest way to get it in there would be to just cherrypick those files?

@incanus
Copy link
Contributor Author

incanus commented Mar 4, 2015

Ew, I think exposing the GLKView is confusing from a public API point of view. We should probably think about a class extension that's private that our own APIs can get to, as opposed to putting that in MGLMapView.h.

@incanus
Copy link
Contributor Author

incanus commented Mar 4, 2015

Or when we build out our actual annotation-adding API in Cocoa, it creates the subviews and adds them to the GLKView itself. The fact that this is in the view controller is an implementation detail right now, but consumers of our API need to be able to just run with MGLMapView and nothing else in their app.

@bleege
Copy link
Contributor

bleege commented Mar 4, 2015

Totally makes sense... I just had put it there more for expediency purposes when I was experimenting earlier today. Maybe the best thing to do is just leave this branch as is and just use it to guide #893 and #941?

@incanus
Copy link
Contributor Author

incanus commented Mar 4, 2015

Yeah, that's kinda what I had in mind with the "what to merge" above. We figured out how we're going to do it technically, but let's get some other pieces into place first.

I see next steps in this department as:

  1. Make a call on allow for user-provided symbol imagery (aka Sprites) #941 for the beta, or just Maki imagery / annotation marker imagery prebuilt into the style(s).
  2. Get merge GeoJSON vector tiles work #893 to land, providing API to say where annotations are at any given point.
  3. Write live annotation C++ API.
  4. Wrap API in MapKit-like Cocoa goodness.

@incanus
Copy link
Contributor Author

incanus commented Mar 11, 2015

Markers always remain upright when the map rotates and order themselves in the z-axis from current top-to-bottom of screen, updating continuously as the map rotates.

We've got a ticket for this over in JS in mapbox/mapbox-gl-js#470 but it seems to still be an issue:

screen shot 2015-03-11 at 1 48 02 pm

@kkaefer @ansis @jfirebaugh Any sense of how much work would be involved with this? We will likely also want the ability to temporarily bring a marker to the front upon selection.

/cc @1ec5

@jfirebaugh
Copy link
Contributor

Seem like it's time to unbundle this into separate followup tickets and close here.

@incanus
Copy link
Contributor Author

incanus commented Mar 12, 2015

Yep, agree. I will ticket some off now.

@incanus
Copy link
Contributor Author

incanus commented Mar 12, 2015

Thus far Cocoa API is meant to work like Apple's, but we may modify that as @1ec5 and I discuss integration steps.

@incanus incanus closed this as completed Mar 12, 2015
@maciekish
Copy link
Contributor

@incanus Is it possible to draw routes in the current version? We are using an RMAnnotation with an array of CLLocations at the moment (in mapbox-ios).

@incanus
Copy link
Contributor Author

incanus commented Mar 24, 2015

No, routes are coming later. Just points for now. Watch #893.

@picciano
Copy link

@incanus Any update on the work to support routes (polylines) now that Beta 1 is about wrapped up? This is our single biggest need right now and I would very much appreciate your thoughts on possible timing.

@incanus
Copy link
Contributor Author

incanus commented May 13, 2015

@picciano Shape annotations are one of the major focuses for b2. We'll have better timelines shortly, but we are thinking on the order of weeks.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature iOS Mapbox Maps SDK for iOS
Projects
None yet
Development

No branches or pull requests

9 participants