Fix: Issue with Cycle aware keying when inserting second key #116943

Christoph Lendenfeld · 2024-01-09T15:11:32+01:00

2024-01-09 15:11:32 +01:00

Caused by #113504: Anim: Insert keyframes without keying sets

While basic cycle aware keying works, there is a special case
when inserting the first key. In that case, after the key has been inserted,
it is duplicated and moved around so the FCurve range from
first to last key is exactly the range of the action.
It also auto-creates the Cycle modifier on the FCurve .

Fix the issue by calling the function that does the key duplication
and cover with unit tests.

For the review.

I am not too happy that I had to make the ReportList argument on insert_keyframe_fcurve_value optional by checking for a nullptr.
Equally I'd like to avoid having to pass that all the way down. (The keying code is too verbose as it is anyway)
What should I do there? First refactor insert_keyframe_fcurve_value to not take that argument (and as such not print messages) or pass the argument down.
Will be cleaned up in 4.2 #117449: Refactor: keyframing functions return more detailed status

Caused by [#113504: Anim: Insert keyframes without keying sets](https://projects.blender.org/blender/blender/pulls/113504) While basic cycle aware keying works, there is a special case when inserting the first key. In that case, after the key has been inserted, it is duplicated and moved around so the FCurve range from first to last key is exactly the range of the action. It also auto-creates the Cycle modifier on the FCurve . Fix the issue by calling the function that does the key duplication and cover with unit tests. ----- For the review. ~~I am not too happy that I had to make the `ReportList` argument on `insert_keyframe_fcurve_value` optional by checking for a `nullptr`. Equally I'd like to avoid having to pass that all the way down. (The keying code is too verbose as it is anyway) What should I do there? First refactor `insert_keyframe_fcurve_value` to not take that argument (and as such not print messages) or pass the argument down.~~ Will be cleaned up in 4.2 [#117449: Refactor: keyframing functions return more detailed status](https://projects.blender.org/blender/blender/pulls/117449)

Christoph Lendenfeld added the

Module

Animation & Rigging

label 2024-01-09 15:11:32 +01:00

Christoph Lendenfeld added 4 commits 2024-01-09 15:11:39 +01:00

87ef870c48 add unit tests

356a2240cf Merge branch 'main' into fix_cycle_aware_keying_first_key

678107c3f9 Merge branch 'main' into fix_cycle_aware_keying_first_key

b9bf84a48f wip

Christoph Lendenfeld referenced this pull request

2024-01-09 17:28:12 +01:00

Insert keyframe wrong with cycle-aware keying after deleting end frame #111322

Christoph Lendenfeld added 2 commits 2024-01-18 17:59:30 +01:00

6850a40dc4 Merge branch 'main' into fix_cycle_aware_keying_first_key

95bf299c8c the fix

Christoph Lendenfeld changed title from ~~WIP: Fix: Issue with Cycle aware keying when inserting second key~~ to Fix: Issue with Cycle aware keying when inserting second key

2024-01-18 17:59:46 +01:00

Christoph Lendenfeld requested review from Nathan Vegdahl 2024-01-18 18:05:52 +01:00

Nathan Vegdahl requested changes 2024-01-19 12:13:15 +01:00

Nathan Vegdahl left a comment

So, I'm not 100% sure, but I think my suggestion below might help clean things up.

source/blender/animrig/intern/keyframing.cc Outdated

						
				@ -873,3 +873,1 @@

				        bmain, action, group.c_str(), ptr, rna_path.c_str(), property_array_index);

				    const bool inserted_key = insert_keyframe_value(

				        fcurve, frame, value, key_type, insert_key_flag);

				    const bool inserted_key = insert_keyframe_fcurve_value(bmain,

Nathan Vegdahl commented

2024-01-19 12:12:43 +01:00

Do I understand correctly that insert_key_action() is a recently added function, more-or-less intended to replace the existing more complicated functions?

If so, then I'd say that rather than calling into insert_keyframe_fcurve_value() here, it would make sense to still call action_fcurve_ensure() and insert_keyframe_value() as before, and just additionally call make_new_fcurve_cyclic() as appropriate.

Do I understand correctly that `insert_key_action()` is a recently added function, more-or-less intended to replace the existing more complicated functions? If so, then I'd say that rather than calling into `insert_keyframe_fcurve_value()` here, it would make sense to still call `action_fcurve_ensure()` and `insert_keyframe_value()` as before, and just additionally call `make_new_fcurve_cyclic()` as appropriate.

Christoph Lendenfeld commented

2024-01-19 12:41:01 +01:00

I thought about this as well. But given how much code is needed for the cyclic logic the body of the for loop would become quite large, in which case I'd like to separate it into a function anyway.
That's why I decided to call the existing function. We can then look at removing the need for the ReportList argument

I thought about this as well. But given how much code is needed for the cyclic logic the body of the for loop would become quite large, in which case I'd like to separate it into a function anyway. That's why I decided to call the existing function. We can then look at removing the need for the `ReportList` argument

Nathan Vegdahl commented

2024-01-19 14:54:44 +01:00

Ah, that's fair. I missed some of the logic in insert_keyframe_fcurve_value(): it actually ensures the cyclic invariant twice, both before and after inserting the key, but I only noticed the one after.

So basically, this seems like an issue of who is responsible for reporting user-level errors. And if insert_keyframe_fcurve_value() is more of a low-level function that other functions call to do their work, then I think a good argument could be made that it shouldn't be doing the user-level error reporting, and instead should propagate errors up to its callers.

We could throw an exception to do that, but I personally think exceptions are awful and make code hard to reason about.

So I would instead advocate for simply returning an error enum from insert_keyframe_fcurve_value() rather than a bool. Something like:

enum class KeyInsertionResult {
  Success,
  CouldNotCreateFCurve,
  CouldNotInsertKey
}

(The naming is just off the top of my head—can probably be improved.)

By propagating this result up the call chain as needed, any higher-level function gets both the low-level context (from the enum) and the high-level context (from itself) about what happened, and can thus report a better error message to the user.

And then insert_keyframe_fcurve_value() doesn't have to be responsible for reporting at all. And other low-level keying functions can also return the same enum (which can be expanded with more error cases as needed), to keep the actual user-level error reporting in the higher level functions.

Having said all that, this is informed heavily by my experience coding in Rust, and I'm not sure how obvious this pattern would be to C/C++ programmers reading the code in the future.

Ah, that's fair. I missed some of the logic in `insert_keyframe_fcurve_value()`: it actually ensures the cyclic invariant twice, both before and after inserting the key, but I only noticed the one after. So basically, this seems like an issue of who is responsible for reporting user-level errors. And if `insert_keyframe_fcurve_value()` is more of a low-level function that other functions call to do their work, then I think a good argument could be made that it shouldn't be doing the user-level error reporting, and instead should propagate errors up to its callers. We could throw an exception to do that, but I personally think exceptions are awful and make code hard to reason about. So I would instead advocate for simply returning an error enum from `insert_keyframe_fcurve_value()` rather than a bool. Something like: ```C++ enum class KeyInsertionResult { Success, CouldNotCreateFCurve, CouldNotInsertKey } ``` (The naming is just off the top of my head—can probably be improved.) By propagating this result up the call chain as needed, any higher-level function gets both the low-level context (from the enum) and the high-level context (from itself) about what happened, and can thus report a better error message to the user. And then `insert_keyframe_fcurve_value()` doesn't have to be responsible for reporting at all. And other low-level keying functions can also return the same enum (which can be expanded with more error cases as needed), to keep the actual user-level error reporting in the higher level functions. Having said all that, this is informed heavily by my experience coding in Rust, and I'm not sure how obvious this pattern would be to C/C++ programmers reading the code in the future.

Christoph Lendenfeld commented

2024-01-19 15:52:44 +01:00

I like that you suggested that because I thought of the same thing.
At a higher level, we return the amount of keys inserted. Do you think it's reasonable to have a struct that contains both the enum and the keys inserted?
Or should I worry about that later, i.e. not for this PR

I like that you suggested that because I thought of the same thing. At a higher level, we return the amount of keys inserted. Do you think it's reasonable to have a `struct` that contains both the enum and the keys inserted? Or should I worry about that later, i.e. not for this PR

Nathan Vegdahl commented

2024-01-22 10:58:43 +01:00

For context: the pattern I'm used to here is Rust's Result<T, E> type, which can be either the expected return value (T) or an error value (E). C++23 actually has something similar, but we can't use it yet, and I'm also a little skeptical of its ergonomics (Rust makes error handling in this way very ergonomic, but C++ has a habit of taking ideas from elsewhere and then making them as awkward as possible).

So with that context out of the way, I think we have a couple of options:

Follow in Go's footsteps, and use std::pair or std::tuple to return both the actual return value and the result enum together. And then calling code simply checks the result enum if relevant.
Write our own templated BLI result type for returning maybe-errors in Blender's code base.

For now I think option 1 is fine. Option 2 would be great, and I think is worth pursing for the medium-to-long term, but will likely need coordination and discussion with other modules and core.

For context: the pattern I'm used to here is Rust's `Result<T, E>` type, which can be *either* the expected return value (`T`) *or* an error value (`E`). C++23 actually [has something similar](https://en.cppreference.com/w/cpp/utility/expected), but we can't use it yet, and I'm also a little skeptical of its ergonomics (Rust makes error handling in this way very ergonomic, but C++ has a habit of taking ideas from elsewhere and then making them as awkward as possible). So with that context out of the way, I think we have a couple of options: 1. Follow in Go's footsteps, and use [std::pair](https://en.cppreference.com/w/cpp/utility/pair) or [std::tuple](https://en.cppreference.com/w/cpp/utility/tuple) to return both the actual return value and the result enum together. And then calling code simply checks the result enum if relevant. 2. Write our own templated BLI result type for returning maybe-errors in Blender's code base. For now I think option 1 is fine. Option 2 would be great, and I think is worth pursing for the medium-to-long term, but will likely need coordination and discussion with other modules and core.

Nathan Vegdahl commented

2024-01-25 10:51:36 +01:00

Speaking of option number 2: https://devtalk.blender.org/t/developer-discussion-handling-errors/33054

(Not that that's immediately relevant to this PR, but just FYI. :-) )

Speaking of option number 2: https://devtalk.blender.org/t/developer-discussion-handling-errors/33054 (Not that that's immediately relevant to this PR, but just FYI. :-) )

nathanvegdahl marked this conversation as resolved

Christoph Lendenfeld added 1 commit 2024-01-23 15:16:05 +01:00

81c853d0a9 Merge branch 'main' into fix_cycle_aware_keying_first_key

Christoph Lendenfeld referenced this pull request

2024-01-23 15:23:48 +01:00

Refactor: keyframing functions return more detailed status #117449

ChrisLend changed target branch from main to blender-v4.1-release

2024-02-08 13:00:24 +01:00

Christoph Lendenfeld added 1 commit 2024-02-08 13:02:01 +01:00

f3889d5140 Merge branch 'blender-v4.1-release' into fix_cycle_aware_keying_first_key

Christoph Lendenfeld requested review from Nathan Vegdahl 2024-02-08 13:06:50 +01:00

Christoph Lendenfeld commented

2024-02-08 13:08:00 +01:00

@nathanvegdahl since #117449: Refactor: keyframing functions return more detailed status didn't land in time for bcon3, I think the best option for now is to have the ReportList optional.
What do you think

@nathanvegdahl since [#117449: Refactor: keyframing functions return more detailed status](https://projects.blender.org/blender/blender/pulls/117449) didn't land in time for bcon3, I think the best option for now is to have the `ReportList` optional. What do you think

Nathan Vegdahl commented

2024-02-09 09:47:06 +01:00

@ChrisLend Yeah, I think that's the way to go.

Nathan Vegdahl approved these changes 2024-02-09 09:47:21 +01:00

Nathan Vegdahl left a comment

Looks good to me!

👍 1

Christoph Lendenfeld referenced this issue from a commit

2024-02-09 10:13:37 +01:00

Fix: Issue with Cycle aware keying when inserting second key

Christoph Lendenfeld merged commit afa4391eeb into blender-v4.1-release

2024-02-09 10:13:37 +01:00

Christoph Lendenfeld deleted branch fix_cycle_aware_keying_first_key

2024-02-09 10:13:39 +01:00

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: Issue with Cycle aware keying when inserting second key #116943