diff options
author | Sybren A. Stüvel <sybren> | 2021-07-06 15:36:27 +0300 |
---|---|---|
committer | Alexander Gavrilov <angavrilov@gmail.com> | 2021-07-09 12:21:12 +0300 |
commit | c04cceb40ed5574dcba8a55cfe97a1132c869895 (patch) | |
tree | ec136545e587ee4443fef8d7ea85673b5c37597b /source/blender/blenkernel/intern | |
parent | b69ab42982a1da921ea9f585fceca1a53589de79 (diff) |
Fix T89435: Reordering FCurves can cause crash or corruption
Correctly reset `prev` and `next` pointers of action group FCurves when
separating them into distinct `ListBase`s per `bActionGroup`.
These `NULL` pointers are necessary to temporarily demarcate the start &
end of the `bActionGroup::channels` list. Having them still point to
other FCurves caused ordering issues when moving curves towards the
start/end of a group.
This commit corrects the above issue and adds versioning code to rectify
any ordering issues that may have been caused. For this purpose the
`BKE_action_groups_reconstruct()` function is rewritten to avoid relying
on the `bAction::curves` list order or `prev` link integrity.
Differential Revision: https://developer.blender.org/D11811
Diffstat (limited to 'source/blender/blenkernel/intern')
-rw-r--r-- | source/blender/blenkernel/intern/action.c | 38 | ||||
-rw-r--r-- | source/blender/blenkernel/intern/action_test.cc | 144 |
2 files changed, 166 insertions, 16 deletions
diff --git a/source/blender/blenkernel/intern/action.c b/source/blender/blenkernel/intern/action.c index 13ca5ecf23c..d55f023d209 100644 --- a/source/blender/blenkernel/intern/action.c +++ b/source/blender/blenkernel/intern/action.c @@ -497,9 +497,8 @@ void action_groups_add_channel(bAction *act, bActionGroup *agrp, FCurve *fcurve) } /* Reconstruct group channel pointers. - * Assumes that the channels are still in the proper order, i.e. that channels of the same group - * are adjacent in the act->channels list. It also assumes that the groups - * referred to by the FCurves are already in act->groups. + * Assumes that the groups referred to by the FCurves are already in act->groups. + * Reorders the main channel list to match group order. */ void BKE_action_groups_reconstruct(bAction *act) { @@ -514,23 +513,30 @@ void BKE_action_groups_reconstruct(bAction *act) BLI_listbase_clear(&group->channels); } - bActionGroup *grp; - bActionGroup *last_grp = NULL; - LISTBASE_FOREACH (FCurve *, fcurve, &act->curves) { - if (fcurve->grp == NULL) { - continue; - } + /* Sort the channels into the group lists, destroying the act->curves list. */ + ListBase ungrouped = {NULL, NULL}; - grp = fcurve->grp; - if (last_grp != grp) { - /* If this is the first time we see this group, this must be the first channel. */ - grp->channels.first = fcurve; + LISTBASE_FOREACH_MUTABLE (FCurve *, fcurve, &act->curves) { + if (fcurve->grp) { + BLI_assert(BLI_findindex(&act->groups, fcurve->grp) >= 0); + + BLI_addtail(&fcurve->grp->channels, fcurve); + } + else { + BLI_addtail(&ungrouped, fcurve); } + } + + /* Recombine into the main list. */ + BLI_listbase_clear(&act->curves); - /* This is the last channel, until it's overwritten by a later iteration. */ - grp->channels.last = fcurve; - last_grp = grp; + LISTBASE_FOREACH (bActionGroup *, group, &act->groups) { + /* Copy the list header to preserve the pointers in the group. */ + ListBase tmp = group->channels; + BLI_movelisttolist(&act->curves, &tmp); } + + BLI_movelisttolist(&act->curves, &ungrouped); } /* Remove the given channel from all groups */ diff --git a/source/blender/blenkernel/intern/action_test.cc b/source/blender/blenkernel/intern/action_test.cc new file mode 100644 index 00000000000..cd8751ec358 --- /dev/null +++ b/source/blender/blenkernel/intern/action_test.cc @@ -0,0 +1,144 @@ +/* + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License + * as published by the Free Software Foundation; either version 2 + * of the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software Foundation, + * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + * + * The Original Code is Copyright (C) 2021 Blender Foundation + * All rights reserved. + */ + +#include "BKE_action.h" + +#include "DNA_action_types.h" +#include "DNA_anim_types.h" + +#include "BLI_listbase.h" + +#include "testing/testing.h" + +namespace blender::bke::tests { + +TEST(action_groups, ReconstructGroupsWithReordering) +{ + // Construct an Action with three groups. + bAction action = {0}; + FCurve groupAcurve1 = {0}; + FCurve groupAcurve2 = {0}; + FCurve groupBcurve1 = {0}; + FCurve groupBcurve2 = {0}; + FCurve groupBcurve3 = {0}; + // Group C has no curves intentionally. + FCurve groupDcurve1 = {0}; + FCurve groupDcurve2 = {0}; + + groupAcurve1.rna_path = (char *)"groupAcurve1"; + groupAcurve2.rna_path = (char *)"groupAcurve2"; + groupBcurve1.rna_path = (char *)"groupBcurve1"; + groupBcurve2.rna_path = (char *)"groupBcurve2"; + groupDcurve1.rna_path = (char *)"groupDcurve1"; + groupBcurve3.rna_path = (char *)"groupBcurve3"; + groupDcurve2.rna_path = (char *)"groupDcurve2"; + + BLI_addtail(&action.curves, &groupAcurve1); + BLI_addtail(&action.curves, &groupAcurve2); + BLI_addtail(&action.curves, &groupBcurve1); + BLI_addtail(&action.curves, &groupBcurve2); + BLI_addtail(&action.curves, &groupDcurve1); + BLI_addtail(&action.curves, &groupBcurve3); // <-- The error that should be corrected. + BLI_addtail(&action.curves, &groupDcurve2); + + // Introduce another error type, by changing some `prev` pointers. + groupBcurve1.prev = NULL; + groupBcurve3.prev = &groupBcurve2; + groupDcurve1.prev = &groupBcurve3; + + bActionGroup groupA = {0}; + bActionGroup groupB = {0}; + bActionGroup groupC = {0}; + bActionGroup groupD = {0}; + strcpy(groupA.name, "groupA"); + strcpy(groupB.name, "groupB"); + strcpy(groupC.name, "groupC"); + strcpy(groupD.name, "groupD"); + + BLI_addtail(&action.groups, &groupA); + BLI_addtail(&action.groups, &groupB); + BLI_addtail(&action.groups, &groupC); + BLI_addtail(&action.groups, &groupD); + + groupAcurve1.grp = &groupA; + groupAcurve2.grp = &groupA; + groupBcurve1.grp = &groupB; + groupBcurve2.grp = &groupB; + groupBcurve3.grp = &groupB; + groupDcurve1.grp = &groupD; + groupDcurve2.grp = &groupD; + + groupA.channels.first = &groupAcurve1; + groupA.channels.last = &groupAcurve2; + groupB.channels.first = &groupBcurve1; + groupB.channels.last = &groupBcurve3; // The last channel in group B, after group C curve 1. + groupD.channels.first = &groupDcurve1; + groupD.channels.last = &groupDcurve2; + + EXPECT_EQ(groupA.channels.first, &groupAcurve1); + EXPECT_EQ(groupA.channels.last, &groupAcurve2); + EXPECT_EQ(groupB.channels.first, &groupBcurve1); + EXPECT_EQ(groupB.channels.last, &groupBcurve3); + EXPECT_EQ(groupC.channels.first, nullptr); + EXPECT_EQ(groupC.channels.last, nullptr); + EXPECT_EQ(groupD.channels.first, &groupDcurve1); + EXPECT_EQ(groupD.channels.last, &groupDcurve2); + + BKE_action_groups_reconstruct(&action); + + EXPECT_EQ(action.curves.first, &groupAcurve1); + EXPECT_EQ(action.curves.last, &groupDcurve2); + + EXPECT_EQ(groupA.prev, nullptr); + EXPECT_EQ(groupB.prev, &groupA); + EXPECT_EQ(groupC.prev, &groupB); + EXPECT_EQ(groupD.prev, &groupC); + + EXPECT_EQ(groupA.next, &groupB); + EXPECT_EQ(groupB.next, &groupC); + EXPECT_EQ(groupC.next, &groupD); + EXPECT_EQ(groupD.next, nullptr); + + EXPECT_EQ(groupA.channels.first, &groupAcurve1); + EXPECT_EQ(groupA.channels.last, &groupAcurve2); + EXPECT_EQ(groupB.channels.first, &groupBcurve1); + EXPECT_EQ(groupB.channels.last, &groupBcurve3); + EXPECT_EQ(groupC.channels.first, nullptr); + EXPECT_EQ(groupC.channels.last, nullptr); + EXPECT_EQ(groupD.channels.first, &groupDcurve1); + EXPECT_EQ(groupD.channels.last, &groupDcurve2); + + EXPECT_EQ(groupAcurve1.prev, nullptr); + EXPECT_EQ(groupAcurve2.prev, &groupAcurve1); + EXPECT_EQ(groupBcurve1.prev, &groupAcurve2); + EXPECT_EQ(groupBcurve2.prev, &groupBcurve1); + EXPECT_EQ(groupBcurve3.prev, &groupBcurve2); + EXPECT_EQ(groupDcurve1.prev, &groupBcurve3); + EXPECT_EQ(groupDcurve2.prev, &groupDcurve1); + + EXPECT_EQ(groupAcurve1.next, &groupAcurve2); + EXPECT_EQ(groupAcurve2.next, &groupBcurve1); + EXPECT_EQ(groupBcurve1.next, &groupBcurve2); + EXPECT_EQ(groupBcurve2.next, &groupBcurve3); + EXPECT_EQ(groupBcurve3.next, &groupDcurve1); + EXPECT_EQ(groupDcurve1.next, &groupDcurve2); + EXPECT_EQ(groupDcurve2.next, nullptr); +} + +} // namespace blender::bke::tests |