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

Nathan Vegdahl · 2024-09-19T10:16:55+02:00

Nathan Vegdahl commented

2024-09-19 10:16:55 +02:00

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.

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.

Nathan Vegdahl added the

Module

Animation & Rigging

label 2024-09-19 10:16:55 +02:00

Nathan Vegdahl added 1 commit 2024-09-19 10:17:06 +02:00

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

WIP, because I still need to flesh out the unit tests.

Nathan Vegdahl added 1 commit 2024-09-19 11:44:21 +02:00

Update existing removal unit test 614c4945ce

Nathan Vegdahl added 1 commit 2024-09-19 11:58:11 +02:00

Add unit test for strip removal with strip instances fe0dc09c23

Nathan Vegdahl added 1 commit 2024-09-19 12:07:41 +02:00

Make the strip data removal method protected e61270c3d9

Nathan Vegdahl added 1 commit 2024-09-19 12:14:38 +02:00

Better doc comment 564fbc9231

Nathan Vegdahl added 1 commit 2024-09-19 12:33:14 +02:00

Add note about not needing to handle corner case d4d6d066b5

Nathan Vegdahl changed title from ~~WIP: Anim: remove strip data as well when an action strip is removed~~ to Anim: remove strip data as well when an action strip is removed

2024-09-19 12:36:53 +02:00

Nathan Vegdahl requested review from Sybren A. Stüvel 2024-09-19 12:37:03 +02:00

Nathan Vegdahl requested review from Christoph Lendenfeld 2024-09-19 12:37:16 +02:00

Nathan Vegdahl added 1 commit 2024-09-19 14:00:15 +02:00

Merge branch 'main' into strip_data_removal 05d3414694

Christoph Lendenfeld reviewed 2024-09-19 16:07:54 +02:00

Christoph Lendenfeld left a comment

lgtm

on a general note I am not super happy that the action has to be friends with the Strip and the Layer but I don't see that as critical

lgtm on a general note I am not super happy that the action has to be friends with the Strip and the Layer but I don't see that as critical

source/blender/animrig/intern/action.cc

						
				@ -146,0 +148,4 @@

				 * 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)

Christoph Lendenfeld commented

2024-09-19 12:50:02 +02:00

I am a bit surprised those functions don't exist somewhere in Blender already. They seem very generic

Nathan Vegdahl commented

2024-09-19 16:34:54 +02:00

Yeah, these should be moved to DNA_array_utils.hh at some point, so they can be used throughout the code base. Might be a good thing for me to tackle as part of my work in Q4?

Yeah, these should be moved to `DNA_array_utils.hh` at some point, so they can be used throughout the code base. Might be a good thing for me to tackle as part of my work in Q4?

👍 1

Christoph Lendenfeld approved these changes 2024-09-19 16:08:04 +02:00

Sybren A. Stüvel approved these changes 2024-09-20 11:49:03 +02:00

Sybren A. Stüvel left a comment

LGTM, with a few small notes that can be handled when landing.

source/blender/animrig/ANIM_action.hh Outdated

						
				@ -360,0 +375,4 @@

				   *

				   * \return True on success, false on failure.

				   */

				  bool strip_keyframe_data_remove(int index);

Sybren A. Stüvel commented

2024-09-20 11:38:23 +02:00

Rename to strip_keyframe_data_remove_if_unused() because otherwise I'd definitely expect it to always remove.

Rename to `strip_keyframe_data_remove_if_unused()` because otherwise I'd definitely expect it to always remove.

👍 1

nathanvegdahl marked this conversation as resolved

source/blender/animrig/intern/action.cc Outdated

						
				@ -875,3 +935,4 @@

				    return false;

				  }

				  const Strip::Type strip_type = this->strip(strip_index)->type();

Sybren A. Stüvel commented

2024-09-20 11:43:46 +02:00

If it's already known that strip has that index, why do you do two lookups again? Why not use strip->type() and strip->data_index here?

If it's already known that `strip` has that index, why do you do two lookups again? Why not use `strip->type()` and `strip->data_index` here?

👍 1

Nathan Vegdahl commented

2024-09-20 12:06:41 +02:00

Because I'm a goofball!

Thanks. :-)

Because I'm a goofball! Thanks. :-)

nathanvegdahl marked this conversation as resolved

source/blender/animrig/intern/action.cc Outdated

						
				@ -878,3 +941,4 @@

				  dna::array::remove_index(

				      &this->strip_array, &this->strip_array_num, nullptr, strip_index, strip_ptr_destructor);

				  switch (strip_type) {

Sybren A. Stüvel commented

2024-09-20 11:45:19 +02:00

Add a comment that things are done in this order because otherwise strip_keyframe_data_remove() will still see the to-be-removed strip as a user of the data, and not do anything.

Add a comment that things are done in this order because otherwise `strip_keyframe_data_remove()` will still see the to-be-removed strip as a user of the data, and not do anything.

👍 1

nathanvegdahl marked this conversation as resolved

source/blender/animrig/intern/action.cc Outdated

						
				@ -880,1 +943,4 @@

				  switch (strip_type) {

				    case Strip::Type::Keyframe:

				      owning_action.strip_keyframe_data_remove(data_index);

Sybren A. Stüvel commented

2024-09-20 11:45:39 +02:00

Why is this function returning a bool if it's ignored in the only call site? It's probably clearer once the function is renamed, because then the "if unused" part is more obvious.

Why is this function returning a `bool` if it's ignored in the only call site? It's probably clearer once the function is renamed, because then the "if unused" part is more obvious.

👍 1

Nathan Vegdahl commented

2024-09-20 12:13:19 +02:00

Yeah, it's not necessary, but seemed like reasonable information to provide to the caller. But you're right, better to keep the API to what's actually used. It's easy to add the return value back in the future if it ends up actually being needed for something.

👍 1

nathanvegdahl marked this conversation as resolved

Nathan Vegdahl added 1 commit 2024-09-20 12:03:09 +02:00

Merge branch 'main' into strip_data_removal 3525a18f0a

Nathan Vegdahl added 1 commit 2024-09-20 12:18:25 +02:00

Address PR comments

buildbot/vexp-code-patch-linux-x86_64 Build done.

Details

buildbot/vexp-code-patch-lint Build done.

Details

buildbot/vexp-code-patch-windows-amd64 Build done.

Details