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

[NativeAnimated] Support the Slow Animations option of the iOS simulator #21157

Closed

Conversation

janicduplessis
Copy link
Contributor

RN animations currently ignore the Slow Animations option on the iOS simulator because we don't use UIKit animations directly. This uses a private api to get the slow coefficient and use it in the native animated driver. We only compile the private api code on simulator so this won't cause issues for app store approval. One possible issue is that the api changes in new iOS versions but I think it's reasonable to do this.

Note that this won't work with JS driven animations, we could expose the slow coefficient as a constant and use that in JS but I decided not to implement it.

Test Plan:

Tested that animations are properly slowed like native UIKit ones in RNTester native animated example.

Release Notes:

[IOS] [ENHANCEMENT] [NativeAnimated] - Support the Slow Animations option of the iOS simulator

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Sep 17, 2018
@janicduplessis
Copy link
Contributor Author

cc @kmagiera

Could be nice to integrate this in reanimated too.

@TheSavior
Copy link
Member

Including a video in your test plan would be cool

Copy link
Contributor

@sahrens sahrens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sweet, I have wanted this for so long. Would be nice if there was a single point you could tweak, rather than hacking the math in all the animation types separately, but probably not worth refactoring for that.

@facebook-github-bot facebook-github-bot added the Import Started This pull request has been imported. This does not imply the PR has been approved. label Sep 20, 2018
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sahrens is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@react-native-bot
Copy link
Collaborator

@janicduplessis merged commit 40bcc38 into facebook:master.


Once this commit is added to a release, you will see the corresponding version tag below the description at 40bcc38. If the commit has a single master tag, it is not yet part of a release.

@facebook facebook locked as resolved and limited conversation to collaborators Sep 20, 2018
@react-native-bot react-native-bot added the Merged This PR has been merged. label Sep 20, 2018
@sahrens
Copy link
Contributor

sahrens commented Sep 27, 2018

@janicduplessis: we're seeing animated test failures after merging this - any idea why that could be? e.g.

RCTNativeAnimatedNodesManagerTests/testSpringAnimationLoop:
Failure: ((didComeToRest) is true) failedRNTester/RNTesterUnitTests/RCTNativeAnimatedNodesManagerTests.m:484

RCTNativeAnimatedNodesManagerTests/testAnimationCallbackFinish:
Failure: ((endCallbackCalls) equal to (1)) failed: ("0") is not equal to ("1")
RNTester/RNTesterUnitTests/RCTNativeAnimatedNodesManagerTests.m:514

CGFloat RCTAnimationDragCoefficient()
{
#if TARGET_IPHONE_SIMULATOR
return (CGFloat)UIAnimationDragCoefficient();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on this POP code, maybe we need to guard against 0 for some reason?

https://github.com/facebook/pop/blob/master/pop/POPAnimationExtras.mm#L37

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope. For some reason the value is 10 when testing...I'll fix it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh that's strange. Thanks for investigating this!

@hramos hramos removed Import Started This pull request has been imported. This does not imply the PR has been approved. labels Feb 6, 2019
t-nanava pushed a commit to microsoft/react-native-macos that referenced this pull request Jun 17, 2019
…1157)

Summary:
RN animations currently ignore the `Slow Animations` option on the iOS simulator because we don't use UIKit animations directly. This uses a private api to get the slow coefficient and use it in the native animated driver. We only compile the private api code on simulator so this won't cause issues for app store approval. One possible issue is that the api changes in new iOS versions but I think it's reasonable to do this.

Note that this won't work with JS driven animations, we could expose the slow coefficient as a constant and use that in JS but I decided not to implement it.
Pull Request resolved: facebook#21157

Differential Revision: D9980306

Pulled By: sahrens

fbshipit-source-id: bdbce2e469261a75cb4b9a251e8e8f212bb9c4e7
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
API: Animated CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. Platform: iOS iOS applications.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants