Add markers to Animation

[chocola-mint]

Bugsquad edited:

• Closes Add a way to loop one section of an animation in AnimationPlayer godot-proposals#2159
• Closes Add markers and regions to Animation godot-proposals#8414
• Closes Allow for playback of only a specific section of an animation godot-proposals#8526

This PR introduces a marker system for Animations. Markers are keys that are inserted alongside the timeline, and can be used to play specific parts of animations. A pair of markers is called a section.

For the Animation resource class, there are now new methods to query marker information with.

For AnimationPlayer, there is now a new play_section method that can be used to specify a pair of markers that denote a playback section.

• When the start marker is set to an empty string, the system interprets it as the start of the original animation, and when the end marker is set to an empty string, the system interprets it as the end of the original animation.
  • The vanilla play method is equivalent to passing empty strings as start and end markers to play_section.
• Markers can be placed beyond the original animation's length, in which case the AnimationMixer effectively does nothing while playing the out-of-range part of the section. (FEEDBACK WELCOME: Is this behavior desirable?) The AnimationMixer will clamp the end of the section so it does not exceed the Animation's length.
• In the Animation editor, new markers can be inserted by right-clicking the timeline and then selecting a name through a dialog.
  • Names have to be unique. The validation panel in the dialog will alert the user and prevent them from creating a second marker with the same name.
• Select two markers and the section between them will be highlighted in red. Clicking play buttons will then make AnimationPlayer play the section.
  • play_section uses the original Animation's loop mode.

godot_anim_markers_v4_7.mp4

For AnimationTree, specifically AnimationNodeAnimation, Custom Timelines can now be configured using the Set Custom Timeline from Marker button.

• Clicking the button opens up a dialog that lets you select a start marker and an end marker. The start marker's time is copied to the timeline's start offset, and the end marker's time is copied to the timeline's length.

godot_anim_markers_v4_5.mp4

[joined72]

Exists the possibility to can loop a specific section only for a finite number of cycles?

[chocola-mint]

Exists the possibility to can loop a specific section only for a finite number of cycles?

It would be easily implementable by GDScript when/if #89525 gets merged.

[fire]

I'm interested in markers for animation, will try to review for Godot Engine 4.4.

[TokageItLab]

I had been discussing and supervising on discord with @chocola-mint before this was sent, so this PR should be quite clean in terms of architectural design.

I will test the behavior in more detail later, but I send feedback at this stage:

---

AnimationPlayer needs an api to set/unset section during playback. It corresponds to the enabling of the use custom timeline option in the AnimationNode in AnimationTree.

The API allows for A-B repeats in a Start-A-B animation, including the intro animation like Start-A-B-A-B-A-B... .

BTW sorry, since the use custom timeline option in AnimationNode is newly implemented and I found a problem with the offset being inverted. I have sent an urgent PR about this as #91822 and would appreciate it if you could take a look at it and proceed with the implementation.

---

The warning about duplicate names when adding markers is not needed. It should be consistent with the GUI when adding other animation resources.

↓

---

New icons for markers need to be added. How about the following?

Marker.zip

[chocola-mint]

AnimationPlayer needs an api to set/unset section during playback. It corresponds to the enabling of the use custom timeline option in the AnimationNode in AnimationTree.

AnimationPlayer::set_current_section lets the user change the current section during playback. To avoid issues with pingpong playback, the current animation position is clamped to the new section.

The warning about duplicate names when adding markers is not needed. It should be consistent with the GUI when adding other animation resources.

godot_anim_markers_v4_8.mp4

[SaracenOne]

I'll give this a look, I have some project setups which should hopefully allow me to test this.

[SaracenOne]

Still investigating, but at first look, it appears this PR might have caused regressions with root motion calculation and will need to be fixed before merging.

[SaracenOne]

Another small issue I noticed is if you have a marker selected and switch from the timeline display in seconds to FPS, the time in the marker changes to reflect the frame number, but if you go back to displaying the timeline in seconds, the time is still displayed as a frame. Small UI issue, but something I noticed in testing.

[TokageItLab]

I double checked and there are some issues with RootMotion, but I don't think they are directly related to this PR.

One problem of the RootMotion I have known is the possibility of calculation discrepancies in the first and last frame, but this is related to #90345, and I don't think this PR affects it.

Also, if it has a few keys, it may be possible for the user to be unintentionally affected by the loop_wrap option when the interval is specified, but this is not a blocker as it is a documentation issue.

I think it is mergeable for now.

[SaracenOne]

I want to look into this a bit deeper myself before I feel fully comfortable giving the go ahead to merge. I agree that even if that even if the root motion problem has issues elsewhere, something here has exacerbated the problem and I want to be sure. The thing I'm discovering now that may help narrow this down is that the root motion breaking doesn't just happen in any context, but its specifically breaking when used in the context of a blendspace an animation tree.

[TokageItLab]

This PR does not change much about the AnimationTree; there are some known issues with RootMotion blending, so #95688 may give you a hint.

[SaracenOne]

Hmm...I've been doing testing myself, but @TokageItLab can you confirm if this is the same issue you're describing: it seems that what is happening on my end is that code that calculates the prev_time value when the root motion track loops is basically being set to NaN, and the reason its doing that is that when played in the AnimationTree, the playback info's end time is 0.0. I'm not 100% if this is the same issue you're describing, or if this is something seperate, but it at least explains why the specific problem I've encountered is only happening in the AnimationTree.

I think what's happening in the context of the animation tree, we're using the explictly set start and end overrides rather than the animation length not setting the start and end overrides when playing back in the animation tree, and when those are not explicitly set, we have have the result I described.

[SaracenOne]

Since I know we want to get this in and I feel like its a good implementation of this feature which will unlock a lot of advanced features we can do with animation playback in the future, I won't block this any further if we would prefer to fully debug root motion issues seperately. I just wanted to be sure we're talking about the same bug.

[SaracenOne]

I could be completely wrong, but is it possible that inserting something like:

		pi.start = start_offset;
		pi.end = cur_len;

On line 255 of animation_blend_tree.cpp be what missing here due to these values seemingly not being set, but used for the root motion calculation since this PR? (This likely isn't fully correct fix, it addresses most situations, but it doesn't handle situations where you have a custom timeline-stretched animation and a custom start offset, but I think this is generally where the worst example of the root motion tracks not blending over the loop points is at least stemming from).

[SaracenOne]

This on line 246 of animation_blend_tree.cpp seems to work pretty reliably.

	if (!p_test_only) {
		AnimationMixer::PlaybackInfo pi = p_playback_info;
		if (play_mode == PLAY_MODE_FORWARD) {
			pi.time = cur_playback_time;
			pi.delta = cur_delta;
		} else {
			pi.time = anim_size - cur_playback_time;
			pi.delta = -cur_delta;
		}
		pi.start = 0.0;
		pi.end = cur_len;
		pi.weight = 1.0;
		pi.looped_flag = looped_flag;
		blend_animation(animation, pi);
	}

[TokageItLab]

@SaracenOne Ah indeed, those values are newly implemented in the PlaybackInfo, but the setting value process on the AnimationTree side seems to be lacking.

So it would make sense to set the values, but in my opinion they need to be set earlier, as they are valid outside of the if !(p_test_only) block. Also, I think it should consider respecting the custom_timeline's offset value instead of setting 0.0 and length constantly, do you have any idea?

[SaracenOne]

@TokageItLab Hmm...do we perhaps want to assign the playback_info's end value in the AnimationNode's base process method from the length value as a default, but then use the varying cur_len variable in the AnimationNodeAnimation's _process function to accomodate for the fact that we can override an animation's length? I'm concerned about nodes where we don't have an overall length we can determine, but it seems the end value is really only currently being used in the context of explicit animation blending with an actual animation with a defined length, so it's probably fine.

[SaracenOne]

I would recommend changing

		pi.start = start_offset;
		pi.end = start_offset + cur_len;

to

		pi.start = 0.0;
		pi.end = cur_len;

since the former approach still has issues with root motion when used with BlendTree animations using custom timelines.

[SaracenOne]

LGTM 👍

[akien-mga]

Thanks!

[ComfyZenny]

Would it be possible to make it so when an animation reaches a marker, a signal is made? This would greatly simplify timer-based features in a sequence.
For example;

func _ready():
   animation.play()
   
func _on_animation_marker_passed(animation_name, marker_name):
   if animation_name = my_animation && marker_name = my_first_marker:
      spawn(my_object_1)
   if marker_name = my_second_marker:   
      spawn(my_object_2)

Thank you for your work btw :D

(edit: thanks to @TokageItLab I have been introduced to method tracks. LOL. I noticed that this comment got some likes(? reactions?) and I will make a simple video on how to quickly do what I exampled in the code block above. If anyone has interpreted this comment for a different use with signals, please make a new comment and explain yourself. Thank you!)

[TokageItLab]

@ComfyZenny Wouldn't it be enough to just call the emit_signal function in MethodTrack? We should be careful about having duplicate features. Also, if the marker is referenced directly, there is a problem that signal is not available in AnimationTree, but only in AnimationPlayer.

To summarize, it is theoretically possible with only AnimationPlayer, but it is a duplication of functionality and not possible for consistency with the AnimationTree. So it should be superseded by an add-on or any script that converts the markers to Method tracks.