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 transition parameters to Tween #82155

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

KoBeWi
Copy link
Member

@KoBeWi KoBeWi commented Sep 22, 2023

Closes godotengine/godot-proposals#7802
tbh this change makes the code more complex than it's probably worth it. I don't think there is better way though; I didn't want it to affect performance.

TODO:

  • figure out amplitude, period for BACK equations It doesn't use them?
  • figure out overshoot, amplitude, period for ELASTIC equations It's period and damping now.
  • TRANS_SPRING might also get parameters, as it's similar to elastic
  • maybe other transitions might get parameters too?
  • add compat methods

I'll need help with the equations. I did overshoot for TRANS_BACK, because it was obvious. I'd appreciate if someone could just drop me implementations for other equations >_> (I have no idea about these numbers)

Here's BACK with default overshoot and 5

godot.windows.editor.dev.x86_64_8ckv8JHLDB.mp4

@Qcaria
Copy link

Qcaria commented Sep 23, 2023

In my opinion only the overshoot parameter makes sense in the BACK equations, as it is a single bump (so the frequency is fixed) and amplitude and overshot would do the same.

On the other hand, for the elastic equations there are two fixed parameters that have the role of frequency and amplitude (again overshoot in this context means the same as amplitude to me).

The amplitude can be controlled by changing for example the 10 in float a = c * pow(2, 10 * t);. The frequency could be controlled by changing the 0.3 in float p = d * 0.3f; which corresponds more or less to a period (so the smaller it is the higher the frequency).

I attach a couple of images to show the effect of modifying these two parameters.
Changing the frequency parameter:
Captura de pantalla 2023-09-23 a las 10 55 27

Changing the amplitude parameter:
Captura de pantalla 2023-09-23 a las 10 56 15

This is my first comment, I hope to not be messing up the style too much.

@KoBeWi
Copy link
Member Author

KoBeWi commented Sep 23, 2023

Thanks, I pushed the parameters for elastic transition. I renamed amplitude to damping, because it seems to have reverse effect.

Here's elastic with different parameters:

5WLq8Nf6gO.mp4

1 is default, 2 is period 0.1, 3 is amplitude 5
The parameters have slightly different effect depending on ease (EASE_OUT doesn't even use damping).

Seems like the third parameter is now unused, so I guess I will be removing it.

@KoBeWi KoBeWi marked this pull request as ready for review September 24, 2023 20:24
@KoBeWi KoBeWi requested review from a team as code owners September 24, 2023 20:24
@KoBeWi
Copy link
Member Author

KoBeWi commented Sep 24, 2023

Ok I removed the third parameter. I also tried to de-hardcode the parameter count. While I can't do much about the arguments, I made arrays use a constant and also changed easing equations to use a macro (the method definitions are super-repetitive).

The nice thing about the new parameters is that, as long as their count remains constant, we can tweak other easings without breaking compatibility at all. So if TRANS_SPRING or any other transitions were to use the parameters, it can be added at any point.

The PR is more or less finished now.

@KoBeWi KoBeWi force-pushed the elastic_tweening branch 2 times, most recently from 8041240 to 04d0734 Compare September 25, 2023 13:10
@KoBeWi KoBeWi requested a review from a team as a code owner September 25, 2023 13:10
@KoBeWi KoBeWi requested a review from a team as a code owner September 25, 2023 15:14
@KoBeWi KoBeWi force-pushed the elastic_tweening branch 3 times, most recently from 130b80c to 5726065 Compare September 25, 2023 16:03
@TokageItLab
Copy link
Member

TokageItLab commented Feb 12, 2024

This looks good, but would it be too many arguments to take into account what I commented on in #82306 (comment)? In that case, I am a little wondering what the order of the arguments should be.

Would it be best to make a structure for the Tween parameters, but since there is no way to expose the structure directly to GDScript, would it be fine to make it RefCounted? There was a similar discussion in #87171 (comment). At least those static functions are not exposed to GDScript, so there should be no problem using the structure internally, but if we want to expose this to GDScript (or indirectly, like play_with_capture()), I don't have an answer as to how to handle such a case with many arguments/returns yet... We hope godotengine/godot-proposals#7329 is implemented soon.

@KoBeWi
Copy link
Member Author

KoBeWi commented Feb 15, 2024

@TokageItLab I could add another method similar to interpolate_variant() that takes a struct (with the intention that you make the struct and then reuse it between calls), though RefCounted structs are kind of pain to write. Maybe this is something to be improved later, I think interpolate_variant() isn't commonly used. RefCounted struct wouldn't be compatible with the new structs once they get added.

As for #82306, I'd just put the custom interpolator at the end. There isn't any argument limit in functions, it only affects convenience.

@KoBeWi KoBeWi force-pushed the elastic_tweening branch 2 times, most recently from 62950c3 to e9dd187 Compare February 15, 2024 21:39
@KoBeWi
Copy link
Member Author

KoBeWi commented Feb 15, 2024

CI fails on compatibility, not sure how to handle it. There was no .expected file for 4.3.

@akien-mga
Copy link
Member

akien-mga commented Feb 15, 2024

CI fails on compatibility, not sure how to handle it. There was no .expected file for 4.3.

The file is 4.2-stable.expected - that's changes between 4.2-stable and HEAD.

Copy link
Member

@TokageItLab TokageItLab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Functionally, it makes sense. Nevertheless, I leave it to the product team to discuss whether this is the correct way to pass additional arguments.

@akien-mga
Copy link
Member

Looks like using inf as default value for parameters breaks godot-cpp, might need a fix there.

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.

Add support for parameters when specifying tweener transition type with "set_trans"
6 participants