Skip to content

Commit

Permalink
Proper NativeAnimated node invalidation on Android
Browse files Browse the repository at this point in the history 
Summary:
This diff attempts to fix a number of Android native animation bugs related to incomplete node invalidation, e.g. #10657 (comment).

For full correctness, we should mark any node as needing update when it is:

- created
- updated (value nodes)
- attached to new parents
- detached from old parents
- attached to a view (prop nodes)

cc/ janicduplessis
Closes #10837

Differential Revision: D4166446

fbshipit-source-id: dbf6b9aa34439e286234627791bb7fef647c8396
  • Loading branch information
@ryangomba
ryangomba authored and Facebook Github Bot committed Nov 11, 2016
1 parent 617432c commit 6f5433f
Showing 1 changed file with 22 additions and 22 deletions.
44 changes: 22 additions & 22 deletions ReactAndroid/src/main/java/com/facebook/react/animated/NativeAnimatedNodesManager.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,8 @@
/*package*/ class NativeAnimatedNodesManager implements EventDispatcherListener {

private final SparseArray<AnimatedNode> mAnimatedNodes = new SparseArray<>();
private final ArrayList<AnimationDriver> mActiveAnimations = new ArrayList<>();
private final ArrayList<AnimatedNode> mUpdatedNodes = new ArrayList<>();
private final SparseArray<AnimationDriver> mActiveAnimations = new SparseArray<>();
private final SparseArray<AnimatedNode> mUpdatedNodes = new SparseArray<>();
private final Map<String, EventAnimationDriver> mEventDrivers = new HashMap<>();
private final Map<String, Map<String, String>> mCustomEventTypes;
private final UIImplementation mUIImplementation;
Expand All @@ -68,7 +68,7 @@ public NativeAnimatedNodesManager(UIManagerModule uiManager) {
}

public boolean hasActiveAnimations() {
return !mActiveAnimations.isEmpty() || !mUpdatedNodes.isEmpty();
return mActiveAnimations.size() > 0 || mUpdatedNodes.size() > 0;
}

public void createAnimatedNode(int tag, ReadableMap config) {
Expand All @@ -82,7 +82,6 @@ public void createAnimatedNode(int tag, ReadableMap config) {
node = new StyleAnimatedNode(config, this);
} else if ("value".equals(type)) {
node = new ValueAnimatedNode(config);
mUpdatedNodes.add(node);
} else if ("props".equals(type)) {
node = new PropsAnimatedNode(config, this);
} else if ("interpolation".equals(type)) {
Expand All @@ -104,10 +103,12 @@ public void createAnimatedNode(int tag, ReadableMap config) {
}
node.mTag = tag;
mAnimatedNodes.put(tag, node);
mUpdatedNodes.put(tag, node);
}

public void dropAnimatedNode(int tag) {
mAnimatedNodes.remove(tag);
mUpdatedNodes.remove(tag);
}

public void startListeningToAnimatedNodeValue(int tag, AnimatedNodeValueListener listener) {
Expand Down Expand Up @@ -135,7 +136,7 @@ public void setAnimatedNodeValue(int tag, double value) {
" does not exists or is not a 'value' node");
}
((ValueAnimatedNode) node).mValue = value;
mUpdatedNodes.add(node);
mUpdatedNodes.put(tag, node);
}

public void setAnimatedNodeOffset(int tag, double offset) {
Expand All @@ -145,7 +146,7 @@ public void setAnimatedNodeOffset(int tag, double offset) {
" does not exists or is not a 'value' node");
}
((ValueAnimatedNode) node).mOffset = offset;
mUpdatedNodes.add(node);
mUpdatedNodes.put(tag, node);
}

public void flattenAnimatedNodeOffset(int tag) {
Expand Down Expand Up @@ -194,7 +195,7 @@ public void startAnimatingNode(
animation.mId = animationId;
animation.mEndCallback = endCallback;
animation.mAnimatedValue = (ValueAnimatedNode) node;
mActiveAnimations.add(animation);
mActiveAnimations.put(animationId, animation);
}

public void stopAnimation(int animationId) {
Expand All @@ -203,13 +204,13 @@ public void stopAnimation(int animationId) {
// object map that would require additional memory just to support the use-case of stopping
// an animation
for (int i = 0; i < mActiveAnimations.size(); i++) {
AnimationDriver animation = mActiveAnimations.get(i);
AnimationDriver animation = mActiveAnimations.valueAt(i);
if (animation.mId == animationId) {
// Invoke animation end callback with {finished: false}
WritableMap endCallbackResponse = Arguments.createMap();
endCallbackResponse.putBoolean("finished", false);
animation.mEndCallback.invoke(endCallbackResponse);
mActiveAnimations.remove(i);
mActiveAnimations.removeAt(i);
return;
}
}
Expand All @@ -231,6 +232,7 @@ public void connectAnimatedNodes(int parentNodeTag, int childNodeTag) {
" does not exists");
}
parentNode.addChild(childNode);
mUpdatedNodes.put(childNodeTag, childNode);
}

public void disconnectAnimatedNodes(int parentNodeTag, int childNodeTag) {
Expand All @@ -245,6 +247,7 @@ public void disconnectAnimatedNodes(int parentNodeTag, int childNodeTag) {
" does not exists");
}
parentNode.removeChild(childNode);
mUpdatedNodes.put(childNodeTag, childNode);
}

public void connectAnimatedNodeToView(int animatedNodeTag, int viewTag) {
Expand All @@ -263,6 +266,7 @@ public void connectAnimatedNodeToView(int animatedNodeTag, int viewTag) {
"already attached to a view");
}
propsAnimatedNode.mConnectedViewTag = viewTag;
mUpdatedNodes.put(animatedNodeTag, node);
}

public void disconnectAnimatedNodeFromView(int animatedNodeTag, int viewTag) {
Expand Down Expand Up @@ -327,7 +331,7 @@ public boolean onEventDispatch(Event event) {
EventAnimationDriver eventDriver = mEventDrivers.get(event.getViewTag() + eventName);
if (eventDriver != null) {
event.dispatch(eventDriver);
mUpdatedNodes.add(eventDriver.mValueNode);
mUpdatedNodes.put(eventDriver.mValueNode.mTag, eventDriver.mValueNode);
return true;
}
}
Expand Down Expand Up @@ -368,7 +372,7 @@ public void runUpdates(long frameTimeNanos) {

Queue<AnimatedNode> nodesQueue = new ArrayDeque<>();
for (int i = 0; i < mUpdatedNodes.size(); i++) {
AnimatedNode node = mUpdatedNodes.get(i);
AnimatedNode node = mUpdatedNodes.valueAt(i);
if (node.mBFSColor != mAnimatedGraphBFSColor) {
node.mBFSColor = mAnimatedGraphBFSColor;
activeNodesCount++;
Expand All @@ -377,7 +381,7 @@ public void runUpdates(long frameTimeNanos) {
}

for (int i = 0; i < mActiveAnimations.size(); i++) {
AnimationDriver animation = mActiveAnimations.get(i);
AnimationDriver animation = mActiveAnimations.valueAt(i);
animation.runAnimationStep(frameTimeNanos);
AnimatedNode valueNode = animation.mAnimatedValue;
if (valueNode.mBFSColor != mAnimatedGraphBFSColor) {
Expand Down Expand Up @@ -422,15 +426,15 @@ 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.get(i);
AnimatedNode node = mUpdatedNodes.valueAt(i);
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.get(i);
AnimationDriver animation = mActiveAnimations.valueAt(i);
AnimatedNode valueNode = animation.mAnimatedValue;
if (valueNode.mActiveIncomingNodes == 0 && valueNode.mBFSColor != mAnimatedGraphBFSColor) {
valueNode.mBFSColor = mAnimatedGraphBFSColor;
Expand Down Expand Up @@ -480,19 +484,15 @@ public void runUpdates(long frameTimeNanos) {
// finished, then resize `mActiveAnimations`.
if (hasFinishedAnimations) {
int dest = 0;

This comment has been minimized.

Sign in to view

Copy link
@baoti

baoti Nov 12, 2016

Unused dest.
for (int i = 0; i < mActiveAnimations.size(); i++) {
AnimationDriver animation = mActiveAnimations.get(i);
if (!animation.mHasFinished) {
mActiveAnimations.set(dest++, animation);
} else {
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);
}
}
for (int i = mActiveAnimations.size() - 1; i >= dest; i--) {
mActiveAnimations.remove(i);
}
}
}
}

0 comments on commit 6f5433f

Please sign in to comment.