From be921b8ddb0901b90ecd9ba3535a5cc1b4bbbb03 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sybren=20A=2E=20St=C3=BCvel?= Date: Tue, 3 Dec 2024 16:15:25 +0100 Subject: [PATCH] Anim: start slot handles at a high number The `Action::last_slot_handle` field is set to a high-ish value, to disambiguate slot handles from array indices. Slot handles are opaque integers, and are just used to find a slot with that particular handle. Its exact value is irrelevant; Blender only ensures that slot handles are never reused within the same Action. This particular value was obtained by taking the 31 most significant bits of the TentHash value (Nathan's hash function) for the string "Quercus&Laksa" (Sybren's cats). Pull Request: https://projects.blender.org/blender/blender/pulls/131310 --- source/blender/animrig/intern/action_test.cc | 48 ++++++++++++------- source/blender/blenkernel/intern/action.cc | 12 ++++- source/blender/makesdna/DNA_action_defaults.h | 19 ++++++++ source/blender/makesdna/intern/dna_defaults.c | 2 + 4 files changed, 62 insertions(+), 19 deletions(-) diff --git a/source/blender/animrig/intern/action_test.cc b/source/blender/animrig/intern/action_test.cc index 6d05498d65f..3bd8da029ab 100644 --- a/source/blender/animrig/intern/action_test.cc +++ b/source/blender/animrig/intern/action_test.cc @@ -12,6 +12,7 @@ #include "BKE_main.hh" #include "BKE_object.hh" +#include "DNA_action_defaults.h" #include "DNA_anim_types.h" #include "DNA_object_types.h" @@ -27,6 +28,17 @@ #include "testing/testing.h" namespace blender::animrig::tests { + +TEST(action, low_level_initialisation) +{ + bAction *action = static_cast(BKE_id_new_nomain(ID_AC, "ACNewAction")); + + EXPECT_NE(action->last_slot_handle, 0) + << "bAction::last_slot_handle should not be initialised to 0"; + + BKE_id_free(nullptr, action); +} + class ActionLayersTest : public testing::Test { public: Main *bmain; @@ -275,8 +287,8 @@ TEST_F(ActionLayersTest, add_slot) { { /* Creating an 'unused' Slot should just be called 'Slot'. */ Slot &slot = action->slot_add(); - EXPECT_EQ(1, action->last_slot_handle); - EXPECT_EQ(1, slot.handle); + EXPECT_EQ(DNA_DEFAULT_ACTION_LAST_SLOT_HANDLE + 1, action->last_slot_handle); + EXPECT_EQ(DNA_DEFAULT_ACTION_LAST_SLOT_HANDLE + 1, slot.handle); EXPECT_STREQ("XXSlot", slot.identifier); EXPECT_EQ(0, slot.idtype); @@ -284,8 +296,8 @@ TEST_F(ActionLayersTest, add_slot) { /* Creating a Slot for a specific ID should name it after the ID. */ Slot &slot = action->slot_add_for_id(cube->id); - EXPECT_EQ(2, action->last_slot_handle); - EXPECT_EQ(2, slot.handle); + EXPECT_EQ(DNA_DEFAULT_ACTION_LAST_SLOT_HANDLE + 2, action->last_slot_handle); + EXPECT_EQ(DNA_DEFAULT_ACTION_LAST_SLOT_HANDLE + 2, slot.handle); EXPECT_STREQ(cube->id.name, slot.identifier); EXPECT_EQ(ID_OB, slot.idtype); @@ -315,20 +327,20 @@ TEST_F(ActionLayersTest, add_slot_multiple) EXPECT_TRUE(assign_action(action, suzanne->id)); EXPECT_EQ(assign_action_slot(&slot_suzanne, suzanne->id), ActionSlotAssignmentResult::OK); - EXPECT_EQ(2, action->last_slot_handle); - EXPECT_EQ(1, slot_cube.handle); - EXPECT_EQ(2, slot_suzanne.handle); + EXPECT_EQ(DNA_DEFAULT_ACTION_LAST_SLOT_HANDLE + 2, action->last_slot_handle); + EXPECT_EQ(DNA_DEFAULT_ACTION_LAST_SLOT_HANDLE + 1, slot_cube.handle); + EXPECT_EQ(DNA_DEFAULT_ACTION_LAST_SLOT_HANDLE + 2, slot_suzanne.handle); } TEST_F(ActionLayersTest, slot_remove) { { /* Canary test: removing a just-created slot on an otherwise empty Action should work. */ Slot &slot = action->slot_add(); - EXPECT_EQ(1, slot.handle); - EXPECT_EQ(1, action->last_slot_handle); + EXPECT_EQ(DNA_DEFAULT_ACTION_LAST_SLOT_HANDLE + 1, slot.handle); + EXPECT_EQ(DNA_DEFAULT_ACTION_LAST_SLOT_HANDLE + 1, action->last_slot_handle); EXPECT_TRUE(action->slot_remove(slot)); - EXPECT_EQ(1, action->last_slot_handle) + EXPECT_EQ(DNA_DEFAULT_ACTION_LAST_SLOT_HANDLE + 1, action->last_slot_handle) << "Removing a slot should not change the last-used slot handle."; EXPECT_EQ(0, action->slot_array_num); } @@ -341,8 +353,8 @@ TEST_F(ActionLayersTest, slot_remove) { /* Removing a slot should remove its Channelbag. */ Slot &slot = action->slot_add(); const slot_handle_t slot_handle = slot.handle; - EXPECT_EQ(2, slot.handle); - EXPECT_EQ(2, action->last_slot_handle); + EXPECT_EQ(DNA_DEFAULT_ACTION_LAST_SLOT_HANDLE + 2, slot.handle); + EXPECT_EQ(DNA_DEFAULT_ACTION_LAST_SLOT_HANDLE + 2, action->last_slot_handle); /* Create an F-Curve in a Channelbag for the slot. */ action->layer_keystrip_ensure(); @@ -352,7 +364,7 @@ TEST_F(ActionLayersTest, slot_remove) /* Remove the slot. */ EXPECT_TRUE(action->slot_remove(slot)); - EXPECT_EQ(2, action->last_slot_handle) + EXPECT_EQ(DNA_DEFAULT_ACTION_LAST_SLOT_HANDLE + 2, action->last_slot_handle) << "Removing a slot should not change the last-used slot handle."; EXPECT_EQ(0, action->slot_array_num); @@ -365,10 +377,10 @@ TEST_F(ActionLayersTest, slot_remove) Slot &slot1 = action->slot_add(); Slot &slot2 = action->slot_add(); Slot &slot3 = action->slot_add(); - EXPECT_EQ(3, slot1.handle); - EXPECT_EQ(4, slot2.handle); - EXPECT_EQ(5, slot3.handle); - EXPECT_EQ(5, action->last_slot_handle); + EXPECT_EQ(DNA_DEFAULT_ACTION_LAST_SLOT_HANDLE + 3, slot1.handle); + EXPECT_EQ(DNA_DEFAULT_ACTION_LAST_SLOT_HANDLE + 4, slot2.handle); + EXPECT_EQ(DNA_DEFAULT_ACTION_LAST_SLOT_HANDLE + 5, slot3.handle); + EXPECT_EQ(DNA_DEFAULT_ACTION_LAST_SLOT_HANDLE + 5, action->last_slot_handle); /* For referencing the slot handle after the slot is removed. */ const slot_handle_t slot2_handle = slot2.handle; @@ -382,7 +394,7 @@ TEST_F(ActionLayersTest, slot_remove) /* Remove the slot. */ EXPECT_TRUE(action->slot_remove(slot2)); - EXPECT_EQ(5, action->last_slot_handle); + EXPECT_EQ(DNA_DEFAULT_ACTION_LAST_SLOT_HANDLE + 5, action->last_slot_handle); /* Check the correct slot + channel-bag are removed. */ EXPECT_EQ(action->slot_for_handle(slot1.handle), &slot1); diff --git a/source/blender/blenkernel/intern/action.cc b/source/blender/blenkernel/intern/action.cc index fe54a7cfc8d..aa340efc245 100644 --- a/source/blender/blenkernel/intern/action.cc +++ b/source/blender/blenkernel/intern/action.cc @@ -20,6 +20,7 @@ #include "DNA_anim_types.h" #include "DNA_armature_types.h" #include "DNA_constraint_types.h" +#include "DNA_defaults.h" #include "DNA_object_types.h" #include "DNA_scene_types.h" @@ -96,6 +97,15 @@ using namespace blender; /*********************** Armature Datablock ***********************/ namespace blender::bke { +static void action_init_data(ID *action_id) +{ + BLI_assert(GS(action_id->name) == ID_AC); + bAction *action = reinterpret_cast(action_id); + + BLI_assert(MEMCMP_STRUCT_AFTER_IS_ZERO(action, id)); + MEMCPY_STRUCT_AFTER(action, DNA_struct_default_get(bAction), id); +} + /** * Only copy internal data of Action ID from source * to already allocated/initialized destination. @@ -745,7 +755,7 @@ IDTypeInfo IDType_ID_AC = { /*flags*/ IDTYPE_FLAGS_NO_ANIMDATA, /*asset_type_info*/ &blender::bke::AssetType_AC, - /*init_data*/ nullptr, + /*init_data*/ blender::bke::action_init_data, /*copy_data*/ blender::bke::action_copy_data, /*free_data*/ blender::bke::action_free_data, /*make_local*/ nullptr, diff --git a/source/blender/makesdna/DNA_action_defaults.h b/source/blender/makesdna/DNA_action_defaults.h index e3a8d3aff8b..75a18661fb5 100644 --- a/source/blender/makesdna/DNA_action_defaults.h +++ b/source/blender/makesdna/DNA_action_defaults.h @@ -12,6 +12,25 @@ /* clang-format off */ +/* -------------------------------------------------------------------- */ +/** \name Action Struct + * \{ */ + +/* The last_slot_handle is set to a high value to disambiguate slot handles from + * array indices. + * + * This particular value was obtained by taking the 31 most-significant bits of + * the TentHash value (Nathan's hash function) for the string "Quercus&Laksa" + * (Sybren's cats). */ +#define DNA_DEFAULT_ACTION_LAST_SLOT_HANDLE 0x37627bf5 + +#define _DNA_DEFAULT_bAction \ + { \ + .last_slot_handle = DNA_DEFAULT_ACTION_LAST_SLOT_HANDLE, \ + } + +/** \} */ + /* -------------------------------------------------------------------- */ /** \name ActionLayer Struct * \{ */ diff --git a/source/blender/makesdna/intern/dna_defaults.c b/source/blender/makesdna/intern/dna_defaults.c index 6522f3cb837..e48b6ee25e5 100644 --- a/source/blender/makesdna/intern/dna_defaults.c +++ b/source/blender/makesdna/intern/dna_defaults.c @@ -149,6 +149,7 @@ static const struct_name DNA_DEFAULT_##struct_name = _DNA_DEFAULT_##struct_name /* DNA_action_defaults.h */ +SDNA_DEFAULT_DECL_STRUCT(bAction); SDNA_DEFAULT_DECL_STRUCT(ActionLayer); SDNA_DEFAULT_DECL_STRUCT(ActionStrip); @@ -399,6 +400,7 @@ extern const bTheme U_theme_default; const void *DNA_default_table[SDNA_TYPE_MAX] = { /* DNA_anim_defaults.h */ + SDNA_DEFAULT_DECL(bAction), SDNA_DEFAULT_DECL(ActionLayer), SDNA_DEFAULT_DECL(ActionStrip),