bartvdbraak/blender
1
0
Fork 4
Code Issues 4 Pull Requests 1 Actions 1 Packages Projects Releases Wiki Activity

BKE lib_query: Improve handling of owner ID info for embedded ID.

Browse Source 
In previous code, the owner ID info would not be available when
processing an embedded ID in two cases, and was incorrectly set to the
processed (embedded) ID instead:
1. When directly calling `BKE_library_foreach_ID_link` on an embedded ID.
2. When using recursive processing (`IDWALK_RECURSE`).

This commit mostly fixes both cases, by using `BKE_id_owner_get` to find
the owner ID when it is unknown.

There are some caveats here though: in a few specific cases (mainly ID
copying, and depsgraph ID copying), `BKE_library_foreach_ID_link` can be
called on embedded IDs which owner ID is not yet valid. In such case, a
new flag can be used to keep using the previous behavior
(`IDWALK_IGNORE_MISSING_OWNER_ID`).

Fixing the issue with copy code being unaware of the owner ID when
copying an embedded one should also be fixed, but this will be addressed
separately.

Note that as 'side efect', this commit also fixes a matching issue in
the `lib_remap` code, where the `IDRemap.id-owner` pointer would also
wrongly be set to the remapped embedded ID instead of its actual owner.

This change is not expected to have any effect in current codebase.
This commit is contained in:
Bastien Montagne 2024-05-01 22:47:19 +02:00
parent eb9bec54ad
commit 36c3504994
5 changed files with 62 additions and 13 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

18
source/blender/blenkernel/BKE_lib_query.hh
View File

@ -158,10 +158,11 @@ enum {
* Recurse into 'descendant' IDs.
* Each ID is only processed once. Order of ID processing is not guaranteed.
*
* Also implies IDWALK_READONLY, and excludes IDWALK_DO_INTERNAL_RUNTIME_POINTERS.
* Also implies #IDWALK_READONLY, and excludes #IDWALK_DO_INTERNAL_RUNTIME_POINTERS.
*
* NOTE: When enabled, embedded IDs are processed separately from their owner, as if they were
* regular IDs. Owner ID is not available then in the #LibraryForeachIDData callback data.
* regular IDs. The owner ID remains available in the #LibraryForeachIDData callback data, unless
* #IDWALK_IGNORE_MISSING_OWNER_ID is passed.
*/
IDWALK_RECURSE = (1 << 1),
/** Include UI pointers (from WM and screens editors). */
@ -183,6 +184,8 @@ enum {
* \note This flag is mutually exclusive with `IDWALK_RECURSE`, since by definition accessing the
* current ID pointer is required for recursion.
*
* \note Also implies #IDWALK_IGNORE_MISSING_OWNER_ID.
*
* \note After remapping, code may access the newly set ID pointer, which is always presumed
* valid.
*
@ -190,6 +193,17 @@ enum {
* (especially when it comes to detecting `IDWALK_CB_EMBEDDED_NOT_OWNING` usages).
*/
IDWALK_NO_ORIG_POINTERS_ACCESS = (1 << 5),
/**
* Do not attempt to find the owner ID of an embedded one if not explicitely given.
*
* \note This is needed in some cases, when the loopback 'owner' ID pointer of the processed
* embeeded data is known to be invalid (e.g. as part of coying an embedded data when copying its
* owner ID, or as part of depsgraph code, where embedded IDs are mostly processed on their own,
* separately from their owner ID).
*
* \note Also implied by #IDWALK_NO_ORIG_POINTERS_ACCESS.
*/
IDWALK_IGNORE_MISSING_OWNER_ID = (1 << 6),
/**
* Also process internal ID pointers like `ID.newid` or `ID.orig_id`.

9
source/blender/blenkernel/intern/lib_id.cc
View File

@ -682,7 +682,14 @@ ID *BKE_id_copy_in_lib(Main *bmain,
data.id_src = id;
data.id_dst = newid;
data.flag = flag;
BKE_library_foreach_ID_link(bmain, newid, id_copy_libmanagement_cb, &data, IDWALK_NOP);
/* When copying an embedded ID, typically at this point its owner ID pinter will still point to
* the owner of the source, this code has no access to its valid (i.e. destination) owner. This
* can be added at some point is needed, but currently the #id_copy_libmanagement_cb callback
* does need this information. */
/* TODO: handle this fully properly by passing the correct owner ID to copy code when copying
* embedded data. */
BKE_library_foreach_ID_link(
bmain, newid, id_copy_libmanagement_cb, &data, IDWALK_IGNORE_MISSING_OWNER_ID);
/* Do not make new copy local in case we are copying outside of main...
* XXX TODO: is this behavior OK, or should we need a separate flag to control that? */

37
source/blender/blenkernel/intern/lib_query.cc
View File

@ -170,8 +170,6 @@ void BKE_library_foreach_ID_embedded(LibraryForeachIDData *data, ID **id_pp)
else if (flag & IDWALK_RECURSE) {
/* Defer handling into main loop, recursively calling BKE_library_foreach_ID_link in
* IDWALK_RECURSE case is troublesome, see #49553. */
/* XXX note that this breaks the 'owner id' thing now, we likely want to handle that
* differently at some point, but for now it should not be a problem in practice. */
if (BLI_gset_add(data->ids_handled, id)) {
BLI_LINKSTACK_PUSH(data->ids_todo, id);
}
@ -211,6 +209,10 @@ static bool library_foreach_ID_link(Main *bmain,
BLI_assert((flag & (IDWALK_NO_ORIG_POINTERS_ACCESS | IDWALK_RECURSE)) !=
(IDWALK_NO_ORIG_POINTERS_ACCESS | IDWALK_RECURSE));
if (flag & IDWALK_NO_ORIG_POINTERS_ACCESS) {
flag |= IDWALK_IGNORE_MISSING_OWNER_ID;
}
if (flag & IDWALK_RECURSE) {
/* For now, recursion implies read-only, and no internal pointers. */
flag |= IDWALK_READONLY;
@ -258,12 +260,31 @@ static bool library_foreach_ID_link(Main *bmain,
for (; id != nullptr; id = (flag & IDWALK_RECURSE) ? BLI_LINKSTACK_POP(data.ids_todo) : nullptr)
{
data.self_id = id;
/* Note that we may call this functions sometime directly on an embedded ID, without any
* knowledge of the owner ID then.
* While not great, and that should be probably sanitized at some point, we can live with it
* for now. */
data.owner_id = ((id->flag & LIB_EMBEDDED_DATA) != 0 && owner_id != nullptr) ? owner_id :
data.self_id;
/* owner ID is same as self ID, except for embedded ID case. */
if (id->flag & LIB_EMBEDDED_DATA) {
if (flag & IDWALK_IGNORE_MISSING_OWNER_ID) {
data.owner_id = owner_id ? owner_id : id;
}
else {
/* NOTE: Unfortunately it is not possible to ensure validity of the set owner_id pointer
* here. `foreach_id` is used a lot by code remapping pointers, and in such cases the
* current owner ID of the processed embedded ID is indeed invalid - and the given one is
* to be assumed valid for the purpose of the current process.
*
* In other words, it is the responsibility of the code calling this `foreach_id` process
* to ensure that the given owner ID is valid for its own purpose, or that it is not used.
*/
// BLI_assert(owner_id == nullptr || BKE_id_owner_get(id) == owner_id);
if (!owner_id) {
owner_id = BKE_id_owner_get(id, false);
}
data.owner_id = owner_id;
}
}
else {
BLI_assert(ELEM(owner_id, nullptr, id));
data.owner_id = id;
}
/* inherit_data is non-nullptr when this function is called for some sub-data ID
* (like root node-tree of a material).

2
source/blender/blenkernel/intern/lib_remap.cc
View File

@ -514,7 +514,7 @@ static void libblock_remap_data(
#ifdef DEBUG_PRINT
printf("\tchecking id %s (%p, %p)\n", id->name, id, id->lib);
#endif
id_remap_data.id_owner = id;
id_remap_data.id_owner = (id->flag & LIB_EMBEDDED_DATA) ? BKE_id_owner_get(id) : id;
libblock_remap_data_preprocess(id_remap_data.id_owner, remap_type, id_remapper);
BKE_library_foreach_ID_link(
nullptr, id, foreach_libblock_remap_callback, &id_remap_data, foreach_id_flags);

9
source/blender/depsgraph/intern/eval/deg_eval_copy_on_write.cc
View File

@ -837,11 +837,18 @@ ID *deg_expand_eval_copy_datablock(const Depsgraph *depsgraph, const IDNode *id_
/* Perform remapping of the nodes. */
RemapCallbackUserData user_data = {nullptr};
user_data.depsgraph = depsgraph;
/* About IDWALK flags:
* - #IDWALK_IGNORE_EMBEDDED_ID: In depsgraph embedded IDs are handled (mostly) as regular IDs,
* and processed on their own, not as part of their owner ID (the owner ID's pointer to its
* embedded data is set to null before actual copying, in #id_copy_inplace_no_main).
* - #IDWALK_IGNORE_MISSING_OWNER_ID is necessary for the same reason: when directly processing
* an embedded ID here, its owner is unknown, and its internal owner ID pointer is not yet
* remapped, so it is currently 'invalid'. */
BKE_library_foreach_ID_link(nullptr,
id_cow,
foreach_libblock_remap_callback,
(void *)&user_data,
IDWALK_IGNORE_EMBEDDED_ID);
IDWALK_IGNORE_EMBEDDED_ID | IDWALK_IGNORE_MISSING_OWNER_ID);
/* Correct or tweak some pointers which are not taken care by foreach
* from above. */
update_id_after_copy(depsgraph, id_node, id_orig, id_cow);