From 0caf6397dfc01f74ed0cbbd43aa1add55491ea72 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sybren=20A=2E=20St=C3=BCvel?= Date: Mon, 13 May 2024 17:30:37 +0200 Subject: [PATCH] Anim, remove `Action::binding_for_id()` method Remove the `Action::binding_for_id()` method, as it is a bit dangerous, only useful in a very specific situation, and can be removed altogether with just a little bit of refactoring. This in turn made some other functions a bit simpler too. Almost no functional changes. The only change is that when through some magic (aka data corruption or other bug) the Binding is no longer valid for the animated ID's type, the IDs properties may still show as animated in the interface. Pull Request: https://projects.blender.org/blender/blender/pulls/121748 --- source/blender/animrig/ANIM_action.hh | 3 - source/blender/animrig/ANIM_animdata.hh | 9 +-- source/blender/animrig/intern/animation.cc | 22 ------- source/blender/animrig/intern/animdata.cc | 21 +++++-- source/blender/blenkernel/BKE_fcurve.hh | 3 +- source/blender/blenkernel/intern/fcurve.cc | 62 ++++++++++--------- .../blender/blenkernel/intern/lib_override.cc | 4 +- .../editors/animation/anim_channels_edit.cc | 4 +- 8 files changed, 58 insertions(+), 70 deletions(-) diff --git a/source/blender/animrig/ANIM_action.hh b/source/blender/animrig/ANIM_action.hh index 872d912a3d7..15a4cb090d1 100644 --- a/source/blender/animrig/ANIM_action.hh +++ b/source/blender/animrig/ANIM_action.hh @@ -163,9 +163,6 @@ class Action : public ::bAction { Binding *binding_find_by_name(StringRefNull binding_name); - Binding *binding_for_id(const ID &animated_id); - const Binding *binding_for_id(const ID &animated_id) const; - /** * Create a new, unused Binding. * diff --git a/source/blender/animrig/ANIM_animdata.hh b/source/blender/animrig/ANIM_animdata.hh index 691b45db32b..2f74d3c157c 100644 --- a/source/blender/animrig/ANIM_animdata.hh +++ b/source/blender/animrig/ANIM_animdata.hh @@ -59,15 +59,16 @@ bool animdata_remove_empty_action(AnimData *adt); * \note The returned FCurve should NOT be used for keyframe manipulation. Its * existence is an indicator for "this property is animated". * + * \note This function assumes that `adt->action` actually points to a layered + * Action. It is a bug to call this with a legacy Action, or without one. + * * This function should probably be limited to the active layer (for the given * property, once pinning to layers is there), so that the "this is keyed" color * is more accurate. * - * Again, this is just to hook up the new Animation data-block to the old - * Blender UI code. + * Again, this is just to hook up the layered Action to the old Blender UI code. */ -const FCurve *fcurve_find_by_rna_path(const Action &anim, - const ID &animated_id, +const FCurve *fcurve_find_by_rna_path(const AnimData &adt, StringRefNull rna_path, int array_index); diff --git a/source/blender/animrig/intern/animation.cc b/source/blender/animrig/intern/animation.cc index c076f9beb6b..eac3dfed2db 100644 --- a/source/blender/animrig/intern/animation.cc +++ b/source/blender/animrig/intern/animation.cc @@ -316,28 +316,6 @@ Binding *Action::binding_find_by_name(const StringRefNull binding_name) return nullptr; } -Binding *Action::binding_for_id(const ID &animated_id) -{ - const Binding *binding = const_cast(this)->binding_for_id(animated_id); - return const_cast(binding); -} - -const Binding *Action::binding_for_id(const ID &animated_id) const -{ - const AnimData *adt = BKE_animdata_from_id(&animated_id); - - /* Note that there is no check that `adt->action` is actually `this`. */ - - const Binding *binding = this->binding_for_handle(adt->binding_handle); - if (!binding) { - return nullptr; - } - if (!binding->is_suitable_for(animated_id)) { - return nullptr; - } - return binding; -} - Binding &Action::binding_allocate() { Binding &binding = MEM_new(__func__)->wrap(); diff --git a/source/blender/animrig/intern/animdata.cc b/source/blender/animrig/intern/animdata.cc index e8329b2ab96..1df10a0d0c4 100644 --- a/source/blender/animrig/intern/animdata.cc +++ b/source/blender/animrig/intern/animdata.cc @@ -184,21 +184,32 @@ void reevaluate_fcurve_errors(bAnimContext *ac) } } -const FCurve *fcurve_find_by_rna_path(const Action &anim, - const ID &animated_id, +const FCurve *fcurve_find_by_rna_path(const AnimData &adt, const StringRefNull rna_path, const int array_index) { - const Binding *binding = anim.binding_for_id(animated_id); + BLI_assert(adt.action); + if (!adt.action) { + return nullptr; + } + + const Action &action = adt.action->wrap(); + BLI_assert(action.is_action_layered()); + + const Binding *binding = action.binding_for_handle(adt.binding_handle); if (!binding) { /* No need to inspect anything if this ID does not have an animation Binding. */ return nullptr; } + /* No check for the binding's ID type. Not only do we not have the actual ID + * to do this check, but also, since the Action and the binding have been + * assigned, just trust that it's valid. */ + /* Iterate the layers top-down, as higher-up animation overrides (or at least can override) * lower-down animation. */ - for (int layer_idx = anim.layer_array_num - 1; layer_idx >= 0; layer_idx--) { - const Layer *layer = anim.layer(layer_idx); + for (int layer_idx = action.layer_array_num - 1; layer_idx >= 0; layer_idx--) { + const Layer *layer = action.layer(layer_idx); /* TODO: refactor this into something nicer once we have different strip types. */ for (const Strip *strip : layer->strips()) { diff --git a/source/blender/blenkernel/BKE_fcurve.hh b/source/blender/blenkernel/BKE_fcurve.hh index 657acfb3b6e..a840a87b474 100644 --- a/source/blender/blenkernel/BKE_fcurve.hh +++ b/source/blender/blenkernel/BKE_fcurve.hh @@ -314,8 +314,7 @@ int BKE_fcurves_filter(ListBase *dst, ListBase *src, const char *dataPrefix, con * property, what is returned is a best-effort guess. The topmost layer has priority, and it is * assumed that when it has a strip, it's infinite. */ -FCurve *BKE_animadata_fcurve_find_by_rna_path(const ID *id, - AnimData *animdata, +FCurve *BKE_animadata_fcurve_find_by_rna_path(AnimData *animdata, const char *rna_path, const int rna_index, bAction **r_action, diff --git a/source/blender/blenkernel/intern/fcurve.cc b/source/blender/blenkernel/intern/fcurve.cc index d667c787196..82218857c03 100644 --- a/source/blender/blenkernel/intern/fcurve.cc +++ b/source/blender/blenkernel/intern/fcurve.cc @@ -17,6 +17,7 @@ #include "ANIM_action.hh" #include "ANIM_animdata.hh" +#include "DNA_action_types.h" #include "DNA_anim_types.h" #include "DNA_object_types.h" #include "DNA_text_types.h" @@ -258,7 +259,7 @@ FCurve *id_data_find_fcurve( * needs to be re-checked I think?. */ bool is_driven = false; FCurve *fcu = BKE_animadata_fcurve_find_by_rna_path( - id, adt, path->c_str(), index, nullptr, &is_driven); + adt, path->c_str(), index, nullptr, &is_driven); if (is_driven) { if (r_driven != nullptr) { *r_driven = is_driven; @@ -355,34 +356,38 @@ int BKE_fcurves_filter(ListBase *dst, ListBase *src, const char *dataPrefix, con return matches; } -FCurve *action_fcurve_find_by_rna_path(const ID *id, - blender::animrig::Action &action, - const char *rna_path, - const int rna_index) +static std::optional> animdata_fcurve_find_by_rna_path( + AnimData &adt, const char *rna_path, const int rna_index) { + if (!adt.action) { + return std::nullopt; + } + + blender::animrig::Action &action = adt.action->wrap(); if (action.is_empty()) { - return nullptr; + return std::nullopt; } + FCurve *fcu = nullptr; if (action.is_action_layered()) { - BLI_assert(id); - const FCurve *fcu = blender::animrig::fcurve_find_by_rna_path( - action, *id, rna_path, rna_index); - - /* The new Animation data-block is stricter with const-ness than older code, hence the + const FCurve *const_fcurve = blender::animrig::fcurve_find_by_rna_path( + adt, rna_path, rna_index); + /* The new layered Action code is stricter with const-ness than older code, hence the * const_cast. */ - return const_cast(fcu); + fcu = const_cast(const_fcurve); + } + else { + fcu = BKE_fcurve_find(&action.curves, rna_path, rna_index); } - return BKE_fcurve_find(&action.curves, rna_path, rna_index); + if (!fcu) { + return std::nullopt; + } + return std::make_pair(fcu, &action); } -FCurve *BKE_animadata_fcurve_find_by_rna_path(const ID *id, - AnimData *animdata, - const char *rna_path, - int rna_index, - bAction **r_action, - bool *r_driven) +FCurve *BKE_animadata_fcurve_find_by_rna_path( + AnimData *animdata, const char *rna_path, int rna_index, bAction **r_action, bool *r_driven) { if (r_driven != nullptr) { *r_driven = false; @@ -391,17 +396,14 @@ FCurve *BKE_animadata_fcurve_find_by_rna_path(const ID *id, *r_action = nullptr; } - /* Action takes priority over drivers. */ - if (animdata->action) { - blender::animrig::Action &action = animdata->action->wrap(); - /* TODO: pass the binding handle instead of the id. */ - FCurve *fcurve = action_fcurve_find_by_rna_path(id, action, rna_path, rna_index); - if (fcurve) { - if (r_action) { - *r_action = &action; - } - return fcurve; + std::optional> found = animdata_fcurve_find_by_rna_path( + *animdata, rna_path, rna_index); + if (found) { + /* Action takes priority over drivers. */ + if (r_action) { + *r_action = found->second; } + return found->first; } /* If not animated, check if driven. */ @@ -496,7 +498,7 @@ FCurve *BKE_fcurve_find_by_rna_context_ui(bContext * /*C*/, /* Standard F-Curve from animdata - Animation (Action) or Drivers. */ FCurve *fcu = BKE_animadata_fcurve_find_by_rna_path( - ptr->owner_id, adt, rna_path->c_str(), rnaindex, r_action, r_driven); + adt, rna_path->c_str(), rnaindex, r_action, r_driven); if (fcu != nullptr && r_animdata != nullptr) { *r_animdata = adt; diff --git a/source/blender/blenkernel/intern/lib_override.cc b/source/blender/blenkernel/intern/lib_override.cc index 43dba1f8f38..3e286dccf10 100644 --- a/source/blender/blenkernel/intern/lib_override.cc +++ b/source/blender/blenkernel/intern/lib_override.cc @@ -361,12 +361,12 @@ bool BKE_lib_override_library_property_is_animated( const char index_token_start_backup = *index_token_start; *index_token_start = '\0'; fcurve = BKE_animadata_fcurve_find_by_rna_path( - id, anim_data, liboverride_prop->rna_path, rnaprop_index, nullptr, nullptr); + anim_data, liboverride_prop->rna_path, rnaprop_index, nullptr, nullptr); *index_token_start = index_token_start_backup; } else { fcurve = BKE_animadata_fcurve_find_by_rna_path( - id, anim_data, liboverride_prop->rna_path, 0, nullptr, nullptr); + anim_data, liboverride_prop->rna_path, 0, nullptr, nullptr); } if (fcurve != nullptr) { return true; diff --git a/source/blender/editors/animation/anim_channels_edit.cc b/source/blender/editors/animation/anim_channels_edit.cc index 101ff5de56d..0cee0dddbd5 100644 --- a/source/blender/editors/animation/anim_channels_edit.cc +++ b/source/blender/editors/animation/anim_channels_edit.cc @@ -4585,7 +4585,7 @@ static blender::Vector get_fcurves_of_property( const int length = RNA_property_array_length(ptr, prop); for (int i = 0; i < length; i++) { FCurve *fcurve = BKE_animadata_fcurve_find_by_rna_path( - id, anim_data, path->c_str(), i, nullptr, nullptr); + anim_data, path->c_str(), i, nullptr, nullptr); if (fcurve != nullptr) { fcurves.append(fcurve); } @@ -4593,7 +4593,7 @@ static blender::Vector get_fcurves_of_property( } else { FCurve *fcurve = BKE_animadata_fcurve_find_by_rna_path( - id, anim_data, path->c_str(), index, nullptr, nullptr); + anim_data, path->c_str(), index, nullptr, nullptr); if (fcurve != nullptr) { fcurves.append(fcurve); }