Add toggle for inserting keys/markers at current time vs mouse cursor's position

[shadow-foss]

Summary

Adds a toggle button in the Animation Track Editor that allows users to choose between inserting animation keys and markers at the timeline cursor position or at the mouse position.

Changes

• Add editors/animation/insert_at_timeline_cursor editor setting to persist toggle state
• Add toggle button UI in Animation Track Editor toolbar
• Modify key/marker insertion logic to use cursor position when toggle is enabled
• Add documentation for the new editor setting
• Icon by @TokageItLab

Fixes #103272

Demo

godot_insert_timeline_button_demo.mp4

Bugsquad edited:

• Supersedes/closes Add a dropdown menu option to insert marker at timeline position in animation track #105774

[shadow-foss]

Quick question: the toggle is enabled by default, so keys/markers go to the timeline cursor.
Previously, they were added at the mouse position by default.
Could this change be confusing?
Should I set the default to disabled for consistency?

Happy to change it if needed!

[SatLess]

Imo I think this should be an opt in, so disabled by default

[TokageItLab]

The implementation approach seems correct. I would like to have more consensus on the setting names and how to save editor settings in addition to my own opinion. Maybe @KoBeWi or @Mickeon ?

[shadow-foss]

Just pushed a small update: changed the default to false for compatibility with the previous behavior (inserting at mouse position).
Also open to renaming the setting,other variables if there's a better consensus on the wording :)

[TokageItLab]

Oh sorry, I overlooked, but we need to use double for the time.

[shadow-foss]

Just discovered a bug when insert_at_current_time is enabled, inserting multiple keys quickly results in insertion of multiple keys away from current time. This doesn't happen with markers.

insert_bug.mp4

Cause:
In animation_track_editor.cpp, inside _insert_key_from_track(), the key insertion offset logic always runs:

while (animation->track_find_key(p_track, p_ofs, Animation::FIND_MODE_APPROX) != -1) {
	p_ofs += SECOND_DECIMAL;
}

Fix:
Skip the offset adjustment when the toggle is enabled:

while (animation->track_find_key(p_track, p_ofs, Animation::FIND_MODE_APPROX) != -1 && !is_insert_at_current_time_enabled()) {
	p_ofs += SECOND_DECIMAL;
}

I'll include this fix along with the other changes. Let me know if it looks okay!

[KoBeWi]

I think it would be nice if it also affected duplicate operation. I remember the AnimationPlayer used to insert duplicated keys at current time, but it was changed at some point.

I also noticed the toolbar in animation player has became quite crowded 🤔

And this adds yet another button.

[TokageItLab]

Skip the offset adjustment when the toggle is enabled:

Some operations (such as baking) require keys to be inserted, but I assume this was a hack to prevent the index from being corrupted due to overwriting (deletion).

In the case of markers, the current skip operation is okay, but in the case of keys, it will be necessary to test whether there are any problems in some operations.

[shadow-foss]

Some operations (such as baking) require keys to be inserted, but I assume this was a hack to prevent the index from being corrupted due to overwriting (deletion).

What do you think about skipping key insertion at the current time if a key already exists and insert_at_current_time is enabled?
Instead of overwriting, we’d just skip insertion and display a warning like:
A key already exists at the current time. Insertion skipped.
or is it fine to have the default behaviour where new key gets inserted to nearby position.

[shadow-foss]

I think it would be nice if it also affected duplicate operation. I remember the AnimationPlayer used to insert duplicated keys at current time, but it was changed at some point.

Currently, AnimationPlayer still pastes or duplicates keys at the timeline cursor when using keyboard shortcuts. However, when using the menu, keys are inserted at the mouse position. I tried to make the toggle affect paste/duplicate as well, but both functions currently overwrite existing keys without any checks, which could lead to unintended behavior—as @TokageItLab pointed out.
I'll add support for the toggle to paste and duplicate once I figure out a proper way to handle overwriting of existing keys.

[TokageItLab]

Existing keys should be always overwritten with the latest pasted/inserted keys.

However, in cases where multiple keys are selected and processed sequentially, care must be taken not to break the indices. At this point, we also need to be careful about the Undo operation.

I think the fundamental problem is that the same method is used for inserts from both external operation and internal sources.

Ideally, it would be great to be able to fix to override always take into account sequence and undo, but that might be a big change to include in this PR. For now, I think you can leave it as the default behavior and then raise an issue saying “keys cannot be inserted in the same position” and fix it in a separate PR.

[shadow-foss]

I tested pasting and duplicating keys at the same position multiple times. They currently overwrite existing keys as expected, and undo/redo works correctly.
Since this PR doesn't change that logic, and insert still avoids collisions via offset adjustment, I think it's safe to leave as-is.
However, for long-term consistency and clarity, I’d be happy to open a follow-up issue discussing whether paste/duplicate should also warn or prevent overwrites like insert does.
Also wanted to share my changes here before pushing it.

made the toggle button affect paste and duplicate operations:

-           emit_signal(SNAME("duplicate_request"), insert_at_pos, true);
+          emit_signal(SNAME("duplicate_request"), insert_at_pos, !editor->is_insert_at_current_time_enabled());
-           emit_signal(SNAME("paste_request"), insert_at_pos, true);
+          emit_signal(SNAME("paste_request"), insert_at_pos, !editor->is_insert_at_current_time_enabled());

changed the function resolve_insertion_offset() to use reference passing and changed function call accord to it:

-float AnimationTrackEditor::get_insert_at_current_time_position_if_enabled(float p_fallback_ofs) const {
+void AnimationTrackEditor::resolve_insertion_offset(float &r_offset) const {
        if (is_insert_at_current_time_enabled()) {
-               return timeline->get_play_position();
-       } else {
-               return p_fallback_ofs;
+               r_offset = timeline->get_play_position();
        }
 }

Let me know if this looks good or if anything should be tweaked before I push it.

[TokageItLab]

Since we need to squash them and make a single commit in the end, you can push them casually so that we can build them :)

[shadow-foss]

Everything requested so far has been addressed and pushed — the toggle now affects insert, paste, and duplicate as expected.
I also refactored the resolve_insertion_offset() function to use reference passing as discussed, and kept it float since it's used in float contexts.
No further changes planned in this PR. If needed, I can open a follow-up issue to improve the insert logic and overwrite handling.
Would be great if someone could take a final look at the code. Thanks again for all the feedback :)

[KoBeWi]

Looks good.
Still needs animation team review probably.

[shadow-foss]

Accidently closed the pr while using mobile client, my bad.

[TokageItLab]

Tested and LGTM, please squash commits into one to follow our PR workflow.

[shadow-foss]

squashed all the commits to a single commit :)

[TokageItLab]

CI test seems to be malfunctioning, so we will run the gitHub action job again later. I think the code is fine now, thanks.

[shadow-foss]

Ran svgo again on the icon file since the CL checks were failing due to it. It reduced the size slightly (~0.02 KB). I've force-pushed the updated commit

[TokageItLab]

If there is a problem with the svg itself, the change suggestion should be displayed as an error, so I suspect that the link to the svgo module on the CI side is broken. I think the svg is fine as it is.

[shadow-foss]

added a newline at the end of svg file, the cl tests would work now ig.

[shadow-foss]

Resolved merge conflicts and force pushed it.