Fix camera animators ordering on Android 6 and lower #597
Conversation
zeac
force-pushed
the
596-fix-camera-issue
branch
from
b8aa498
to
5fc6d28
Compare
...in-animation/src/test/java/com/mapbox/maps/plugin/animation/CameraAnimationsListenersTest.kt
Outdated
Show resolved
Hide resolved
| assertEquals(10.0, updateList[1].pitch!!, EPS) | ||
| // Both, tick and bearing are updated to the start values at first tick. | ||
| assertEquals(20.0, updateList.first().bearing!!, EPS) | ||
| assertEquals(10.0, updateList.first().pitch!!, EPS) |
There was a problem hiding this comment.
The resulting behavior seems like more correct that the previous one.
There was a problem hiding this comment.
Originally, the test was added here - #352.
The problem i am trying to fix is similar, this solution covers them both.
zeac
force-pushed
the
596-fix-camera-issue
branch
6 times, most recently
from
4508752
to
2e5e990
Compare
plugin-animation/src/main/java/com/mapbox/maps/plugin/animation/CameraAnimationsPluginImpl.kt
Outdated
Show resolved
Hide resolved
plugin-animation/src/main/java/com/mapbox/maps/plugin/animation/CameraAnimationsPluginImpl.kt
Show resolved
Hide resolved
plugin-animation/src/main/java/com/mapbox/maps/plugin/animation/CameraAnimationsPluginImpl.kt
Outdated
Show resolved
Hide resolved
plugin-animation/src/main/java/com/mapbox/maps/plugin/animation/animator/CameraAnimator.kt
Outdated
Show resolved
Hide resolved
| if (animator.haveUserListeners) { | ||
| // If the animator have third-party listeners camera changes must be applied immediately | ||
| // to be seen from the listeners. | ||
| if (commitScheduled) handler.removeCallbacks(commitChangesRunnable) |
There was a problem hiding this comment.
nit: let's use {} even for single line and separate with new lines
There was a problem hiding this comment.
Generally I'm seeing
if (commitScheduled) handler.removeCallbacks(commitChangesRunnable)
commitChangesRunnable.run()
for 3 times, let's move it to private function
There was a problem hiding this comment.
@zeac so do I get it correctly that accumulating camera optimization is not working if we do have userUpdateListeners? Could there be a better way?
thanks to @LukasPaczos for stating it out.
There was a problem hiding this comment.
Accumulated camera changes is flushed after value updates from Animators with third-party listeners only, so this optimization still works for NavigationCamera or GesturePlugin animations.
The only other way i see is to call all userUpdateListeners manually after camera is updated. I'm not sure that it is a better way.
plugin-animation/src/main/java/com/mapbox/maps/plugin/animation/CameraAnimationsPluginImpl.kt
Show resolved
Hide resolved
zeac
force-pushed
the
596-fix-camera-issue
branch
from
2e5e990
to
120568e
Compare
kiryldz
changed the title
Pull request checklist:
mapbox-maps-androidchangelog:<changelog>Fix issue with camera animators ordering on Android 6 and lower by revisiting overall approach of applying accumulated camera changes.</changelog>.Fixes: #596
Summary of changes
The root of the problem is in the order of callbacks from ValueAnimators, that is different on Android 6. This PR changes core logic of applying animators making it easier to support and delegating updating accumulated
CameraOptionstoHandlerinstead of relying on update of "oldest" animator (which as said before is not working as expected on Android 6 and below). This should not affect camera animation in any way, the only possible drawback is applying camera changes on each animator update if user has addedValueAnimator.AnimatorUpdateListenerinstead of accumulating which should be a pretty rare use-case.