godot icon indicating copy to clipboard operation
godot copied to clipboard

Add `AnimationMixer::capture()` and `AnimationPlayer::play_with_capture()` as substitute of update mode capture

Open TokageItLab opened this issue 1 year ago • 6 comments

Prerequisite https://github.com/godotengine/godot/pull/86687

  • Closes https://github.com/godotengine/godot-proposals/issues/8513
  • Closes https://github.com/godotengine/godot/issues/83166
  • Closes https://github.com/godotengine/godot/issues/55110
  • Closes https://github.com/godotengine/godot/issues/48432
  • Closes https://github.com/godotengine/godot/issues/78234

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).

https://github.com/godotengine/godot/assets/61938263/eb14c671-cb4b-4920-9d63-71f1b29dff55

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.

https://github.com/godotengine/godot/assets/61938263/d51d29e4-466e-495d-8830-3a76cc59f5b9

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.

https://github.com/godotengine/godot/assets/61938263/ad4fab8d-fd27-41ab-84d1-70eac3ae08d3

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

capture() also works with trees.

https://github.com/godotengine/godot/assets/61938263/3972d945-4d81-45cc-b0dc-8edb212f35e4

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

  • Finally, closes https://github.com/godotengine/godot/issues/83401

TokageItLab avatar Jan 02 '24 16:01 TokageItLab

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?

KoBeWi avatar Feb 06 '24 23:02 KoBeWi

@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.

TokageItLab avatar Feb 07 '24 00:02 TokageItLab

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.

KoBeWi avatar Feb 07 '24 13:02 KoBeWi

Comments from the production team:

  • We need to make sure that the old projects that used capture can work.
  • Can you clarify what you mean by "compatibility breakage"? What does break in your PR to append this label?
    • We do reassess that compatibility breakage is an extreme measure, that should that we must make sure that there's no other viable way to resolve the issue before merging (cc. @lyuma, @fire and @SaracenOne)

adamscott avatar Feb 08 '24 15:02 adamscott

The actual compatibility breakdown was already in 4.2, as it depended on the AnimationMixer playback process. It is absolutely impossible to port an old capture as is.

Old capture animations cannot be migrated as they are and the user has to use the new method capture(). This allows the old behaviour to be reproduced.

However, the only case that cannot be covered is when an animation contains capture tracks with different first key positions, it is not possible to respect the time of each and only one interpolation duration is always allowed.

Cutting out the old capture and adding this new limitation along with some improvements is what this PR means by the break compat label.

TokageItLab avatar Feb 08 '24 18:02 TokageItLab

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 avatar Feb 08 '24 21:02 TokageItLab

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.

KoBeWi avatar Feb 09 '24 12:02 KoBeWi

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.

TokageItLab avatar Feb 09 '24 16:02 TokageItLab

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.

Mickeon avatar Feb 10 '24 15:02 Mickeon

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.

TokageItLab avatar Feb 10 '24 21:02 TokageItLab

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.

akien-mga avatar Feb 11 '24 11:02 akien-mga

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.

TokageItLab avatar Feb 12 '24 08:02 TokageItLab

Thanks!

akien-mga avatar Feb 12 '24 12:02 akien-mga