blender/blender
168
526
Fork 800
Code Issues 6.6k Pull Requests 957 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 +5 -11
1 changed files with 5 additions and 11 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 4602cc3021 - Show all commits

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

@ -1003,17 +1003,11 @@ CombinedKeyingResult insert_keyframes(Main *bmain,
}
Vector<float> rna_values = get_keyframe_values(&ptr, prop, visual_keyframing);
BitVector<> elements_to_key(rna_values.size(), false);
BitVector<> rna_values_mask(rna_values.size(), false);
bool force_all;
/* NOTE: this function call is complex with interesting effects. Of
* particular note is that in addition to doing value remapping, it also:
* - Fills in `elements_to_key`, flagging which elements of an array
* property should actually get keyed.
* - Sets `force_all`, which if true means that an array property should be
* treated as all-or-nothing: either all elements get keyed or none do,
* regardless of how keying flags might have otherwise treated different
* elements differently. */
/* NOTE: this function call is complex with interesting/non-obvious effects.
* Please see its documentation for details. */
BKE_animsys_nla_remap_keyframe_values(nla_context,
struct_pointer,
nathanvegdahl marked this conversation as resolved Outdated
Sybren A. Stüvel commented 2024-06-11 11:37:54 +02:00
Outdated
Review
Copy Link

The functionality of BKE_animsys_nla_remap_keyframe_values() should be documented at the declaration of that function, not here. The note that this is an "interesting" function with various potentially-unexpected effects is very welcome though.

The functionality of `BKE_animsys_nla_remap_keyframe_values()` should be documented at the declaration of that function, not here. The note that this is an "interesting" function with various potentially-unexpected effects is very welcome though.
Nathan Vegdahl commented 2024-06-11 15:39:33 +02:00
Outdated
Review
Copy Link

Good call. I've made a separate PR that can be landed either before or after this one that (hopefully) improves the documentation of BKE_animsys_nla_remap_keyframe_values() in the ways needed to understand this call site's usage of it: #123081

Good call. I've made a separate PR that can be landed either before or after this one that (hopefully) improves the documentation of `BKE_animsys_nla_remap_keyframe_values()` in the ways needed to understand this call site's usage of it: #123081
Nathan Vegdahl commented 2024-06-11 15:43:53 +02:00
Outdated
Review
Copy Link

I've now removed the output parameter descriptions, leaving that to #123081. I also renamed the elements_to_key variable to rna_values_mask, to make its relationship to rna_values more obvious.

I've now removed the output parameter descriptions, leaving that to #123081. I also renamed the `elements_to_key` variable to `rna_values_mask`, to make its relationship to `rna_values` more obvious.
👍 1
prop,
@ -1021,7 +1015,7 @@ CombinedKeyingResult insert_keyframes(Main *bmain,
rna_path.index.value_or(-1),
&anim_eval_context,
&force_all,
elements_to_key);
rna_values_mask);
const std::optional<std::string> rna_path_id_to_prop = RNA_path_from_ID_to_property(&ptr,
prop);
@ -1080,7 +1074,7 @@ CombinedKeyingResult insert_keyframes(Main *bmain,
rna_values.as_span(),
insert_key_flags_adjusted,
key_type,
elements_to_key);
rna_values_mask);
combined_result.merge(result);
}