Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add animation utilities + demo. #17

Closed
wants to merge 6 commits into from
Closed

Conversation

broady
Copy link
Contributor

@broady broady commented Aug 21, 2013

No description provided.

@broady
Copy link
Contributor Author

broady commented Aug 21, 2013

@cyrilmottier haha - looking at logcat while running the demo is a bit sad. So many GC ops :-(

@cyrilmottier
Copy link

@broady Looks like we have the exact same problems then ... I've taken some time a while ago to tune the snippet of code and reduce the GC ops but unfortunately it's kinda hard/impossible because of the nature of the Google Maps Android API v2.

First, the LatLngEvaluator (or LatLngTypeEvaluator in your case) allocates a new LatLng object for each frame. This should be avoided but LatLng being immutable prevent us from working around it.

Secondly, when animating multiple Markers, I personally create a new ObjectAnimator for each Marker. We could create a single ValueAnimator and listen to frame "ticks" in order to move all Markers. I tried to do that in my app but I had some issues I don't remember and, to be honest, I'm not sure it would be such a performance enhancement.

Finally, IPCs create tons of objects. This is probably the worst part of animating Markers and I don't think of a way to work around that :(

The result is rather disappointing from a UI point of view. I mean, the animations are slow and janky which is a real nightmare to me :) I hate janky UIs.

@broady
Copy link
Contributor Author

broady commented Aug 21, 2013

+1 to everything you said. Fixing any of that would either mean completely changing the API, or just adding complexity (and many ways to do one thing). Any good suggestions are welcomed :)

There's also another source of jank: the map is drawn in a different thread (GL thread, not main Android UI thread). The Animator's loop is tied to the main thread.

@cyrilmottier
Copy link

Yep, Forgot about this too. I definitely have a take a look back at my code and start having fun with Google Maps Android API v2 again (haven't used with it since for 6 months :s). Maybe there is a way to smooth things up ...

…Animator. ObjectAnimator is final, so if we have more flexibility if we have a better animation strategy in the future.
@cyrilmottier
Copy link

I've always been afraid of using a TextureView/SurfaceView rather that a regular ViewGroup with View. I'm sure, there are good reasons behind using such an architecture in Google Maps Android API v2 (surely performance) but it brings some other issues (no way to manually draw - i.e. Canvasdrawing style - on the map, no way to animate subviews, info windows are actually a snapshot of the actual Views, etc.). Because of that, developers are basically using a completely different rendering pipeline to the one the are used to.

If I remember correctly my old days on iOS, the MKMapView is a regular UIView which allows developers to do almost anything they could have done with any other View in the framework.

@broady
Copy link
Contributor Author

broady commented Aug 21, 2013

It could certainly be attributed to a preference for fast vector map rendering (OpenGL) over performance of other (developer-provided) overlays. It delivers a good (and consistent w/ Maps app) experience for the majority of use cases. Unfortunately, outside of that, there's a much steeper learning curve.

I don't have firsthand experience with MKMapView, but yes, that sounds right. I haven't had a look at the API in iOS7, but I would be interested to see whether it has similar limitations to Android Maps API v2.

@broady
Copy link
Contributor Author

broady commented Oct 3, 2013

Holding off on this. Lots of questions around backwards compatibility:

  • Should we use nineoldandroids (NOA)?
  • Should this library depend on it?
  • Should animation be disabled if NOA isn't on the classpath?
  • If not, do we return some sort of "Union" object that either has an Android Animator or NOA Animator object?
  • ... or do we return some sort of wrapper object that looks like either, and delegates appropriately?

@cyrilmottier
Copy link

@broady FYI, here is what I've done for AVélov:

The first version of AVélov bundle with clustering was completely backward compatible thanks to NOA. This was working like a charm but when I started investigating performance issues I noticed NOA was kind of slowing down animations. Without deep diving into the actual reasons (probably wrapping & object allocation), I decided to remove it and the animations were smoother ... or at least less janky :p. Hence, I'd say option 1 is bad.

AVélov now have a simple backward compatibility because it doesn't animate on pre-HC devices :).

Regarding android-maps-utils I have always considered a library must be as independent as possible. It should not rely on another library to work (this is the exact same reason I hate the fact GoogleMap#getMyLocation() has been deprecated). Ensuring android-maps-util is external libraries-free is a way to make it a simple drop&start coding library. As a consequence, option 2 is bad to my mind.

Personally, I'd go for the 5th option or simply no animation at all on pre-HC devices.

@broady
Copy link
Contributor Author

broady commented Oct 3, 2013

@cyrilmottier appreciate the comments. Your conclusion is what I lean towards.

There's also the possibility that we can do animation better without Android's animation framework, in which case returning an Android Animator object would be bad. #5 would be nice then, but you then lose compatibility with AnimatorSet or be wasteful and keep an Animator object around unnecessarily.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants