Add markers to Animation

[chocola-mint]

Bugsquad edited:

• Closes https://github.com/godotengine/godot-proposals/issues/2159
• Closes https://github.com/godotengine/godot-proposals/issues/8414
• Closes https://github.com/godotengine/godot-proposals/issues/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.

https://github.com/godotengine/godot/assets/56677134/a57ed7ee-8287-4b77-9fce-a50410e92218

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.

https://github.com/godotengine/godot/assets/56677134/46368b3d-40ee-4ba2-834e-1cc1bda9343a

[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 https://github.com/godotengine/godot/pull/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 https://github.com/godotengine/godot/pull/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.

https://github.com/godotengine/godot/assets/56677134/6246032c-ab65-4742-b5f0-dc728bb62251

[TokageItLab]

For usability, can the marker name be displayed in tool tip when focused? Also, it would be helpful to have a shortcut such as ctrl+m (or other shortcut is fine as well as it doesn't conflict) to display the name under the all markers.

[Calinou]

For usability, can the marker name be displayed in tool tip when focused? Also, it would be helpful to have a shortcut such as ctrl+m (or other shortcut is fine as well as it doesn't conflict) to display the name under the all markers.

Markers could also be given a color (either random with a seed based on their name[^1], or a custom color) so you can distinguish them from each other.

[^1]: This approach is used by tags in the project manager.

[chocola-mint]

https://github.com/godotengine/godot/assets/56677134/66ed4c41-81c0-4ff6-b410-fda0575b7a00

(Shortcut: Key M when AnimationMarkerEdit is focused) (Marker colors are derived from name hashes)

As it's not possible to force the tooltip to show up on demand, I've opted to just draw the markers' names beneath their marker keys. Would this work?

As AnimationMarkerEdit is currently a descendant of AnimationTimelineEdit in the scene tree, it will be drawn before other AnimationTracks, and so the marker names will get darkened by AnimationTrack groups. I haven't figured out a workaround for this yet, unfortunately.

[SatLess]

This is looking pretty neat and it's really exciting having markers in Godot. Also, these are more cosmetic suggestions, but I think they would be nice additions:

• being able so select which color the marker is when creating them (or after, in the dropdown menu with the mouse on it) — allows color-coding markers for better organization.
• Using a more blue-ish color for highlighting sections — it would fit more with Godot's color scheme.
• Have markers' names displayed only by using shortcut and the dropdown option — This makes it so it shows up only when necessary.

[TokageItLab]

Using a more blue-ish color for highlighting sections — it would fit more with Godot's color scheme.

I don't think this needs to be changed since we are already using the same color code as the animation key; Perhaps currently it have used multiplication for coloring, so it just has to be reverted back to the original color during the selection. In other words, coloring should only be done when it is not selected.

However, I agree with the rest.

[chocola-mint]

https://github.com/godotengine/godot/assets/56677134/b7cfd71d-ba84-482b-a5b9-4a75a50225cb

• You can now pick the color when inserting a new marker.
• You can now use the dropdown menu option "Edit Marker" to change an existing marker's name or color. The shortcut key E also works.
• Marker color won't be used when the marker is selected.
• Marker names are not shown by default and can be toggled with the shortcut key M or the dropdown menu option "Show Marker Names".

[chocola-mint]

Just cleaned up the commit history. No additional changes have been made.

[chocola-mint]

EDIT: Lowered the vertical lines' opacity.

---

@Calinou

The last created marker's color seems to occasionally reset to white while editing animations for an unknown reason.

• I had apparently forgotten to set the marker's color after moving. This has now been fixed.

Marker names can be unreadable on light theme depending on their color.

• Fixed by adding 1px outlines to the text. The outline color is the same as the theme's font color, therefore it should have good visibility regardless of the theme.

The Edit Marker dialog should allow precisely setting the time the marker is located at. This also allows you to visually confirm the marker is positioned at the right time.

When a section of the timeline is highlighted in red, I suggest drawing the highlight on the background of the animation tracks as well. This makes it easier to see where the markers begin/end when your animation editor is made tall (which is common when editing complex animations).

For reasons similar to the above, I suggest making markers draw a thin vertical line (with low opacity) as a background of the animation tracks, with their respective color.

• Added. See clip below.

https://github.com/godotengine/godot/assets/56677134/322f7944-7134-4582-9f13-d0e28b273c86

@AThousandShips Styling-related changes have also been applied as requested. Please let me know if there are other parts I should change.

[Cronos87]

As 4.3 has reached beta, will this feature be included in 4.4? Am I correct?

Thank you for the work; it's so useful 🙏 I can't wait to use it since I need it for my game!

[TokageItLab]

@chocola-mint For consistency and fewer clicks, can you provide an inspector for markers instead of opening the edit window from the right-click menu? You can refer to the EditorInspectorPluginAnimationTrackKeyEdit implementation.

BTW, it seems marker icon has been rolled back, needs to fix.

[Calinou]

As 4.3 has reached beta, will this feature be included in 4.4? Am I correct?

Like most feature PRs where the functionality is desired and the implementation is sound, the goal is to merge it in 4.4, but we can't guarantee this will happen (e.g. due to external factors).

[Cronos87]

Like most feature PRs where the functionality is desired and the implementation is sound, the goal is to merge it in 4.4, but we can't guarantee this will happen (e.g. due to external factors).

Sounds fair. As usual no pressure about it, I am always amazed by the work done and excited to try it 😁

Thank you for the answer and great PR! No pressure, take time to deliver quality and obviously everyone needs to take care of themselves first 🙏

[chocola-mint]

@TokageItLab

BTW, it seems marker icon has been rolled back, needs to fix.

Fixed. See below.

For consistency and fewer clicks, can you provide an inspector for markers instead of opening the edit window from the right-click menu? You can refer to the EditorInspectorPluginAnimationTrackKeyEdit implementation.

https://github.com/godotengine/godot/assets/56677134/31cd6b51-38c9-45b6-9350-f950eb27f038

Also, when the input name is invalid (marker already exists / name is empty), a warning message is shown and the faulty input is ignored.

https://github.com/godotengine/godot/assets/56677134/44c95c1f-1823-4349-a8aa-761dca51a300

[chocola-mint]

(Sorry about all the unrelated commits earlier, which probably caused a lot of unnecessary tagging. Messed up some git commands when squash-merging)

@TokageItLab

I've handled the suggested changes. See the clip below:

https://github.com/godotengine/godot/assets/56677134/fa62fdeb-fe88-4122-9cda-fd3d0972250c

However, I wasn't able to replicate this bug:

Deleting a marker after changing the marker's color in the inspector causes an error

It'd be very helpful if you could give me the exact steps needed to replicate the bug (or a video showing it happening). Thanks in advance.

[mihe]

Would it be possible to incorporate the description field mentioned in godotengine/godot-proposals#10223 into this PR as well, and have that show when you hover over a marker, same as Time and Name currently?

[TokageItLab]

@mihe It may be possible to add a description field, but it must be editor-specific.

However, I personally disagree with that, as I don't see the need to have some information on the marker besides the marker name; We should not have the property of the user description specifically only for the marker as ignoring consistency with any other resource.

If you really want it, I think you should create an add-on that stores them in the meta data of the Animation with a map that corresponds markers to descriptions.

[mihe]

It may be possible to add a description field, but it must be editor-specific

@TokageItLab Yes, sorry, I should've clarified that. This would purely be a design-time thing, and could probably be excluded from exports entirely, if that's possible.

However, I personally disagree with that, as I don't see the need to have some information on the marker besides the marker name

When working on larger teams of people and complex animations it can become necessary to put longer notes/comments in the timeline, not much different from code comments I guess, and seeing as how the marker names in this PR are largely meant to be code identifiers rather than lengthy descriptions I don't see them obviating the need for a proper description field.

We should not have the property of the user description specifically only for the marker as ignoring consistency with any other resource.

Sure, Resource-derived objects don't typically have this kind of description field, but seeing as how Node does have editor_description there's at least some kind of (relatively widespread) precedent for this kind of thing in Godot already.

If you really want it, I think you should create an add-on that stores them in the meta data of the Animation with a map that corresponds markers to descriptions.

That's certainly a possibility, if nobody else sees the motivation for this.

[chocola-mint]

Brought the PR up to date with master (4.3). Mostly just replacing float comparisons with approx comparison functions.

To add to the discussion of having addons that associate data with markers, though...

Currently each marker's data is serialized as a Dictionary with the following fields:

• name: The marker's name.
• time: The marker's position in the timeline.
• color: The marker's color.

I'm considering replacing the color field with:

• data: A Dictionary that contains the marker's color field.

When markers are moved, the data Dictionary is moved as well. This would make it easier for other PRs in the future to associate additional data with each marker. (By just adding a new field to the Dictionary) I think this is a more future-proof design and would make adding additional data less error-prone.

In the same vein, I want to add a meta field to the data Dictionary, which is just a Dictionary that the user can freely extend with get_marker_meta(name : StringName) and set_marker_meta(name: StringName, value: Variant). (Similar API to Object.get_meta and Object.set_meta)

Any feedback on this idea would be appreciated. Thanks in advance!

[TokageItLab]

@chocola-mint Most of the bugs seem to be resolved. Need to rebase.

I would like to add a feedback, it should avoid the side-effect seek that occurs with a single click to select a marker, but seek to the marker position on a double click to a marker.

When markers are moved, the data Dictionary is moved as well. This would make it easier for other PRs in the future to associate additional data with each marker. (By just adding a new field to the Dictionary) I think this is a more future-proof design and would make adding additional data less error-prone.

In the same vein, I want to add a meta field to the data Dictionary, which is just a Dictionary that the user can freely extend with get_marker_meta(name : StringName) and set_marker_meta(name: StringName, value: Variant). (Similar API to Object.get_meta and Object.set_meta)

I disagree both ways. In order to enable merging, I suggest not adding any meta, data or description for now. It should probably be discussed carefully, and in my opinion, should be added to a base class such as the Resource class. If you want to add data later, you can write compatibility methods in the _set(), _get() method later to store past color into data, as some migrated resources already do so. Also, in any case, we must avoid to use Dictionary internally in most cases from a performance standpoint.

[chocola-mint]

@TokageItLab

I would like to add a feedback, it should avoid the side-effect seek that occurs with a single click to select a marker, but seek to the marker position on a double click to a marker.

Done.

https://github.com/user-attachments/assets/b6c234ca-5405-4f79-ade2-13293d8de14c

I disagree both ways. In order to enable merging, I suggest not adding any meta, data or description for now. It should probably be discussed carefully, and in my opinion, should be added to a base class such as the Resource class. If you want to add data later, you can write compatibility methods in the _set(), _get() method later to store past color into data, as some migrated resources already do so. Also, in any case, we must avoid to use Dictionary internally in most cases from a performance standpoint.

That makes sense. I'll drop the idea for now, then. We can revisit this part if/when there's a new proposal for it in the future.

---

Regarding the suggested changes: I've updated the code accordingly. However, implementing custom parsing in parse_begin means that the AnimationNodeAnimation block will appear after the custom editor.

void EditorInspectorPluginAnimationNodeAnimation::parse_begin(Object *p_object) {
	Ref<AnimationNodeAnimation> animation_node_animation(cast_to<AnimationNodeAnimation>(p_object));

	List<PropertyInfo> props;
	ClassDB::get_property_list("AnimationNodeAnimation", &props, true);
	for (const PropertyInfo &prop : props) {
		EditorProperty *editor = EditorInspectorDefaultPlugin::get_editor_for_property(p_object, prop.type, prop.name, prop.hint, prop.hint_string, prop.usage, false);
		add_property_editor(prop.name, editor, false, prop.name.capitalize());
	}

	add_custom_control(memnew(AnimationNodeAnimationEditor(animation_node_animation)));
}

If this isn't desirable, I can also use parse_property to draw loop_mode's editor manually, then insert custom editor after that. The code would then look like this:

bool EditorInspectorPluginAnimationNodeAnimation::parse_property(Object *p_object, const Variant::Type p_type, const String &p_path, const PropertyHint p_hint, const String &p_hint_text, const BitField<PropertyUsageFlags> p_usage, const bool p_wide) {
	Ref<AnimationNodeAnimation> animation_node_animation(cast_to<AnimationNodeAnimation>(p_object));

	if (p_path == "loop_mode") {
		EditorProperty* editor = EditorInspectorDefaultPlugin::get_editor_for_property(p_object, p_type, p_path, p_hint, p_hint_text, p_usage, p_wide);
		add_property_editor(p_path, editor, false, p_path.capitalize());
		
		add_custom_control(memnew(AnimationNodeAnimationEditor(animation_node_animation)));

		return true;
	}
	
	return false;
}

---

@AThousandShips Suggested changes have been added to the PR.

[TokageItLab]

@chocola-mint Oh sorry, you are right. But the button does not absolutely have to be placed at the bottom, it just needs to be under custom_timeline, so it can be written as follows:

bool EditorInspectorPluginAnimationNodeAnimation::parse_property(Object *p_object, const Variant::Type p_type, const String &p_path, const PropertyHint p_hint, const String &p_hint_text, const BitField<PropertyUsageFlags> p_usage, const bool p_wide) {
	Ref<AnimationNodeAnimation> ana = Ref<AnimationNodeAnimation>(Object::cast_to<AnimationNodeAnimation>(p_object));
	ERR_FAIL_NULL_V(ana.ptr(), false);

	if (p_path == "timeline_length") {
		add_custom_control(memnew(AnimationNodeAnimationEditor(ana)));
	}

	return false;
}

[chocola-mint]

@TokageItLab Got it. I've also taken care of the suggested changes.

Here's how the inspector looks now.

[chocola-mint]

@TokageItLab I've merged the patch into the PR (and reading the patch it did make a lot of sense), though regarding this line in particular:

undo_redo->add_do_method(*animation_node_animation, "set_timeline_length", end_time);

This is intentional. Custom timelines, as far as I can tell from the code, uses "start offset" in the same sense as "start time" (how much to trim off the start of the animation), and "timeline length" in the same sense as "end time" (how much to trim off the end of the animation). Therefore using the actual length of the section here would yield incorrect results.

As for the rest of the suggested changes, I've included them as well.

Also, some behavior on some editors is a bit erratic and needs to be fixed:

• Behavior when a section is set while playing without a section
• When a section is playing with 3 or more markers selected, and the number of markers selected is reduced
• Behavior when a section outside the animation length is specified

• Behavior when a section is set while playing without a section
  • New behavior: Animation preview is paused if a section is set here.
• When a section is playing with 3 or more markers selected, and the number of markers selected is reduced
  • New behavior: If the section formed by the leftmost and rightmost marker selected has changed, animation preview is paused. Otherwise nothing happens.
• Behavior when a section outside the animation length is specified
  • The current behavior is to clamp the end of the section within the animation's original length. This is consistent with the current markers API and so I don't think it needs to be changed.

[chocola-mint]

@TokageItLab I've updated the playback behavior of sections accordingly. The AnimationPlayer will still be paused and reset if the new section is invalid (when the leftmost marker is beyond the animation length), but otherwise it will use set_current_section to dynamically clamp the playback position to the new section. If the user tries to play an invalid section, the attempt will also be ignored.

https://github.com/user-attachments/assets/69bebab0-c4e8-4603-b13a-d6cb58dcfa22

I've also merged most of the suggested changes. Input validation for sections is also added to play_section, and the documentation has been updated accordingly.

[chocola-mint]

@TokageItLab Here's the new version. The system no longer pauses when sections go out-of-range. To generalize this more nicely, start/end times greater than the animation length will also be considered illegal values and use default values instead.

https://github.com/user-attachments/assets/996a11c3-8826-43b6-8ba6-e924166dd919

Currently there's a bug with how AnimationPlayer::playback's current isn't actually set until after the first playback, even though assigned has been set. (This is because stop is immediately called, causing current.from to be set to nullptr) This will cause AnimationPlayer::set_current_section to fail initially before the first playback, but does not actually result in incorrect results. I can't think of a good workaround for this outside of exposing a function that returns playback.current.from != nullptr. Might need more feedback here.