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

Add TRANS_SPRING interpolation option to tweens #58999

Merged
merged 0 commits into from
Aug 21, 2022

Conversation

rakkarage
Copy link
Contributor

Please consider adding this sping tween.
I really miss it.
I am not sure where this easing function came from?
I think I first saw it in iTween for Unity. https://github.com/jtothebell/iTween/blob/master/iTween.cs
It is similar to back or elastic but different enough to notice and seems better suited for ui elements like when OSX and iOS scroll a list past the end and 'spring' back?

Here is an animation from (https://www.protopie.io/learn/docs/interactions/animation-curves) just to show what it looks like kinda? This one is simpler with no parameters.
spring.gif

It only has one direction, out. Possibly more could be calculated for symmetry, but I don't think it would ever be used anyway, and that is the way I always 'find' the code.

Here is a very simple test project: https://github.com/rakkarage/TestTween (4.0 branch!)

Please & Thanks.

@rakkarage rakkarage requested a review from a team as a code owner March 10, 2022 21:06
@Calinou
Copy link
Member

Calinou commented Mar 10, 2022

Thanks for opening a pull request 🙂

Feature pull requests should be associated to a feature proposal to justify the need for implementing the feature in Godot core. Please open a proposal on the Godot proposals repository (and link to this pull request in the proposal's body).

@KoBeWi
Copy link
Member

KoBeWi commented Apr 12, 2022

From what you said about spring transition, it has some parameters to customize it. Is it possible to tweak it, so that it doesn't behave the same with different eases? The PR looks good otherwise.

@rakkarage
Copy link
Contributor Author

https://humpf.etienne.tech/article

this was just to kinda show how it works...

  1. It has to be simple with no parameters to fit into the easing system. It only takes four parameters. I think someone just made a version of this damped spring code with hard coded parameters maybe idk. None of the eases have any extra parameters. can maybe add another more complex damped spring system to the physics system if needed?
  2. It is not exactly the same each ease. If you ease far in short rime the spring is fast else it springs slower.
  3. having a 'fixed 'spring better then having no spring?

Thanks.

@KoBeWi
Copy link
Member

KoBeWi commented Apr 13, 2022

Well, currently Tweens don't support easing/transition parameters, because there was no need. Adding a new transition type that fits the current system is ok, but any more than that... It'd need some specific tweening functions, and they can be implemented in GDScript using method tweening. Supporting it in core would require some more changes.

@Chaosus Chaosus added this to the 4.0 milestone Aug 13, 2022
@YuriSizov YuriSizov modified the milestones: 4.0, 4.x Aug 18, 2022
@YuriSizov
Copy link
Contributor

Based on @KoBeWi's review it seems that a proper solution would need more complex changes than proposed here. Either way it's not breaking compat and won't break compat, so we can bump it to 4.x until an agreement is reached.

@rakkarage
Copy link
Contributor Author

rakkarage commented Aug 18, 2022

Well, currently Tweens don't support easing/transition parameters, because there was no need.

That is fine, because this code/proposal does not need any new parameters and fits perfectly into the existing system with no changes.

Adding a new transition type that fits the current system is ok, but any more than that... It'd need some specific tweening functions, and they can be implemented in GDScript using method tweening. Supporting it in core would require some more changes.

That is what I did. Added a new transition that fits the current system. It has been tested and works in the linked pull request. as can be seen the in gifs. What other functions does it need?

The linked article is not the proposal... just showing what a spring tween looks like.

it seems that a proper solution would need more complex changes than proposed here

I do not think that is the case... I think there was a misunderstanding when I posted that article but this is a simple four line function with the same parameters and inputs and outputs as all the other tween functions... the missing spring tween

s it possible to tweak it, so that it doesn't behave the same with different eases

I am not sure exactly how it works or what your are asking but it does seem to 'spring' more if the point are further away already

Thanks.

@TokisanGames
Copy link
Contributor

This looks like a great function that I think belongs in the base system, being a very common transition type.

@rakkarage You should squash your commits into one.

@YuriSizov
Copy link
Contributor

I do not think that is the case... I think there was a misunderstanding when I posted that article but this is a simple four line function with the same parameters and inputs and outputs as all the other tween functions... the missing spring tween

Either way, it's not blocking 4.0. It needs to be reviewed by a maintainer of the area (likely @KoBeWi himself). It also needs to be rebased and squashed, as mentioned above.

@KoBeWi
Copy link
Member

KoBeWi commented Aug 18, 2022

This

&spring::out, &spring::out, &spring::out, &spring::out }, // Spring is the same for each easing.

Can you come up with different easing for in, in/out and out/in, by tweaking the parameters? So that the transition doesn't use the same function for each easing.

@rakkarage
Copy link
Contributor Author

rakkarage commented Aug 18, 2022

Thanks. I will try to squish.
Sorry I don't really know what I am doing... I might just refork...

&spring::out, &spring::out, &spring::out, &spring::out }, // Spring is the same for each easing.

Can you come up with different easing for in, in/out and out/in, by tweaking the parameters? So that the transition doesn't use the same function for each easing.

no i do not think i can... it always springs at end and I think that makes sense logically... Other tween (linear) behave same way
if it is simulating a spring is there ever a use case where you would not want it to spring after
I guess it could be easily flipped with clever math? idk but this is the function as it is commonly used

@KoBeWi
Copy link
Member

KoBeWi commented Aug 18, 2022

Linear has single easing, because it's impossible to tweak it. But spring does have some parameters, so it should be possible. If it's not easy to do then it's probably ok to have it like this for now. But it should be mentioned in the docs.

@TokisanGames
Copy link
Contributor

TokisanGames commented Aug 18, 2022

Thanks. I will try to squish. Sorry I don't really know what I am doing... I might just refork...

Don't refork. Just squash or fixup.
https://docs.godotengine.org/en/latest/community/contributing/pr_workflow.html#the-interactive-rebase

Also rebase. Really you should read the whole page.
https://docs.godotengine.org/en/latest/community/contributing/pr_workflow.html#updating-your-branch

@rakkarage rakkarage requested review from a team as code owners August 19, 2022 14:47
@YuriSizov
Copy link
Contributor

You need to rebase this PR and squash your commits. Your PR must only contain one commit that only includes your changes. Currently it contains unrelated changes, probably because you merged master in your branch. You need to rebase instead.

@YuriSizov YuriSizov changed the title spring tween Add TRANS_SPRING interpolation option to tweens Aug 19, 2022
@rakkarage rakkarage requested review from a team as code owners August 19, 2022 18:28
@rakkarage
Copy link
Contributor Author

thanks...
rakkarage@4e00512
i think this did it?
sorry

@aaronfranke
Copy link
Member

@rakkarage At the top of the PR, it says "Commits 61". This number needs to be 1.

@rakkarage rakkarage merged commit d609017 into godotengine:master Aug 21, 2022
@TokageItLab
Copy link
Member

TokageItLab commented Aug 21, 2022

@rakkarage You seem to have reset too much and the all commit are gone. I recommend opening the PR again as new PR.

@KoBeWi
Copy link
Member

KoBeWi commented Aug 21, 2022

Make sure to open it on a new branch that isn't master.

@YuriSizov
Copy link
Contributor

YuriSizov commented Aug 22, 2022

For future references, this was not merged and is superseded by #64680 #76899.

@akien-mga akien-mga modified the milestones: 4.x, 4.0 Dec 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants