Skip to content

Fix native animated event lag on Android #11994

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.

Sign up for GitHub

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

Closed
janicduplessis wants to merge 6 commits into from
Closed

Fix native animated event lag on Android #11994

Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
5861677
Fix native animated event lag on Android
janicduplessis Jan 19, 2017
ecf6558
Merge branch 'master' into native-animed-event-fix
janicduplessis Feb 3, 2017
46127a6
Merge branch 'master' into native-animed-event-fix
janicduplessis Feb 7, 2017
d742c41
Merge branch 'master' into native-animed-event-fix
janicduplessis Feb 13, 2017
6acd26c
Use static list to avoid allocations
janicduplessis Feb 17, 2017
790d88d
static -> ivar
janicduplessis Feb 20, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
100 changes: 50 additions & 50 deletions ReactAndroid/src/main/java/com/facebook/react/animated/NativeAnimatedNodesManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@

import java.util.ArrayDeque;
import java.util.ArrayList;
import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
Expand All @@ -51,6 +52,9 @@
*/
/*package*/ class NativeAnimatedNodesManager implements EventDispatcherListener {

// Used to avoid allocating a new array on every frame in runUpdates.
private final static List<AnimatedNode> sRunUpdateNodeList = new ArrayList<>();

@ide ide Feb 17, 2017

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.

can you make this an instance variable unless it truly needs to be static and shared across NodesManagers?

@janicduplessis janicduplessis Feb 17, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Both would work, having it as a static ivar avoids having multiple copies in case there are multiple instances of the animated module (like in exponent). The array gets cleaned up after each invocation of runUpdates and this always runs from the same thread so it is safe.

If you feel it should be a normal ivar I don't really mind, just saves a few bytes in rare situations.

@astreet astreet Feb 20, 2017

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.

Yeah, I'd prefer an ivar in case someone somewhere decides to have multiple RN instances running at the same time :)


private final SparseArray<AnimatedNode> mAnimatedNodes = new SparseArray<>();
private final SparseArray<AnimationDriver> mActiveAnimations = new SparseArray<>();
private final SparseArray<AnimatedNode> mUpdatedNodes = new SparseArray<>();
Expand Down Expand Up @@ -334,7 +338,8 @@ public void onEventDispatch(Event event) {
EventAnimationDriver eventDriver = mEventDrivers.get(event.getViewTag() + eventName);
if (eventDriver != null) {
event.dispatch(eventDriver);
mUpdatedNodes.put(eventDriver.mValueNode.mTag, eventDriver.mValueNode);

updateNodes(Collections.singletonList((AnimatedNode) eventDriver.mValueNode));

@astreet astreet Feb 14, 2017

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.

You have an updateNodes here when you receive an event, and then in the same frame you could runUpdates and updateNodes again: is that correct?

@janicduplessis janicduplessis Feb 14, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

There are cases where this won't happen in the same frame, like during momentum scrolling, this is why we need to update it immediatly instead of relying on runUpdates which is called on each frame by the Choreographer.

These functions only update nodes that are marked as needing an update so it won't update twice in a frame.

}
}
}
Expand All @@ -353,15 +358,52 @@ public void onEventDispatch(Event event) {
*/
public void runUpdates(long frameTimeNanos) {
UiThreadUtil.assertOnUiThread();
boolean hasFinishedAnimations = false;

for (int i = 0; i < mUpdatedNodes.size(); i++) {
AnimatedNode node = mUpdatedNodes.valueAt(i);
sRunUpdateNodeList.add(node);
}

// Clean mUpdatedNodes queue
mUpdatedNodes.clear();

for (int i = 0; i < mActiveAnimations.size(); i++) {
AnimationDriver animation = mActiveAnimations.valueAt(i);
animation.runAnimationStep(frameTimeNanos);

@astreet astreet Feb 14, 2017

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.

I'm not familiar with this code, but this seems odd: why are we running the animation and then updating all the nodes for the frame below on line L376? I.e., shouldn't we be updating first?

@janicduplessis janicduplessis Feb 14, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This code didn't change, I just moved it in it's own method. How it works is we run the animation drivers first, which update the value nodes then update all the other nodes that are connected to these updated value nodes, which will cause props nodes to update their connected view.

AnimatedNode valueNode = animation.mAnimatedValue;
sRunUpdateNodeList.add(valueNode);
if (animation.mHasFinished) {
hasFinishedAnimations = true;
}
}

updateNodes(sRunUpdateNodeList);
sRunUpdateNodeList.clear();

// Cleanup finished animations. Iterate over the array of animations and override ones that has
// finished, then resize `mActiveAnimations`.
if (hasFinishedAnimations) {
for (int i = mActiveAnimations.size() - 1; i >= 0; i--) {
AnimationDriver animation = mActiveAnimations.valueAt(i);
if (animation.mHasFinished) {
WritableMap endCallbackResponse = Arguments.createMap();
endCallbackResponse.putBoolean("finished", true);
animation.mEndCallback.invoke(endCallbackResponse);
mActiveAnimations.removeAt(i);
}
}
}
}

private void updateNodes(List<AnimatedNode> nodes) {
int activeNodesCount = 0;
int updatedNodesCount = 0;
boolean hasFinishedAnimations = false;

// STEP 1.
// BFS over graph of nodes starting from ones from `mUpdatedNodes` and ones that are attached to
// active animations (from `mActiveAnimations)`. Update `mIncomingNodes` attribute for each node
// during that BFS. Store number of visited nodes in `activeNodesCount`. We "execute" active
// animations as a part of this step.
// BFS over graph of nodes. Update `mIncomingNodes` attribute for each node during that BFS.
// Store number of visited nodes in `activeNodesCount`. We "execute" active animations as a part
// of this step.

mAnimatedGraphBFSColor++; /* use new color */
if (mAnimatedGraphBFSColor == AnimatedNode.INITIAL_BFS_COLOR) {
Expand All @@ -371,29 +413,14 @@ public void runUpdates(long frameTimeNanos) {
}

Queue<AnimatedNode> nodesQueue = new ArrayDeque<>();
for (int i = 0; i < mUpdatedNodes.size(); i++) {
AnimatedNode node = mUpdatedNodes.valueAt(i);
for (AnimatedNode node : nodes) {
if (node.mBFSColor != mAnimatedGraphBFSColor) {
node.mBFSColor = mAnimatedGraphBFSColor;
activeNodesCount++;
nodesQueue.add(node);
}
}

for (int i = 0; i < mActiveAnimations.size(); i++) {
AnimationDriver animation = mActiveAnimations.valueAt(i);
animation.runAnimationStep(frameTimeNanos);
AnimatedNode valueNode = animation.mAnimatedValue;
if (valueNode.mBFSColor != mAnimatedGraphBFSColor) {
valueNode.mBFSColor = mAnimatedGraphBFSColor;
activeNodesCount++;
nodesQueue.add(valueNode);
}
if (animation.mHasFinished) {
hasFinishedAnimations = true;
}
}

while (!nodesQueue.isEmpty()) {
AnimatedNode nextNode = nodesQueue.poll();
if (nextNode.mChildren != null) {
Expand Down Expand Up @@ -425,23 +452,13 @@ public void runUpdates(long frameTimeNanos) {

// find nodes with zero "incoming nodes", those can be either nodes from `mUpdatedNodes` or
// ones connected to active animations
for (int i = 0; i < mUpdatedNodes.size(); i++) {
AnimatedNode node = mUpdatedNodes.valueAt(i);
for (AnimatedNode node : nodes) {
if (node.mActiveIncomingNodes == 0 && node.mBFSColor != mAnimatedGraphBFSColor) {
node.mBFSColor = mAnimatedGraphBFSColor;
updatedNodesCount++;
nodesQueue.add(node);
}
}
for (int i = 0; i < mActiveAnimations.size(); i++) {
AnimationDriver animation = mActiveAnimations.valueAt(i);
AnimatedNode valueNode = animation.mAnimatedValue;
if (valueNode.mActiveIncomingNodes == 0 && valueNode.mBFSColor != mAnimatedGraphBFSColor) {
valueNode.mBFSColor = mAnimatedGraphBFSColor;
updatedNodesCount++;
nodesQueue.add(valueNode);
}
}

// Run main "update" loop
while (!nodesQueue.isEmpty()) {
Expand Down Expand Up @@ -486,22 +503,5 @@ public void runUpdates(long frameTimeNanos) {
throw new IllegalStateException("Looks like animated nodes graph has cycles, there are "
+ activeNodesCount + " but toposort visited only " + updatedNodesCount);
}

// Clean mUpdatedNodes queue
mUpdatedNodes.clear();

// Cleanup finished animations. Iterate over the array of animations and override ones that has
// finished, then resize `mActiveAnimations`.
if (hasFinishedAnimations) {
for (int i = mActiveAnimations.size() - 1; i >= 0; i--) {
AnimationDriver animation = mActiveAnimations.valueAt(i);
if (animation.mHasFinished) {
WritableMap endCallbackResponse = Arguments.createMap();
endCallbackResponse.putBoolean("finished", true);
animation.mEndCallback.invoke(endCallbackResponse);
mActiveAnimations.removeAt(i);
}
}
}
}
}