Fix: Cycle aware keying inserting first key #116356

Christoph Lendenfeld · 2023-12-19T17:00:41+01:00

2023-12-19 17:00:41 +01:00

Issue

When inserting the first key to a channel with cycle aware keying enabled the bookend keys would end up in the wrong position.
E.g. in this screenshot, the action range is 1 to 11

To reproduce:

Download the attached file
Hit I in the viewport and insert Location
Notice the keys are outside the actions range (1, 11)

Solution

If I understand the intention correctly, the code is meant to add the bookend keys to the animation cycle, so it cycles correctly for new F-Curves.
To do that I replaced the existing logic with a simple check if the inserted key is at the start or the end of the cycle.
This ensures the loop is properly bookended.

# Issue When inserting the first key to a channel with cycle aware keying enabled the bookend keys would end up in the wrong position. E.g. in this screenshot, the action range is 1 to 11 ![image](/attachments/022b87fd-20d8-42ce-8d0f-8dea74c650d7) To reproduce: * Download the attached file * Hit `I` in the viewport and insert `Location` * Notice the keys are outside the actions range (1, 11) # Solution If I understand the intention correctly, the code is meant to add the bookend keys to the animation cycle, so it cycles correctly for new F-Curves. To do that I replaced the existing logic with a simple check if the inserted key is at the start or the end of the cycle. This ensures the loop is properly bookended. ![image](/attachments/4937a173-51ef-4c03-a597-74260f4df3b4)

cycle_aware_keying.blend

864 KiB

image.png

11 KiB

image.png

10 KiB

Christoph Lendenfeld added the

Module

Animation & Rigging

label 2023-12-19 17:00:41 +01:00

Christoph Lendenfeld added 1 commit 2023-12-19 17:00:52 +01:00

4bd35e459c fix

Christoph Lendenfeld requested review from Alexander Gavrilov 2023-12-19 17:01:30 +01:00

Alexander Gavrilov commented

2023-12-19 17:28:44 +01:00

There is no such thing as 'bookend keys'. It would be completely detrimental to mandate keys to be placed at any specific point on all channels. The only thing that matters is the distance between the first and last keyframes of each curve.

Christoph Lendenfeld commented

2023-12-19 17:32:31 +01:00

So it is a feature to create keys outside the defined action range?
Sorry that wasn't clear to me. If you can confirm I will just leave a comment in that place that clarifies that

So it is a feature to create keys outside the defined action range? Sorry that wasn't clear to me. If you can confirm I will just leave a comment in that place that clarifies that

Alexander Gavrilov commented

2023-12-19 17:34:27 +01:00

The real solution for cycling would be to avoid any repetition by implementing cycling implicitly based on the action frame range. Thus there would be no need to repeat any keys in the first place, and there would be no special 'first' and 'last' keys. However this would mean refactoring cyclic behavior implementation to be an integral part of action and F-Curve behavior, rather than some bolted-on modifier or NLA thing.

Alexander Gavrilov commented

2023-12-19 17:42:57 +01:00

The basic idea of cyclic aware is that cyclic curves repeat infinitely, so inserting a key at any point in the timeline should automatically update the interval where the keys are "really" defined, so that "real" location should not matter to the user.

The action frame range defines the "window" where the action is intended to be played, and also the cycle length. This allows additionally taking the interval and length into account in cycle aware to ensure new curves are also cyclic with the right period, and to preferentially use the window for placing the "real" keys. However it absolutely does not mean there cannot be keys outside the window, and staggering keys between channels should be allowed.

The current implementation is based on the simple idea that you inserted one key, so it should remain a single key: even though it has to be duplicated to ensure the cycle, this duplication is a technical hack necessary for the cycle to work, and logically this is one key. Unfortunately, this of course can result in "weird" placement of the "real" key interval. Your version on the other hand actually inserts two separate logical keys.

The basic idea of cyclic aware is that cyclic curves repeat infinitely, so inserting a key at any point in the timeline should automatically update the interval where the keys are "really" defined, so that "real" location should not matter to the user. The action frame range defines the "window" where the action is intended to be played, and also the cycle length. This allows additionally taking the interval and length into account in cycle aware to ensure new curves are also cyclic with the right period, and to preferentially use the window for placing the "real" keys. However it absolutely does not mean there cannot be keys outside the window, and staggering keys between channels should be allowed. The current implementation is based on the simple idea that you inserted one key, so it should remain a single key: even though it has to be duplicated to ensure the cycle, this duplication is a technical hack necessary for the cycle to work, and logically this is one key. Unfortunately, this of course can result in "weird" placement of the "real" key interval. Your version on the other hand actually inserts two separate logical keys.

Alexander Gavrilov commented

2023-12-19 17:45:24 +01:00

I actually envisioned an operator to move the "real" interval by rotating the keys left or right along the cycle (thus changing which key is the duplicated end point), but never got around to implementing it either in C or python. It would have allowed easily fixing weird key placement later.

Christoph Lendenfeld commented

2023-12-19 17:53:39 +01:00

ok I see what you mean, I will close the PR in that case.

About the offset of keys from the cycle range. The use case I see for that is offsetting animations to do e.g. overlaps which is usually a pain with bookend keys.
We could explore other ways to do that in the future. E.g. update the keyframes while translating them to respect the loop. Kind of what the "Time Offset" operator in the graph editor does

ok I see what you mean, I will close the PR in that case. About the offset of keys from the cycle range. The use case I see for that is offsetting animations to do e.g. overlaps which is usually a pain with bookend keys. We could explore other ways to do that in the future. E.g. update the keyframes while translating them to respect the loop. Kind of what the "Time Offset" operator in the graph editor does

Christoph Lendenfeld closed this pull request

2023-12-19 17:53:41 +01:00

Alexander Gavrilov commented

2023-12-19 18:04:02 +01:00

Potentially that rotate keys operator (naming inspired by the 'bit rotate' bit shift operation) could be implemented in such a way that key insertion could automatically run it in the cycle aware mode after the actual key insertion to fit the key placement to the action frame range as much as possible given the existing key locations.

Pull request closed

Please reopen this pull request to perform a merge.

Sign in to join this conversation.

No reviewers

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

Fix: Cycle aware keying inserting first key #116356

Issue

Solution

Pull request closed