Update the timeline when the current keyframe is edited in the inspector #76785
Conversation
|
Hey! Thanks for contributing. Pleae use more descriptive commit messages and PR descriptions, stating at least what was fixed where. |
…yframe under the play position is edited in the inspector
jcb936
force-pushed
the
76606-AnimationPlayer-TimelineUpdate
branch
from
c313add
to
bc1ead0
Compare
jcb936
changed the title
|
--rebased the branch and edited the PR title and first comment! Thanks! |
YuriSizov
changed the title
|
There looks to be a bug from either the recent refactoring, or even predating it. We call Not sure if this is the reason for the entire problem, or just something that you've accidentally stumbled upon, so qualified reviews would be appreciated. |
YuriSizov
added
the
cherrypick:4.1
|
This bug seems to have been around for a long time, probably before the refactoring. |
| if (!timeline_changed_bound && Math::is_equal_approx(timeline->get_play_position(), ofs)) { | ||
| timeline_changed_bound = true; | ||
| multi_key_edit->connect(CoreStringNames::get_singleton()->property_list_changed, callable_mp(this, &AnimationTrackEditor::_timeline_changed).bind(ofs, false, false)); | ||
| } |
There was a problem hiding this comment.
It looks redundant. You can rearrange the code to avoid to write twice for the update process by putting the timeline_changed flag outside of the if statement block, and only updating it once after the block.
|
Regarding concerned by @YuriSizov: In order to perform UndoRedo by changing an animation key, the value of the object must actually be changed instead of "updating the timeline" when the key is changed. Alternatively, we could implement a flag for UndoRedo to the "Update Timeline" argument, or the "Update Timeline" action could be bound as a method to register the UndoRedo action. However, we need to be cautious about pointer safety. |
|
This does not seem to work correctly. The timeline only updates correctly only if you start the editor at the exact time, otherwise nothing happens. Looking at the code, you are connecting the signal at some unspecified point and then never disconnect it. Not sure how it's supposed to work. Maybe the signal could always be connected, but then the change would be filtered depending on the current timeline position? |
| @@ -5156,6 +5157,12 @@ void AnimationTrackEditor::_update_key_edit() { | |||
| key_edit->hint = _find_hint_for_track(key_edit->track, np); | |||
| key_edit->base = np; | |||
|
|
|||
| // If we are editing the keyframe under the play position, we make sure | |||
| // to update the timeline when the properties are changed | |||
There was a problem hiding this comment.
| // to update the timeline when the properties are changed | |
| // to update the timeline when the properties are changed. |
Comment style nitpick
|
|
||
| // If we are editing the keyframe under the play position, we make sure | ||
| // to update the timeline when the properties are changed, we use the boolean | ||
| // to bind this signal once |
There was a problem hiding this comment.
| // to bind this signal once | |
| // to bind this signal once. |
YuriSizov
removed
the
cherrypick:4.1
Fixes #76606
When editing values in the inspector for a keyframe in the animation timeline, the timeline would not get updated, even if you were editing the keyframe under the current play position of the timeline. This resulted in awkward behaviour where you had to click on the timeline to see your keyframe edits applied. This fixes that by adding a signal connection to update the timeline when we are editing a keyframe where our play position is currently, as suggested by @KoBeWi on #76606