Bypass animation queue for onScroll events #312
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Failing tests are unrelated, have a PR to fix those here: #313 |
|
I'll rerun detox! Chapeau bas! |
|
Merged master! |
osdnk
approved these changes
Jun 23, 2019
There was a problem hiding this comment.
Following my intuition, it's okay cause ScrollView provides throttling itself.
I wouldn't really care about the order of events in this case. I cannot imagine a situation when it matters considering this set of events.
This appears not to affect the whole codebase and is not very hacky so I'll happily see it left in a current state.
Would you mind just adding few comments in the codebase, cause it might be an unclear solution in the future (maybe including a link for issues)?
Also, can you tell us why this issue was not present on Android?
|
Also, thank you @oblador! I wished to make similar changes by myself but didn't find enough time yet! |
|
Hello guys. I would like to ask you if you could make a subrelease featuring this change. I'm really excited about this one |
|
@osdnk This issue is present on Android, but it doesn't have the battery saving FPS throttle that iOS does, so it's less obvious (although still noticeable). This should be fixed too, but I'd prefer to keep it out of scope from this PR as my Android skills are limited. I'm happy to add some comments 👍 |
|
@sinodko You can install it from source/git if you urgently need this. |
|
Ohh my bad. I thought that the pr is already in master 😁 I will try your fork |
osdnk
pushed a commit
that referenced
this pull request
Nov 11, 2019
* Add CI integration (#278) Motivation In React Native Gesture Handler repo we have integration with travisCI to test if our Example apps are building and some detox e2e tests on iOS. I thought that it is not much work to add CI here as we already have config and project config is similar so it would most likely work out of the box with little tweaks. Changes Added YAML config file and Travis CI integration * Clarify set node description (#285) * Add a mock implementation for test runners (#276) I was recently using reanimated in react-navigation where I needed to mock it to be able to run it in Jest. It'll be great if the mock was in the library so we don't have to duplicate it in every project. I looked through the typescript definitions and added a mock function for almost everything. I didn't use anything specific to a test runner such as Jest, so it should be possible to use the mock with any test runner. * Create ReanimatedModule.web.js (#242) Prevents crashing when referenced in libs like react-navigation. * Fix example app's metro config on windows machines (#266) glob-to-regexp does not escape backslashes as of version 0.4 which causes issues on windows machines. replacing backslashes with slashes only on windows fixes the issue. * TVos support (#291) * Add typing for concat#0 concat#1 (#293) concat#1 can be useful to convert a numerical animated value to a string. * Update README.md (#301) * Remove react/react-native peerDependencies (#298) * 💄Refine TypeScript type for boolean functions (#305) * Get rid of Bezier's arguments check (#307) * Bump version -> 1.1.0 * Add Math.log() operator (#320) I had a need for Math.log in an Animation calculation. This code all seemed to work and I think I found all the touch points, but I'm not well versed on the native side of things so please let me know if anything needs to change. * 💄 Refine TS type (#314) * Fix android CI setup (#313) * fix(🐛): Add TS support for animated transforms (#323) * Replace forEach with for loop (#329) Since `__addChild` etc. are called so many times, using a for loop makes it faster by around 10ms at least (on iOS simulator, on device it might be even more). * Bypass animation queue for onScroll events (#312) This PR is a proof of concept on how to address issues like #160 #309. As outlined in my comment, the problem with the current batched updates are that reanimated lags behind event processing by at least one event, in some cases up to 3-4. If using something like react-native-gesture-handler this is generally not noticeable as reanimated will be responsible for positioning all elements, but when deriving an animated value from a scroll event this would cause the elements positioned by reanimated to be out of sync with the scroll view. To address the lag, this PR introduces a concept of "direct events", that will bypass the queue and apply its effects immediately. Due to my unfamiliarity with the codebase I didn't really know how to do this in a nice way without also doing a quite big refactor. This is a low invasive change to illustrate how it can be addressed, I'm happy to receive feedback on how this can be done in a cleaner way. Test plan Using @lindesvard's reproduction of the issue https://github.com/lindesvard/reanimated-scroll-perf on a physical device, applying these changes should remove the lag and stuttering. Adverse effects Since the direct effects will bypass the queue, it will not retain the same order. Given the declarative nature of reanimated I think this should be OK in most cases, but still worth mentioning. A possible workaround is to flush the queue before applying direct events. The current way to determine direct events are by event name which is not particularly specific. In the odd case of naming collision, I don't see it having profound negative effects however. * (react-native-reanimated.d.ts) fix TransformsStyle (#339) This fixes typescript error `Cannot find name 'TransformStyle'.`. * target iOS 9.0 (#346) * Make `ReanimatedModule.NAME` public (#356) This allow using the constant directly with `TurboReactPackage`. * Fix: Support optional string style types (#352) * Add support for nodes inside proc (#354) Motivation When creating multiple dynamic animations with react-native-reanimated the number of nodes that needs to be transported over the bridge is a performance challenge. One of the limitations of the current primitives offered by reanimated is the lack of function nodes. Being able to define node trees that can be reused will offer better performance. A previous attempt has been done in this pull request: #42 Implementation The suggested implementation tries to limit changes to the current source code by adding new nodes without changing the existing implementation too much. Three new nodes have been added: ParamNode CallFunctionNode FunctionNode A few changes to the NodeManager to be able to force reevaluation when calling functions have been added. The implementation uses a simple proc function in Javascript to create function nodes. Following is an example of how a function is defined and used. // Defining the function const mulXY = proc((x, y) => multipy(x, y)); // Using the function const myX = new Animated.Value(12); const myY = new Animated.Value(14); const myResult = mulXY(myX, myY); Technical The solution I have used in this implementation is to implement the ParamNode as a node that holds a reference to other nodes - returning the referenced node's value when evaluated and setting the referenced node's value when changed. Calling the function node is done using the CallFuncNode that is created when calling the function (on the JS side). When the CallFuncNode is evaluated, it performs the following steps: Start a new context Copy current argument nodes to the param nodes (using a Stack in the param node) Evaluate the function node's expression Pop the current argument nodes from the param nodes End context I have also changed NodeManager's invalidate strategy so that it will force a reevaluation of nodes when we are in a function call to avoid returning the same value for multiple functions calls with different parameter references. Using a stack in the ParamNode ensures that we can create recursive functions. Memory A single proc node should be created as a global constant in javascript since they are immediately attached and will never be removed. Platforms Both Android and iOS platform-specific code has been added in addition to the required JS code. Further plans Rewriting functionality in reanimated called often should be considered. Moving complex spring functions to functions could also be considered but is not necessarily part of this PR. Testing Convert built-in derived functionality to use proc nodes and verify that all examples work nicely. I have already done this for interpolate and easings and it seems to be working fine. Verify that no memory leaks exist. I believe that the current implementation supports caching of function evaluation - would be happy if someone else could confirm this. Co-authored-by: osdnk <micosa97@gmail.com> * 💄Refine typing for cond() (#359) * Fix: jest mock should use Animated.ScrollView (#344) Ran into the issue here: satya164/react-native-tab-view#845 This is because `ScrollView` doesn't have `.getNode()` but `Animated.ScrollView` does. This change fixes the error. I think this is the correct fix. cc @simonbuerger * doc: fix broken code block (#365) replaced backticks(?) to "`" * Remove deprecated componentWillMount (#342) Refactors the create animated component removing the deprecated componentWillMount lifecycle and moving the attachProps to the constructor. * Bump version -> 1.2.0 * Update README.md (#384) Fix typo * FIX javaCompileProvider (#393) FIX for #315 * Remove prevPosition in SpringState (#386) Creates a type name that is non-specific to one animation function. This is useful when an animation state is shared between animation functions (spring + decay for instance). * add event mapping type (#388) * Add createAnimatedComponent to mock (#389) * Specify component typings as React.Component classes (#373) *Rationale* Currently `Animated.View`, `Animated.Text`, `Animated.Image`, `Animated.ScrollView` are exported as `const`ants with a `React.ComponentClass` type. This means they cannot be used as generics in (for example) `React.Ref<Animated.View>`, and that the useful `getNode()` method is not typed. PR addresses this by changing the constants to `React.Component` class exports, decorated with the `getNode()` method * migrate to AndroidX (#395) Migrated source code to use AndroidX, and Example to use React Native 0.60.5. Also removed unnecessary files like flow and buck configurations, Gradle wrapper in android, etc... Removed Geolocation module from Example on iOS. I can compile for Android and iOS versions of the example app, but metro fails to resolve react-native-reanimation from '../src/Animated' via babel configuration. **HELP**: Help make metro work for example app. * Update Example app to use RN 0.61 (#401) * Optimize constant nodes creation and cleanup (#403) This PR optimizes the way we create and cleanup value nodes. When adapting constants (e.g. when doing interpolate({ inputRange: [0,1]}) we generate new nodes for start and end of the range) we've been creating new animated value nodes. This kind of nodes can never be updated on the native side and hence there is no need to send its final value back to JS when they get detached. On top of there there are several constant values that are frequently used across the library and product code with reanimated. Those can only be instantiated once and reused in all the places. That list includes 0, 1 and -1 for now. These two changes reduce the number of nodes instantiated by one third on colors/index.js example (from 88 down to 57) and reduce the number of nodes we call getValue for by 94% (from 88 down to 5). * Update iOS Example project to build with RN 0.61 (#402) * Update android Example project build.gradle to build with RN 0.61 (#407) * fix(🐛): Fix Android bug when dealing with string animated values (#311) This PR takes @msand and @osdnk from #176 into consideration. fixes #300 * Bump version -> 1.3.0 * Added web support (#390) # Why - [Preview](https://twitter.com/Baconbrix/status/1173825701192929280?s=20) - [Demo](https://reanimated.netlify.com) # How - Enabled JS runner - Added JS to broken runner nodes: `CallFunc`, `Param` - Added support for web gesture-handler - Added expo-web to Example - Added `format` node for web color transform # Test Plan - [Example](https://reanimated.netlify.com) should run - Transitions API is not supported yet * [iOS] Fix floor() implementation (#362) fixes #316 * Fix invalid url in readme.md (example in event handling) (#418) * fixed EventArgFunc typings (#422) * 💄Export AnimateProps & AnimateStyle (#429) Exporting these types can be useful for component definition. For instance: ```ts interface FlexibleCardProps extends CardProps { style?: Animated.AnimateStyle<ImageStyle>; } export const FlexibleCard = ({ card, style }: FlexibleCardProps) => ( <Animated.Image style={[styles.flexibleContainer, style]} source={card.source} /> ); ``` ``` * bump -> 1.3.1 * Fix web support (#445) * bump -> 1.3.2 * Add tvos podspec support (#440) * 💄Refine debug() typing (#434) Non-node value crash with debug() * Bump morgan from 1.9.0 to 1.9.1 in /Example (#444) Bumps [morgan](https://github.com/expressjs/morgan) from 1.9.0 to 1.9.1. - [Release notes](https://github.com/expressjs/morgan/releases) - [Changelog](https://github.com/expressjs/morgan/blob/master/HISTORY.md) - [Commits](expressjs/morgan@1.9.0...1.9.1) Signed-off-by: dependabot[bot] <support@github.com> * Bump extend from 3.0.1 to 3.0.2 in /Example (#442) Bumps [extend](https://github.com/justmoon/node-extend) from 3.0.1 to 3.0.2. - [Release notes](https://github.com/justmoon/node-extend/releases) - [Changelog](https://github.com/justmoon/node-extend/blob/master/CHANGELOG.md) - [Commits](justmoon/node-extend@v3.0.1...v3.0.2) Signed-off-by: dependabot[bot] <support@github.com> * Bump js-yaml from 3.12.0 to 3.13.1 in /Example (#441) Bumps [js-yaml](https://github.com/nodeca/js-yaml) from 3.12.0 to 3.13.1. - [Release notes](https://github.com/nodeca/js-yaml/releases) - [Changelog](https://github.com/nodeca/js-yaml/blob/master/CHANGELOG.md) - [Commits](nodeca/js-yaml@3.12.0...3.13.1) Signed-off-by: dependabot[bot] <support@github.com> * Bump mixin-deep from 1.3.1 to 1.3.2 in /Example (#443) Bumps [mixin-deep](https://github.com/jonschlinkert/mixin-deep) from 1.3.1 to 1.3.2. - [Release notes](https://github.com/jonschlinkert/mixin-deep/releases) - [Commits](jonschlinkert/mixin-deep@1.3.1...1.3.2) Signed-off-by: dependabot[bot] <support@github.com> * fix: mock.js NOOP to allow for "new" constructor call (#439) i'm getting errors in my tests from this mock as the new Value(0) call throws for arrow functions, this seems to work when i updated my mock.js in node_modules. * 🎣Allow to pass a callback as parameter of useCode() (#408) fixes #343 This can enable us to save some unnecessary node creation but more importantly, it will enable us to benefit from the Exhaustive Deps eslint rule. This rule expect a callback as first parameter: https://github.com/facebook/react/blob/master/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js#L64 This rule is incredibly useful to avoid bugs when using hooks that have dependencies. * Add Jest doc to the README Following #355 (comment) it's true that there is no easily accessible documentation about how to set up Jest tests with reanimated. * Fix node dirty marking when new connection between nodes is made (#450) This change fixes the problem where a parent node were marked as dirty while adding connection between nodes instead of the children node. We should be only marking children node as if we mark parent node there is a chance that this would trigger updates in leave nodes (final nodes) that should not be affected as the children added to parent node cannot impact its value. On top of that the marking change relealed a bug where we initialized loop ID inproperly. We should be initializing it with a value that'd allow the node to be evaluated at least once. It's been working before as every node would be dirty marked. With the above change the root nodes were no longer being marked but we'd still require them to be evaluated. The invalid initial setting for last loop ID was making us skip evaluation of those parent nodes. In order to test this change I created the following sample app: https://gist.github.com/7fce6b4a891f0284baa17d0d63455495 In this app there is a button that creates a new view that hooks into an existing animated value. Before this change when the view was added we'd reevaluate all the nodes that depend on that value and hence we'd get a consol.warn being shown twice (first on the initial render and then second time when we add the view). WIth this change in the `call` node should not be evaluated more than once and only on the initial render. * Bump -> 1.4.0 * Fix ESLint dev setup and errors (#421) * Fix ESLint setup and devDependencies - Fix version of types/react-native - Update husky and lint-staged * Fix lint errors * Rewrite README.md * Format and update 04.transitions.md * Format and update 05.value.md * Format and update 07.block.md * Format and update 09.code.md * Format and update 10.event.md * Format and update 11.baseNodes/set.md * Add 11.baseNodes/log.md * Format and update 11.baseNodes/floor.md * Format and update 11.baseNodes/ceil.md * Update min, max docs * Add proc node docs * Format pages * Update README.md with latest version * Remove unnecessary assets folders * Update README.md * Remove meme :c * Fix links and move 14.backward.md to 13.declarative.md * Remove 1st person narrative in about page * Remove assets/ from component-docs config We don't need static assets right now
kmagiera
added a commit
that referenced
this pull request
Feb 24, 2023
## Summary Before this change we'd only call `performOperations` for a subset of filtered events (initially introduced in #312). The event's that weren't considered "direct" would require additional animation frame for having their updates flushed. This change makes it so that we call `performOperations` for all types of events therefore matching the behavior [on Android](https://github.com/software-mansion/react-native-reanimated/blob/99b8b3ed56e36ca615cce7164ccaf04d154571b1/android/src/main/java/com/swmansion/reanimated/NodesManager.java#L283) A consequence of this change was that for some event types, the updates reanimated was performing weren't getting flushed onto screen. Namely, we noticed this problem in Pager example where a view pager component with some custom set of events is used and event handlers update shared values. Even though such event would trigger shared value update, and these updates would trigger the style to recalculate and we'd even call updateProps method to apply the updated props, we'd still see no result as in that example the changes require layout run. Without `performOperation` call the layout would not be executed unless react would rerender or other time-based animation would run. The problem became apparent after #3970 where we changed the place where updates are performed from requestAnimationFrame to setImmediate. Before this change, since we were running the updates in "animation frame" the `performOperation` method was being run by the frame scheduler. We, however were getting these updates delayed by one frame because of that. This issue also wasn't noticed prior to shareable rewrite from #3722 because before, we were always starting frame updater for every single update happening to shared value even if it was due to an event. As a result, we were getting the stuff updated on screen but again, with a delay of one frame. ## Test plan Run pager example on iOS.
fluiddot
pushed a commit
to wordpress-mobile/react-native-reanimated
that referenced
this pull request
Jun 5, 2023
## Summary Before this change we'd only call `performOperations` for a subset of filtered events (initially introduced in software-mansion#312). The event's that weren't considered "direct" would require additional animation frame for having their updates flushed. This change makes it so that we call `performOperations` for all types of events therefore matching the behavior [on Android](https://github.com/software-mansion/react-native-reanimated/blob/99b8b3ed56e36ca615cce7164ccaf04d154571b1/android/src/main/java/com/swmansion/reanimated/NodesManager.java#L283) A consequence of this change was that for some event types, the updates reanimated was performing weren't getting flushed onto screen. Namely, we noticed this problem in Pager example where a view pager component with some custom set of events is used and event handlers update shared values. Even though such event would trigger shared value update, and these updates would trigger the style to recalculate and we'd even call updateProps method to apply the updated props, we'd still see no result as in that example the changes require layout run. Without `performOperation` call the layout would not be executed unless react would rerender or other time-based animation would run. The problem became apparent after software-mansion#3970 where we changed the place where updates are performed from requestAnimationFrame to setImmediate. Before this change, since we were running the updates in "animation frame" the `performOperation` method was being run by the frame scheduler. We, however were getting these updates delayed by one frame because of that. This issue also wasn't noticed prior to shareable rewrite from software-mansion#3722 because before, we were always starting frame updater for every single update happening to shared value even if it was due to an event. As a result, we were getting the stuff updated on screen but again, with a delay of one frame. ## Test plan Run pager example on iOS.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR is a proof of concept on how to address issues like #160 #309.
As outlined in my comment, the problem with the current batched updates are that reanimated lags behind event processing by at least one event, in some cases up to 3-4. If using something like react-native-gesture-handler this is generally not noticeable as reanimated will be responsible for positioning all elements, but when deriving an animated value from a scroll event this would cause the elements positioned by reanimated to be out of sync with the scroll view.
To address the lag, this PR introduces a concept of "direct events", that will bypass the queue and apply its effects immediately. Due to my unfamiliarity with the codebase I didn't really know how to do this in a nice way without also doing a quite big refactor. This is a low invasive change to illustrate how it can be addressed, I'm happy to receive feedback on how this can be done in a cleaner way.
Test plan
Using @lindesvard's reproduction of the issue https://github.com/lindesvard/reanimated-scroll-perf on a physical device, applying these changes should remove the lag and stuttering.
Adverse effects