blender/blender
172
534
Fork 808
Code Issues 6.5k Pull Requests 976 Projects 21 Wiki Activity

Refactor: combine insert_keyframe() and insert_key_rna() into a single function #122053

Merged
Nathan Vegdahl merged 49 commits from nathanvegdahl/blender:combine_keying_functions into main 2024-06-11 16:43:08 +02:00
Conversation 8 Commits 49 Files Changed 9 +15 -3
1 changed files with 15 additions and 3 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit a7b2c97c20 - Show all commits

18
source/blender/animrig/intern/keyframing.cc
View File

@ -886,7 +886,15 @@ CombinedKeyingResult insert_keyframes(Main *bmain,
prop);
/* Handle the `force_all` condition mentioned above, ensuring the
* "all-or-nothing" behavior if needed. */
* "all-or-nothing" behavior if needed.
nathanvegdahl marked this conversation as resolved Outdated
Christoph Lendenfeld commented 2024-05-23 17:03:21 +02:00
Outdated
Review
Copy Link

In what case would the action from the AnimData and the action we get from id_action_ensure not be the same?

In what case would the action from the AnimData and the action we get from `id_action_ensure` not be the same?
Nathan Vegdahl commented 2024-05-23 17:38:36 +02:00
Outdated
Review
Copy Link

That's a great question. I'm not sure myself, but insert_keyframe() checked for this, so I'm keeping it in for now just in case. (At first I thought it might be related to the NLA, but IIRC if there's an NLA action for keying then it's temporarily stored as the animdata action anyway.) It's something I think we can probably investigate and remove later, but for now it's a "don't remove code you don't understand" kind of situation.

That's a great question. I'm not sure myself, but `insert_keyframe()` checked for this, so I'm keeping it in for now just in case. (At first I thought it might be related to the NLA, but IIRC if there's an NLA action for keying then it's temporarily stored as the animdata action anyway.) It's something I think we can probably investigate and remove later, but for now it's a "don't remove code you don't understand" kind of situation.
Christoph Lendenfeld commented 2024-05-23 17:39:55 +02:00
Outdated
Review
Copy Link

Maybe add your findings as a comment then so it's easier to investigate later when we actually want to remove it.

Maybe add your findings as a comment then so it's easier to investigate later when we actually want to remove it.
👍 1
*
* TODO: this currently doesn't account for the "Only Insert Available"
* flag, which also needs to be accounted for to actually ensure
* all-or-nothing behavior. This is because the function this part of the
* code originally came from (see #122053) also didn't account for it.
* Presumably that was an oversight, and should be addressed. But for now
* we're faithfully reproducing the original behavior.
*/
eInsertKeyFlags insert_key_flags_adjusted = insert_key_flags;
if (force_all && rna_values.size() > 0 &&
(insert_key_flags & (INSERTKEY_REPLACE | INSERTKEY_AVAILABLE)) != 0)
@ -898,11 +906,16 @@ CombinedKeyingResult insert_keyframes(Main *bmain,
if (!fcu) {
continue;
}
else if (!(insert_key_flags & INSERTKEY_REPLACE)) {
/* We found an fcurve, and "Only Replace" is not on, so a key insertion
* would succeed according to the two flags we're accounting for. */
if (!(insert_key_flags & INSERTKEY_REPLACE)) {
at_least_one_would_succeed = true;
break;
}
/* "Only Replace" *is* on, so a key insertion would succeed only if we
* actually replace an existing keyframe. */
bool replace;
BKE_fcurve_bezt_binarysearch_index(fcu->bezt, nla_frame, fcu->totvert, &replace);
if (replace) {
@ -915,7 +928,6 @@ CombinedKeyingResult insert_keyframes(Main *bmain,
* would prevent the other elements from getting keyed as well. */
if (at_least_one_would_succeed) {
insert_key_flags_adjusted &= ~(INSERTKEY_REPLACE | INSERTKEY_AVAILABLE);
break;
}
}