Remove concept of "normalizedPosition" and "normalizedVelocity"
ClosedPublic

Authored by skevy on Aug 9 2017, 8:41 PM.

Details

Summary

Removes the normalizedPosition and normalizedVelocity methods, and modifies the math to not lerp the position/velocity.

Test Plan

Run tests. Test this version with the demo-react in the material-motion-js repo.

Diff Detail

Repository
R51 wobble
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
skevy created this revision.Aug 9 2017, 8:41 PM
skevy added inline comments.Aug 9 2017, 8:43 PM
src/index.js
161

When this is set, it seems to always break the animation.

Restricted Application completed building Restricted Buildable.Aug 9 2017, 8:43 PM
appsforartists added inline comments.Aug 9 2017, 9:54 PM
src/index.js
42

rAF returns a non-zero number. Should we just use 0 as the bottom value and simplify this type?

84

which would simplify these checks too.

115–116

dead comment

124–125

more dead comment

161

This was a side effect of the normalization, so it's probably OK that it's removed here. Comment should be cleaned up to reflect it.

But, there's probably some cleverness that needs to happen here. If you have a simulation running with currentValue = 50 and someone calls fromValue: 100, toValue: 200, how should the spring respond? It's probably

if (updatedConfig.hasOwnProperty('fromValue')) {
  this._currentPosition = fromValue;
}
if (updatedConfig.hasOwnProperty('toValue')) {
  this._config.fromValue = this._currentPosition;
}
225

It's interesting that fromValue isn't used in the actual math. If fromValue was non-zero, should it appear in the equation, or is this enough to take care of it?

293

From a naming POV, there's room to disambiugate the method _isSpringAtRest() and the state _springAtRest.

331

isNoDisplacement && isNoVelocity would be more accurate names.

appsforartists added inline comments.Aug 9 2017, 11:35 PM
wobble.d.ts
29

We can remove the calculated against sections in both these comments, since there's only one of each property now.

29

I also wonder if we should call this value, to be symmetrical with fromValue and toValue.

appsforartists added inline comments.Aug 9 2017, 11:37 PM
wobble.d.ts
29

or perhaps currentValue and currentVelocity.

skevy updated this revision to Diff 11386.Aug 10 2017, 4:28 AM
skevy marked 9 inline comments as done.
  • Address nits
skevy added inline comments.Aug 10 2017, 4:30 AM
src/index.js
161

Talked about this in Discord -- addressing in new revision

225

I did test this -- but you're right that it's non-intuitive. I'll add a regression test.

It is indeed enough to take care of it -- x0 is the initial displacement of the spring, and x(t) (with the math as it is right now) will always move toward 0.

Where t_0 = 0 and t_n = time it takes for the spring to settle

x(t_0) = toValue - x0 = toValue - (toValue - fromValue) = fromValue
x(t_n) = toValue - lim(t -> inf)(x(t)) = toValue - 0 = toValue

Lol this is hard to write without math symbols but hopefully you get the point.

331

Indeed

wobble.d.ts
29

My bad -- edited this at the last minute and didn't notice the comments haha. Thanks for the catch!

Restricted Application failed to build Restricted Buildable!Aug 10 2017, 4:30 AM
skevy updated this revision to Diff 11387.Aug 10 2017, 4:33 AM

Fix diff

Restricted Application failed to build Restricted Buildable!Aug 10 2017, 4:35 AM
This revision was automatically updated to reflect the committed changes.