Only start spring if toValue has changed
ClosedPublic

Authored by appsforartists on Aug 10 2017, 10:58 PM.

Details

Summary

I have a hypothesis that the only time the spring should automatically start is if toValue changes. If the spring is already animating when the config is updated, it will continue to animate with new parameters. If it is already at rest when a setting or an initial value is changed, the spring should probably stay put.

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.
Restricted Application added a reviewer: skevy. · View Herald TranscriptAug 10 2017, 10:58 PM
Restricted Application completed building Restricted Buildable.Aug 10 2017, 11:00 PM
skevy added a comment.Aug 13 2017, 1:57 PM

I think if we do this, we should write a test for it.

It seems like if we set fromValue or initialVelocity in updateConfig without restarting the spring, some weird things would happen because we're changing x0 and v0.

Hmm, maybe I should try this and graph the values so I can visualize what happens.

Ahh - right. You need to reset the time for the simulation to continue with those values.

We could either break out the spring reset logic, or only call start if either toValue is set or initialVelocity/fromValue is set and _currentAnimationStep is set.

Do you agree that setting values other than toValue shouldn't automatically start the simulation?

Breaking _reset() out into its own method to handle changes to fromValue or initialVelocity while at rest.

Restricted Application completed building Restricted Buildable.Aug 14 2017, 6:08 PM
Restricted Application completed building Restricted Buildable.Aug 14 2017, 6:10 PM
skevy requested changes to this revision.Aug 14 2017, 6:17 PM
skevy added inline comments.
src/index.js
219

Since calling _reset resets currentTime to 0.0, it's unclear to me exactly what change this diff makes in practical terms. It seems like calling _reset is basically just like calling start, except without queuing the rAF.

If the point is to just not start the animation when it isn't already running if the values change -- it seems like we should:

  • Keep the _reset method you pulled out
  • _never_ start the animation with a call to updateConfig
  • Change updateConfig back to it's initial implementation, replacing this.start() with this._reset().

To be explicit, the new behavior will allow an animating spring that has it's config changed to behave correctly, and a spring that is at rest needs to be explicitly started again with .start().

This revision now requires changes to proceed.Aug 14 2017, 6:17 PM

Rebase off HEAD

Restricted Application completed building Restricted Buildable.Aug 14 2017, 9:12 PM
skevy accepted this revision.Aug 14 2017, 10:05 PM
This revision is now accepted and ready to land.Aug 14 2017, 10:05 PM
This revision was automatically updated to reflect the committed changes.