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

Simplify Tween, remove some stuff, make it easier to maintain. #655

Open
1 of 15 tasks
trusktr opened this issue Jun 23, 2023 · 9 comments
Open
1 of 15 tasks

Simplify Tween, remove some stuff, make it easier to maintain. #655

trusktr opened this issue Jun 23, 2023 · 9 comments

Comments

@trusktr
Copy link
Member

trusktr commented Jun 23, 2023

Proposed (welcome to debate):

  • Ensure maximal ES Module support
  • remove legacy CommonJS after Electron ESM comes out
  • Remove the autostart parameter from tween.update(time, autostart). It makes the API less intuitive: for example if you call stop() and then the next animation frame calls update() and restarts the tween, it seems like stop() is not working. Better to make the APIs explicit: tweens will start and stop only when you tell them to, regardless if update is being called, and update does only one thing well (update the time).
    • This also allows us to eliminate confusion with onStart. Currently you call tween.start(), but onStart will be be fired until update() is called (f.e. in the next animation frame). Instead, we can make it be called immediately so that any state that should be initially set for the tween is guaranteed to be set, regardless of update().
  • Remove the TWEEN singleton, make Tweens start with no Group by default
    • docs: Teach people how to make a Tween and update it directly.
    • docs: Teach them that if they want to update multiple Tweens at once, they must make a Group.
    • This eliminates possible errors people will have when they clash tweens from two or more separate pieces of code (when using the current TWEEN)
    • explicitly managing each tween, or explicitly making Groups, will naturally lead to people writing componentized code with lowered chance of issues.
    • this leads to understanding of what is going on, rather than proper code being non-obvious up front
  • Remove relative values. It involves detecting strings for little convenience. People can just use math on their side with not much more effort.
  • Remove the dynamic-to feature. It isn't used often, and much internal complication.
  • Remove array of to-values.
    • Also adds quite some complexity.
    • It seems that when we do this, we inadvertently destroy the meaning of an easing curve because the actual number output is no longer on that curve so the meaning is obscure, unless we do very special math to determine proper values to use such that a new curve makes sense.
    • The same math skill required for the previous point may instead be used to chain multiple tweens together, which gives more capabilities such as specific curves between each value. To emulate the array of values, chain tweens together with linear curves, then interpolate all of them with a single higher-level tween using a single curve.
    • The previous point allows much more options than the inflexible feature of to-values. To-values is a shortcut for the previous point with a linear chain and directing tween.
    • Downside: lose ability to choose interpolation method, f.e. bezier, which is useful for making a smooth curve out of the discrete to-values (f.e. "animate along a smooth path calculated from the discrete sharp values").
      • provide an alternative that doesn't complicate tween directly? F.e. Three.js has Path and Curve tools, instead animate position along such primitives specialized for that purpose. Perhaps we show a demo of that here, after removing to-values.
  • Remove or replace .yoyo. Provide a reverse function that can return a new tween instance with reversed data.
    • simplifies tween logic.
    • then: tween.chain(tween.reverse().chain(tween))
    • Perhaps instead of removing, tween.yoyo() can be a shortcut for the previous point. The main idea is that the yoyo state is not intertwined inside the regular tween state like it currently is. Instead, two separate tweens each have their own state.
  • Replace tween.chain with a Sequence (or more advanced Timeline) feature.
    • This composes tween independently of each other, so they don't need to have state remembering who they are chained to. This allows way more flexible new options (Timeline) like playing tweens in parallel, not just in sequence, etc.
  • improved testing.
    • run unit tests in browsers, because that's where Tween is used more often.
    • Add a way to perform visual snapshot testing so that we may catch issues that unit tests may not, by snapshotting results of visual animations

What else?

@trusktr trusktr pinned this issue Jul 9, 2023
@trusktr
Copy link
Member Author

trusktr commented Jul 9, 2023

@MasatoMakino hello! :) If you have some time, we could jam on these ideas.

@MasatoMakino
Copy link
Contributor

@trusktr hello!

TWEEN singleton.

docs: Teach them that if they want to update multiple Tweens at once, they must make a Group.

Does this mean the mainGroup is deprecated?

Remove the dynamic-to and array of to-values feature

I agree with the removal of dynamic-to and to-array. These are very large features. If necessary, I think they should be separated into independent npm packages or replaced with reliable packages.

I agree with the policy of duplicating tween in utility functions rather than adding functionality to tween instances. it makes Tween.ts easier to maintain.

Deprecated steps

Due to the removal of feature, Tween.js users should be given time to prepare.

  • Add deprecated comment to tsdoc
  • Display a warning in the browser console if a deprecated feature is used.
  • Change the major version of the package after removing the feature.

To display warning messages, you can refer to deprecation.ts in pixi.js. This approach is user-friendly because the same warning is not displayed more than once.

https://github.com/pixijs/pixijs/blob/v7.2.4/packages/utils/src/logging/deprecation.ts

Visual test

This is related to what is core of Tween.js.

  1. Tween.js is a library for value completion.
  2. Tween.js is a library for drawing animations.

If 1, then visual testing is not necessary. Tween.js is only responsible for outputting reproducible values, not for rendering.
If it is 2, then browser testing is required.

I use Tween.js in combination with three.js and Pixi.js. In this case, Tween.js is responsible for 1.

@trusktr
Copy link
Member Author

trusktr commented Jul 12, 2023

Does this mean the mainGroup is deprecated?

Yeah, that would be removed. Instead, people would:

  • create and control individual tweens
  • create groups if they want to control multiple tweens

Similar to the good practice of avoiding global variables.


Pretty much agree with all you said. We definitely need to release the breaking changes in a major version.

Tween has numerical outputs, but a few visual tests might catch some edge cases, maybe later once we have a separate Timeline feature. Not sure if worth it yet.

@NyaNguyen
Copy link

dynamic-to option is an amazing thing and should not be removed. I've used it and that adds AI-like behaviour to the tween.

The Group object still needs a lot of work. Pausing tweens inside a Group terminates them completely, which is not correct. We should be able to do a .startAll() and .pauseAll() on a Group. A .restartAll() and .reverseAll() would be great also. Of course, those functions used on a single tween would be good also.

@trusktr
Copy link
Member Author

trusktr commented Jul 15, 2023

@nya13 thank you for the input!

Pausing tweens inside a Group terminates them completely, which is not correct

That may indeed be true. The currentTWEEN group should have the same behavior, as it is just a Group, so I'd consider it a separate issue. Can you please open a separate ticket for that? EDIT: I see the new ticket, #664. Better to just do that at first. Thanks for opening!

dynamic-to option is an amazing thing and should not be removed. I've used it and that adds AI-like behaviour to the tween.

Indeed I agree the use case can be desirable, but perhaps the way we currently achieve that use case blurs the concept of a Tween (the curve on which a number transitions between two values), plus makes the Tween class a bunch more complicated.

Instead, I believe we can come up with cleaner ways to achieve the same effects. For example, if we keep Tween simple, then we can instead animate along a path, and modify the path dynamically (f.e. modify the path connecting 🐇 that the 🦊 is chasing). In that rabbit-fox example, the concept is actually two concepts that are currently blended together in an unclear way: we have a tween, and we're animating along a path (and the path happens to change shape dynamically). If we update that example to clearly show those two concepts separately, the example will be semantically clear while the Tween class can be simplified.

I think we can perhaps provide additional utilities (at copyable from examples) to make this sort of thing easy.

Can you provide any more use cases?

@trusktr
Copy link
Member Author

trusktr commented Aug 6, 2023

I added a new one to the list:

Remove the autostart parameter from tween.update(time, autostart). It makes the API less intuitive: for example if you call stop() and then the next animation frame calls update() and restarts the tween, it seems like stop() is not working. Better to make the APIs explicit: tweens will start and stop only when you tell them to, regardless if update is being called, and update does only one thing well (update the time).

@ronaldcurtis
Copy link

ronaldcurtis commented Aug 27, 2023

remove legacy CommonJS

Would be good if we don't remove CommonJS compatibility just yet, as Electron does not yet support ES modules (link).

@trusktr
Copy link
Member Author

trusktr commented Jan 15, 2024

@ronaldcurtis Good point! Haven't removed CJS yet (in fact, the CJS exports condition in package.json is fixed).

Looks like ESM support in Electron finally landed in August! Docs:

https://www.electronjs.org/docs/latest/tutorial/esm

Are you using Tween.js with Electron? If so, can you try it out? If it works fine, we can mark the CommonJS module as deprecated with a console.warn, and plan to remove it after some time.

@ronaldcurtis
Copy link

Looks like ESM support in Electron finally landed in August! Docs:
🎉

@trusktr Not using Tween.js anymore, but as you mentioned it looks like Electron supports ESM now, so it should be fine to deprecate ^_^

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants