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 markers to Animation #91765

Merged
merged 1 commit into from
Oct 1, 2024
Merged

Conversation

chocola-mint
Copy link
Contributor

@chocola-mint chocola-mint commented May 9, 2024

Bugsquad edited:

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
Copy link
Contributor

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

@chocola-mint
Copy link
Contributor Author

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
Copy link
Member

fire commented May 10, 2024

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

@chocola-mint chocola-mint marked this pull request as ready for review May 11, 2024 05:15
@chocola-mint chocola-mint requested review from a team as code owners May 11, 2024 05:15
@TokageItLab TokageItLab self-requested a review May 11, 2024 07:18
@TokageItLab
Copy link
Member

TokageItLab commented May 11, 2024

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.

image

image


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

Marker
MarkerSelected

Marker.zip

@chocola-mint chocola-mint requested a review from a team as a code owner May 11, 2024 10:48
@chocola-mint
Copy link
Contributor Author

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

@chocola-mint chocola-mint force-pushed the feat-markers branch 6 times, most recently from 56a12f1 to 08afead Compare May 12, 2024 01:28
@SaracenOne
Copy link
Member

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
Copy link
Member

SaracenOne commented Sep 15, 2024

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
Copy link
Member

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
Copy link
Member

TokageItLab commented Sep 15, 2024

@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
Copy link
Member

SaracenOne commented Sep 17, 2024

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

@chocola-mint chocola-mint force-pushed the feat-markers branch 4 times, most recently from 4e15b81 to 14cf17a Compare September 28, 2024 07:42
Copy link
Member

@SaracenOne SaracenOne left a comment

Choose a reason for hiding this comment

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

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.

Copy link
Member

@SaracenOne SaracenOne left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@akien-mga akien-mga merged commit 8c16e67 into godotengine:master Oct 1, 2024
19 checks passed
@akien-mga
Copy link
Member

Thanks!

@ComfyZenny
Copy link

ComfyZenny commented Oct 4, 2024

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
Copy link
Member

TokageItLab commented Oct 4, 2024

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

@kaiiboraka
Copy link

Hi, I'm having trouble using this feature and I wasn't sure where else to ask about it.
image

Trying to play the marker section from "loop_start" to "stand_start", which should include frames 3, 4, and5. If the marker "stand_start" is set to frame 5, all of frame 3 and 4 are played, but it barely grazes frame 5. Thus it's too short. If I set it to frame 6, then it plays all of 3, 4, and 5, but it also erroneously includes barely hitting the next frame 6 where the character begins to stand. So it's too long. Does this make sense?

I think the problem is essentially that I need the marker to somehow have an option of being inclusive or exclusive of the time it's sitting on. As far as I can tell this is mostly just a problem with discrete frame by frame animation and using timeline snapping. If I could somehow set the marker to 99.9% of the length it is now, it would end just shy of the next marker and not accidentally include the follow-up frame.

tl;dr
marker on frame 5: too short.
marker on frame 6: right length, but includes next frame too, which is bad.

Unless I'm just doing it wrong? Any tips on proper workflow here would be stupendous. Thanks.

@fire
Copy link
Member

fire commented Feb 2, 2025

@kaiiboraka Can you file a bug report on this with the form information like a test program?

I am reading the problem is need the marker to somehow have an option of being inclusive or exclusive of the time it's sitting

@TokageItLab
Copy link
Member

TokageItLab commented Feb 2, 2025

I think it is a matter of double precision. I remember that before this implementation, the last frame was not processed when looping, and the last frame was processed when there was no loop. I think it is necessary to check if the behavior of the end key acquisition changes in loops/non-loops with and without markers (4 patterns).

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