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

Merged

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
Christoph Lendenfeld added 2 commits 2024-01-18 17:59:30 +01:00
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
Member

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

So, I'm not 100% sure, but I think my suggestion below might help clean things up.
@ -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,
Member

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.
Author
Member

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
Member

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.
Author
Member

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
Member

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:

  1. 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.
  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.

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.
Member

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
Christoph Lendenfeld 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
Christoph Lendenfeld requested review from Nathan Vegdahl 2024-02-08 13:06:50 +01:00
Author
Member

@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
Member

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

@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
Member

Looks good to me!

Looks good to me!
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
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset System
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Viewport & EEVEE
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Asset Browser Project
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Module
Viewport & EEVEE
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Severity
High
Severity
Low
Severity
Normal
Severity
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
No project
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#116943
No description provided.