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

[Native Animated] Support for animated tracking in native driver #17896

Closed
wants to merge 12 commits into from
Closed
Prev Previous commit
Next Next commit
Tests for animated tracking on Android
kmagiera committed Feb 9, 2018
commit 0590afaddcb244540ad5dbaf4faca04e63788f12
Original file line number Diff line number Diff line change
@@ -57,7 +57,7 @@ public void runAnimationStep(long frameTimeNanos) {
mFromValue = mAnimatedValue.mValue;
}
long timeFromStartMillis = (frameTimeNanos - mStartFrameTimeNanos) / 1000000;
int frameIndex = (int) (timeFromStartMillis / FRAME_TIME_MILLIS);
int frameIndex = (int) Math.round(timeFromStartMillis / FRAME_TIME_MILLIS);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was this change needed? Does it match the iOS / JS implementation better like this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tried to explain that in my PR description, you might have missed it as it is at the very end:

In addition this PR fixes an issue with frames animation driver on Android that because of rounding issues was taking one extra frame to start. Because of that change I had to update a number of Android unit tests that were relying on that behavior and running that one additional animation step prior to performing checks.

So basically because of unfortunate rounding here frames driver on android would always "play" first frame twice. This could also be seen in all the unit tests where we run update loop twice at the beginning for all the framedriver tests. I am also updating these in the PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this line is starting to cause IllegalStateException in apps-

Caused by: java.lang.IllegalStateException: Calculated frame index should never be lower than 0
	at com.facebook.react.animated.FrameBasedAnimationDriver.runAnimationStep(FrameBasedAnimationDriver.java:60)
	at com.facebook.react.animated.NativeAnimatedNodesManager.runUpdates(NativeAnimatedNodesManager.java:444)
	at com.facebook.react.animated.NativeAnimatedModule$1.doFrameGuarded(NativeAnimatedModule.java:100)
	at com.facebook.react.uimanager.GuardedFrameCallback.doFrame(GuardedFrameCallback.java:29)

Is there a reason why timeFromStartMills will be negative ? The exception seems to have started with this PR merged in

if (frameIndex < 0) {
throw new IllegalStateException("Calculated frame index should never be lower than 0");
} else if (mHasFinished) {
@@ -68,7 +68,7 @@ public void runAnimationStep(long frameTimeNanos) {
if (frameIndex >= mFrames.length - 1) {
nextValue = mToValue;
if (mIterations == -1 || mCurrentLoop < mIterations) { // looping animation, return to start
mStartFrameTimeNanos = frameTimeNanos;
mStartFrameTimeNanos = frameTimeNanos + ((long) FRAME_TIME_MILLIS) * 1000000L;
Copy link
Contributor

Choose a reason for hiding this comment

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

@axemclion @kmagiera I think this line could be the culprit. I guess it could be possible that mStartFrameTimeNanos is bigger than the curent clock time.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this would be better as mStartFrameTimeNanos = -1;.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I remember there was some other problem when I was setting it to -1 but can't remember what exactly. Will try that and send a new PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, ok I remember now, I didn't want mFromValue to be updated with the next loop and that happens when mStartFrameTimeNanos is -1. Will add another check for this case and send a PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Submitted it here: #18061

mCurrentLoop++;
} else { // animation has completed, no more frames left
mHasFinished = true;
Original file line number Diff line number Diff line change
@@ -171,11 +171,6 @@ public void testFramesAnimation() {
ArgumentCaptor<ReactStylesDiffMap> stylesCaptor =
ArgumentCaptor.forClass(ReactStylesDiffMap.class);

reset(mUIImplementationMock);
mNativeAnimatedNodesManager.runUpdates(nextFrameTime());
verify(mUIImplementationMock).synchronouslyUpdateViewOnUIThread(eq(1000), stylesCaptor.capture());
assertThat(stylesCaptor.getValue().getDouble("opacity", Double.NaN)).isEqualTo(0);

for (int i = 0; i < frames.size(); i++) {
reset(mUIImplementationMock);
mNativeAnimatedNodesManager.runUpdates(nextFrameTime());
@@ -205,11 +200,6 @@ public void testFramesAnimationLoopsFiveTimes() {
ArgumentCaptor<ReactStylesDiffMap> stylesCaptor =
ArgumentCaptor.forClass(ReactStylesDiffMap.class);

reset(mUIImplementationMock);
mNativeAnimatedNodesManager.runUpdates(nextFrameTime());
verify(mUIImplementationMock).synchronouslyUpdateViewOnUIThread(eq(1000), stylesCaptor.capture());
assertThat(stylesCaptor.getValue().getDouble("opacity", Double.NaN)).isEqualTo(0);

for (int iteration = 0; iteration < 5; iteration++) {
for (int i = 0; i < frames.size(); i++) {
reset(mUIImplementationMock);
@@ -270,9 +260,6 @@ public void testNodeValueListenerIfListening() {
JavaOnlyMap.of("type", "frames", "frames", frames, "toValue", 1d),
animationCallback);

mNativeAnimatedNodesManager.runUpdates(nextFrameTime());
verify(valueListener).onValueUpdate(eq(0d));

for (int i = 0; i < frames.size(); i++) {
reset(valueListener);
mNativeAnimatedNodesManager.runUpdates(nextFrameTime());
@@ -600,7 +587,6 @@ public void testAnimationCallbackFinish() {

reset(animationCallback);
mNativeAnimatedNodesManager.runUpdates(nextFrameTime());
mNativeAnimatedNodesManager.runUpdates(nextFrameTime());
verifyNoMoreInteractions(animationCallback);

reset(animationCallback);
@@ -629,10 +615,10 @@ private void createAnimatedGraphWithAdditionNode(
double secondValue) {
mNativeAnimatedNodesManager.createAnimatedNode(
1,
JavaOnlyMap.of("type", "value", "value", 100d, "offset", 0d));
JavaOnlyMap.of("type", "value", "value", firstValue, "offset", 0d));
mNativeAnimatedNodesManager.createAnimatedNode(
2,
JavaOnlyMap.of("type", "value", "value", 1000d, "offset", 0d));
JavaOnlyMap.of("type", "value", "value", secondValue, "offset", 0d));

mNativeAnimatedNodesManager.createAnimatedNode(
3,
@@ -648,7 +634,7 @@ private void createAnimatedGraphWithAdditionNode(
mNativeAnimatedNodesManager.connectAnimatedNodes(2, 3);
mNativeAnimatedNodesManager.connectAnimatedNodes(3, 4);
mNativeAnimatedNodesManager.connectAnimatedNodes(4, 5);
mNativeAnimatedNodesManager.connectAnimatedNodeToView(5, 50);
mNativeAnimatedNodesManager.connectAnimatedNodeToView(5, viewTag);
}

@Test
@@ -677,12 +663,6 @@ public void testAdditionNode() {
verify(mUIImplementationMock).synchronouslyUpdateViewOnUIThread(eq(50), stylesCaptor.capture());
assertThat(stylesCaptor.getValue().getDouble("translateX", Double.NaN)).isEqualTo(1100d);

reset(mUIImplementationMock);
mNativeAnimatedNodesManager.runUpdates(nextFrameTime());
verify(mUIImplementationMock)
.synchronouslyUpdateViewOnUIThread(eq(50), stylesCaptor.capture());
assertThat(stylesCaptor.getValue().getDouble("translateX", Double.NaN)).isEqualTo(1100d);

reset(mUIImplementationMock);
mNativeAnimatedNodesManager.runUpdates(nextFrameTime());
verify(mUIImplementationMock)
@@ -722,12 +702,6 @@ public void testViewReceiveUpdatesIfOneOfAnimationHasntStarted() {
verify(mUIImplementationMock).synchronouslyUpdateViewOnUIThread(eq(50), stylesCaptor.capture());
assertThat(stylesCaptor.getValue().getDouble("translateX", Double.NaN)).isEqualTo(1100d);

reset(mUIImplementationMock);
mNativeAnimatedNodesManager.runUpdates(nextFrameTime());
verify(mUIImplementationMock)
.synchronouslyUpdateViewOnUIThread(eq(50), stylesCaptor.capture());
assertThat(stylesCaptor.getValue().getDouble("translateX", Double.NaN)).isEqualTo(1100d);

reset(mUIImplementationMock);
mNativeAnimatedNodesManager.runUpdates(nextFrameTime());
verify(mUIImplementationMock)
@@ -777,11 +751,6 @@ public void testViewReceiveUpdatesWhenOneOfAnimationHasFinished() {
verify(mUIImplementationMock).synchronouslyUpdateViewOnUIThread(eq(50), stylesCaptor.capture());
assertThat(stylesCaptor.getValue().getDouble("translateX", Double.NaN)).isEqualTo(1100d);

reset(mUIImplementationMock);
mNativeAnimatedNodesManager.runUpdates(nextFrameTime());
verify(mUIImplementationMock).synchronouslyUpdateViewOnUIThread(eq(50), stylesCaptor.capture());
assertThat(stylesCaptor.getValue().getDouble("translateX", Double.NaN)).isEqualTo(1100d);

for (int i = 1; i < secondFrames.size(); i++) {
reset(mUIImplementationMock);
mNativeAnimatedNodesManager.runUpdates(nextFrameTime());
@@ -843,11 +812,6 @@ public void testMultiplicationNode() {
verify(mUIImplementationMock).synchronouslyUpdateViewOnUIThread(eq(50), stylesCaptor.capture());
assertThat(stylesCaptor.getValue().getDouble("translateX", Double.NaN)).isEqualTo(5d);

reset(mUIImplementationMock);
mNativeAnimatedNodesManager.runUpdates(nextFrameTime());
verify(mUIImplementationMock).synchronouslyUpdateViewOnUIThread(eq(50), stylesCaptor.capture());
assertThat(stylesCaptor.getValue().getDouble("translateX", Double.NaN)).isEqualTo(5d);

reset(mUIImplementationMock);
mNativeAnimatedNodesManager.runUpdates(nextFrameTime());
verify(mUIImplementationMock).synchronouslyUpdateViewOnUIThread(eq(50), stylesCaptor.capture());
@@ -949,11 +913,6 @@ public void testInterpolationNode() {
ArgumentCaptor<ReactStylesDiffMap> stylesCaptor =
ArgumentCaptor.forClass(ReactStylesDiffMap.class);

reset(mUIImplementationMock);
mNativeAnimatedNodesManager.runUpdates(nextFrameTime());
verify(mUIImplementationMock).synchronouslyUpdateViewOnUIThread(eq(50), stylesCaptor.capture());
assertThat(stylesCaptor.getValue().getDouble("opacity", Double.NaN)).isEqualTo(0d);

for (int i = 0; i < frames.size(); i++) {
reset(mUIImplementationMock);
mNativeAnimatedNodesManager.runUpdates(nextFrameTime());
@@ -1088,11 +1047,6 @@ public void testRestoreDefaultProps() {
ArgumentCaptor<ReactStylesDiffMap> stylesCaptor =
ArgumentCaptor.forClass(ReactStylesDiffMap.class);

reset(mUIImplementationMock);
mNativeAnimatedNodesManager.runUpdates(nextFrameTime());
verify(mUIImplementationMock).synchronouslyUpdateViewOnUIThread(eq(viewTag), stylesCaptor.capture());
assertThat(stylesCaptor.getValue().getDouble("opacity", Double.NaN)).isEqualTo(1);

for (int i = 0; i < frames.size(); i++) {
reset(mUIImplementationMock);
mNativeAnimatedNodesManager.runUpdates(nextFrameTime());
@@ -1106,4 +1060,231 @@ public void testRestoreDefaultProps() {
verify(mUIImplementationMock).synchronouslyUpdateViewOnUIThread(eq(viewTag), stylesCaptor.capture());
assertThat(stylesCaptor.getValue().isNull("opacity"));
}


/**
* Creates a following graph of nodes:
* Value(3, initialValue) ----> Style(4) ---> Props(5) ---> View(viewTag)
*
* Value(3) is set to track Value(1) via Tracking(2) node with the provided animation config
*/
private void createAnimatedGraphWithTrackingNode(
int viewTag,
double initialValue,
JavaOnlyMap animationConfig) {
mNativeAnimatedNodesManager.createAnimatedNode(
1,
JavaOnlyMap.of("type", "value", "value", initialValue, "offset", 0d));
mNativeAnimatedNodesManager.createAnimatedNode(
3,
JavaOnlyMap.of("type", "value", "value", initialValue, "offset", 0d));

mNativeAnimatedNodesManager.createAnimatedNode(
2,
JavaOnlyMap.of("type", "tracking", "animationId", 70, "value", 3, "toValue", 1, "animationConfig", animationConfig));

mNativeAnimatedNodesManager.createAnimatedNode(
4,
JavaOnlyMap.of("type", "style", "style", JavaOnlyMap.of("translateX", 3)));
mNativeAnimatedNodesManager.createAnimatedNode(
5,
JavaOnlyMap.of("type", "props", "props", JavaOnlyMap.of("style", 4)));
mNativeAnimatedNodesManager.connectAnimatedNodes(1, 2);
mNativeAnimatedNodesManager.connectAnimatedNodes(3, 4);
mNativeAnimatedNodesManager.connectAnimatedNodes(4, 5);
mNativeAnimatedNodesManager.connectAnimatedNodeToView(5, viewTag);
}

/**
* In this test we verify that when value is being tracked we can update destination value in the
* middle of ongoing animation and the animation will update and animate to the new spot. This is
* tested using simple 5 frame backed timing animation.
*/
@Test
public void testTracking() {
JavaOnlyArray frames = JavaOnlyArray.of(0d, 0.25d, 0.5d, 0.75d, 1d);
JavaOnlyMap animationConfig = JavaOnlyMap.of("type", "frames", "frames", frames);

createAnimatedGraphWithTrackingNode(1000, 0d, animationConfig);

ArgumentCaptor<ReactStylesDiffMap> stylesCaptor =
ArgumentCaptor.forClass(ReactStylesDiffMap.class);

reset(mUIImplementationMock);
mNativeAnimatedNodesManager.runUpdates(nextFrameTime());
verify(mUIImplementationMock).synchronouslyUpdateViewOnUIThread(eq(1000), stylesCaptor.capture());
assertThat(stylesCaptor.getValue().getDouble("translateX", Double.NaN)).isEqualTo(0d);

// update "toValue" to 100, we expect tracking animation to animate now from 0 to 100 in 5 steps
mNativeAnimatedNodesManager.setAnimatedNodeValue(1, 100d);
mNativeAnimatedNodesManager.runUpdates(nextFrameTime()); // kick off the animation

for (int i = 0; i < frames.size(); i++) {
reset(mUIImplementationMock);
mNativeAnimatedNodesManager.runUpdates(nextFrameTime());
verify(mUIImplementationMock)
.synchronouslyUpdateViewOnUIThread(eq(1000), stylesCaptor.capture());
assertThat(stylesCaptor.getValue().getDouble("translateX", Double.NaN))
.isEqualTo(frames.getDouble(i) * 100d);
}

// update "toValue" to 0 but run only two frames from the animation,
// we expect tracking animation to animate now from 100 to 75
mNativeAnimatedNodesManager.setAnimatedNodeValue(1, 0d);
mNativeAnimatedNodesManager.runUpdates(nextFrameTime()); // kick off the animation

for (int i = 0; i < 2; i++) {
reset(mUIImplementationMock);
mNativeAnimatedNodesManager.runUpdates(nextFrameTime());
verify(mUIImplementationMock)
.synchronouslyUpdateViewOnUIThread(eq(1000), stylesCaptor.capture());
assertThat(stylesCaptor.getValue().getDouble("translateX", Double.NaN))
.isEqualTo(100d * (1d - frames.getDouble(i)));
}

// at this point we expect tracking value to be 25
assertThat(((ValueAnimatedNode) mNativeAnimatedNodesManager.getNodeById(3)).getValue())
.isEqualTo(75d);

// we update "toValue" again to 100 and expect the animation to restart from the current place
mNativeAnimatedNodesManager.setAnimatedNodeValue(1, 100d);
mNativeAnimatedNodesManager.runUpdates(nextFrameTime()); // kick off the animation

for (int i = 0; i < frames.size(); i++) {
reset(mUIImplementationMock);
mNativeAnimatedNodesManager.runUpdates(nextFrameTime());
verify(mUIImplementationMock)
.synchronouslyUpdateViewOnUIThread(eq(1000), stylesCaptor.capture());
assertThat(stylesCaptor.getValue().getDouble("translateX", Double.NaN))
.isEqualTo(50d + 50d * frames.getDouble(i));
}
}

/**
* In this test we verify that when tracking is set up for a given animated node and when the
* animation settles it will not be registered as an active animation and therefore will not
* consume resources on running the animation that has already completed. Then we verify that when
* the value updates the animation will resume as expected and the complete again when reaches the
* end.
*/
@Test
public void testTrackingPausesWhenEndValueIsReached() {
JavaOnlyArray frames = JavaOnlyArray.of(0d, 0.5d, 1d);
JavaOnlyMap animationConfig = JavaOnlyMap.of("type", "frames", "frames", frames);

createAnimatedGraphWithTrackingNode(1000, 0d, animationConfig);
mNativeAnimatedNodesManager.setAnimatedNodeValue(1, 100d);
mNativeAnimatedNodesManager.runUpdates(nextFrameTime()); // make sure animation starts

reset(mUIImplementationMock);
for (int i = 0; i < frames.size(); i++) {
assertThat(mNativeAnimatedNodesManager.hasActiveAnimations()).isTrue();
mNativeAnimatedNodesManager.runUpdates(nextFrameTime());
}
verify(mUIImplementationMock, times(frames.size()))
.synchronouslyUpdateViewOnUIThread(eq(1000), any(ReactStylesDiffMap.class));

// the animation has completed, we expect no updates to be done
reset(mUIImplementationMock);
assertThat(mNativeAnimatedNodesManager.hasActiveAnimations()).isFalse();
mNativeAnimatedNodesManager.runUpdates(nextFrameTime());
verifyNoMoreInteractions(mUIImplementationMock);


// we update end value and expect the animation to restart
mNativeAnimatedNodesManager.setAnimatedNodeValue(1, 200d);
mNativeAnimatedNodesManager.runUpdates(nextFrameTime()); // make sure animation starts

reset(mUIImplementationMock);
for (int i = 0; i < frames.size(); i++) {
assertThat(mNativeAnimatedNodesManager.hasActiveAnimations()).isTrue();
mNativeAnimatedNodesManager.runUpdates(nextFrameTime());
}
verify(mUIImplementationMock, times(frames.size()))
.synchronouslyUpdateViewOnUIThread(eq(1000), any(ReactStylesDiffMap.class));

// the animation has completed, we expect no updates to be done
reset(mUIImplementationMock);
assertThat(mNativeAnimatedNodesManager.hasActiveAnimations()).isFalse();
mNativeAnimatedNodesManager.runUpdates(nextFrameTime());
verifyNoMoreInteractions(mUIImplementationMock);
}

/**
* In this test we verify that when tracking is configured to use spring animation and when the
* destination value updates the current speed of the animated value will be taken into account
* while updating the spring animation and it will smoothly transition to the new end value.
*/
@Test
public void testSpringTrackingRetainsSpeed() {
// this spring config correspomds to tension 20 and friction 0.5 which makes the spring settle
// very slowly
JavaOnlyMap springConfig = JavaOnlyMap.of(
"type",
"spring",
"restSpeedThreshold",
0.001,
"mass",
1d,
"restDisplacementThreshold",
0.001,
"initialVelocity",
0.5d,
"damping",
2.5,
"stiffness",
157.8,
"overshootClamping",
false);

createAnimatedGraphWithTrackingNode(1000, 0d, springConfig);

ArgumentCaptor<ReactStylesDiffMap> stylesCaptor =
ArgumentCaptor.forClass(ReactStylesDiffMap.class);

// update "toValue" to 1, we expect tracking animation to animate now from 0 to 1
mNativeAnimatedNodesManager.setAnimatedNodeValue(1, 1d);
mNativeAnimatedNodesManager.runUpdates(nextFrameTime());
mNativeAnimatedNodesManager.runUpdates(nextFrameTime());

// we run several steps of animation until the value starts bouncing, has negative speed and
// passes the final point (that is 1) while going backwards
boolean isBoucingBack = false;
double previousValue = ((ValueAnimatedNode) mNativeAnimatedNodesManager.getNodeById(3)).getValue();
for (int maxFrames = 500; maxFrames > 0; maxFrames--) {
mNativeAnimatedNodesManager.runUpdates(nextFrameTime());
double currentValue = ((ValueAnimatedNode) mNativeAnimatedNodesManager.getNodeById(3)).getValue();
if (previousValue >= 1d && currentValue < 1d) {
isBoucingBack = true;
break;
}
previousValue = currentValue;
}
assertThat(isBoucingBack).isTrue();

// we now update "toValue" to 1.5 but since the value have negative speed and has also pretty
// low friction we expect it to keep going in the opposite direction for a few more frames
mNativeAnimatedNodesManager.setAnimatedNodeValue(1, 1.5d);
int bounceBackInitialFrames = 0;
boolean hasTurnedForward = false;

// we run 8 seconds of animation
for (int i = 0; i < 8 * 60; i++) {
mNativeAnimatedNodesManager.runUpdates(nextFrameTime());
double currentValue = ((ValueAnimatedNode) mNativeAnimatedNodesManager.getNodeById(3)).getValue();
if (!hasTurnedForward) {
if (currentValue <= previousValue) {
bounceBackInitialFrames++;
} else {
hasTurnedForward = true;
}
}
previousValue = currentValue;
}
assertThat(hasTurnedForward).isEqualTo(true);
assertThat(bounceBackInitialFrames).isGreaterThan(4);

// we verify that the value settled at 2
assertThat(previousValue).isEqualTo(1.5d);
}
}