Skip to content
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

Add support for animating all properties using native driver #27506

Open
wants to merge 24 commits into
base: master
from

Conversation

@chrfalch
Copy link

chrfalch commented Dec 13, 2019

Summary

Native animation only supports properties that can be updated through the shadow view tree, limiting it to a small subset of properties and styles available for animation. Whenever a developer wants to animate something like size, color etc. they revert to JS based animation leading to complaints about stutters in the UX.

Inspired by the wide usage of Reanimated I've created a solution that uses the same principles as Reanimated. This is basically two separate paths to update a property - one is for shadow tree properties that can be synchronously updated through UIManager .synchronouslyUpdateViewOnUIThread, and one that uses a batched update queue that is updated through UIManager.updateView.

This might have some performance issues, but the result will be much smoother and faster than using JS based animation.

I'm aware that a Fabric based animation library is in the works, but believe that this PR can add value for developers until Fabric is available.

Changelog

  • [iOS][Added] Added support for animating unsupported props with useNativeDriver
  • [General][Added] Added support for animating all props using native driver
  • [Android][Added] Added support for animating all props with useNativeDriver

Test Plan

  • Added examples that shows native animations of size and color in RNTester
  • Removed unit tests for illegal native properties

return `rgba(${r}, ${g}, ${b}, ${a})`;
}

This comment has been minimized.

Copy link
@JoshuaGross

JoshuaGross Dec 16, 2019

Contributor

let's add some basic tests for this?

This comment has been minimized.

Copy link
@JoshuaGross

JoshuaGross Dec 16, 2019

Contributor

Ah I see that you're just moving this, but if tests don't already exist, it'd be nice to add a few

@JoshuaGross

This comment has been minimized.

Copy link
Contributor

JoshuaGross commented Dec 16, 2019

Hey @chrfalch, thanks for doing this! In general I think this is something I'm supportive of, and seems like a fine thing to do. I'll need to spend some time with this to see if it has implications on native driver animations in Fabric, since they share most code currently. I'll try to set aside some time this week to play around with it.

Copy link

facebook-github-bot left a comment

@JoshuaGross has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Copy link
Contributor

janicduplessis left a comment

Looks great! I added some comments about naming that I found confusing.

Currently it seems like what is called shadow view props is actually non-layout props that are updatable directly on the ui thread. Shadow view props are the ones that need to be updated through the regular UIManager pipeline.

@chrfalch

This comment has been minimized.

Copy link
Author

chrfalch commented Dec 19, 2019

Thanks so much for the feedback. I’m currently unavailable for a few days :( will set a side time as soon as possible to fix the issues you pointed out.

@terrysahaidak terrysahaidak mentioned this pull request Dec 21, 2019
Christian Falch and others added 18 commits Dec 13, 2019
- Added fix to interpolation node to make it work with colors
- Added support for differentiating between shadow view nodes and nodes that needs to be updated through the uimanager
- Added interfaces and configuration methods for setting up properties
- Added support for sharing the colorToRgba function
- Added configuration of allowed shadow view properties on startup
- Removed test to avoid using illegal props
- Added correct handling of colors
- Updated example with colors and size interpolation
- Added support for configuration of allowed properties from js
- Fixed color interpolation in interpolation node
- Added support for differentiating props in style and prop nodes
- Made member getUIViewOperationQueue public in UIManager to be able to test if we are currently updating our view so we don't call dispatchViewUpdates to often.
- Removed validateStyles and the Styles whitelist.
- Added static ui thread styles
- Renamed methods after code review
- Removed configureProps from iOS
- Renamed members from code review
- Removed styles properties white list
@chrfalch chrfalch force-pushed the chrfalch:Feature/add-native-animation-of-all-props branch from a42aafa to 8f37658 Dec 22, 2019
@chrfalch

This comment has been minimized.

Copy link
Author

chrfalch commented Dec 28, 2019

Hi, @JoshuaGross and @janicduplessis - how should we continue with this PR? I see that some of the tests are failing, but I think the test_ios_frameworks test is failing for everyone these days, and the test_android fail seems to be an infrastructure failure?

@chrfalch chrfalch requested a review from JoshuaGross Jan 1, 2020
@janicduplessis

This comment has been minimized.

Copy link
Contributor

janicduplessis commented Jan 5, 2020

@chrfalch Looks like we got a part wrong. In the original implementation only style props are validated, this means that any regular prop should still go through synchronouslyUpdateViewOnUIThread. However we don't have this context anymore in RCTPropsAnimatedNode.

I suggest we invert our list and instead of having uiProps which are props that can be updated with synchronouslyUpdateViewOnUIThread we go with something like layoutProps which are props that need to use the regular UIManager pipeline (enqueueUpdateViewOnUIManager ). The list of layout props can be found here https://github.com/facebook/react-native/blob/master/React/Views/RCTViewManager.m#L339.

An example of this is animating the value of a slider https://github.com/facebook/react-native/blob/master/RNTester/js/examples/NativeAnimation/NativeAnimationsExample.js#L661. With the current changes it would go through enqueueUpdateViewOnUIManager instead of synchronouslyUpdateViewOnUIThread.

@chrfalch

This comment has been minimized.

Copy link
Author

chrfalch commented Jan 5, 2020

@chrfalch Looks like we got a part wrong. In the original implementation only style props are validated, this means that any regular prop should still go through synchronouslyUpdateViewOnUIThread. However we don't have this context anymore in RCTPropsAnimatedNode.

I suggest we invert our list and instead of having uiProps which are props that can be updated with synchronouslyUpdateViewOnUIThread we go with something like layoutProps which are props that need to use the regular UIManager pipeline (enqueueUpdateViewOnUIManager ). The list of layout props can be found here https://github.com/facebook/react-native/blob/master/React/Views/RCTViewManager.m#L339.

An example of this is animating the value of a slider https://github.com/facebook/react-native/blob/master/RNTester/js/examples/NativeAnimation/NativeAnimationsExample.js#L661. With the current changes it would go through enqueueUpdateViewOnUIManager instead of synchronouslyUpdateViewOnUIThread.

Great! Thanks - will see if I can make the required changes. Thanks for the pointers as well.

@chrfalch

This comment has been minimized.

Copy link
Author

chrfalch commented Jan 5, 2020

@chrfalch Looks like we got a part wrong. In the original implementation only style props are validated, this means that any regular prop should still go through synchronouslyUpdateViewOnUIThread. However we don't have this context anymore in RCTPropsAnimatedNode.
I suggest we invert our list and instead of having uiProps which are props that can be updated with synchronouslyUpdateViewOnUIThread we go with something like layoutProps which are props that need to use the regular UIManager pipeline (enqueueUpdateViewOnUIManager ). The list of layout props can be found here https://github.com/facebook/react-native/blob/master/React/Views/RCTViewManager.m#L339.
An example of this is animating the value of a slider https://github.com/facebook/react-native/blob/master/RNTester/js/examples/NativeAnimation/NativeAnimationsExample.js#L661. With the current changes it would go through enqueueUpdateViewOnUIManager instead of synchronouslyUpdateViewOnUIThread.

Great! Thanks - will see if I can make the required changes. Thanks for the pointers as well.

Should be fixed now, @janicduplessis :)

Btw: I have a use case where I'm creating an Animated TextInput and set its text property (I know it is not public) to update text from Animated. It looks like it needs to go the enqueueUpdateViewOnUIManager route to be updated - even though it is not a layout property.

Any suggestion on how this should be handled? Could this be an issue with other props like SVG properties that some people like to animate?

@janicduplessis

This comment has been minimized.

Copy link
Contributor

janicduplessis commented Jan 5, 2020

Hmm maybe we could just expose the layoutProps array to make it possible to add to it from objc / java by getting the native animated module instance with bridge.moduleForClass. It can also be possible to add a new method on the native animated module like addLayoutProp. We might want to address this is a separate PR though.

@janicduplessis

This comment has been minimized.

Copy link
Contributor

janicduplessis commented Jan 5, 2020

As for SVG, afaik it already works with the current implementation using synchronouslyUpdateViewOnUIThread.

@chrfalch

This comment has been minimized.

Copy link
Author

chrfalch commented Jan 5, 2020

Hmm maybe we could just expose the layoutProps array to make it possible to add to it from objc / java by getting the native animated module instance with bridge.moduleForClass. It can also be possible to add a new method on the native animated module like addLayoutProp. We might want to address this is a separate PR though.

What about adding support for updating it from JavaScript to make it easier for devs to use it?

Christian Falch
… that needs to be called through enqueueUpdateViewOnUIManager
Copy link
Contributor

janicduplessis left a comment

Left a few more comments, after those are resolved it is good to ship for me, I looked mostly at the iOS implementation. I'll let @JoshuaGross pick it up from here.

@chrfalch

This comment has been minimized.

Copy link
Author

chrfalch commented Jan 7, 2020

Left a few more comments, after those are resolved it is good to ship for me, I looked mostly at the iOS implementation. I'll let @JoshuaGross pick it up from here.

Thanks so much for helping out, @janicduplessis

@JoshuaGross: Ping me on Discord or here if you have questions.

@JoshuaGross

This comment has been minimized.

Copy link
Contributor

JoshuaGross commented Jan 16, 2020

@shergin or @sammy-SC do you want to test or review the iOS side of this PR?

Copy link

facebook-github-bot left a comment

@JoshuaGross has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@chrfalch

This comment has been minimized.

Copy link
Author

chrfalch commented Jan 17, 2020

@shergin or @sammy-SC do you want to test or review the iOS side of this PR?

@janicduplessis reviewed the iOS code - but feel free to review this part of the implementation if you want. :)

@kelset kelset mentioned this pull request Jan 21, 2020
Copy link
Contributor

shergin left a comment

@chrfalch Is this change implied that we run layout during updating the layout props? If so, which exact change enables that?

@chrfalch

This comment has been minimized.

Copy link
Author

chrfalch commented Jan 23, 2020

@chrfalch Is this change implied that we run layout during updating the layout props? If so, which exact change enables that?

I've added calls through an operation queue in RCTNativeAnimatedNodesManager.m that ends up calling enqueueUpdateViewOnUIManager for properties that need to use the layout pipeline.

Not sure if that was the answer you were looking for?

@shergin

This comment has been minimized.

Copy link
Contributor

shergin commented Jan 27, 2020

@chrfalch Thanks!
Looks good to me!

@chrfalch

This comment has been minimized.

Copy link
Author

chrfalch commented Jan 27, 2020

@chrfalch Thanks!
Looks good to me!

Thanks so much! Anyone specific I should poke to get it merged? I also discussed briefly with @JoshuaGross about having to run some internal codegen tools to update the API before merging. @shergin

@terrysahaidak

This comment has been minimized.

Copy link

terrysahaidak commented Jan 28, 2020

After some testing realized fontSize, letterSpacing and lineHeight are missing in layoutProps array. They cannot be updated on UIThread, so animating them makes no effect right now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.