Update AnimationPlayer in real-time when keyframe properties change

[dnllowe]

Implementation for proposal: godotengine/godot-proposals#9686

• Reflect changes to keyframed properties in AnimationPlayer in realtime
• Move AnimationPlayer position to key frame when key frame is selected

---

Bugsquad edit:

• closes: Update AnimationPlayer in real-time when keyframe properties change godot-proposals#9686
• closes Scene not updated when editing animation track key value in AnimationTrackKeyEdit Inspector panel #78131

[Calinou]

Move AnimationPlayer position to key frame when key frame is selected

Won't this cause issues when selecting multiple keyframes, particularly across different tracks?

[Calinou]

Tested locally, it mostly works as expected.

Testing project: test_pr_91599.zip

I noticed 2 issues though:

• Changing a key's easing in the inspector does not update what you see in the editor in real-time. It only updates when you scrub the timeline again:

simplescreenrecorder-2024-05-06_22.15.57.mp4

• Using undo after modifying a key's value in the inspector does not reflect in real-time until you scrub the timeline:

simplescreenrecorder-2024-05-06_22.16.43.mp4

[dnllowe]

Tested locally, it mostly works as expected.

I noticed 2 issues though:

• Changing a key's easing in the inspector does not update what you see in the editor in real-time. It only updates when you scrub the timeline again:

• Using undo after modifying a key's value in the inspector does not reflect in real-time until you scrub the timeline:

Thanks for your review -- I'll iron these issues out

[TokageItLab]

From a code standpoint, it looks that the specified arguments of the AnimationPlayer methods are fine. I will wait for the fixes to some of the behaviors pointed out above.

[dnllowe]

Good suggestions / catches. Both issues should be resolved now:

Changing easing, then performing undo/redo a couple of times

undo-redo-and-easing.mp4

[TokageItLab]

Pointing out two things from the test:

fb.mp4

• Selecting a keyframe causes a seek due to the KeyEdit updating on the inspector, which does not exist previously and probably was not the intended change and needs to be fixed
• Inconsistencies occur when key times are moved by the inspector

[dnllowe]

I fixed it so that changing time will update the animation.

The issue in the video where editing the easing is the expected behavior, as the last frame's easing is being edited, which won't have an effect on the animation. The first keyframe would need to be selected and its easing changed for a change to occur.

I'm unsure what this really refers to:

Selecting a keyframe causes a seek due to the KeyEdit updating on the inspector, which does not exist previously and probably was not the intended change and needs to be fixed

I do intend to update the behavior so that selecting a keyframe seeks to it and updates the animation, but I'm not sure if that's the issue you're referring to.

[patwork]

Hello, does this fix also take into account the need to refresh the scene when moving a keyframe in the timeline?

[TokageItLab]

I do intend to update the behavior so that selecting a keyframe seeks to it and updates the animation, but I'm not sure if that's the issue you're referring to.

The problem is that the "current time" in the editor is changed by selecting the key.

That behavior does not exist in other animation software, such as AfterEffects, Blender or etc., and is wrong as an affordance so must be reverted. The user just wants to edit the value of the key, not intend to seek the animation, so the current time must be retained.

In summary, it is good to change the preview when the ease is changed, but it is not acceptable to just click on a key and the current time will be moved to that key.

[dnllowe]

I do intend to update the behavior so that selecting a keyframe seeks to it and updates the animation, but I'm not sure if that's the issue you're referring to.

The problem is that the "current time" in the editor is changed by selecting the key.

That behavior does not exist in other animation software, such as AfterEffects, Blender or etc., and is wrong as an affordance so must be reverted. The user just wants to edit the value of the key, not intend to seek the animation, so the current time must be retained.

In summary, it is good to change the preview when the ease is changed, but it is not acceptable to just click on a key and the current time will be moved to that key.

Alright, I can revert this. For reference, Unreal's animation timeline does update the position when selecting keyframes. But I double checked with Unity and my own video editor of choice (Da Vinci Resolve) and neither of them update the timeline position when selecting keyframes. Having immediate feedback makes more sense to me, but I'm not really an animator by trade, so perhaps I'm an edge case.

[dnllowe]

Hello, does this fix also take into account the need to refresh the scene when moving a keyframe in the timeline?

Good suggestion / find. I'm updating the branch to reflect updating the animation after changing the keyframe position

[dnllowe]

Here's what the latest behavior looks like. Update the animation when...

• Keyframe properties change
• Easing is changed
• Keyframe position is changed
• Undo / Redo will update the animation accordingly
• (Edit) When changing keyframe times from the inspector (not shown in the video, but works the same as dragging a keyframe)

godot-animation-timeline-changes.mp4

[dnllowe]

@TokageItLab , @Calinou Just a friendly ping to see if this is still on your radar

[Calinou]

Tested locally (rebased on top of master 374807f), it mostly works as expected.

There are 2 more remaining issues I noticed:

• Changing playback mode or interpolation mode with these icons doesn't update the animation preview until you scrub the timeline:

• If you drag a keyframe, it only updates the animation preview once the mouse cursor is released. I think it should update in real-time, without requiring you to release the mouse cursor. This is how it works already if you edit a property by dragging in the inspector.

[mihe]

@dnllowe Just a friendly ping to see if you've had a chance to look at the issues mentioned in the change request above. I can try to look into them otherwise, to help move things along here.

[TokageItLab]

Has the rebase failed? It seems that most PR fixes are not working.

[dnllowe]

@mihe Thanks for the friendly reminder -- this did drop off my radar. I'll take a look today
@TokageItLab I think the rebase went ok

[dnllowe]

@mihe & @TokageItLab Unfortunately, my changes no longer seem to work when applied to the latest master branch. I don't think I'll have the time or the context to re-apply this functionality without potentially breaking something that was fixed between then and now. This weekend I'll take a closer look at what was changed in the meantime and see what the solution might be, but it might be drifting away from my grasp of the engine.

[TokageItLab]

Perhaps this PR is affected by my fix #95405 disallowing seek to the same time in the editor.

You should create a new method that uses advance(0) for safer updates, or add an argument to _seek_value_changed() to allow same-time seek.

However, the former is much preferred if possible. Because if you use seek instead of advance, it will probably cause a bug that unintentionally snaps current position to the grid if current position is not on the grid and you update the key immediate after toggling the snap.

[dnllowe]

Perhaps this PR is affected by my fix #95405 disallowing seek to the same time in the editor.

You should create a new method that uses advance(0) for safer updates, or add an argument to _seek_value_changed() to allow same-time seek.

However, the former is much preferred if possible. Because if you use seek instead of advance, it will probably cause a bug that unintentionally snaps to the current position cursor if you update the key immediate after toggling the snap.

Thanks for the tip! I'll give this a try

[dnllowe]

@mihe, @TokageItLab I've updated the branch to work with the latest from master (using advance(0) did the trick).

@Calinou I was able to update the track when the animation modes change, but dragging a keyframe is a bit trickier. The keyframe position isn't updated until the mouse button is released, and logic about where the mouse drag started and stopped (seems) to be tied to clicking and releasing. So it becomes tough to try and A) Get the correct mouse drag distance in real-time and B) Update the animation keyframe in real-time (without inundating the undo history with every mouse movement). And even if done, the results are extremely erratic (at least for me, because I couldn't get a good method for the mouse drag distance in real-time). But perhaps it's possible somehow from gui_input.

Given that users were making do with none of these real-time updates / feedback before, I think this is as far as I can push these changes for now.

[TokageItLab]

Need to squash.

The AnimationPlaybackTrack is not updated in real time, but since the AnimationPlaybackTrack is special case, so I judge that it is fine for now. I think we can follow up as needed, but probably refactoring godotengine/godot-proposals#8783 first.

[TokageItLab]

@dnllowe To squash commits into one, please refer to https://docs.godotengine.org/en/stable/contributing/workflow/pr_workflow.html#the-interactive-rebase.

[dnllowe]

Need to squash.

The AnimationPlaybackTrack is not updated in real time, but since the AnimationPlaybackTrack is special case, so I judge that it is fine for now. I think we can follow up as needed, but probably refactoring godotengine/godot-proposals#8783 first.

@TokageItLab Thanks for that link!

Yes, a follow-up proposal with anything missing or more edge cases and polish would be a good way to go I think. Once godotengine/godot-proposals#8783 is done, feel free to ping me and I could probably have a look at updating the AnimationPlaybackTrack in real time (unless someone else is already further along with it)

Thanks for all the help and suggestions with the PR TokageItLab, Calinou, mihe, and patwork

[Calinou]

Looks good to me now. Great work!

[akien-mga]

Thanks! And congrats for your first merged Godot contribution 🎉

[dnllowe]

Thanks! And congrats for your first merged Godot contribution 🎉

Thanks! My first real open source contribution in general really. It was fun :)