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

Implement reverse playback and ping-pong loop in AnimationPlayer and NodeAnimation #48332

Merged
merged 1 commit into from
Oct 11, 2021

Conversation

TokageItLab
Copy link
Member

@TokageItLab TokageItLab commented Apr 30, 2021

For this proposal:

  • #2664 Add to support ping-pong loop in AnimationPlayer and playing backwards and ping-pong loop in NodeAnimation of AnimationTree

playmode

https://www.youtube.com/watch?v=mBOkTA4_HZE

Note:
This PR contains two PRs haven't merged as a prerequisite. One of them has been rejected in the past because its use was unclear, but this PR will require it. If you want to do the merge in stages, I recommend that you start with those PRs merges.

  • #41728 Implement reverse playback for animtree
  • #46346 Added pingpong built-in function

Note2:
One problem is that if the TimeScale is set to negative, it is not possible to tell when the animation is finished. This can be solved by using the Backward option in NodeAnimation itself.

For example, if you set

[Animation(forward)]->[TimeScale(-1)]->[OneShot]

this won't work as playing through backward animation (but if you want to temporarily reverse the animation, this is correct).

If you set

[Animation(backward)]->[TimeScale(1)]->[OneShot]
or
[Animation(forward)]->[OneShot]->[TimeScale(-1)]

these will work fine.

@fire
Copy link
Member

fire commented Apr 30, 2021

When you are ready for review can you squash the merge commits?

@TokageItLab TokageItLab force-pushed the implement-ping-pong branch from 374da57 to e048e58 Compare April 30, 2021 15:48
@TokageItLab
Copy link
Member Author

@fire Sure. I left a commit for other PRs to easily see, but I squashed it just now.

@fire
Copy link
Member

fire commented Apr 30, 2021

Review from pouley said we wanted demos of more cases.

@TokageItLab
Copy link
Member Author

Yes, I already understood that as of #41728, as I needed to test it for all track types. I will have a project file for testing later.

@TokageItLab TokageItLab force-pushed the implement-ping-pong branch 3 times, most recently from d466928 to 488db24 Compare April 30, 2021 21:43
@TokageItLab TokageItLab marked this pull request as draft May 1, 2021 13:29
@TokageItLab TokageItLab force-pushed the implement-ping-pong branch from 488db24 to 979eda6 Compare May 2, 2021 05:45
@TokageItLab TokageItLab marked this pull request as ready for review May 2, 2021 05:45
@TokageItLab TokageItLab force-pushed the implement-ping-pong branch 2 times, most recently from e89fded to 9bea72c Compare May 3, 2021 04:03
@TokageItLab TokageItLab marked this pull request as draft May 3, 2021 04:27
@TokageItLab TokageItLab force-pushed the implement-ping-pong branch from 9bea72c to 6ac3b62 Compare May 4, 2021 14:33
@TokageItLab TokageItLab marked this pull request as ready for review May 4, 2021 14:35
@TokageItLab TokageItLab force-pushed the implement-ping-pong branch 2 times, most recently from d529442 to aa8ff80 Compare May 4, 2021 18:43
@TokageItLab TokageItLab force-pushed the implement-ping-pong branch 3 times, most recently from f4d1e18 to c73906e Compare September 2, 2021 08:56
@TokageItLab TokageItLab requested a review from a team as a code owner September 2, 2021 08:56
@TokageItLab TokageItLab force-pushed the implement-ping-pong branch 5 times, most recently from ae36b8c to d392e5b Compare September 2, 2021 14:58
@TokageItLab TokageItLab force-pushed the implement-ping-pong branch 2 times, most recently from 47dfb03 to 5fd089f Compare September 21, 2021 10:58
@TokageItLab TokageItLab force-pushed the implement-ping-pong branch 3 times, most recently from f365491 to 3d2d5dd Compare September 23, 2021 01:22
@reduz
Copy link
Member

reduz commented Oct 2, 2021

My feeling about this PR is that the implementation is very complex for something that is a very corner case. If it could be done in a way that it does not require modifying Animation and it could just be contained in AnimationPlayer and AnimationTree, it could be better.

@reduz
Copy link
Member

reduz commented Oct 2, 2021

I see no other way to do this though, so I guess its ok.

@@ -650,7 +650,7 @@ void AudioStreamSample::_bind_methods() {

BIND_ENUM_CONSTANT(LOOP_DISABLED);
BIND_ENUM_CONSTANT(LOOP_FORWARD);
BIND_ENUM_CONSTANT(LOOP_PING_PONG);
BIND_ENUM_CONSTANT(LOOP_PINGPONG);
Copy link
Member

@fire fire Oct 2, 2021

Choose a reason for hiding this comment

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

I'm curious is PINGPONG the more popular spelling compared to PING_PONG? Trying to avoid renames in the future.

Copy link
Member Author

Choose a reason for hiding this comment

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

Since @Chaosus 's implementation of the math function is pingpong(), it is easy to see when mixed with other enumerated values, I decided to use pingpong(), XXX_PINGPONG, and pingponged.
However, since I am not so particular about it, I can consider using XXX_PING_PONG as the enumerated type and ping_pong() as the math function.

Co-authored-by: Chaosus <chaosus89@gmail.com>
@TokageItLab
Copy link
Member Author

@akien-mga Could you give me a review when you have a time?

@akien-mga akien-mga merged commit 9ed4f83 into godotengine:master Oct 11, 2021
@akien-mga
Copy link
Member

Thanks!

@reduz
Copy link
Member

reduz commented Oct 11, 2021

I sincerely apologize I had to revert this PR. I am refactoring large part of animation playback code (#53689) and this PR gets in the way. Please wait a couple of days until I am done with my changes to the animation system and attempt a PR against the new code.

@TokageItLab TokageItLab deleted the implement-ping-pong branch May 2, 2022 19:39
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.

5 participants