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

Replace Tween interpolaters by properly maintained library of easing equations #22513

Closed
akien-mga opened this issue Sep 28, 2018 · 2 comments · Fixed by #50165
Closed

Replace Tween interpolaters by properly maintained library of easing equations #22513

akien-mga opened this issue Sep 28, 2018 · 2 comments · Fixed by #50165

Comments

@akien-mga
Copy link
Member

Godot version:
Master branch (82f2674)

OS/device including version:
Any.

Issue description:
The Tween implementation initially contributed in #628 came with a stealth copy of https://github.com/jesusgollonet/ofpennereasing in the file scene/animation/tween_interpolaters.cpp, since renamed to thirdparty/misc/easing_equations.cpp for clarity.

It's heavily modified to use Godot types like real_t and hook into the Tween API, and eventually passed through our clang-format before we knew that it was thirdparty code, so it's now difficult to sync with upstream. "Luckily", upstream is inactive and there are bad unresolved issues in that code which range from unreadable code to undefined behaviour.

To anyone interested, I would suggest to:

  • Either find a replacement library that implements similar functionality in a cleaner way. The library should be small (ideally one header) and have good code quality. It could be another, better maintained implementation of Penner's equations.
  • Or refactor the current file to fix the various issues in it (which are not simply a matter of renaming variables like in Readability of tween_interpolaters.cpp improved, fixes #21600 #21608, there's actual bad code in there beyond the name of its variables). If so we can maybe move it back to scene/animation/, but at any case I'd like the Tween glue in this file to be moved back to tween.cpp, and the easing_equations.cpp turned into a header.

References:

@spongeboburu
Copy link

Hi! :)

Since we were talking about it the other day, my suggestion would be to just write our own class and integrate it like a core type of Godot.

All of Penner's functions are well described (here for instance http://www.gizma.com/easing/), so it shouldn't be more than a quick copy paste with a few improvements.

Since interpolation is both generic and common providing Penner's functions along with a few other useful ones (such as bezier) should be enough for all scenarios, if not we can provide a "custom" function (as is done already).

The interpolation type could replace the code in both Tween and Curve, have it's own editor plugin and be available in the script languages. Maybe it also contain the "fire and forget", possibly integrating it into Timer.

@KoBeWi
Copy link
Member

KoBeWi commented Jul 3, 2021

I thought about giving a shot at finally closing this issue, but I have some questions.

The bugged behavior is mostly caused by in-equation assignments. This should be very easy to fix by doing general refactoring to make the code clean™, but is it enough to resolve the issue? I mean, does refactored third-party code count as original code or does it need to be rewritten completely? (I don't know how it could be, if we are implementing the same equations)

Another thing is the code organisation. It uses namespaces with static methods and then stores method pointers in array. It's extremely performant, but if we were to move it out of thirdparty, can this be preserved? We don't use namespaces anywhere (I think) and the whole structure doesn't look like typical Godot code. IMO it's not wrong, but I'm just not sure if it has right to exist outside third-party xd It could be changed, maybe even in a way that allows for easy custom interpolaters, but then it might be less performant.

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

Successfully merging a pull request may close this issue.

3 participants