Fix #1975 - improve animation fluidity and response

[adam-mapbox]

No description provided.

[tomtaylor]

@adam-mapbox this works beautifully for me!

[incanus]

Taking the discussion from problem (#1975 (comment)) to proposed fix here.

This looks great @adam-mapbox. It's only in the iOS code, which is good because:

1. It scopes changes to just that platform, and
2. It confirms that we're not dealing with a more insidious core issue.

It basically moves from UIView internal dirtying/refresh timing to external, metered CADisplayLink locked at 30Hz.

I can't speak to the std::mutex use yet (@jfirebaugh?) but one thing I'd change is maybe to rename the needsDisplay property since that mirrors UIView API and we could run into symbol problems (I think?).

There's only one final detail to think about, and that's our desired frame rate. Right now I capped it at 30. That works pretty well on almost all devices and in 95% of all cases. We may have select reasons to want to go up or down, though, in specific situations. For example on an older iPod touch or a crappy old iPhone, we may want it to be 20. And on a very fast machine under certain circumstances we may want to shoot for 60. I would also be in favor of making this a user-controllable setting, if we think that's wise.

Another set of questions/comments around the 30Hz:

1. I don't think we should expose a switch for this, but rather should determine if different frame rates should exist by device and handle it ourselves internally (which I also think we can punt on right now). Consumers are going to want a map and not have to care about the OpenGL internals.
2. Why 30Hz — just because we can and can save some mojo over 60Hz?

I think we can merge this today once we clear these up.

[mb12]

@adam-mapbox and @incanus mutex is not needed to synchronize accesses to _needsDisplay since both updateFromDisplayLink (CADisplay link is created on UIThread) and invalidate are called on UI thread. We can just put the MGLAssertIsMainThread in updateFromDisplayLink as well.

[incanus]

Made some minor cleanups & changes per @mb12's (accurate) comment, as well as more clearly indicating what we're doing with regard to FPS.

Will do some testing on this, particularly with @tomtaylor's app.

[jfirebaugh]

This is great. Do we have a similar FPS limiter on Android @ljbade?

This should go in ASAP. As a future item, is there a standard way to dynamically adjust the frame rate to the maximum that can be comfortably sustained? I believe that e.g. browsers try to do 60hz, and if that is janky, drop to 30hz.

[incanus]

I think we'd have to build out some FPS introspection tools manually to look at this. IMHO we should ticket this for future investigation.

[adam-mapbox]

The answer to the 30 FPS question is simply that I was on an iPod Touch and it can't handle 60 FPS. As to doing automatic rate-limiting, yeah, I think it's a distinct possibility. What makes it hard in our case is that we're so dynamic (because of data and usage patterns). With rate limiting, the challenge is how to go back up. If we start at 60, we will almost always - on most devices - drop to 30 under load, and then we'll have no way to get back up to 60. Also, just visually speaking, 30 looks pretty damn good - when it's smooth (which it now is, mostly).

[adam-mapbox]

Just wanted to say a public thanks to @tomtaylor for sending in his code, it helped a great deal with tracking this down!

[incanus]

🎉

[tomtaylor]

💯 thanks everyone!

[ljbade]

@jfirebaugh Opened Android ticket in #2931

[tmpsantos]

I've been debugging Mapbox GL Native performance too and I found ways of improving performance that are somehow related to what was described here but not exactly implies reducing the frame rate to 30FPS. Instead, I'm throttling how frequently we update the tile state.

I have a branch (not final) and I would love we someone could try it:
https://github.com/mapbox/mapbox-gl-native/commits/throttle_update

This commit here explains on the changelog the motivation behind the throttling:
44bde0b

[incanus]

Since this PR ticket is merged, moving @kkaefer's concerns in #2922 (comment) to #2985 for further investigation.