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 all 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
99 changes: 49 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 Down Expand Up @@ -58,6 +59,8 @@
private final Map<String, Map<String, String>> mCustomEventTypes;
private final UIImplementation mUIImplementation;
private int mAnimatedGraphBFSColor = 0;
// Used to avoid allocating a new array on every frame in runUpdates.
private final List<AnimatedNode> mRunUpdateNodeList = new ArrayList<>();

public NativeAnimatedNodesManager(UIManagerModule uiManager) {
mUIImplementation = uiManager.getUIImplementation();
Expand Down Expand Up @@ -334,7 +337,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));
Copy link
Contributor

@astreet astreet Feb 14, 2017

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?

Copy link
Contributor Author

@janicduplessis janicduplessis Feb 14, 2017

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 +357,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);
mRunUpdateNodeList.add(node);
}

// Clean mUpdatedNodes queue
mUpdatedNodes.clear();

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

updateNodes(mRunUpdateNodeList);
mRunUpdateNodeList.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 +412,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 +451,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 +502,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);
}
}
}
}
}