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 AnimationMixer::capture() and AnimationPlayer::play_with_capture() as substitute of update mode capture #86715

Merged
merged 1 commit into from
Feb 12, 2024

Conversation

TokageItLab
Copy link
Member

@TokageItLab TokageItLab commented Jan 2, 2024

Prerequisite #86687

It caches the current object's value by capture() and blends it into the AnimationMixer's blending result.

This behavior is not fully compatible with original capture feature, since it is not possible to specify duration on a per track, but given the use case, I think it satisfies the requirement.

When call capture(). if the old captured cache already exists, the new captured cache will take precedence. It is theoretically possible to have multiple capture caches, but I'm not sure how much demand there would be for that, so for now it allows only one cache as a basic feature.


Demo

capture_demo.zip

You can use a Tween for the interpolation curve.

If the first key is at a non-zero time, as the original capture feature assumed, you can reproduce it with TRANS_LINEAR (default).

capture_01_linear.mp4
animation_player.play_with_capture("new_animation", 0.5)

As a new feature, the capture interpolation will work even if the capture is called while the key is being interpolated.

capture_02_linear.mp4
animation_player.play_with_capture("new_animation_2", 1)

However, the interpolation between the static (captured) key and the result during playback is not linear actually. As long as the playbacking animation is a linear interpolation, you may be able to improve the looks by specifying TRANS_QUAD or others.

capture_02_quad.mp4
animation_player.play_with_capture("new_animation_2", 1, Tween.TRANS_QUAD)

capture() also works with trees.

capture_03_tree.mp4
animation_tree.capture("new_animation", 0.5)
animation_tree["parameters/OneShot/request"] = AnimationNodeOneShot.ONE_SHOT_REQUEST_FIRE

Production edit: closes godotengine/godot-roadmap#82

@TokageItLab TokageItLab force-pushed the revive-capture branch 3 times, most recently from bbcb9f0 to 803d264 Compare January 2, 2024 16:47
@TokageItLab TokageItLab force-pushed the revive-capture branch 5 times, most recently from 6f165c3 to 2322a0d Compare January 8, 2024 19:06
@KoBeWi
Copy link
Member

KoBeWi commented Feb 6, 2024

I gave the new play_with_capture() some test, trying to use it like the old capture track and

  • play_with_capture() can't be used continuously like play(). When you play the same animation multiple times, the subsequent plays are ignored if the animation is still playing. This is not the case with play_with_capture(), which seems to reset the duration every time. This is not really a bug (and creates somewhat unique animation effect), but maybe it's worth documenting it
  • would it be possible for play_with_capture() to use automatically use the time of the first key in track as a duration? Not sure if per-track durations are supported, maybe it could use the first track 🤔 I think if the duration argument was optional, it would match the old Capture usage
  • using play() with animation that has Capture tracks, without using capture() first, could prompt a warning, to point users that they should use another method
  • the duration seems to be independent from AnimationPlayer's speed_scale. Is this intended?

@TokageItLab
Copy link
Member Author

TokageItLab commented Feb 7, 2024

@KoBeWi Thanks a lot!

play_with_capture() can't be used continuously like play(). When you play the same animation multiple times, the subsequent plays are ignored if the animation is still playing. This is not the case with play_with_capture(), which seems to reset the duration every time. This is not really a bug (and creates somewhat unique animation effect), but maybe it's worth documenting it

It may be possible to add arguments for how to handle old captures, or to adapt to the old behaviour (a good MRP or video for comparison would be helpful so we know how I should do it).

We want to support the old behaviour as much as possible, but if we can't help it, we will document it.

would it be possible for play_with_capture() to use automatically use the time of the first key in track as a duration? Not sure if per-track durations are supported, maybe it could use the first track 🤔 I think if the duration argument was optional, it would match the old Capture usage

using play() with animation that has Capture tracks, without using capture() first, could prompt a warning, to point users that they should use another method

Thanks for the feedback, I will work on these.

the duration seems to be independent from AnimationPlayer's speed_scale. Is this intended?

This is a bit more difficult to handle.

The progression of the capture time is handled by the AnimationMixer, but as the AnimationMixer does not have a speed scale, it may have to be independent of the speed scale. To handle this correctly, the AnimationPlayer would need to be redesigned so that it passes the multiplied delta by scale to the AnimationMixer instead of original delta, but the scope of influence would be too large, so I think it's better not to do anything for now.

But it might be possible to do something by virtualising _blend_capture(), so I will try it.

@KoBeWi
Copy link
Member

KoBeWi commented Feb 7, 2024

It may be possible to add arguments for how to handle old captures, or to adapt to the old behaviour (a good MRP or video for comparison would be helpful so we know how I should do it).
We want to support the old behaviour as much as possible, but if we can't help it, we will document it.

tbh I misunderstood behavior of play_with_capture() when writing that part. Since it's basically the same as capture() and then play(), this behavior makes sense.

@TokageItLab
Copy link
Member Author

Alright, I made change to support them.

would it be possible for play_with_capture() to use automatically use the time of the first key in track as a duration? Not sure if per-track durations are supported, maybe it could use the first track 🤔 I think if the duration argument was optional, it would match the old Capture usage

Do this when a negative value is set for duration. -1.0 is set as default.

using play() with animation that has Capture tracks, without using capture() first, could prompt a warning, to point users that they should use another method

This check is a bit heavy processing, so it is output only when running script in TOOLS_ENABLED.

the duration seems to be independent from AnimationPlayer's speed_scale. Is this intended?

Now this takes speed_scale into account. However, this is not the case for custom_scale because as noted in the documentation, the capture is no longer an interpolation for individual animations.

@TokageItLab TokageItLab requested a review from KoBeWi February 8, 2024 21:50
@TokageItLab TokageItLab force-pushed the revive-capture branch 2 times, most recently from 132591a to 44b37ec Compare February 8, 2024 22:16
Copy link
Member

@KoBeWi KoBeWi left a comment

Choose a reason for hiding this comment

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

Looks fine functionality-wise. This pretty much restores the Capture track functionality, with a caveat that you need to use play_with_capture() instead of play(). However there is a warning that informs about it.

The documentation looks ok content-wise, but I spotted many grammar mistakes. Needs proofreading.

@akien-mga akien-mga requested a review from a team February 8, 2024 23:38
@Mickeon Mickeon self-requested a review February 9, 2024 10:13
Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Tested locally with godotengine/tps-demo@374bd8d (last commit of TPS demo to use the Capture update mode for aiming).

After replacing play() with play_with_capture() for the far and shoot animations, it works, but easing is always linear even though the first keyframe has an easing value of 0.3. Previously, this easing was applied during the initial transition smoothing. Changing the track type to Cubic doesn't smooth out the transition either.

image

4.1.2 with 4.1-compatible commit of the demo

What the animation is meant to look like.

simplescreenrecorder-2024-02-09_12.56.56.mp4

Before

simplescreenrecorder-2024-02-09_12.33.02.mp4

After (this PR)

simplescreenrecorder-2024-02-09_12.52.04.mp4

@KoBeWi
Copy link
Member

KoBeWi commented Feb 9, 2024

Previously, this easing was applied during the initial transition smoothing.

That would be another difference I think? 🤔 Easing is now one of the arguments for play_with_capture(), it literally uses Tween's interpolation.

@TokageItLab
Copy link
Member Author

TokageItLab commented Feb 9, 2024

Not only cubic, but nearest is also not respected. Well, in the case of nearest, I don't think it is necessary to have a capture in the first place.

As KoBeWi said, if you need easing for interpolation, you can now use a tween curve. Yes, this is also one of the break compat things.

doc/classes/AnimationMixer.xml Outdated Show resolved Hide resolved
doc/classes/AnimationMixer.xml Outdated Show resolved Hide resolved
doc/classes/AnimationPlayer.xml Outdated Show resolved Hide resolved
doc/classes/AnimationPlayer.xml Outdated Show resolved Hide resolved
@Mickeon
Copy link
Contributor

Mickeon commented Feb 10, 2024

I've been meaning to chime in on reworking the documentation, but delayed it every time because I'm not sure where to start. There's a few concept here outside my current knowledge here and the class reference added in the PR is a bit... odd, and it does not help me understand this any better.

@TokageItLab
Copy link
Member Author

TokageItLab commented Feb 10, 2024

I don't have an idea how new users of this feature will feel about it. Well, I believe it will be fine for migration.

I am now preparing an article for migration for the whole animation system from 4.0 to 4.3. So now this PR should be ready for merging.

@akien-mga
Copy link
Member

I am now preparing an article for migration for the whole animation system from 4.0 to 4.3.

Great initiative! There's been a lot of changes in each minor release and it can be difficult for users to understand the rationales and how to port their projects, so an overview of the overall design change will help a lot.

@TokageItLab
Copy link
Member Author

TokageItLab commented Feb 12, 2024

Moved the argument of play_with_capture for tween to the back of the list to take into account the possibility that additional arguments may be added in the future by #82306 and #82155.

Or perhaps it could be possible to add a tween object, but this might be a bit confusing. It would be best to have a structure for Tween parameters, but since there is no structure in GDScript, I am not sure how best to do this.

@akien-mga akien-mga merged commit 9b189d2 into godotengine:master Feb 12, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

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