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 auto_capture option to AnimationPlayer #91437

Merged
merged 1 commit into from
May 2, 2024

Conversation

TokageItLab
Copy link
Member

@TokageItLab TokageItLab commented May 2, 2024

Add auto_capture option to AnimationPlayer.

image

I have received a request from @YeldhamDev to perform capture by play().

My concern is that play() is used by many internal APIs, so it is risky to hardcode capture to work there.

Also, the AnimationNode that performs some of the playback of the AnimationTree does not automatically handle capture, so if AnimationPlayer did it, it would have to be explicit.

This PR adds an auto_capture option to Playback Option.

I found out during the implementation that it is quite similar to default_blend_time, so it makes sense to have it in close proximity. It seems a bit funny that double interpolation is performed when both are enabled, but computationally it seems to work correctly.

Before, I thought about adding them to the play() argument, but didn't do it. Because making them properties of the AnimationPlayer makes them explicit and allows the user to control the animation as it is being executed. Also, there are no compatibility issues.

For example, when queueing playback of a capture animation, there may be cases where the user wants capture to occur at all times during the animation, or only at the first playback. The latter can be implemented by the user disabling the auto_capture option after the first playback.

This PR revives compatibility with the old capture behavior without replacing play() with play_with_capture(). (Note that there is no compatibility for cubic interpolation.)

Then, play_with_capture() is left for more detailed options; For auto_capture, options like default_capture_duration, default_capture_tween_transition_type could be added in the same way as dafault_blend_time, but it is possible to follow up later on demands.

capture_demo.zip

@TokageItLab TokageItLab added this to the 4.3 milestone May 2, 2024
@TokageItLab TokageItLab requested review from KoBeWi and YeldhamDev May 2, 2024 02:48
@TokageItLab TokageItLab requested review from a team as code owners May 2, 2024 02:48
@TokageItLab TokageItLab force-pushed the auto-capture branch 4 times, most recently from 574925e to ee7004b Compare May 2, 2024 03:25
Copy link
Member

@YeldhamDev YeldhamDev left a comment

Choose a reason for hiding this comment

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

Seems to be working well, great job!

On a slightly unrelated note, the docs on play_with_capture() states:

If name is blank, it specifies [member assigned_animation].

Besides "name" needing to be changed to "[param name]", name should likely allow to be left blank, like its play() counterpart.

@TokageItLab
Copy link
Member Author

Besides "name" needing to be changed to "[param name]", name should likely allow to be left blank, like its play() counterpart.

Fixed, thank you for testing!

doc/classes/Animation.xml Outdated Show resolved Hide resolved
doc/classes/Animation.xml Outdated Show resolved Hide resolved
scene/animation/animation_player.cpp Outdated Show resolved Hide resolved
scene/resources/animation.cpp Outdated Show resolved Hide resolved
scene/animation/animation_player.cpp Show resolved Hide resolved
@akien-mga akien-mga merged commit 06d105e into godotengine:master May 2, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@TokageItLab TokageItLab deleted the auto-capture branch May 2, 2024 12:05
@KoBeWi
Copy link
Member

KoBeWi commented May 2, 2024

capture_included will stay in the animation after track is changed back to other update mode. Adding or removing a track fixes this.

ADD_PROPERTY(PropertyInfo(Variant::FLOAT, "length", PROPERTY_HINT_RANGE, "0.001,99999,0.001,suffix:s"), "set_length", "get_length");
ADD_PROPERTY(PropertyInfo(Variant::INT, "loop_mode", PROPERTY_HINT_ENUM, "None,Linear,Ping-Pong"), "set_loop_mode", "get_loop_mode");
ADD_PROPERTY(PropertyInfo(Variant::FLOAT, "step", PROPERTY_HINT_RANGE, "0,4096,0.001,suffix:s"), "set_step", "get_step");
ADD_PROPERTY(PropertyInfo(Variant::BOOL, "capture_included", PROPERTY_HINT_NONE, "", PROPERTY_USAGE_STORAGE | PROPERTY_USAGE_READ_ONLY | PROPERTY_USAGE_NO_EDITOR), "_set_capture_included", "is_capture_included");
Copy link
Member

Choose a reason for hiding this comment

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

PROPERTY_USAGE_NO_EDITOR and PROPERTY_USAGE_STORAGE have the same value. You should include only one of them.

@@ -210,6 +211,9 @@
If [code]true[/code] and the engine is running in Movie Maker mode (see [MovieWriter]), exits the engine with [method SceneTree.quit] as soon as an animation is done playing in this [AnimationPlayer]. A message is printed when the engine quits for this reason.
[b]Note:[/b] This obeys the same logic as the [signal AnimationMixer.animation_finished] signal, so it will not quit the engine if the animation is set to be looping.
</member>
<member name="playback_auto_capture" type="bool" setter="set_auto_capture" getter="is_auto_capture" default="true">
If [code]true[/code], performs [method AnimationMixer.capture] before playback automatically. This means just [method play_with_capture] is executed with default arguments instead of [method play].
Copy link
Member

Choose a reason for hiding this comment

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

I think this should clarify that it only happens when the animation has a capture track.

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