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

[Animated] Implement modulo operator method #5743

Closed
wants to merge 1 commit into from
Closed

[Animated] Implement modulo operator method #5743

wants to merge 1 commit into from

Conversation

rt2zz
Copy link
Contributor

@rt2zz rt2zz commented Feb 3, 2016

This is an initial take on how to allow for value wrapping in Animated (currently .timing only). It adds a wrap config options which is an array specifying a range of values to be wrapped around. Anytime the delta of toValue and fromValue > wrapDistance / 2 instead of animating the long way it will animate towards the range extremity and then after breaching the range it will readjust to animated to the true toValue.

Example Usage:

Animated.timing(
  this.state.animatedDegrees, 
  {toValue: Math.random()*360, wrap: [0, 360]}
).start()

This is presumably very valuable in rotation based animations, but could also be useful in other applications like wrapping the edges of the viewport.

Questions & Todo:

  • Is wrap as a config key semantically meaningful and accurate?
  • Is there a way to expose this as a config option on .interpolate rather than timing
  • Generalize to all animated API's (spring, decay), and will this work with ValueXY out of the box?
  • Add tests

@facebook-github-bot
Copy link
Contributor

By analyzing the blame information on this pull request, we identified @sahrens, @lelandrichardson and @ide to be potential reviewers.

@facebook-github-bot facebook-github-bot added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Feb 3, 2016
@lelandrichardson
Copy link
Contributor

I was just thinking about ways to do this the other day.
What if we did something like:

var newAnimatedValue = Animated.modulo(animatedValue, num);

Similar to Animated.multiply and Animated.add

@rt2zz
Copy link
Contributor Author

rt2zz commented Feb 3, 2016

@lelandrichardson I like that api a lot better, but I could not figure out how to make it work since the underlying Animation (spring/timing/decay) has already chosen the direction to go.

I suppose it could work as an Animated.modulo method which as a consumer you would pair with some wrapping logic like follows:

state = {
  rotation: Animated.value(0)
}

// ...

let degrees = Math.random()*360
if (lastDegreeValue - degrees > 180) degrees += 360
if (lastDegreeValue - degrees < -180) degrees -= 360
Animated.timing(this.state.rotation, {toValue: degrees}).start()

// ...

<Thingy style={{transform: [{rotate: Animated.modulo(this.state.rotation, 360)]}} />

@lelandrichardson
Copy link
Contributor

@rt2zz right. I think this is also better since it doesn't depend on using the .timing(...) API. For example, if you want to have the dial of a clock move around, tracking the value of a pan or gesture. This is also more consistent with the existing add and multiply APIs.

@rt2zz
Copy link
Contributor Author

rt2zz commented Feb 3, 2016

I love the purity of the .modulo(value, modulus) but I think we need to figure out a way to support arbitrary ranges e.g. [-90, 90].

Because modulo in js is already messed up ( -1 % 2 = -1), imo we should adopt a more explicit array based api such as:
Animated.modulo(value, [start, end])
Animated.moduloRange(value, [start, end])

@lelandrichardson
Copy link
Contributor

@rt2zz any modulo range could be expressed with a combination of modulo + interpolate though.

@rt2zz
Copy link
Contributor Author

rt2zz commented Feb 4, 2016

True but as a consumer you would then always have to map your raw input values to a (0, n) or (-n, n) range only to interpolate them back.

Maybe @sahrens or others can weigh in on api design - I am ok either way but prefer having an explicit range.

@lelandrichardson
Copy link
Contributor

That's fair. A range would be very easy to do regardless....

(val) => val % mod;

versus

(val) => ((val - min) % (max - min)) + min;

@@ -145,6 +145,7 @@ type TimingAnimationConfigSingle = AnimationConfig & {
easing?: (value: number) => number;
duration?: number;
delay?: number;
wrap?: array;

Choose a reason for hiding this comment

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

identifier array Could not resolve name

@rt2zz
Copy link
Contributor Author

rt2zz commented Feb 4, 2016

Note: I will be completely redoing this next week per @lelandrichardson suggestion

@facebook-github-bot
Copy link
Contributor

@rt2zz updated the pull request.

@rt2zz rt2zz changed the title [Animated] Add wrap config to timing [Animated] Implement modulo operator method Feb 4, 2016
@rt2zz
Copy link
Contributor Author

rt2zz commented Feb 4, 2016

Ok I updated the PR per leland's recommendation. This now adds a Animated.modulo method.

Note: ecmascript thinks -a % b should be a negative number which is annoying. I implemented the non-negative version of modulus via (a % modulus + modulus) % modulus. We can probably get some performance gains by only conditionally applying the adjustment for negative values of a.

@lelandrichardson
Copy link
Contributor

@rt2zz is the -a % b thing just an order of operations? ie, would (-a) % b fix it?

Edit: nvm, that doesn't change anything.

@rt2zz
Copy link
Contributor Author

rt2zz commented Feb 5, 2016

@lelandrichardson ya I guess I should have been more explicit i.e. a % b < 0 for any a less than zero

@facebook-github-bot
Copy link
Contributor

@rt2zz updated the pull request.

@rt2zz
Copy link
Contributor Author

rt2zz commented Feb 25, 2016

I just rebased master. The diff is pretty trivial and this functionality cannot be achieved through any other means.

@lelandrichardson
Copy link
Contributor

@facebook-github-bot shipit

@facebook-github-bot
Copy link
Contributor

Thanks for importing. If you are an FB employee go to Phabricator to review.

@ghost ghost closed this in ae0ad1f Feb 26, 2016
pglotov pushed a commit to pglotov/react-native that referenced this pull request Mar 15, 2016
Summary:This is an initial take on how to allow for value wrapping in Animated (currently `.timing` only). It adds a wrap config options which is an array specifying a range of values to be wrapped around. Anytime the delta of `toValue and fromValue > wrapDistance / 2` instead of animating the long way it will animate towards the range extremity and then after breaching the range it will readjust to animated to the true toValue.

Example Usage:
```js
Animated.timing(
  this.state.animatedDegrees,
  {toValue: Math.random()*360, wrap: [0, 360]}
).start()
```

This is presumably very valuable in rotation based animations, but could also be useful in other applications like wrapping the edges of the viewport.

Questions & Todo:
- Is `wrap` as a config key semantically meaningful and accurate?
- Is there a way to expose this as a config option on `.interpolate` rather than timing
- Generalize to all animated API's (spring, decay), and will this work with ValueXY out of the box?
- Add tests
Closes facebook#5743

Differential Revision: D2979992

fb-gh-sync-id: 69be510feba8c43acb10c253036f5854adff9258
shipit-source-id: 69be510feba8c43acb10c253036f5854adff9258
@rt2zz rt2zz deleted the AnimatedWrap branch May 5, 2016 15:38
@mileung
Copy link

mileung commented Jul 10, 2017

-a % b should be a negative number because that's how js behaves. In my case, I have a swipeX Animated Value and it can be + or -. When I use modulo on swipeX, I lose which direction swipeX is going in. Now I guess I need to have another swipeDirection variable which is either 1 or -1 and multiply it by the result of modulo to make it the value it should be if it behaved like js %.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. JavaScript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants