griefith/test
1
0
Fork 0
Code Issues Pull Requests Actions 1 Packages Projects Releases Wiki Activity

Anim: remove strip data as well when an action strip is removed

Browse Source 
This is a follow up to #126559. In that PR, strip data is never deleted even
when the strip that uses it is. Thus, the strip data array just keeps growing as
more strips are added, and never shrinks.

This PR implements that deletion. The approach is simple: remove the strip data
from the array using a swap-remove that swaps in the last item in the data
array, and then loop over the strips in the action to update any that were
referencing that swapped-in item. Additionally, before removal we check to make
sure the data item isn't still being used by any strips, and if it is then we
don't remove it.

Pull Request: https://projects.blender.org/blender/blender/pulls/127837
This commit is contained in:
Nathan Vegdahl
2024-09-20 13:07:15 +02:00
committed by Nathan Vegdahl
parent 439d4dfc3c
commit b200d1f975
4 changed files with 188 additions and 19 deletions
Download Patch File Download Diff File Expand all files Collapse all files

25
source/blender/animrig/ANIM_action.hh
View File

@@ -336,10 +336,11 @@ class Action : public ::bAction {
void slot_setup_for_id(Slot &slot, const ID &animated_id);
protected:
/* To give access to `strip_keyframe_data_append()` (and in the future,
* corresponding functions for other strip data types), needed for creating
* new strips. */
/* Friends for the purpose of adding/removing strip data on the action's strip
* data arrays. This is needed for the strip creation and removal code in
* `Strip` and `Layer`'s methods. */
friend Strip;
friend Layer;
/** Return the layer's index, or -1 if not found in this Action. */
int64_t find_layer_index(const Layer &layer) const;
@@ -357,6 +358,22 @@ class Action : public ::bAction {
*/
int strip_keyframe_data_append(StripKeyframeData *strip_data);
/**
* Remove the keyframe strip data at `index` if it is no longer used anywhere
* in the action.
*
* If the strip data is unused, it is both removed from the array *and* freed.
* Otherwise no changes are made and the action remains as-is.
*
* Note: this may alter the indices of some strip data items, due to items
* shifting around to fill the gap left by the removed item. This method
* ensures that all indices stored within the action (e.g. in the strips
* themselves) are properly updated to the new values so that everything is
* still referencing the same data. However, if any indices are stored
* *outside* the action, they will no longer be valid.
*/
void strip_keyframe_data_remove_if_unused(int index);
private:
Slot &slot_allocate();
@@ -553,7 +570,7 @@ class Layer : public ::ActionLayer {
*
* \return true when the strip was found & removed, false if it wasn't found.
*/
bool strip_remove(Strip &strip);
bool strip_remove(Action &owning_action, Strip &strip);
/**
* Remove all data belonging to the given slot.

73
source/blender/animrig/intern/action.cc
View File

@@ -144,6 +144,28 @@ template<typename T> static void shrink_array_and_remove(T **array, int *num, co
*num = new_array_num;
}
/**
* Same as `shrink_array_and_remove()` above, except instead of shifting all the
* elements after the removed item over to fill the gap, it just swaps in the last
* element to where the removed element was.
*/
template<typename T> static void shrink_array_and_swap_remove(T **array, int *num, const int index)
{
BLI_assert(index >= 0 && index < *num);
const int new_array_num = *num - 1;
T *new_array = MEM_cnew_array<T>(new_array_num, __func__);
blender::uninitialized_move_n(*array, index, new_array);
if (index < new_array_num) {
new_array[index] = (*array)[new_array_num];
blender::uninitialized_move_n(*array + index + 1, *num - index - 2, new_array + index + 1);
}
MEM_freeN(*array);
*array = new_array;
*num = new_array_num;
}
/**
* Moves the given (end exclusive) range to index `to`, shifting other items
* before/after to make room.
@@ -578,6 +600,42 @@ int Action::strip_keyframe_data_append(StripKeyframeData *strip_data)
return this->strip_keyframe_data_array_num - 1;
}
void Action::strip_keyframe_data_remove_if_unused(const int index)
{
BLI_assert(index >= 0 && index < this->strip_keyframe_data_array_num);
/* Make sure the data isn't being used anywhere. */
for (Layer *layer : this->layers()) {
for (Strip *strip : layer->strips()) {
if (strip->type() == Strip::Type::Keyframe && strip->data_index == index) {
return;
}
}
}
/* Free the item to be removed. */
MEM_delete<StripKeyframeData>(
static_cast<StripKeyframeData *>(this->strip_keyframe_data_array[index]));
/* Remove the item, swapping in the item at the end of the array. */
shrink_array_and_swap_remove<ActionStripKeyframeData *>(
&this->strip_keyframe_data_array, &this->strip_keyframe_data_array_num, index);
/* Update strips that pointed at the swapped-in item.
*
* Note that we don't special-case the corner-case where the removed data was
* at the end of the array, but it ends up not mattering because then
* `old_index == index`. */
const int old_index = this->strip_keyframe_data_array_num;
for (Layer *layer : this->layers()) {
for (Strip *strip : layer->strips()) {
if (strip->type() == Strip::Type::Keyframe && strip->data_index == old_index) {
strip->data_index = index;
}
}
}
}
Span<const StripKeyframeData *> Action::strip_keyframe_data() const
{
/* The reinterpret cast is needed because `strip_keyframe_data_array` is for
@@ -869,16 +927,29 @@ static void strip_ptr_destructor(ActionStrip **dna_strip_ptr)
MEM_delete(&strip);
};
bool Layer::strip_remove(Strip &strip)
bool Layer::strip_remove(Action &owning_action, Strip &strip)
{
const int64_t strip_index = this->find_strip_index(strip);
if (strip_index < 0) {
return false;
}
const Strip::Type strip_type = strip.type();
const int data_index = strip.data_index;
dna::array::remove_index(
&this->strip_array, &this->strip_array_num, nullptr, strip_index, strip_ptr_destructor);
/* It's important that we do this *after* removing the strip itself
* (immediately above), because otherwise the strip will be found as a
* still-existing user of the strip data and thus the strip data won't be
* removed even if this strip was the last user. */
switch (strip_type) {
case Strip::Type::Keyframe:
owning_action.strip_keyframe_data_remove_if_unused(data_index);
break;
}
return true;
}

102
source/blender/animrig/intern/action_test.cc
View File

@@ -156,38 +156,118 @@ TEST_F(ActionLayersTest, remove_strip)
Strip &strip0 = layer.strip_add(*action, Strip::Type::Keyframe);
Strip &strip1 = layer.strip_add(*action, Strip::Type::Keyframe);
Strip &strip2 = layer.strip_add(*action, Strip::Type::Keyframe);
Strip &strip3 = layer.strip_add(*action, Strip::Type::Keyframe);
StripKeyframeData &strip_data0 = strip0.data<StripKeyframeData>(*action);
StripKeyframeData &strip_data1 = strip1.data<StripKeyframeData>(*action);
StripKeyframeData &strip_data2 = strip2.data<StripKeyframeData>(*action);
StripKeyframeData &strip_data3 = strip3.data<StripKeyframeData>(*action);
/* Add some keys to check that also the strip data is freed correctly. */
const KeyframeSettings settings = get_keyframe_settings(false);
Slot &slot = action->slot_add();
strip0.data<StripKeyframeData>(*action).keyframe_insert(
bmain, slot, {"location", 0}, {1.0f, 47.0f}, settings);
strip1.data<StripKeyframeData>(*action).keyframe_insert(
bmain, slot, {"location", 0}, {1.0f, 47.0f}, settings);
strip2.data<StripKeyframeData>(*action).keyframe_insert(
bmain, slot, {"location", 0}, {1.0f, 47.0f}, settings);
strip_data0.keyframe_insert(bmain, slot, {"location", 0}, {1.0f, 47.0f}, settings);
strip_data1.keyframe_insert(bmain, slot, {"location", 0}, {1.0f, 48.0f}, settings);
strip_data2.keyframe_insert(bmain, slot, {"location", 0}, {1.0f, 49.0f}, settings);
strip_data3.keyframe_insert(bmain, slot, {"location", 0}, {1.0f, 50.0f}, settings);
EXPECT_TRUE(layer.strip_remove(strip1));
EXPECT_EQ(2, layer.strips().size());
EXPECT_EQ(4, action->strip_keyframe_data().size());
EXPECT_EQ(0, strip0.data_index);
EXPECT_EQ(1, strip1.data_index);
EXPECT_EQ(2, strip2.data_index);
EXPECT_EQ(3, strip3.data_index);
EXPECT_TRUE(layer.strip_remove(*action, strip1));
EXPECT_EQ(3, action->strip_keyframe_data().size());
EXPECT_EQ(3, layer.strips().size());
EXPECT_EQ(&strip0, layer.strip(0));
EXPECT_EQ(&strip2, layer.strip(1));
EXPECT_EQ(&strip3, layer.strip(2));
EXPECT_EQ(0, strip0.data_index);
EXPECT_EQ(2, strip2.data_index);
EXPECT_EQ(1, strip3.data_index); /* Swapped in when removing strip 1's data. */
EXPECT_EQ(&strip_data0, &strip0.data<StripKeyframeData>(*action));
EXPECT_EQ(&strip_data2, &strip2.data<StripKeyframeData>(*action));
EXPECT_EQ(&strip_data3, &strip3.data<StripKeyframeData>(*action));
EXPECT_TRUE(layer.strip_remove(strip2));
EXPECT_TRUE(layer.strip_remove(*action, strip2));
EXPECT_EQ(2, action->strip_keyframe_data().size());
EXPECT_EQ(2, layer.strips().size());
EXPECT_EQ(&strip0, layer.strip(0));
EXPECT_EQ(&strip3, layer.strip(1));
EXPECT_EQ(0, strip0.data_index);
EXPECT_EQ(1, strip3.data_index);
EXPECT_EQ(&strip_data0, &strip0.data<StripKeyframeData>(*action));
EXPECT_EQ(&strip_data3, &strip3.data<StripKeyframeData>(*action));
EXPECT_TRUE(layer.strip_remove(*action, strip3));
EXPECT_EQ(1, action->strip_keyframe_data().size());
EXPECT_EQ(1, layer.strips().size());
EXPECT_EQ(&strip0, layer.strip(0));
EXPECT_EQ(0, strip0.data_index);
EXPECT_EQ(&strip_data0, &strip0.data<StripKeyframeData>(*action));
EXPECT_TRUE(layer.strip_remove(strip0));
EXPECT_TRUE(layer.strip_remove(*action, strip0));
EXPECT_EQ(0, action->strip_keyframe_data().size());
EXPECT_EQ(0, layer.strips().size());
{ /* Test removing a strip that is not owned. */
Layer &other_layer = action->layer_add("Another Layer");
Strip &other_strip = other_layer.strip_add(*action, Strip::Type::Keyframe);
EXPECT_FALSE(layer.strip_remove(other_strip))
EXPECT_FALSE(layer.strip_remove(*action, other_strip))
<< "Removing a strip not owned by the layer should be gracefully rejected";
}
}
/* NOTE: this test creates strip instances in a bespoke way for the purpose of
* exercising the strip removal code, because at the time of writing we don't
* have a proper API for creating strip instances. When such an API is added,
* this test should be updated to use it. */
TEST_F(ActionLayersTest, remove_strip_instances)
{
Layer &layer = action->layer_add("Test Læür");
Strip &strip0 = layer.strip_add(*action, Strip::Type::Keyframe);
Strip &strip1 = layer.strip_add(*action, Strip::Type::Keyframe);
Strip &strip2 = layer.strip_add(*action, Strip::Type::Keyframe);
/* Make on of the strips an instance of another. */
strip0.data_index = strip1.data_index;
StripKeyframeData &strip_data_0_1 = strip0.data<StripKeyframeData>(*action);
StripKeyframeData &strip_data_2 = strip2.data<StripKeyframeData>(*action);
/* Add some keys to check that also the strip data is freed correctly. */
const KeyframeSettings settings = get_keyframe_settings(false);
Slot &slot = action->slot_add();
strip_data_0_1.keyframe_insert(bmain, slot, {"location", 0}, {1.0f, 47.0f}, settings);
strip_data_2.keyframe_insert(bmain, slot, {"location", 0}, {1.0f, 48.0f}, settings);
EXPECT_EQ(3, action->strip_keyframe_data().size());
EXPECT_EQ(1, strip0.data_index);
EXPECT_EQ(1, strip1.data_index);
EXPECT_EQ(2, strip2.data_index);
/* Removing an instance should not delete the underlying data as long as there
* is still another strip using it. */
EXPECT_TRUE(layer.strip_remove(*action, strip1));
EXPECT_EQ(3, action->strip_keyframe_data().size());
EXPECT_EQ(2, layer.strips().size());
EXPECT_EQ(&strip0, layer.strip(0));
EXPECT_EQ(&strip2, layer.strip(1));
EXPECT_EQ(1, strip0.data_index);
EXPECT_EQ(2, strip2.data_index);
EXPECT_EQ(&strip_data_0_1, &strip0.data<StripKeyframeData>(*action));
EXPECT_EQ(&strip_data_2, &strip2.data<StripKeyframeData>(*action));
/* Removing the last user of strip data should also delete the data. */
EXPECT_TRUE(layer.strip_remove(*action, strip0));
EXPECT_EQ(2, action->strip_keyframe_data().size());
EXPECT_EQ(1, layer.strips().size());
EXPECT_EQ(&strip2, layer.strip(0));
EXPECT_EQ(1, strip2.data_index);
EXPECT_EQ(&strip_data_2, &strip2.data<StripKeyframeData>(*action));
}
TEST_F(ActionLayersTest, add_slot)
{
{ /* Creating an 'unused' Slot should just be called 'Slot'. */

7
source/blender/makesrna/intern/rna_action.cc
View File

@@ -439,18 +439,19 @@ ActionStrip *rna_ActionStrips_new(
}
void rna_ActionStrips_remove(
ID *action, ActionLayer *dna_layer, bContext *C, ReportList *reports, PointerRNA *strip_ptr)
ID *action_id, ActionLayer *dna_layer, bContext *C, ReportList *reports, PointerRNA *strip_ptr)
{
animrig::Action &action = reinterpret_cast<bAction *>(action_id)->wrap();
animrig::Layer &layer = dna_layer->wrap();
animrig::Strip &strip = rna_data_strip(strip_ptr);
if (!layer.strip_remove(strip)) {
if (!layer.strip_remove(action, strip)) {
BKE_report(reports, RPT_ERROR, "This strip does not belong to this layer");
return;
}
RNA_POINTER_INVALIDATE(strip_ptr);
WM_event_add_notifier(C, NC_ANIMATION | ND_ANIMCHAN, nullptr);
DEG_id_tag_update(action, ID_RECALC_ANIMATION);
DEG_id_tag_update(action_id, ID_RECALC_ANIMATION);
}
static std::optional<std::string> rna_ActionStrip_path(const PointerRNA *ptr)