Drive any numerical prop via NativeAnimated #10658

ryangomba · 2016-10-31T19:54:58Z

In theory, we should be able to animate any non-layout property, including custom ones. While there is still work to be done on the native side to fully enable this, we should start by dropping the prop whitelist.

facebook-github-bot · 2016-10-31T19:55:14Z

By analyzing the blame information on this pull request, we identified @janicduplessis and @kmagiera to be potential reviewers.

janicduplessis · 2016-11-02T21:14:10Z

Should we wait for some progress on the native side before shipping this? I think it should (almost) work on Android since we're using UIManager to update props. On iOS we don't so there is quite a bit more work to do for it to work.

ryangomba · 2016-11-02T21:16:30Z

Sure, we can hold off until I have that ready

ryangomba · 2016-11-03T16:19:54Z

@janicduplessis I updated this to drive any numerical prop on iOS. Note that this depends on #10663. Check out the third and final commit in this diff for my most recent changes.

janicduplessis

Does this also already works on Android?

janicduplessis · 2016-11-04T05:22:47Z

Libraries/NativeAnimation/Nodes/RCTTransformAnimatedNode.m

@@ -82,7 +82,12 @@ - (void)performUpdate
    }
  }

-  _propsDictionary[@"transform"] = [NSValue valueWithCATransform3D:transform];
+  _propsDictionary[@"transform"] = @[


Could we just pass the transform config as is now and do the conversion to CATransform3D when setting the prop in the UIViewManager. This is what android does now, I think iOS still does some parsing in JS it could be a good opportunity to fix that as well. This would avoid having multiple representations of the transform (Array of objects -> CATransform -> number array -> CATransform). That way we'd always just have Array of objects -> CATransform.

See https://github.com/facebook/react-native/blob/master/Libraries/StyleSheet/processTransform.js#L36

janicduplessis · 2016-11-28T20:18:51Z

@ryangomba Can you rebase this now that #10663 landed.

ryangomba · 2016-11-28T20:40:01Z

@janicduplessis yep, done.

janicduplessis

LGTM, just a few changes

@javache Could you also have a look at the iOS part to make sure I didn't overlook anything since it will affect transforms a lot.

janicduplessis · 2016-11-29T04:56:10Z

React/Views/RCTConvert+Transform.m

+
+#import "RCTConvert+Transform.h"
+
+@implementation RCTConvert(CoreLocation)


Should be RCTConvert (Transform)

janicduplessis · 2016-11-29T05:04:17Z

React/Views/RCTConvert+Transform.m

+    return transform;
+  }
+  if (![json isKindOfClass:[NSArray class]]) {
+    RCTLogError(@"Expected array for transform matrix");


Shoud that also be RCTLogConvertError?

janicduplessis · 2016-11-29T05:04:24Z

React/Views/RCTConvert+Transform.m

+    return transform;
+  }
+  if ([json count] != 16) {
+    RCTLogError(@"Expected 4x4 matrix array, but count is %zd: %@", [json count], json);


janicduplessis · 2016-11-29T05:05:52Z

React/Views/RCTConvert+Transform.m

+      CGFloat rotate = [self convertToRadians:value];
+      transform = CATransform3DRotate(transform, rotate, 0, 1, 0);
+
+    } else if ([property isEqualToString:@"rotate"]) {


Need to also support rotateZ in that case.

janicduplessis · 2016-11-29T05:15:02Z

@ryangomba Could you also try rebasing again to have tests passing, looks like tests are on a bad commit and fail on an invalid packager command.

javache

This looks like a huge race condition.

javache · 2016-11-30T15:18:08Z

React/Modules/RCTUIManager.m

+  RCTAssertMainQueue();
+  RCTComponentData *componentData = _componentDataByName[viewName];
+  UIView *view = _viewRegistry[reactTag];
+  [componentData setProps:props forView:view];


What if some of the props you're changing also need to be set on the shadow view? What if there's pending changes in flight from the shadowqueue to this view? They'll overwrite the ones you just set synchronously.

Hmm, in what scenario would this be true? No prop should be driven both by the shadow view and a native animation, it's one or the other. Native animated props actually get stripped from the set of props sent to the shadow view; they are mutated independently via this package, completely outside the shadow view hierarchy. It's a limitation for sure, e.g. we can't drive layout props like width or height, but this should be safe from a race.

It seems like you've added that restriction at the Animated ViewManager logic, but this code is not aware of it. I'm worried that 3rd party view manager authors may use this API incorrectly.

Perhaps we could have this use a separate API and in RCTComponentData as well and verify that the props you're setting are main-thread only?

Do you feel that the comment in the header file is insufficient?

/** * Used by native animated module to bypass the process of updating the values through the shadow * view hierarchy. This method will directly update native views, which means that updates for * layout-related propertied won't be handled properly. * Make sure you know what you're doing before calling this method :) */

This warning also exists in the Android codebase.

@javache I don't think the current solution is perfect but this is how it is done on Android so I think this is a good first step (vs the adhoc property setter that we had before).

Ideally we'd want to have the normal prop setting mechanism aware of native animated props and use that to properly update views instead of not passing the props at all but I haven't looked at this yet so I'm not sure how this could be implemented.

I'm still a bit worried about this but feel free to go ahead with docs added.

Summary: This improves JS validations of the transform object and makes it a bit stricter (hence the breaking change). When moving transform objects parsing to native (facebook#10658) the validations got out of sync a bit, this makes sure JS validations are the same or stricter than the native ones to make sure we get consistent errors across platforms. See facebook#12110 for an example of an error that now gets caught by JS validations. Also added snapshot tests for the errors to make sure `processTransform` throws when passing invalid values. It only tests the validation since the object parsing is now done natively for iOS and Android. **Test plan** Test that there are no errors in UIExplorer Run new unit tests Closes facebook#12115 Differential Revision: D4488933 Pulled By: mkonicek fbshipit-source-id: a714e6175b2892284a44c870506165099efec1ed

Summary: In theory, we should be able to animate any non-layout property, including custom ones. While there is still work to be done on the native side to fully enable this, we should start by dropping the prop whitelist. Closes facebook/react-native#10658 Differential Revision: D4379031 Pulled By: ericvicenti fbshipit-source-id: fe9c30ea101e93a8b260d7d09a909fafbb82fee6

Summary: In theory, we should be able to animate any non-layout property, including custom ones. While there is still work to be done on the native side to fully enable this, we should start by dropping the prop whitelist. Closes facebook#10658 Differential Revision: D4379031 Pulled By: ericvicenti fbshipit-source-id: fe9c30ea101e93a8b260d7d09a909fafbb82fee6