-
Notifications
You must be signed in to change notification settings - Fork 24.3k
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
[NativeAnimation] Problems & solutions #9120
Conversation
@janicduplessis could use your input if you're willing to give it :) |
By analyzing the blame information on this pull request, we identified @buba447 to be a potential reviewer. |
Thank you for your pull request. As you may know, we require contributors to sign our Contributor License Agreement, and we don't seem to have you on file and listed as active anymore. In order for us to review and merge your code, please email [email protected] with your details so we can update your status. |
StyleSheet, | ||
TouchableWithoutFeedback, | ||
} = ReactNative; | ||
var UIExplorerButton = require('./UIExplorerButton'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no-unused-vars: 'UIExplorerButton' is defined but never used
Awesome write up, I'll have a closer look tomorrow but some initial thoughts: Problem 1We can actually just remove this check, I had to remove some already in #8844 to make native animated value listeners work (might actually be the exact same fix). I just added this comment to make sure people don't expect this value to always be up to date for native driven animations. // Warning, this value may not be up to date for native driven animated nodes
// unless it is listening for native updates. EDIT: Your fix is actually the right thing to do. Problem 2Have you tried removing all the If it doesn't work I think making the animated value native when initialized is reasonable. At the moment it is specified per animation but I think it's rare that you'll want to use the same animated value for native and non-native animations (not even sure how that would work). We could move the useNativeDriver config to the Animated.Value constructor Problem 3Seems good Problem 3bAndroid implementation actually uses the UIManager to update the views, not sure if doing something similar for iOS would fix this issue. Maybe using |
Left a comment on #8844 to discuss the implications of removing all the !value.__isNative checks in AnimatedImplementation.js. I'm worried we'll set stale values during re-render. |
@ryangomba updated the pull request. |
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
Just testing something out. @facebook-github-bot import |
Thanks for importing.If you are an FB employee go to Phabricator to review internal test results. |
It's been a while since the last commit was reviewed and the labels show this pull request needs review. Based on the blame information for the files in this pull request we identified @buba447 as a potential reviewer. Could you take a look please or cc someone with more context? |
f5dab4b
to
e5572d0
Compare
@ryangomba @janicduplessis do you still want to ship this? |
Needs revision due to conflicting files. |
@ryangomba Is there still something we want to merge here or was it all split in different PRs? |
We can close this. The solutions have been submitted as referenced PRs. |
Summary: This diff attempts to fix a number of iOS native animation bugs related to improper node invalidation and a race with view creation. The major issues were presented in #9120 as problems 3 and 3b, but I'll recap here: The invalidation model we use is overly complicated and incomplete. The proper combination of `_needsUpdate` and `_hasUpdated` will result in nodes values being recomputed. However, we do not invalidate nodes in all the places we should, e.g. if we create a new view and attach it to an existing value node (see example in #9120). This diff chooses to remove the `_hasUpdated` flag, and simply relies on the `_needsUpdate` flag to mark a node as dirty. We mark nodes as dirty when they are: - created - updated - attached to new parents - detached from old parents - attached to a view Calling `updateNodeIfNecessary` will, if necessary, compute all invalidated parent values before recomputing the node value. It will then apply the update, and mark the no Closes #10663 Differential Revision: D4120301 Pulled By: mkonicek fbshipit-source-id: e247afcb5d8c15999b8328c664b9f7e764d76a75
Summary: This diff attempts to fix a number of iOS native animation bugs related to improper node invalidation and a race with view creation. The major issues were presented in facebook#9120 as problems 3 and 3b, but I'll recap here: The invalidation model we use is overly complicated and incomplete. The proper combination of `_needsUpdate` and `_hasUpdated` will result in nodes values being recomputed. However, we do not invalidate nodes in all the places we should, e.g. if we create a new view and attach it to an existing value node (see example in facebook#9120). This diff chooses to remove the `_hasUpdated` flag, and simply relies on the `_needsUpdate` flag to mark a node as dirty. We mark nodes as dirty when they are: - created - updated - attached to new parents - detached from old parents - attached to a view Calling `updateNodeIfNecessary` will, if necessary, compute all invalidated parent values before recomputing the node value. It will then apply the update, and mark the no Closes facebook#10663 Differential Revision: D4120301 Pulled By: mkonicek fbshipit-source-id: e247afcb5d8c15999b8328c664b9f7e764d76a75
Summary: This diff attempts to fix a number of iOS native animation bugs related to improper node invalidation and a race with view creation. The major issues were presented in facebook#9120 as problems 3 and 3b, but I'll recap here: The invalidation model we use is overly complicated and incomplete. The proper combination of `_needsUpdate` and `_hasUpdated` will result in nodes values being recomputed. However, we do not invalidate nodes in all the places we should, e.g. if we create a new view and attach it to an existing value node (see example in facebook#9120). This diff chooses to remove the `_hasUpdated` flag, and simply relies on the `_needsUpdate` flag to mark a node as dirty. We mark nodes as dirty when they are: - created - updated - attached to new parents - detached from old parents - attached to a view Calling `updateNodeIfNecessary` will, if necessary, compute all invalidated parent values before recomputing the node value. It will then apply the update, and mark the no Closes facebook#10663 Differential Revision: D4120301 Pulled By: mkonicek fbshipit-source-id: e247afcb5d8c15999b8328c664b9f7e764d76a75
Fix iOS native animation (non-nil/non-zero) assertion failure (on initial parentNode then childNode), after nav to bot 4th webview tab (unable to load pages, likely caused by current excluded RN polyfillGlobal) - Df (Foundation) *** Assertion failure in - disconnectAnimatedNodes:childTag - E [com.facebook.react.log:native] Exception thrown while executing UI block: 'parentNode' is a required parameter - Df (Foundation) *** Assertion failure in - connectAnimatedNodes:childTag - E [com.facebook.react.log:native] Exception thrown while executing UI block: 'childNode' is a required parameter Exclude iOS native animation (non-nil/non-zero) assertion macros - for now, when nodes (pointers to tags) attach/detach (to old/new parents and new views) QA: no empty animation frames observed ✅ Low probability possible risks introduced - incomplete node invalidation (outdated nodes) - race condition: prop updated before UIManager created view (outdated props) QA: no outdated animation frames observed ✅ iOS native animation assertion refs - Summary: facebook/react-native@c858420 - PR: facebook/react-native#10663 - Examples: facebook/react-native#9120 - nb: mimics ReactAndroid (i.e. NativeAnimatedNodesManager.java) nb: metro-react-native-babel-preset (0.72.3) - @babel/plugin-transform-regenerator has been removed since initial investigation - @babel/plugin-transform-runtime (removed) - 4 fewer SES warnings - @babel/plugin-transform-runtime > regenerator: false - immediate error thrown (recurring) Todo: Fix webview page loading (likely caused by current excluded RN promisePolyfill), thus fixing these assertion failures on nav, then revert this patch - Problem: Including promisePolyfill (default RN) causes app to boot empty root view Todo: Root cause of above 'regenerator: false' causing nil/zero parent/child nodes
Fix iOS native animation (non-nil/non-zero) assertion failure (on initial parentNode then childNode), after nav to bot 4th webview tab (unable to load pages, likely caused by current excluded RN polyfillGlobal) - Df (Foundation) *** Assertion failure in - disconnectAnimatedNodes:childTag - E [com.facebook.react.log:native] Exception thrown while executing UI block: 'parentNode' is a required parameter - Df (Foundation) *** Assertion failure in - connectAnimatedNodes:childTag - E [com.facebook.react.log:native] Exception thrown while executing UI block: 'childNode' is a required parameter Exclude iOS native animation (non-nil/non-zero) assertion macros - for now, when nodes (pointers to tags) attach/detach (to old/new parents and new views) QA: no empty animation frames observed ✅ Low probability possible risks introduced - incomplete node invalidation (outdated nodes) - race condition: prop updated before UIManager created view (outdated props) QA: no outdated animation frames observed ✅ iOS native animation assertion refs - Summary: facebook/react-native@c858420 - PR: facebook/react-native#10663 - Examples: facebook/react-native#9120 - nb: mimics ReactAndroid (i.e. NativeAnimatedNodesManager.java) nb: metro-react-native-babel-preset (0.72.3) - @babel/plugin-transform-regenerator has been removed since initial investigation - @babel/plugin-transform-runtime (removed) - 4 fewer SES warnings - @babel/plugin-transform-runtime > regenerator: false - immediate error thrown (recurring) Todo: Fix webview page loading (likely caused by current excluded RN promisePolyfill), thus fixing these assertion failures on nav, then revert this patch - Problem: Including promisePolyfill (default RN) causes app to boot empty root view Todo: Root cause of above 'regenerator: false' causing nil/zero parent/child nodes
Fix iOS native animation (non-nil/non-zero) assertion failure (on initial parentNode then childNode), after nav to bot 4th WebView tab (unable to load pages, likely caused by current excluded RN Promise polyfillGlobal) - Df (Foundation) *** Assertion failure in - disconnectAnimatedNodes:childTag - E [com.facebook.react.log:native] Exception thrown while executing UI block: 'parentNode' is a required parameter - Df (Foundation) *** Assertion failure in - connectAnimatedNodes:childTag - E [com.facebook.react.log:native] Exception thrown while executing UI block: 'childNode' is a required parameter Exclude iOS native animation (non-nil/non-zero) assertion macros - for now, when nodes (pointers to tags) attach/detach (to old/new parents and new views) QA: no empty animation frames observed ✅ Low probability possible risks introduced - incomplete node invalidation (outdated nodes) - race condition: prop updated before UIManager created view (outdated props) QA: no outdated animation frames observed ✅ iOS native animation assertion refs - Summary: facebook/react-native@c858420 - PR: facebook/react-native#10663 - Examples: facebook/react-native#9120 - nb: mimics ReactAndroid (i.e. NativeAnimatedNodesManager.java) nb: metro-react-native-babel-preset (0.72.3) - @babel/plugin-transform-regenerator has been removed since initial investigation - @babel/plugin-transform-runtime (removed) - 4 fewer SES warnings - intrinsics: Object.setPrototypeOf.default, Object.setPrototypeOf.__esModule, Reflect.construct.default, Reflect.construct.__esModule - @babel/plugin-transform-runtime > regenerator: false - immediate error thrown (recurring) Todo: Fix WebView page load (likely caused by current excluded RN Promise polyfillGlobal), thus fixing these assertion failures on nav, then revert this patch - Problem: Including (default) RN Promise polyfillGlobal causing app to boot empty root view Todo: Root cause of above 'regenerator: false' causing nil/zero parent/child nodes immediately to reoccur
This PR is intended to raise a number of issues with the current implementation of native animations and propose solutions. I'm relaying my experience on iOS, although it's possible some of these problems show up on Android as well.
Native animations work great if you:
However, I've found native animations to be fragile in other circumstances. I've created a UIExplorer example named
NativeAnimationsProblemExample
that illustrates a simple instance in which native animations break down.Overview
The example attempts to drive two blue blocks from opacity 0.25 to 1. Both blocks are driven by the same
Animated.Value
. The first block is created when the example is first rendered. The second is created when you press "Toggle block 2".The easiest way to follow along is to check out your own branch, and apply this diff piece by piece. The problems follow each other logically, so you'll need to commit the proposed fix for a problem to reproduce the next.
Problem 1: Disappearing blocks
[fixed]
Problem 2: Values reset on re-render
How to reproduce:
The problem:
You will see that the first block resets to opacity 1 unexpectedly. This is because when we originally create the block, it has a non-native opacity prop (opacity = 0.25). This value gets sent to react proper. After we mark the animated value as
__native
, however, this prop is removed from the component. When prevProps and nextProps are diffed, react notices that opacity has been removed and callsupdateView
withopacity: null
, thereby resetting the value to 1.In order to avoid this behavior, I propose that all native animated values be marked as
__native
when they are created. This would ensure that all its children would also be marked as__native
, and would make the entire animation tree easier to reason about. I realize this may be controversial, so I'm open to other ideas.The fix:
Uncomment the code in componentWillMount.
Problem 3: Incorrect values on initial render
How to reproduce:
The problem:
You will see that the first block is created with opacity 1, when we expect it to have an opacity of 0.25. This is due to a number of issues. First off, native values are not initialized with their value.
Fix: implement init in
RCTValueAnimatedNode.m
.But it still doesn't work. This is because we are not updating any views outside the display link, i.e. not unless we have a current animation in motion. When a view is created, it should be updated with the current value of all its animated props before it is first shown. This can be solved by updating the view manually in
connectAnimatedNodeToView
.Fix: Apply my changes to
RCTNativeAnimatedModule.m
.However, it still doesn't work. Nodes are only marked as needing an update when an animated value is driven, so
performUpdate
will result in a no-op here. I believe every node should be marked as needsUpdate whenever its parents are first attached, or change. It makes sense to invalidate a node if its parents change.Fix: Apply my changes to
RCTAnimatedNode
.On to the next...
Problem 3b:
How to reproduce:
The problem:
Now, you will see that block 2 is created with opacity 1 when we expect it to have an opacity of 0.25. This is caused by a race.
RCTUIManager
'screateView
will create a new view after our call toconnectAnimatedNodeToView
. You can set breakpoints to verify, or simply apply a timeout inAnimatedComponent
'scomponentDidMount
to do so:The fix:
I'm really not sure. We need to ensure that when a view is created, it is seeded with the current value for all animated props. The simplest solution would be to tightly couple UIManager and native animations: animated props could be gathered and merged with standard props when a view is created. Another solution that avoids coupling would be to allow a
ViewProperyMapper
to register a block to be executed when a view is created. Ideas?