diff --git a/GNUmakefile b/GNUmakefile index 20a7fcd2ebc..aae7f4d2c04 100644 --- a/GNUmakefile +++ b/GNUmakefile @@ -578,6 +578,8 @@ source_archive_complete: .FORCE -DCMAKE_BUILD_TYPE_INIT:STRING=$(BUILD_TYPE) -DPACKAGE_USE_UPSTREAM_SOURCES=OFF # This assumes CMake is still using a default `PACKAGE_DIR` variable: @$(PYTHON) ./build_files/utils/make_source_archive.py --include-packages "$(BUILD_DIR)/source_archive/packages" +# We assume that the tests will not change for minor releases so only package them for major versions + @$(PYTHON) ./build_files/utils/make_source_archive.py --package-test-data icons_geom: .FORCE @BLENDER_BIN=$(BLENDER_BIN) \ diff --git a/build_files/utils/make_source_archive.py b/build_files/utils/make_source_archive.py index 647d3a396e1..6eb2b060aaf 100755 --- a/build_files/utils/make_source_archive.py +++ b/build_files/utils/make_source_archive.py @@ -28,15 +28,20 @@ from typing import ( # NOTE: while the Python part of this script is portable, # it relies on external commands typically found on GNU/Linux. # Support for other platforms could be added by moving GNU `tar` & `md5sum` use to Python. +# This also relies on having a Unix shell (sh) to run some git commands. -SKIP_NAMES = { +SKIP_NAMES = ( ".gitignore", ".gitmodules", ".gitattributes", ".git-blame-ignore-revs", ".arcconfig", ".svn", -} +) + +SKIP_FOLDERS = ( + "release/datafiles/assets/working", +) def main() -> None: @@ -46,7 +51,8 @@ def main() -> None: description="Create a tarball of the Blender sources, optionally including sources of dependencies.", epilog="This script is intended to be run by `make source_archive_complete`.", ) - cli_parser.add_argument( + group = cli_parser.add_mutually_exclusive_group() + group.add_argument( "-p", "--include-packages", type=Path, @@ -54,6 +60,12 @@ def main() -> None: metavar="PACKAGE_PATH", help="Include all source files from the given package directory as well.", ) + group.add_argument( + "-t", + "--package-test-data", + action='store_true', + help="Package all test data into its own archive", + ) cli_args = cli_parser.parse_args() @@ -63,6 +75,10 @@ def main() -> None: os.chdir(curdir) blender_srcdir = blender_srcdir.relative_to(curdir) + # Update our SKIP_FOLDERS blacklist with the source directory name + global SKIP_FOLDERS + SKIP_FOLDERS = tuple([f"{blender_srcdir}/{entry}" for entry in SKIP_FOLDERS]) + print(f"Output dir: {curdir}") version = make_utils.parse_blender_version() @@ -70,7 +86,12 @@ def main() -> None: manifest = manifest_path(tarball) packages_dir = packages_path(curdir, cli_args) - create_manifest(version, manifest, blender_srcdir, packages_dir) + if cli_args.package_test_data: + print("Creating an archive of all test data.") + create_manifest(version, manifest, blender_srcdir / "tests/data", packages_dir) + else: + create_manifest(version, manifest, blender_srcdir, packages_dir) + create_tarball(version, tarball, manifest, blender_srcdir, packages_dir) create_checksum_file(tarball) cleanup(manifest) @@ -81,6 +102,8 @@ def tarball_path(output_dir: Path, version: make_utils.BlenderVersion, cli_args: extra = "" if cli_args.include_packages: extra = "-with-libraries" + elif cli_args.package_test_data: + extra = "-test-data" return output_dir / f"blender{extra}-{version}.tar.xz" @@ -125,7 +148,6 @@ def create_manifest( print(f'Building manifest of files: "{outpath}"...', end="", flush=True) with outpath.open("w", encoding="utf-8") as outfile: main_files_to_manifest(blender_srcdir, outfile) - assets_to_manifest(blender_srcdir, outfile) if packages_dir: packages_to_manifest(outfile, packages_dir) @@ -134,20 +156,9 @@ def create_manifest( def main_files_to_manifest(blender_srcdir: Path, outfile: TextIO) -> None: assert not blender_srcdir.is_absolute() - for path in git_ls_files(blender_srcdir): - print(path, file=outfile) - - -def assets_to_manifest(blender_srcdir: Path, outfile: TextIO) -> None: - assert not blender_srcdir.is_absolute() - - assets_dir = blender_srcdir / "release" / "datafiles" / "assets" - for path in assets_dir.glob("*"): - if path.name == "working": - continue - if path.name in SKIP_NAMES: - continue - print(path, file=outfile) + for git_repo in git_gather_all_folders_to_package(blender_srcdir): + for path in git_ls_files(git_repo): + print(path, file=outfile) def packages_to_manifest(outfile: TextIO, packages_dir: Path) -> None: @@ -227,28 +238,57 @@ def cleanup(manifest: Path) -> None: # Low-level commands +def git_gather_all_folders_to_package(directory: Path = Path(".")) -> Iterable[Path]: + """Generator, yields lines which represents each directory to gather git files from. + + Each directory represents either the top level git repository or a submodule. + All submodules that have the 'update = none' setting will be excluded from this list. + + The directory path given to this function will be included in the yielded paths + """ + + # For each submodule (recurse into submodules within submodules if they exist) + git_main_command = "submodule --quiet foreach --recursive" + # Return the path to the submodule and what the value is of their "update" setting + # If the "update" setting doesn't exist, only the path to the submodule is returned + git_command_args = "'echo $displaypath $(git config --file \"$toplevel/.gitmodules\" --get submodule.$name.update)'" + + # Yield the root directory as this is our top level git repo + yield directory + + for line in git_command(f"-C '{directory}' {git_main_command} {git_command_args}"): + # Check if we shouldn't include the directory on this line + split_line = line.rsplit(maxsplit=1) + if len(split_line) > 1 and split_line[-1] == "none": + continue + path = directory / split_line[0] + yield path + + def git_ls_files(directory: Path = Path(".")) -> Iterable[Path]: """Generator, yields lines of output from 'git ls-files'. Only lines that are actually files (so no directories, sockets, etc.) are returned, and never one from SKIP_NAMES. """ - for line in git_command("-C", str(directory), "ls-files"): + for line in git_command(f"-C '{directory}' ls-files"): path = directory / line if not path.is_file() or path.name in SKIP_NAMES: continue + if path.as_posix().startswith(SKIP_FOLDERS): + continue yield path -def git_command(*cli_args: Union[bytes, str, Path]) -> Iterable[str]: +def git_command(cli_args: str) -> Iterable[str]: """Generator, yields lines of output from a Git command.""" - command = ("git", *cli_args) + command = "git " + cli_args # import shlex # print(">", " ".join(shlex.quote(arg) for arg in command)) git = subprocess.run( - command, stdout=subprocess.PIPE, check=True, text=True, timeout=30 + command, stdout=subprocess.PIPE, shell=True, check=True, text=True, timeout=30 ) for line in git.stdout.split("\n"): if line: diff --git a/source/blender/editors/animation/anim_filter.cc b/source/blender/editors/animation/anim_filter.cc index 75a5970bfd0..3d4833b276e 100644 --- a/source/blender/editors/animation/anim_filter.cc +++ b/source/blender/editors/animation/anim_filter.cc @@ -1552,16 +1552,37 @@ static size_t animfilter_act_group(bAnimContext *ac, return items; } -/** - * Add a channel for each Slot, with their FCurves when the Slot is expanded. - */ -static size_t animfilter_action_slot(bAnimContext *ac, - ListBase *anim_data, - animrig::Action &action, - animrig::Slot &slot, - const eAnimFilter_Flags filter_mode, - ID *animated_id) +size_t ANIM_animfilter_action_slot(bAnimContext *ac, + ListBase * /* bAnimListElem */ anim_data, + animrig::Action &action, + animrig::Slot &slot, + const eAnimFilter_Flags filter_mode, + ID *animated_id) { + BLI_assert(ac); + + /* In some cases (see `ob_to_keylist()` and friends) fake bDopeSheet and fake bAnimContext are + * created. These are mostly null-initialized, and so do not have a bmain. This means that + * lookup of the animated ID is not possible, which can result in failure to look up the proper + * F-Curve display name. For the `..._to_keylist` functions that doesn't matter, as those are + * only interested in the key data anyway. So rather than trying to get a reliable `bmain` + * through the maze, this code just treats it as optional (even though ideally it should always + * be known). */ + ID *slot_user_id = nullptr; + if (ac->bmain) { + slot_user_id = animrig::action_slot_get_id_best_guess(*ac->bmain, slot, animated_id); + } + if (!slot_user_id) { + BLI_assert(animated_id); + /* At the time of writing this (PR #134922), downstream code (see e.g. + * `animfilter_fcurves_span()`) assumes this is non-null, so we need to set + * it to *something*. If it's not an actual user of the slot then channels + * might not resolve to an actual property and thus be displayed oddly in + * the channel list, but that's not technically a problem, it's just a + * little strange for the end user. */ + slot_user_id = animated_id; + } + /* Don't include anything from this animation if it is linked in from another * file, and we're getting stuff for editing... */ if ((filter_mode & ANIMFILTER_FOREDIT) && @@ -1586,7 +1607,7 @@ static size_t animfilter_action_slot(bAnimContext *ac, const bool show_slot_channel = (is_action_mode && selection_ok_for_slot && include_summary_channels); if (show_slot_channel) { - ANIMCHANNEL_NEW_CHANNEL(ac->bmain, &slot, ANIMTYPE_ACTION_SLOT, animated_id, &action.id); + ANIMCHANNEL_NEW_CHANNEL(ac->bmain, &slot, ANIMTYPE_ACTION_SLOT, slot_user_id, &action.id); items++; } @@ -1607,7 +1628,7 @@ static size_t animfilter_action_slot(bAnimContext *ac, /* Add channel groups and their member channels. */ for (bActionGroup *group : channelbag->channel_groups()) { items += animfilter_act_group( - ac, anim_data, &action, slot.handle, group, filter_mode, animated_id); + ac, anim_data, &action, slot.handle, group, filter_mode, slot_user_id); } /* Add ungrouped channels. */ @@ -1621,7 +1642,7 @@ static size_t animfilter_action_slot(bAnimContext *ac, Span fcurves = channelbag->fcurves().drop_front(first_ungrouped_fcurve_index); items += animfilter_fcurves_span( - ac, anim_data, fcurves, slot.handle, filter_mode, animated_id, &action.id); + ac, anim_data, fcurves, slot.handle, filter_mode, slot_user_id, &action.id); } return items; @@ -1645,22 +1666,7 @@ static size_t animfilter_action_slots(bAnimContext *ac, for (animrig::Slot *slot : action.slots()) { BLI_assert(slot); - /* In some cases (see `ob_to_keylist()` and friends) fake bDopeSheet and fake bAnimContext are - * created. These are mostly null-initialized, and so do not have a bmain. This means that - * lookup of the animated ID is not possible, which can result in failure to look up the proper - * F-Curve display name. For the `..._to_keylist` functions that doesn't matter, as those are - * only interested in the key data anyway. So rather than trying to get a reliable `bmain` - * through the maze, this code just treats it as optional (even though ideally it should always - * be known). */ - ID *animated_id = nullptr; - if (ac->bmain) { - animated_id = animrig::action_slot_get_id_best_guess(*ac->bmain, *slot, owner_id); - } - if (!animated_id) { - /* This is not necessarily correct, but at least it prevents nullptr dereference. */ - animated_id = owner_id; - } - num_items += animfilter_action_slot(ac, anim_data, action, *slot, filter_mode, animated_id); + num_items += ANIM_animfilter_action_slot(ac, anim_data, action, *slot, filter_mode, owner_id); } return num_items; @@ -1728,7 +1734,7 @@ static size_t animfilter_action(bAnimContext *ac, /* Can happen when an Action is assigned, but not a Slot. */ return 0; } - return animfilter_action_slot(ac, anim_data, action, *slot, filter_mode, owner_id); + return ANIM_animfilter_action_slot(ac, anim_data, action, *slot, filter_mode, owner_id); } /* Include NLA-Data for NLA-Editor: diff --git a/source/blender/editors/animation/keyframes_draw.cc b/source/blender/editors/animation/keyframes_draw.cc index 2a3246ccae5..0bf1b2ef61c 100644 --- a/source/blender/editors/animation/keyframes_draw.cc +++ b/source/blender/editors/animation/keyframes_draw.cc @@ -422,6 +422,7 @@ struct ChannelListElement { bDopeSheet *ads; Scene *sce; Object *ob; + ID *animated_id; /* The ID that adt (below) belongs to. */ AnimData *adt; FCurve *fcu; bAction *act; @@ -455,18 +456,36 @@ static void build_channel_keylist(ChannelListElement *elem, blender::float2 rang break; } case ChannelType::ACTION_LAYERED: { - action_to_keylist(elem->adt, elem->act, elem->keylist, elem->saction_flag, range); + /* This is only called for action summaries in the Dopesheet, *not* the + * Action Editor. Therefore despite the name `ACTION_LAYERED`, this is + * only used to show a *single slot* of the action: the slot used by the + * ID the action is listed under. + * + * Thus we use the same function as the `ChannelType::ACTION_SLOT` case + * below because in practice the only distinction between these cases is + * where they get the slot from. In this case, we get it from `elem`'s + * ADT. */ + BLI_assert(elem->act); + BLI_assert(elem->adt); + action_slot_summary_to_keylist(elem->ac, + elem->animated_id, + elem->act->wrap(), + elem->adt->slot_handle, + elem->keylist, + elem->saction_flag, + range); break; } case ChannelType::ACTION_SLOT: { BLI_assert(elem->act); BLI_assert(elem->action_slot); - action_slot_to_keylist(elem->adt, - elem->act->wrap(), - elem->action_slot->handle, - elem->keylist, - elem->saction_flag, - range); + action_slot_summary_to_keylist(elem->ac, + elem->animated_id, + elem->act->wrap(), + elem->action_slot->handle, + elem->keylist, + elem->saction_flag, + range); break; } case ChannelType::ACTION_LEGACY: { @@ -718,6 +737,7 @@ void ED_add_fcurve_channel(ChannelDrawList *channel_list, ChannelListElement *draw_elem = channel_list_add_element( channel_list, ChannelType::FCURVE, ypos, yscale_fac, eSAction_Flag(saction_flag)); + draw_elem->animated_id = ale->id; draw_elem->adt = ale->adt; draw_elem->fcu = fcu; draw_elem->channel_locked = locked; @@ -737,12 +757,14 @@ void ED_add_action_group_channel(ChannelDrawList *channel_list, ChannelListElement *draw_elem = channel_list_add_element( channel_list, ChannelType::ACTION_GROUP, ypos, yscale_fac, eSAction_Flag(saction_flag)); + draw_elem->animated_id = ale->id; draw_elem->adt = ale->adt; draw_elem->agrp = agrp; draw_elem->channel_locked = locked; } void ED_add_action_layered_channel(ChannelDrawList *channel_list, + bAnimContext *ac, bAnimListElem *ale, bAction *action, const float ypos, @@ -757,12 +779,15 @@ void ED_add_action_layered_channel(ChannelDrawList *channel_list, ChannelListElement *draw_elem = channel_list_add_element( channel_list, ChannelType::ACTION_LAYERED, ypos, yscale_fac, eSAction_Flag(saction_flag)); + draw_elem->ac = ac; + draw_elem->animated_id = ale->id; draw_elem->adt = ale->adt; draw_elem->act = action; draw_elem->channel_locked = locked; } void ED_add_action_slot_channel(ChannelDrawList *channel_list, + bAnimContext *ac, bAnimListElem *ale, animrig::Action &action, animrig::Slot &slot, @@ -775,6 +800,8 @@ void ED_add_action_slot_channel(ChannelDrawList *channel_list, ChannelListElement *draw_elem = channel_list_add_element( channel_list, ChannelType::ACTION_SLOT, ypos, yscale_fac, eSAction_Flag(saction_flag)); + draw_elem->ac = ac; + draw_elem->animated_id = ale->id; draw_elem->adt = ale->adt; draw_elem->act = &action; draw_elem->action_slot = &slot; @@ -795,6 +822,7 @@ void ED_add_action_channel(ChannelDrawList *channel_list, ChannelListElement *draw_elem = channel_list_add_element( channel_list, ChannelType::ACTION_LEGACY, ypos, yscale_fac, eSAction_Flag(saction_flag)); + draw_elem->animated_id = ale->id; draw_elem->adt = ale->adt; draw_elem->act = act; draw_elem->channel_locked = locked; @@ -815,6 +843,7 @@ void ED_add_grease_pencil_datablock_channel(ChannelDrawList *channel_list, eSAction_Flag(saction_flag)); /* GreasePencil properties can be animated via an Action, so the GP-related * animation data is not limited to GP drawings. */ + draw_elem->animated_id = ale->id; draw_elem->adt = ale->adt; draw_elem->act = ale->adt ? ale->adt->action : nullptr; draw_elem->grease_pencil = grease_pencil; diff --git a/source/blender/editors/animation/keyframes_keylist.cc b/source/blender/editors/animation/keyframes_keylist.cc index 03359714dea..5961c585546 100644 --- a/source/blender/editors/animation/keyframes_keylist.cc +++ b/source/blender/editors/animation/keyframes_keylist.cc @@ -978,6 +978,50 @@ void summary_to_keylist(bAnimContext *ac, ANIM_animdata_freelist(&anim_data); } +void action_slot_summary_to_keylist(bAnimContext *ac, + ID *animated_id, + animrig::Action &action, + const animrig::slot_handle_t slot_handle, + AnimKeylist *keylist, + const int /* eSAction_Flag */ saction_flag, + blender::float2 range) +{ + /* TODO: downstream code depends on this being non-null (see e.g. + * `ANIM_animfilter_action_slot()` and `animfilter_fcurves_span()`). Either + * change this parameter to be a reference, or modify the downstream code to + * not assume that it's non-null and do something reasonable when it is null. */ + BLI_assert(animated_id); + + if (!ac) { + return; + } + + animrig::Slot *slot = action.slot_for_handle(slot_handle); + BLI_assert(slot); + + ListBase anim_data = {nullptr, nullptr}; + + /* Get F-Curves to take keyframes from. */ + const eAnimFilter_Flags filter = ANIMFILTER_DATA_VISIBLE; + ANIM_animfilter_action_slot(ac, &anim_data, action, *slot, filter, animated_id); + + LISTBASE_FOREACH (const bAnimListElem *, ale, &anim_data) { + /* As of the writing of this code, Actions ultimately only contain FCurves. + * If/when that changes in the future, this may need to be updated. */ + if (ale->datatype != ALE_FCURVE) { + continue; + } + fcurve_to_keylist(ale->adt, + static_cast(ale->data), + keylist, + saction_flag, + range, + ANIM_nla_mapping_allowed(ale)); + } + + ANIM_animdata_freelist(&anim_data); +} + void scene_to_keylist(bDopeSheet *ads, Scene *sce, AnimKeylist *keylist, @@ -1238,19 +1282,6 @@ void action_group_to_keylist(AnimData *adt, } } -void action_slot_to_keylist(AnimData *adt, - animrig::Action &action, - const animrig::slot_handle_t slot_handle, - AnimKeylist *keylist, - const int saction_flag, - blender::float2 range) -{ - BLI_assert(GS(action.id.name) == ID_AC); - for (FCurve *fcurve : fcurves_for_action_slot(action, slot_handle)) { - fcurve_to_keylist(adt, fcurve, keylist, saction_flag, range, true); - } -} - void action_to_keylist(AnimData *adt, bAction *dna_action, AnimKeylist *keylist, @@ -1276,7 +1307,9 @@ void action_to_keylist(AnimData *adt, * have things like reference strips, where the strip can reference another slot handle. */ BLI_assert(adt); - action_slot_to_keylist(adt, action, adt->slot_handle, keylist, saction_flag, range); + for (FCurve *fcurve : fcurves_for_action_slot(action, adt->slot_handle)) { + fcurve_to_keylist(adt, fcurve, keylist, saction_flag, range, true); + } } void gpencil_to_keylist(bDopeSheet *ads, bGPdata *gpd, AnimKeylist *keylist, const bool active) diff --git a/source/blender/editors/animation/keyframes_keylist_test.cc b/source/blender/editors/animation/keyframes_keylist_test.cc index 6002e92a83a..6769f47a4dd 100644 --- a/source/blender/editors/animation/keyframes_keylist_test.cc +++ b/source/blender/editors/animation/keyframes_keylist_test.cc @@ -4,6 +4,10 @@ #include "testing/testing.h" +#include "ANIM_action.hh" +#include "ANIM_fcurve.hh" + +#include "ED_anim_api.hh" #include "ED_keyframes_keylist.hh" #include "DNA_anim_types.h" @@ -11,7 +15,19 @@ #include "MEM_guardedalloc.h" +#include "BKE_action.hh" +#include "BKE_armature.hh" #include "BKE_fcurve.hh" +#include "BKE_global.hh" +#include "BKE_idtype.hh" +#include "BKE_lib_id.hh" +#include "BKE_main.hh" +#include "BKE_object.hh" + +#include "BLI_string.h" + +#include "CLG_log.h" +#include "testing/testing.h" #include #include @@ -138,4 +154,186 @@ TEST(keylist, find_exact) ED_keylist_free(keylist); } +class KeylistSummaryTest : public testing::Test { + public: + Main *bmain; + blender::animrig::Action *action; + Object *cube; + Object *armature; + bArmature *armature_data; + Bone *bone1; + Bone *bone2; + + SpaceAction saction = {nullptr}; + bAnimContext ac = {nullptr}; + + static void SetUpTestSuite() + { + /* BKE_id_free() hits a code path that uses CLOG, which crashes if not initialized properly. */ + CLG_init(); + + /* To make id_can_have_animdata() and friends work, the `id_types` array needs to be set up. */ + BKE_idtype_init(); + } + + static void TearDownTestSuite() + { + CLG_exit(); + } + + void SetUp() override + { + bmain = BKE_main_new(); + G_MAIN = bmain; /* For BKE_animdata_free(). */ + + action = &static_cast(BKE_id_new(bmain, ID_AC, "ACÄnimåtië"))->wrap(); + cube = BKE_object_add_only_object(bmain, OB_EMPTY, "Küüübus"); + + armature_data = BKE_armature_add(bmain, "ARArmature"); + bone1 = reinterpret_cast(MEM_callocN(sizeof(Bone), "KeylistSummaryTest")); + bone2 = reinterpret_cast(MEM_callocN(sizeof(Bone), "KeylistSummaryTest")); + STRNCPY(bone1->name, "Bone.001"); + STRNCPY(bone2->name, "Bone.002"); + BLI_addtail(&armature_data->bonebase, bone1); + BLI_addtail(&armature_data->bonebase, bone2); + BKE_armature_bone_hash_make(armature_data); + + armature = BKE_object_add_only_object(bmain, OB_ARMATURE, "OBArmature"); + armature->data = armature_data; + BKE_pose_ensure(bmain, armature, armature_data, false); + + /* + * Fill in the common bits for the mock bAnimContext, for an Action editor. + * + * Tests should fill in: + * - saction.action_slot_handle + * - ac.obact + */ + saction.action = action; + saction.ads.filterflag = eDopeSheet_FilterFlag(0); + ac.bmain = bmain; + ac.datatype = ANIMCONT_ACTION; + ac.data = action; + ac.spacetype = SPACE_ACTION; + ac.sl = reinterpret_cast(&saction); + ac.ads = &saction.ads; + } + + void TearDown() override + { + saction.action_slot_handle = blender::animrig::Slot::unassigned; + ac.obact = nullptr; + + BKE_main_free(bmain); + G_MAIN = nullptr; + } +}; + +TEST_F(KeylistSummaryTest, slot_summary_simple) +{ + /* Test that a key summary is generated correctly for a slot that's animating + * an object's transforms. */ + + using namespace blender::animrig; + + Slot &slot_cube = action->slot_add_for_id(cube->id); + ASSERT_EQ(ActionSlotAssignmentResult::OK, assign_action_and_slot(action, &slot_cube, cube->id)); + Channelbag &channelbag = action_channelbag_ensure(*action, cube->id); + + FCurve &loc_x = channelbag.fcurve_ensure(bmain, {"location", 0}); + FCurve &loc_y = channelbag.fcurve_ensure(bmain, {"location", 1}); + FCurve &loc_z = channelbag.fcurve_ensure(bmain, {"location", 2}); + + ASSERT_EQ(SingleKeyingResult::SUCCESS, insert_vert_fcurve(&loc_x, {1.0, 0.0}, {}, {})); + ASSERT_EQ(SingleKeyingResult::SUCCESS, insert_vert_fcurve(&loc_x, {2.0, 1.0}, {}, {})); + ASSERT_EQ(SingleKeyingResult::SUCCESS, insert_vert_fcurve(&loc_y, {2.0, 2.0}, {}, {})); + ASSERT_EQ(SingleKeyingResult::SUCCESS, insert_vert_fcurve(&loc_y, {3.0, 3.0}, {}, {})); + ASSERT_EQ(SingleKeyingResult::SUCCESS, insert_vert_fcurve(&loc_z, {2.0, 4.0}, {}, {})); + ASSERT_EQ(SingleKeyingResult::SUCCESS, insert_vert_fcurve(&loc_z, {5.0, 5.0}, {}, {})); + + /* Generate slot summary keylist. */ + AnimKeylist *keylist = ED_keylist_create(); + saction.action_slot_handle = slot_cube.handle; + ac.obact = cube; + action_slot_summary_to_keylist( + &ac, &cube->id, *action, slot_cube.handle, keylist, 0, {0.0, 6.0}); + ED_keylist_prepare_for_direct_access(keylist); + + const ActKeyColumn *col_0 = ED_keylist_find_exact(keylist, 0.0); + const ActKeyColumn *col_1 = ED_keylist_find_exact(keylist, 1.0); + const ActKeyColumn *col_2 = ED_keylist_find_exact(keylist, 2.0); + const ActKeyColumn *col_3 = ED_keylist_find_exact(keylist, 3.0); + const ActKeyColumn *col_4 = ED_keylist_find_exact(keylist, 4.0); + const ActKeyColumn *col_5 = ED_keylist_find_exact(keylist, 5.0); + const ActKeyColumn *col_6 = ED_keylist_find_exact(keylist, 6.0); + + /* Check that we only have columns at the frames with keys. */ + EXPECT_EQ(nullptr, col_0); + EXPECT_NE(nullptr, col_1); + EXPECT_NE(nullptr, col_2); + EXPECT_NE(nullptr, col_3); + EXPECT_EQ(nullptr, col_4); + EXPECT_NE(nullptr, col_5); + EXPECT_EQ(nullptr, col_6); + + /* Check that the right number of keys are indicated in each column. */ + EXPECT_EQ(1, col_1->totkey); + EXPECT_EQ(3, col_2->totkey); + EXPECT_EQ(1, col_3->totkey); + EXPECT_EQ(1, col_5->totkey); + + ED_keylist_free(keylist); +} + +TEST_F(KeylistSummaryTest, slot_summary_bone_selection) +{ + /* Test that a key summary is generated correctly, excluding keys for + * unselected bones when filter-by-selection is on. */ + + using namespace blender::animrig; + + Slot &slot_armature = action->slot_add_for_id(armature->id); + ASSERT_EQ(ActionSlotAssignmentResult::OK, + assign_action_and_slot(action, &slot_armature, armature->id)); + Channelbag &channelbag = action_channelbag_ensure(*action, armature->id); + + FCurve &bone1_loc_x = channelbag.fcurve_ensure( + bmain, {"pose.bones[\"Bone.001\"].location", 0, std::nullopt, "Bone.001"}); + FCurve &bone2_loc_x = channelbag.fcurve_ensure( + bmain, {"pose.bones[\"Bone.002\"].location", 0, std::nullopt, "Bone.002"}); + + ASSERT_EQ(SingleKeyingResult::SUCCESS, insert_vert_fcurve(&bone1_loc_x, {1.0, 0.0}, {}, {})); + ASSERT_EQ(SingleKeyingResult::SUCCESS, insert_vert_fcurve(&bone1_loc_x, {2.0, 1.0}, {}, {})); + ASSERT_EQ(SingleKeyingResult::SUCCESS, insert_vert_fcurve(&bone2_loc_x, {2.0, 2.0}, {}, {})); + ASSERT_EQ(SingleKeyingResult::SUCCESS, insert_vert_fcurve(&bone2_loc_x, {3.0, 3.0}, {}, {})); + + /* Select only Bone.001. */ + bone1->flag |= BONE_SELECTED; + bone2->flag &= ~BONE_SELECTED; + + /* Generate slot summary keylist. */ + AnimKeylist *keylist = ED_keylist_create(); + saction.ads.filterflag = ADS_FILTER_ONLYSEL; /* Filter by selection. */ + saction.action_slot_handle = slot_armature.handle; + ac.obact = armature; + action_slot_summary_to_keylist( + &ac, &armature->id, *action, slot_armature.handle, keylist, 0, {0.0, 6.0}); + ED_keylist_prepare_for_direct_access(keylist); + + const ActKeyColumn *col_1 = ED_keylist_find_exact(keylist, 1.0); + const ActKeyColumn *col_2 = ED_keylist_find_exact(keylist, 2.0); + const ActKeyColumn *col_3 = ED_keylist_find_exact(keylist, 3.0); + + /* Check that we only have columns at the frames with keys for Bone.001. */ + EXPECT_NE(nullptr, col_1); + EXPECT_NE(nullptr, col_2); + EXPECT_EQ(nullptr, col_3); + + /* Check that the right number of keys are indicated in each column. */ + EXPECT_EQ(1, col_1->totkey); + EXPECT_EQ(1, col_2->totkey); + + ED_keylist_free(keylist); +} + } // namespace blender::editor::animation::tests diff --git a/source/blender/editors/include/ED_anim_api.hh b/source/blender/editors/include/ED_anim_api.hh index 0b72a1e36fc..54d1d23464f 100644 --- a/source/blender/editors/include/ED_anim_api.hh +++ b/source/blender/editors/include/ED_anim_api.hh @@ -55,6 +55,11 @@ struct PropertyRNA; struct MPathTarget; +namespace blender::animrig { +class Action; +class Slot; +} // namespace blender::animrig + /* ************************************************ */ /* ANIMATION CHANNEL FILTERING */ /* `anim_filter.cc` */ @@ -517,6 +522,33 @@ ENUM_OPERATORS(eAnimFilter_Flags, ANIMFILTER_TMP_IGNORE_ONLYSEL); /** \name Public API * \{ */ +/** + * Add the channel and sub-channels for an Action Slot to `anim_data`, filtered + * according to `filter_mode`. + * + * \param action: the action containing the slot to generate the channels for. + * + * \param slot: the slot to generate the channels for. + * + * \param filter_mode: the filters to use for deciding what channels get + * included. + * + * \param animated_id: the particular animated ID that the slot channels are + * being generated for. This is needed for filtering channels based on bone + * selection, and also for resolving the names of animated properties. This + * should never be null, but it's okay(ish) if it's an ID not actually animated + * by the slot, in which case it will act as a fallback in case an ID actually + * animated by the slot can't be found. + * + * \return The number of items added to `anim_data`. + */ +size_t ANIM_animfilter_action_slot(bAnimContext *ac, + ListBase * /* bAnimListElem */ anim_data, + blender::animrig::Action &action, + blender::animrig::Slot &slot, + eAnimFilter_Flags filter_mode, + ID *animated_id); + /** * This function filters the active data source to leave only animation channels suitable for * usage by the caller. It will return the length of the list diff --git a/source/blender/editors/include/ED_keyframes_draw.hh b/source/blender/editors/include/ED_keyframes_draw.hh index d8ba6c2fc0f..1d6ff267c51 100644 --- a/source/blender/editors/include/ED_keyframes_draw.hh +++ b/source/blender/editors/include/ED_keyframes_draw.hh @@ -75,6 +75,7 @@ void ED_add_action_group_channel(ChannelDrawList *channel_list, int saction_flag); /* Layered Action Summary. */ void ED_add_action_layered_channel(ChannelDrawList *channel_list, + bAnimContext *ac, bAnimListElem *ale, bAction *action, const float ypos, @@ -82,6 +83,7 @@ void ED_add_action_layered_channel(ChannelDrawList *channel_list, int saction_flag); /* Action Slot summary. */ void ED_add_action_slot_channel(ChannelDrawList *channel_list, + bAnimContext *ac, bAnimListElem *ale, blender::animrig::Action &action, blender::animrig::Slot &slot, diff --git a/source/blender/editors/include/ED_keyframes_keylist.hh b/source/blender/editors/include/ED_keyframes_keylist.hh index 66bb5982040..669bdee30f7 100644 --- a/source/blender/editors/include/ED_keyframes_keylist.hh +++ b/source/blender/editors/include/ED_keyframes_keylist.hh @@ -180,17 +180,27 @@ void action_group_to_keylist(AnimData *adt, int saction_flag, blender::float2 range); /* Action */ + +/** + * Generate a full list of the keys in `dna_action` that are within the frame + * range `range`. + * + * For layered actions, this is limited to the keys that are for the slot + * assigned to `adt`. + * + * Note: this should only be used in places that need or want the *full* list of + * keys, without any filtering by e.g. channel selection/visibility, etc. For + * use cases that need such filtering, use `action_slot_summary_to_keylist()` + * instead. + * + * \see action_slot_summary_to_keylist() + */ void action_to_keylist(AnimData *adt, bAction *dna_action, AnimKeylist *keylist, int saction_flag, blender::float2 range); -void action_slot_to_keylist(AnimData *adt, - blender::animrig::Action &action, - blender::animrig::slot_handle_t slot_handle, - AnimKeylist *keylist, - int saction_flag, - blender::float2 range); + /* Object */ void ob_to_keylist( bDopeSheet *ads, Object *ob, AnimKeylist *keylist, int saction_flag, blender::float2 range); @@ -208,6 +218,42 @@ void summary_to_keylist(bAnimContext *ac, int saction_flag, blender::float2 range); +/** + * Generate a summary channel keylist for the specified slot, merging it into + * `keylist`. + * + * This filters the keys to be consistent with the visible channels in the + * editor indicated by `ac` + * + * \param animated_id: the particular animated ID that the slot summary is being + * generated for. This is needed for filtering channels based on bone selection, + * etc. NOTE: despite being passed as a pointer, this should never be null. It's + * currently passed as a pointer to be defensive because I (Nathan) am not 100% + * confident at the time of writing (PR #134922) that the callers of this + * actually guarantee a non-null pointer (they should, but bugs). This way we + * can assert internally to catch if that ever happens. + * + * \param action: the action containing the slot to generate the summary for. + * + * \param slot_handle: the handle of the slot to generate the summary for. + * + * \param keylist: the keylist that the generated summary will be merged into. + * + * \param saction_flag: needed for the `SACTION_SHOW_EXTREMES` flag, to + * determine whether to compute and store the data needed to determine which + * keys are "extremes" (local maxima/minima). + * + * \param range: only keys within this time range will be included in the + * summary. + */ +void action_slot_summary_to_keylist(bAnimContext *ac, + ID *animated_id, + blender::animrig::Action &action, + blender::animrig::slot_handle_t slot_handle, + AnimKeylist *keylist, + int /* eSAction_Flag */ saction_flag, + blender::float2 range); + /* Grease Pencil datablock summary (Legacy) */ void gpencil_to_keylist(bDopeSheet *ads, bGPdata *gpd, AnimKeylist *keylist, bool active); diff --git a/source/blender/editors/sculpt_paint/grease_pencil_erase.cc b/source/blender/editors/sculpt_paint/grease_pencil_erase.cc index 1ffdccc440f..5d80804dd7b 100644 --- a/source/blender/editors/sculpt_paint/grease_pencil_erase.cc +++ b/source/blender/editors/sculpt_paint/grease_pencil_erase.cc @@ -1023,10 +1023,9 @@ struct EraseOperationExecutor { /* Erase on all editable drawings. */ const Vector drawings = ed::greasepencil::retrieve_editable_drawings(*scene, grease_pencil); - threading::parallel_for_each( - drawings, [&](const ed::greasepencil::MutableDrawingInfo &info) { - execute_eraser_on_drawing(info.layer_index, info.frame_number, info.drawing); - }); + for (const ed::greasepencil::MutableDrawingInfo &info : drawings) { + execute_eraser_on_drawing(info.layer_index, info.frame_number, info.drawing); + } } if (changed) { diff --git a/source/blender/editors/space_action/action_draw.cc b/source/blender/editors/space_action/action_draw.cc index 2f8cd8ad959..67c8c3f1cb8 100644 --- a/source/blender/editors/space_action/action_draw.cc +++ b/source/blender/editors/space_action/action_draw.cc @@ -372,6 +372,7 @@ static void draw_keyframes(bAnimContext *ac, break; case ALE_ACTION_LAYERED: ED_add_action_layered_channel(draw_list, + ac, ale, static_cast(ale->key_data), ycenter, @@ -380,6 +381,7 @@ static void draw_keyframes(bAnimContext *ac, break; case ALE_ACTION_SLOT: ED_add_action_slot_channel(draw_list, + ac, ale, static_cast(ale->key_data)->wrap(), *static_cast(ale->data), diff --git a/source/blender/editors/space_action/action_select.cc b/source/blender/editors/space_action/action_select.cc index f851f3d3472..f26448ca686 100644 --- a/source/blender/editors/space_action/action_select.cc +++ b/source/blender/editors/space_action/action_select.cc @@ -111,8 +111,19 @@ static void actkeys_list_element_to_keylist(bAnimContext *ac, break; } case ALE_ACTION_LAYERED: { - bAction *action = (bAction *)ale->key_data; - action_to_keylist(ale->adt, action, keylist, 0, range); + /* This is only called for action summaries in the Dopesheet, *not* the + * Action Editor. Therefore despite the name `ALE_ACTION_LAYERED`, this + * is only used to show a *single slot* of the action: the slot used by + * the ID the action is listed under. + * + * Thus we use the same function as the `ALE_ACTION_SLOT` case below + * because in practice the only distinction between these cases is where + * they get the slot from. In this case, we get it from `elem`'s ADT. */ + animrig::Action *action = static_cast(ale->key_data); + BLI_assert(action); + BLI_assert(ale->adt); + action_slot_summary_to_keylist( + ac, ale->id, *action, ale->adt->slot_handle, keylist, 0, range); break; } case ALE_ACTION_SLOT: { @@ -120,10 +131,11 @@ static void actkeys_list_element_to_keylist(bAnimContext *ac, animrig::Slot *slot = static_cast(ale->data); BLI_assert(action); BLI_assert(slot); - action_slot_to_keylist(ale->adt, *action, slot->handle, keylist, 0, range); + action_slot_summary_to_keylist(ac, ale->id, *action, slot->handle, keylist, 0, range); break; } case ALE_ACT: { + /* Legacy action. */ bAction *act = (bAction *)ale->key_data; action_to_keylist(ale->adt, act, keylist, 0, range); break;