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

Refactor user location annotation; add accuracy ring - #1222

Closed
friedbunny wants to merge 12 commits into
mapbox:masterfrom
friedbunny:location-annotation
Closed

Refactor user location annotation; add accuracy ring#1222
friedbunny wants to merge 12 commits into
mapbox:masterfrom
friedbunny:location-annotation

Conversation

@friedbunny

Copy link
Copy Markdown
Contributor

In digging around the user location annotation to fix its shadow (#1204), I saw some things that were a little rough and ended up reworking the entire thing. Hopefully this can be considered for beta 2, if everything hasn't moved entirely GL by that time.

I cribbed from Sam Vermette's SVPulsingAnnotationView and converted entirely to CALayer, which has a couple advantages for this application: smoother edges on the animated circles and the ability to use a single, more comprehensible API.

Even though pre-rendered UIImage is the almost always the most performant and best way to go, I was surprised that I couldn't find any measurable difference or slowdown when using rasterized CALayer here. Please test and let me know how you measured!


screen shot 2015-04-06 at 10 49 59 pm

Things to look for, mostly inspired by MapKit:

  • Hooked up the accuracy ring (MapKit-like filled circle, not an actual ring)
  • Accuracy ring animates changes (but faster than MapKit)
  • Linked the fading sonar-wave animation to accuracy ring size (when visible)
  • Tinted pulsing inner dot scales more smoothly, fades opacity
  • Inner dot and outer wave pulses are synced

Things needing attention:

  • Heading indication arrow/flashlight graphic
  • Add a pop-in animation when it first appears
  • Accuracy ring changes are out of sync (should probably be converted to a GL circle annotation)
  • Correct heading-indicator direction on device rotation

Oh, I also tested this on my sluggish iPad Mini (A5) and even it seemed fine.

@1ec5 1ec5 added the iOS Mapbox Maps SDK for iOS label Apr 7, 2015
@1ec5 1ec5 self-assigned this Apr 7, 2015
@1ec5 1ec5 added this to the iOS Beta 2 milestone Apr 7, 2015
@1ec5 1ec5 removed their assignment Apr 7, 2015
@1ec5

1ec5 commented Apr 7, 2015

Copy link
Copy Markdown
Contributor

Thanks for completing and cleaning up this feature, @friedbunny! It looks good to me, although we’re going to save this PR for beta 2. We’d like to reduce code churn and focus on stability in beta 1. Feel free to run with the tail work you listed above in the meantime.

@friedbunny

Copy link
Copy Markdown
Contributor Author

Sure thing, I definitely agree that these enhancements are a better fit for beta 2. I'm planning on having a go at the heading indicator today/soon, we'll see what else comes up during that.

For now, I've got every CALayer set as drawsAsynchronously = YES for the potential performance gains, but I believe #1125 is the underlying issue causing accuracy ring resizing to lag behind input.

@picciano

picciano commented Apr 7, 2015

Copy link
Copy Markdown

If we could set the color (tint color?) of the location annotation from the client, that would be awesome. Just my $0.02.

@friedbunny

Copy link
Copy Markdown
Contributor Author

@picciano This and the current implementation both sync their color with the map view's tint color, which should probably be the same as the global app tint color. If you do something like mapView.tintColor = [UIColor whateverColor] that change will propagate to the location annotation and the ℹ️ button in the lower-right corner.

I think it might be interesting to add setters for the different layers' colors, but I'm going to leave any developer API choices to the professionals. ;)

@picciano

picciano commented Apr 7, 2015

Copy link
Copy Markdown

@friedbunny Ah, didn't even realize that, perfect.

@incanus

incanus commented Apr 7, 2015

Copy link
Copy Markdown
Contributor

@friedbunny

Copy link
Copy Markdown
Contributor Author

Here's how the heading indicator is looking:

screen-shot-2015-04-08-at-9 47 53-pm

Initially went with the MapKit heading mask image resources to see how they looked (fine), but decided to do it the hard way and draw it myself. Doesn't support changing the heading accuracy or tint yet.

@friedbunny

Copy link
Copy Markdown
Contributor Author

Now supports heading indicator tint adjustments and accuracy-based size changes.

This looks useful if there's ever a want to animate the heading accuracy changes.

@kristfal

Copy link
Copy Markdown

@friedbunny Tested this branch, and it is very neat – awesome!

Found one issue with heading tracking though. If you enable viewport rotation in the test app and rotate the device to landscape, the heading indicator is 90deg off. Resetting heading tacking after rotation fixes this.

@friedbunny

Copy link
Copy Markdown
Contributor Author

Thanks @kristfal, I'll fix that when I come back to this (which should be next week or so).

@kristfal

Copy link
Copy Markdown

@friedbunny Cheers, no need to rush it though ;)

@friedbunny

Copy link
Copy Markdown
Contributor Author

Rebased onto master 1d38b63 (git pull --rebase origin master) and force-pushed. Still working out my personal git strategy. 😐

@jfirebaugh

Copy link
Copy Markdown
Contributor

@1ec5 Can you do a code review here and merge when ready?

@1ec5 1ec5 self-assigned this May 13, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ivars should start with _.

@incanus

incanus commented May 13, 2015

Copy link
Copy Markdown
Contributor

While I love the looks of this, b2 is focused on features leading to increased adoption right now. This is a lot of code, makes things diverge from our iOS SDK codebase, and isn't directly related to adoption.

@friedbunny

Copy link
Copy Markdown
Contributor Author

@1ec5 Thanks, I'll push those changes shortly.

@incanus 😢 These changes are fairly significant, but self-contained. Even with the rotation fix I need to add, merging should be painless, whenever it does go in.

I've been using this in Treble and I'll keep putting it through its paces there, in the meantime. I don't think #1550 is related, but that will need investigating.

The only side effects I foresee at this point are potential performance issues with the switch to CALayer, but that hasn't been borne out in my use so far.

@incanus

incanus commented May 13, 2015

Copy link
Copy Markdown
Contributor

These changes are fairly significant, but self-contained.

Yeah, agreed. Nothing personal 😄 We need to get our b2 game rolling and ship quicker than b1, but once we get into that flow, we'll keep gobbling the submissions.

But even now, b2 talk is cart before horse...

@friedbunny

Copy link
Copy Markdown
Contributor Author

Rebased this on master @ af89714. Fixed heading indicator not rotating.

friedbunny added a commit that referenced this pull request Aug 6, 2015
Adds:
- Accuracy ring
- Heading indicator

Fixes:
- Pulse sync
- Visual appearance (removes slight aliasing)

Closes #1222 (squashed)
@mourner mourner assigned friedbunny and unassigned 1ec5 Aug 6, 2015
@friedbunny

Copy link
Copy Markdown
Contributor Author

Closed in favor of squash in #2010.

@friedbunny friedbunny closed this Aug 6, 2015
friedbunny added a commit that referenced this pull request Aug 10, 2015
Adds:
- Accuracy ring
- Heading indicator

Fixes:
- Pulse sync
- Visual appearance (removes slight aliasing)

Closes #1222 (squashed)
AndwareSsj pushed a commit to AndwareSsj/mapbox-gl-native that referenced this pull request Nov 6, 2015
Adds:
- Accuracy ring
- Heading indicator

Fixes:
- Pulse sync
- Visual appearance (removes slight aliasing)

Closes mapbox#1222 (squashed)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

iOS Mapbox Maps SDK for iOS

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants