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

Fix: handle embedded IDs when upgrading to slotted actions

Browse Source 
The versioning code that upgrades legacy actions to new slotted actions
also needs to properly assign slots to the IDs that use those upgraded
actions. It was doing this correctly except for not traversing into and
assigning slots to embedded IDs.

This commit adds the code to handle embedded IDs as well.

Additionally, this changes how mismatched `id_type`s are handled when upgrading
actions. Rather than refusing to assign the slot created during the upgrade if
the `id_type` doesn't match the ID, we assign it anyway with a warning. The
rationale is that this represents a case where the Action `idroot` was already
mismatched, and it turns out that has always been possible. So we now opt to
simply preserve that state of affairs rather than attempt to "fix" it.

Pull Request: https://projects.blender.org/blender/blender/pulls/129002
This commit is contained in:
Nathan Vegdahl
2024-10-15 11:04:04 +02:00
committed by Nathan Vegdahl
parent 26edd760bb
commit 989634c0a1
6 changed files with 104 additions and 48 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

@@ -711,17 +711,25 @@ class Slot : public ::ActionSlot {
*/
static void users_invalidate(Main &bmain);
protected:
friend Action;
/**
* Ensure the first two characters of the name match the ID type.
*
* \note This does NOT ensure name uniqueness within the Action. That is
* the responsibility of the caller.
* This typically should not be called directly. Prefer assigning to an ID to
* get the idtype and name prefix properly set. Prefer calling
* `Action::slot_name_set()` if you want to set the slot name. Both of those
* approaches take care of ensuring uniqueness and other invariants.
*
* \note This does NOT ensure name uniqueness within the Action. That is the
* responsibility of the caller.
*
* \see #assign_action_slot
* \see #Action::slot_name_set
*/
void name_ensure_prefix();
protected:
friend Action;
/**
* Set the 'Active' flag. Only allowed to be called by Action.
*/

19
source/blender/animrig/ANIM_action_iterators.hh
View File

@@ -64,16 +64,23 @@ bool foreach_action_slot_use(
FunctionRef<bool(const Action &action, slot_handle_t slot_handle)> callback);
/**
* Same as foreach_action_slot_use(), except that it reports some pointers so the callback can
* modify which Action/slot is assigned.
* Essentially the same as foreach_action_slot_use(), except that it provides
* the ID as well as pointers via which the callback can modify which
* Action/slot is assigned.
*
* The ID passed to the callback is always the same `animated_id` as is passed
* to this function. The actions & slots passed to the callback are *not*
* necessarily the direct action & slot of that ID: they can also be the action
* & slot of an Action Constraint or NLA Strip owned by the ID.
*
* \see blender::animrig::generic_assign_action
* \see blender::animrig::generic_assign_action_slot
* \see blender::animrig::generic_assign_action_slot_handle
*/
bool foreach_action_slot_use_with_references(
ID &animated_id,
FunctionRef<bool(bAction *&action_ptr_ref, slot_handle_t &slot_handle_ref, char *slot_name)>
callback);
bool foreach_action_slot_use_with_references(ID &animated_id,
FunctionRef<bool(ID &animated_id,
bAction *&action_ptr_ref,
slot_handle_t &slot_handle_ref,
char *slot_name)> callback);
} // namespace blender::animrig

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

@@ -2971,8 +2971,10 @@ void move_slot(Main &bmain, Slot &source_slot, Action &from_action, Action &to_a
/* Reassign all users of `source_slot` to the action `to_action` and the slot `target_slot`. */
for (ID *user : source_slot.users(bmain)) {
const auto assign_other_action =
[&](bAction *&action_ptr_ref, slot_handle_t &slot_handle_ref, char *slot_name) -> bool {
const auto assign_other_action = [&](ID & /* animated_id */,
bAction *&action_ptr_ref,
slot_handle_t &slot_handle_ref,
char *slot_name) -> bool {
/* Only reassign if the reference is actually from the same action. Could be from a different
* action when using the NLA or action constraints. */
if (action_ptr_ref != &from_action) {

20
source/blender/animrig/intern/action_iterators.cc
View File

@@ -75,7 +75,8 @@ bool foreach_action_slot_use(
FunctionRef<bool(const Action &action, slot_handle_t slot_handle)> callback)
{
const auto forward_to_callback = [&](bAction *&action_ptr_ref,
const auto forward_to_callback = [&](ID & /* animated_id */,
bAction *&action_ptr_ref,
const slot_handle_t &slot_handle_ref,
char * /*slot_name*/) -> bool {
if (!action_ptr_ref) {
@@ -88,17 +89,18 @@ bool foreach_action_slot_use(
forward_to_callback);
}
bool foreach_action_slot_use_with_references(
ID &animated_id,
FunctionRef<bool(bAction *&action_ptr_ref, slot_handle_t &slot_handle_ref, char *slot_name)>
callback)
bool foreach_action_slot_use_with_references(ID &animated_id,
FunctionRef<bool(ID &animated_id,
bAction *&action_ptr_ref,
slot_handle_t &slot_handle_ref,
char *slot_name)> callback)
{
AnimData *adt = BKE_animdata_from_id(&animated_id);
if (adt) {
if (adt->action) {
/* Direct assignment. */
if (!callback(adt->action, adt->slot_handle, adt->slot_name)) {
if (!callback(animated_id, adt->action, adt->slot_handle, adt->slot_name)) {
return false;
}
}
@@ -106,7 +108,8 @@ bool foreach_action_slot_use_with_references(
/* NLA strips. */
const bool looped_until_last_strip = bke::nla::foreach_strip_adt(*adt, [&](NlaStrip *strip) {
if (strip->act) {
if (!callback(strip->act, strip->action_slot_handle, strip->action_slot_name)) {
if (!callback(animated_id, strip->act, strip->action_slot_handle, strip->action_slot_name))
{
return false;
}
}
@@ -138,7 +141,8 @@ bool foreach_action_slot_use_with_references(
if (!constraint_data->act) {
return true;
}
return callback(constraint_data->act,
return callback(animated_id,
constraint_data->act,
constraint_data->action_slot_handle,
constraint_data->action_slot_name);
};

6
source/blender/animrig/intern/action_iterators_test.cc
View File

@@ -123,8 +123,10 @@ TEST_F(ActionIteratorsTest, foreach_action_slot_use_with_references)
std::optional<ActionSlotAssignmentResult> slot_assignment_result;
bool all_assigns_ok = true;
const auto assign_other_action =
[&](bAction *&action_ptr_ref, slot_handle_t &slot_handle_ref, char *slot_name) -> bool {
const auto assign_other_action = [&](ID & /* animated_id */,
bAction *&action_ptr_ref,
slot_handle_t &slot_handle_ref,
char *slot_name) -> bool {
/* Assign the other Action. */
all_assigns_ok &= generic_assign_action(
cube->id, &other_action, action_ptr_ref, slot_handle_ref, slot_name);

83
source/blender/blenloader/intern/versioning_400.cc
View File

@@ -47,6 +47,7 @@
#include "BLI_set.hh"
#include "BLI_string.h"
#include "BLI_string_ref.hh"
#include "BLI_string_utf8.h"
#include "BKE_anim_data.hh"
#include "BKE_animsys.h"
@@ -62,6 +63,7 @@
#include "BKE_grease_pencil.hh"
#include "BKE_idprop.hh"
#include "BKE_image_format.h"
#include "BKE_lib_query.hh"
#include "BKE_main.hh"
#include "BKE_material.h"
#include "BKE_mesh_legacy_convert.hh"
@@ -120,9 +122,17 @@ static void convert_action_in_place(blender::animrig::Action &action)
if (action.is_action_layered()) {
return;
}
Slot &slot = action.slot_add();
slot.idtype = action.idroot;
/* Store this ahead of time, because adding the slot sets the action's idroot
* to 0. We also set the action's idroot to 0 manually, just to be defensive
* so we don't depend on esoteric behavior in `slot_add()`. */
const int16_t idtype = action.idroot;
action.idroot = 0;
Slot &slot = action.slot_add();
slot.idtype = idtype;
slot.name_ensure_prefix();
Layer &layer = action.layer_add("Layer");
blender::animrig::Strip &strip = layer.strip_add(action,
blender::animrig::Strip::Type::Keyframe);
@@ -180,8 +190,10 @@ static void version_legacy_actions_to_layered(Main *bmain)
ID *id;
FOREACH_MAIN_ID_BEGIN (bmain, id) {
auto callback =
[&](bAction *&action_ptr_ref, slot_handle_t &slot_handle_ref, char *slot_name) -> bool {
auto callback = [&](ID &animated_id,
bAction *&action_ptr_ref,
slot_handle_t &slot_handle_ref,
char *slot_name) -> bool {
blender::Vector<ActionUserInfo> *action_user_vector = action_users.lookup_ptr(
action_ptr_ref);
/* Only actions that need to be converted are in this map. */
@@ -189,7 +201,7 @@ static void version_legacy_actions_to_layered(Main *bmain)
return true;
}
ActionUserInfo user_info;
user_info.id = id;
user_info.id = &animated_id;
user_info.action_ptr_ptr = &action_ptr_ref;
user_info.slot_handle = &slot_handle_ref;
user_info.slot_name = slot_name;
@@ -197,7 +209,33 @@ static void version_legacy_actions_to_layered(Main *bmain)
return true;
};
auto embedded_id_callback = [&](LibraryIDLinkCallbackData *cb_data) -> int {
ID *linked_id = *cb_data->id_pointer;
/* We only process embedded IDs with this callback. */
if (!linked_id || (linked_id->flag & ID_FLAG_EMBEDDED_DATA) == 0) {
return IDWALK_RET_STOP_RECURSION;
}
foreach_action_slot_use_with_references(*linked_id, callback);
return IDWALK_RET_NOP;
};
/* Process the main ID itself. */
foreach_action_slot_use_with_references(*id, callback);
/* Process embedded IDs, as these are not listed in bmain, but still can
* have their own Action+Slot. */
BKE_library_foreach_ID_link(
bmain,
id,
embedded_id_callback,
nullptr,
IDWALK_RECURSE | IDWALK_READONLY |
/* This is more about "we don't care" than "must be ignored". We don't pass an owner
* ID, and it's not used in the callback either, so don't bother looking it up. */
IDWALK_IGNORE_MISSING_OWNER_ID);
}
FOREACH_MAIN_ID_END;
@@ -223,32 +261,27 @@ static void version_legacy_actions_to_layered(Main *bmain)
case ActionSlotAssignmentResult::OK:
break;
case ActionSlotAssignmentResult::SlotNotSuitable:
/* The slot assignment can fail in the following scenario, when dealing
* with "old Blender" (only supporting legacy Actions) and "new Blender"
* (versions supporting slotted/layered Actions).
/* If the slot wasn't suitable for the ID, we force assignment anyway,
* but with a warning.
*
* - New Blender: create an action with two slots, ME and KE, and assign
* to respectively a Mesh and a Shape Key. Save the file.
* - Old Blender: load the file. This will load the legacy data, but still
* keep the assignments. This means that the Shape Key will get a ME
* Action assigned, which is incompatible. Save the file.
* - New Blender: upgrades the Action (this code here), and tries to
* assign the first (and by now only) slot. This will fail for the shape
* key, as the ID type doesn't match.
*
* The failure is in itself okay, as there was actual data loss in this
* scenario, and so issuing a warning is the right way to go about this.
* The Action is still assigned, but the data-block won't get a slot
* assigned.
* This happens when the legacy action assigned to the ID had a
* mismatched idroot, and therefore the created slot does as well.
* This mismatch can happen in a variety of ways, and we opt to
* preserve this unusual (but technically valid) state of affairs.
*/
*action_user.slot_handle = slot_to_assign.handle;
BLI_strncpy_utf8(action_user.slot_name, slot_to_assign.name, Slot::name_length_max);
printf(
"Warning: while upgrading legacy Action \"%s\", its slot \"%s\" could not be "
"assigned to data-block \"%s\" because it was meant for ID type \"%s\". The Action "
"assignment will be kept, but \"%s\" will not be animated.\n",
"Warning: legacy action \"%s\" is assigned to \"%s\", which does not match the "
"action's id_root \"%s\". The action has been upgraded to a slotted action with "
"slot \"%s\" with an id_type \"%s\", which has also been assigned to \"%s\" despite "
"this type mismatch. This likely indicates something odd about the blend file.\n",
action.id.name + 2,
slot_to_assign.name_without_prefix().c_str(),
action_user.id->name,
slot_to_assign.name_prefix_for_idtype().c_str(),
slot_to_assign.name_without_prefix().c_str(),
slot_to_assign.name_prefix_for_idtype().c_str(),
action_user.id->name);
break;
case ActionSlotAssignmentResult::SlotNotFromAction: