From 77f4fd2637f90318e326bec1e96bf41524eee1bc Mon Sep 17 00:00:00 2001 From: Nathan Vegdahl Date: Thu, 30 Jan 2025 10:24:47 +0100 Subject: [PATCH] Cleanup: Anim: rename `*_move()` methods to `*_move_to_index()` There were three methods with succinct but slightly unclear names: - Action::slot_move() - Channelbag::channel_group_move() - Channelbag::fcurve_move() The ambiguity is due to other functions with similar names that e.g. move fcurves *between* Channelbags, etc. Whereas these methods only move items to other positions within the array they're already in. To make this clearer, this PR adds `_to_index` to the end of these array-oriented methods. Pull Request: https://projects.blender.org/blender/blender/pulls/133730 --- source/blender/animrig/ANIM_action.hh | 6 ++-- source/blender/animrig/intern/action.cc | 6 ++-- source/blender/animrig/intern/action_test.cc | 30 ++++++++-------- .../editors/animation/anim_channels_edit.cc | 35 ++++++++++--------- 4 files changed, 39 insertions(+), 38 deletions(-) diff --git a/source/blender/animrig/ANIM_action.hh b/source/blender/animrig/ANIM_action.hh index e3281c9c4a5..7db2a3f91d5 100644 --- a/source/blender/animrig/ANIM_action.hh +++ b/source/blender/animrig/ANIM_action.hh @@ -325,7 +325,7 @@ class Action : public ::bAction { * `slot` must belong to this action, and `to_slot_index` must be a * valid index in the slot array. */ - void slot_move(Slot &slot, int to_slot_index); + void slot_move_to_index(Slot &slot, int to_slot_index); /** * Set the active Slot, ensuring only one Slot is flagged as the Active one. @@ -1131,7 +1131,7 @@ class Channelbag : public ::ActionChannelbag { * `fcurve` must belong to this channel bag, and `to_fcurve_index` must be a * valid index in the fcurve array. */ - void fcurve_move(FCurve &fcurve, int to_fcurve_index); + void fcurve_move_to_index(FCurve &fcurve, int to_fcurve_index); /** * Remove all F-Curves from this Channelbag. @@ -1211,7 +1211,7 @@ class Channelbag : public ::ActionChannelbag { * `group` must belong to this channel bag, and `to_group_index` must be a * valid index in the channel group array. */ - void channel_group_move(bActionGroup &group, int to_group_index); + void channel_group_move_to_index(bActionGroup &group, int to_group_index); /** * Assigns the given FCurve to the given channel group. diff --git a/source/blender/animrig/intern/action.cc b/source/blender/animrig/intern/action.cc index 46b21b02116..732863f7414 100644 --- a/source/blender/animrig/intern/action.cc +++ b/source/blender/animrig/intern/action.cc @@ -586,7 +586,7 @@ bool Action::slot_remove(Slot &slot_to_remove) return true; } -void Action::slot_move(Slot &slot, const int to_slot_index) +void Action::slot_move_to_index(Slot &slot, const int to_slot_index) { BLI_assert(this->slots().index_range().contains(to_slot_index)); @@ -1936,7 +1936,7 @@ void Channelbag::fcurve_detach_by_index(const int64_t fcurve_index) * depsgraph evaluation results though. */ } -void Channelbag::fcurve_move(FCurve &fcurve, int to_fcurve_index) +void Channelbag::fcurve_move_to_index(FCurve &fcurve, int to_fcurve_index) { BLI_assert(to_fcurve_index >= 0 && to_fcurve_index < this->fcurves().size()); @@ -2288,7 +2288,7 @@ bool Channelbag::channel_group_remove(bActionGroup &group) return true; } -void Channelbag::channel_group_move(bActionGroup &group, const int to_group_index) +void Channelbag::channel_group_move_to_index(bActionGroup &group, const int to_group_index) { BLI_assert(to_group_index >= 0 && to_group_index < this->channel_groups().size()); diff --git a/source/blender/animrig/intern/action_test.cc b/source/blender/animrig/intern/action_test.cc index ae5aac5b25c..a25ec6a7b91 100644 --- a/source/blender/animrig/intern/action_test.cc +++ b/source/blender/animrig/intern/action_test.cc @@ -448,7 +448,7 @@ TEST_F(ActionLayersTest, slot_remove) } } -TEST_F(ActionLayersTest, slot_move) +TEST_F(ActionLayersTest, slot_move_to_index) { Slot &slot_a = action->slot_add_for_id_type(ID_ME); Slot &slot_b = action->slot_add_for_id_type(ID_CA); @@ -475,7 +475,7 @@ TEST_F(ActionLayersTest, slot_move) ASSERT_EQ(action->slot(3)->users(*bmain)[0], &suzanne->id); /* First "move" a slot to its own location, which should do nothing. */ - action->slot_move(slot_b, 1); + action->slot_move_to_index(slot_b, 1); EXPECT_EQ(action->slot(0)->handle, handle_a); EXPECT_EQ(action->slot(0)->identifier_prefix_for_idtype(), "ME"); EXPECT_EQ(action->slot(1)->handle, handle_b); @@ -489,7 +489,7 @@ TEST_F(ActionLayersTest, slot_move) /* Then move slots around in various ways. */ - action->slot_move(slot_a, 2); + action->slot_move_to_index(slot_a, 2); EXPECT_EQ(action->slot(0)->handle, handle_b); EXPECT_EQ(action->slot(0)->identifier_prefix_for_idtype(), "CA"); EXPECT_EQ(action->slot(1)->handle, handle_cube); @@ -501,7 +501,7 @@ TEST_F(ActionLayersTest, slot_move) EXPECT_EQ(action->slot(3)->identifier_prefix_for_idtype(), "OB"); EXPECT_EQ(action->slot(3)->users(*bmain)[0], &suzanne->id); - action->slot_move(slot_suzanne, 1); + action->slot_move_to_index(slot_suzanne, 1); EXPECT_EQ(action->slot(0)->handle, handle_b); EXPECT_EQ(action->slot(0)->identifier_prefix_for_idtype(), "CA"); EXPECT_EQ(action->slot(1)->handle, handle_suzanne); @@ -513,7 +513,7 @@ TEST_F(ActionLayersTest, slot_move) EXPECT_EQ(action->slot(3)->handle, handle_a); EXPECT_EQ(action->slot(3)->identifier_prefix_for_idtype(), "ME"); - action->slot_move(slot_cube, 3); + action->slot_move_to_index(slot_cube, 3); EXPECT_EQ(action->slot(0)->handle, handle_b); EXPECT_EQ(action->slot(0)->identifier_prefix_for_idtype(), "CA"); EXPECT_EQ(action->slot(1)->handle, handle_suzanne); @@ -525,7 +525,7 @@ TEST_F(ActionLayersTest, slot_move) EXPECT_EQ(action->slot(3)->identifier_prefix_for_idtype(), "OB"); EXPECT_EQ(action->slot(3)->users(*bmain)[0], &cube->id); - action->slot_move(slot_suzanne, 0); + action->slot_move_to_index(slot_suzanne, 0); EXPECT_EQ(action->slot(0)->handle, handle_suzanne); EXPECT_EQ(action->slot(0)->identifier_prefix_for_idtype(), "OB"); EXPECT_EQ(action->slot(0)->users(*bmain)[0], &suzanne->id); @@ -1391,7 +1391,7 @@ class ChannelbagTest : public testing::Test { } }; -TEST_F(ChannelbagTest, fcurve_move) +TEST_F(ChannelbagTest, fcurve_move_to_index) { FCurve &fcu0 = channelbag->fcurve_ensure(nullptr, {"fcu0", 0, std::nullopt, "group0"}); FCurve &fcu1 = channelbag->fcurve_ensure(nullptr, {"fcu1", 0, std::nullopt, "group0"}); @@ -1406,7 +1406,7 @@ TEST_F(ChannelbagTest, fcurve_move) bActionGroup &group1 = *channelbag->channel_group(1); /* Moving an fcurve to where it already is should be fine. */ - channelbag->fcurve_move(fcu0, 0); + channelbag->fcurve_move_to_index(fcu0, 0); EXPECT_EQ(&fcu0, channelbag->fcurve(0)); EXPECT_EQ(&fcu1, channelbag->fcurve(1)); EXPECT_EQ(&fcu2, channelbag->fcurve(2)); @@ -1419,7 +1419,7 @@ TEST_F(ChannelbagTest, fcurve_move) EXPECT_EQ(nullptr, fcu4.grp); /* Move to first. */ - channelbag->fcurve_move(fcu4, 0); + channelbag->fcurve_move_to_index(fcu4, 0); EXPECT_EQ(0, group0.fcurve_range_start); EXPECT_EQ(2, group0.fcurve_range_length); EXPECT_EQ(2, group1.fcurve_range_start); @@ -1436,7 +1436,7 @@ TEST_F(ChannelbagTest, fcurve_move) EXPECT_EQ(nullptr, fcu3.grp); /* Move to last. */ - channelbag->fcurve_move(fcu1, 4); + channelbag->fcurve_move_to_index(fcu1, 4); EXPECT_EQ(0, group0.fcurve_range_start); EXPECT_EQ(2, group0.fcurve_range_length); EXPECT_EQ(2, group1.fcurve_range_start); @@ -1453,7 +1453,7 @@ TEST_F(ChannelbagTest, fcurve_move) EXPECT_EQ(nullptr, fcu1.grp); /* Move to middle. */ - channelbag->fcurve_move(fcu4, 2); + channelbag->fcurve_move_to_index(fcu4, 2); EXPECT_EQ(0, group0.fcurve_range_start); EXPECT_EQ(2, group0.fcurve_range_length); EXPECT_EQ(2, group1.fcurve_range_start); @@ -1789,7 +1789,7 @@ TEST_F(ChannelbagTest, channel_group_fcurve_removal) ASSERT_EQ(0, channelbag->channel_groups().size()); } -TEST_F(ChannelbagTest, channel_group_move) +TEST_F(ChannelbagTest, channel_group_move_to_index) { FCurve &fcu0 = channelbag->fcurve_ensure(nullptr, {"fcu0", 0, std::nullopt, "group0"}); FCurve &fcu1 = channelbag->fcurve_ensure(nullptr, {"fcu1", 0, std::nullopt, "group1"}); @@ -1804,7 +1804,7 @@ TEST_F(ChannelbagTest, channel_group_move) bActionGroup &group1 = *channelbag->channel_group(1); bActionGroup &group2 = *channelbag->channel_group(2); - channelbag->channel_group_move(group0, 2); + channelbag->channel_group_move_to_index(group0, 2); EXPECT_EQ(&group1, channelbag->channel_group(0)); EXPECT_EQ(&group2, channelbag->channel_group(1)); EXPECT_EQ(&group0, channelbag->channel_group(2)); @@ -1825,7 +1825,7 @@ TEST_F(ChannelbagTest, channel_group_move) EXPECT_EQ(&group0, fcu0.grp); EXPECT_EQ(nullptr, fcu4.grp); - channelbag->channel_group_move(group1, 1); + channelbag->channel_group_move_to_index(group1, 1); EXPECT_EQ(&group2, channelbag->channel_group(0)); EXPECT_EQ(&group1, channelbag->channel_group(1)); EXPECT_EQ(&group0, channelbag->channel_group(2)); @@ -1846,7 +1846,7 @@ TEST_F(ChannelbagTest, channel_group_move) EXPECT_EQ(&group0, fcu0.grp); EXPECT_EQ(nullptr, fcu4.grp); - channelbag->channel_group_move(group0, 0); + channelbag->channel_group_move_to_index(group0, 0); EXPECT_EQ(&group0, channelbag->channel_group(0)); EXPECT_EQ(&group2, channelbag->channel_group(1)); EXPECT_EQ(&group1, channelbag->channel_group(2)); diff --git a/source/blender/editors/animation/anim_channels_edit.cc b/source/blender/editors/animation/anim_channels_edit.cc index 82a42b81024..46feb909919 100644 --- a/source/blender/editors/animation/anim_channels_edit.cc +++ b/source/blender/editors/animation/anim_channels_edit.cc @@ -1724,7 +1724,7 @@ static bool rearrange_layered_action_slots(bAnimContext *ac, const eRearrangeAni continue; } - action.slot_move(slot, to_index); + action.slot_move_to_index(slot, to_index); total_moved++; } break; @@ -1740,7 +1740,7 @@ static bool rearrange_layered_action_slots(bAnimContext *ac, const eRearrangeAni const int current_index = action.slots().first_index_try(&slot); const int to_index = 0; if (current_index != to_index) { - action.slot_move(slot, to_index); + action.slot_move_to_index(slot, to_index); total_moved++; } } @@ -1766,7 +1766,7 @@ static bool rearrange_layered_action_slots(bAnimContext *ac, const eRearrangeAni continue; } - action.slot_move(slot, to_index); + action.slot_move_to_index(slot, to_index); total_moved++; } break; @@ -1782,7 +1782,7 @@ static bool rearrange_layered_action_slots(bAnimContext *ac, const eRearrangeAni const int current_index = action.slots().first_index_try(&slot); const int to_index = action.slots().size() - 1; if (current_index != to_index) { - action.slot_move(slot, to_index); + action.slot_move_to_index(slot, to_index); total_moved++; } } @@ -1801,8 +1801,8 @@ static bool rearrange_layered_action_slots(bAnimContext *ac, const eRearrangeAni * * NOTE: the current implementation has quadratic performance with respect to * the number of groups in a `Channelbag`, due to both `Span::first_index_try()` - * and `Channelbag::channel_group_move()` having linear performance. If this - * becomes a performance bottleneck in practice, we can create a dedicated + * and `Channelbag::channel_group_move_to_index()` having linear performance. If + * this becomes a performance bottleneck in practice, we can create a dedicated * method on `Channelbag` for collectively moving a non-contiguous set of * channel groups that works in linear time. * @@ -1846,7 +1846,7 @@ static void rearrange_layered_action_channel_groups(bAnimContext *ac, continue; } - bag.channel_group_move(*group, to_index); + bag.channel_group_move_to_index(*group, to_index); } break; } @@ -1859,7 +1859,7 @@ static void rearrange_layered_action_channel_groups(bAnimContext *ac, continue; } blender::animrig::Channelbag &bag = group->channelbag->wrap(); - bag.channel_group_move(*group, 0); + bag.channel_group_move_to_index(*group, 0); } break; } @@ -1884,7 +1884,7 @@ static void rearrange_layered_action_channel_groups(bAnimContext *ac, continue; } - bag.channel_group_move(*group, to_index); + bag.channel_group_move_to_index(*group, to_index); } break; } @@ -1897,7 +1897,7 @@ static void rearrange_layered_action_channel_groups(bAnimContext *ac, continue; } blender::animrig::Channelbag &bag = group->channelbag->wrap(); - bag.channel_group_move(*group, bag.channel_groups().size() - 1); + bag.channel_group_move_to_index(*group, bag.channel_groups().size() - 1); } break; } @@ -1911,9 +1911,9 @@ static void rearrange_layered_action_channel_groups(bAnimContext *ac, * * NOTE: the current implementation has quadratic performance with respect to * the number of fcurves in a `Channelbag`, due to both - * `Span::first_index_try()` and `Channelbag::fcurve_move()` having linear - * performance. If this becomes a performance bottleneck in practice, we can - * create a dedicated method on `Channelbag` for collectively moving a + * `Span::first_index_try()` and `Channelbag::fcurve_move_to_index()` having + * linear performance. If this becomes a performance bottleneck in practice, we + * can create a dedicated method on `Channelbag` for collectively moving a * non-contiguous set of fcurves that works in linear time. * * TODO: there's a fair amount of apparent repetition in this code and the code @@ -1997,7 +1997,7 @@ static void rearrange_layered_action_fcurves(bAnimContext *ac, continue; } - bag.fcurve_move(*fcurve, to_index); + bag.fcurve_move_to_index(*fcurve, to_index); } return; } @@ -2013,7 +2013,7 @@ static void rearrange_layered_action_fcurves(bAnimContext *ac, } blender::animrig::Channelbag &bag = group.channelbag->wrap(); - bag.fcurve_move(*fcurve, group.fcurve_range_start); + bag.fcurve_move_to_index(*fcurve, group.fcurve_range_start); } return; } @@ -2042,7 +2042,7 @@ static void rearrange_layered_action_fcurves(bAnimContext *ac, continue; } - bag.fcurve_move(*fcurve, to_index); + bag.fcurve_move_to_index(*fcurve, to_index); } return; } @@ -2058,7 +2058,8 @@ static void rearrange_layered_action_fcurves(bAnimContext *ac, } blender::animrig::Channelbag &bag = group.channelbag->wrap(); - bag.fcurve_move(*fcurve, group.fcurve_range_start + group.fcurve_range_length - 1); + bag.fcurve_move_to_index(*fcurve, + group.fcurve_range_start + group.fcurve_range_length - 1); } return; }