[Bf-blender-cvs] [9afa7385428] blender-v2.93-release: Fix bug/crash in ID bulk deletion code.
Bastien Montagne
noreply at git.blender.org
Fri Apr 23 14:34:57 CEST 2021
Commit: 9afa738542881cf69aa89fd893351200324c107f
Author: Bastien Montagne
Date: Fri Apr 23 12:28:54 2021 +0200
Branches: blender-v2.93-release
https://developer.blender.org/rB9afa738542881cf69aa89fd893351200324c107f
Fix bug/crash in ID bulk deletion code.
This is complex situation. Tagged ID deletion (used to delete several
data-blocks at once) removes IDs to be deleted from Main.
But when we remove deleted IDs' usages of other IDs (using
`BKE_libblock_relink_ex`), some specific post-process is required on
some types, like Collections. Those post-processes would in some cases
rely on data actually being in Main.
That failing condition would lead in existing code on missing processing
the very ID (collection) we were working on, leading to missing removing
some child collection pointers, leading to the crash (later on in
LayerCollection resync process).
For now we go with an optimization & fix that avoids processing all
collections in Main when we actually know which one we are working one
(case of `BKE_libblock_relink_ex`, but not of
`BKE_libblock_remap_locked`).
This is however yet another demonstration of the need to rework that
whole collection/layer resync process, since it is not only extremely
inneficient currently, but it also requires valid Main/ID state way too
deep into the remapping code.
NOTE: This fix may very well not catch/address all possible fail cases
here, dealing with the double parent/child relationships of collections
is challenging...
Issue reported by @eyecandy from the studio, thanks.
===================================================================
M source/blender/blenkernel/BKE_collection.h
M source/blender/blenkernel/intern/collection.c
M source/blender/blenkernel/intern/lib_remap.c
===================================================================
diff --git a/source/blender/blenkernel/BKE_collection.h b/source/blender/blenkernel/BKE_collection.h
index 3412be92a3a..7963d54126e 100644
--- a/source/blender/blenkernel/BKE_collection.h
+++ b/source/blender/blenkernel/BKE_collection.h
@@ -112,7 +112,9 @@ bool BKE_scene_collections_object_remove(struct Main *bmain,
struct Object *object,
const bool free_us);
void BKE_collections_object_remove_nulls(struct Main *bmain);
-void BKE_collections_child_remove_nulls(struct Main *bmain, struct Collection *old_collection);
+void BKE_collections_child_remove_nulls(struct Main *bmain,
+ struct Collection *parent_collection,
+ struct Collection *child_collection);
/* Dependencies. */
diff --git a/source/blender/blenkernel/intern/collection.c b/source/blender/blenkernel/intern/collection.c
index 89bb7b36406..b29dd007c51 100644
--- a/source/blender/blenkernel/intern/collection.c
+++ b/source/blender/blenkernel/intern/collection.c
@@ -1302,41 +1302,50 @@ static void collection_missing_parents_remove(Collection *collection)
*
* \note caller must ensure #BKE_main_collection_sync_remap() is called afterwards!
*
- * \param collection: may be \a NULL,
+ * \param parent_collection: The collection owning the pointers that were remapped. May be \a NULL,
+ * in which case whole \a bmain database of collections is checked.
+ * \param child_collection: The collection that was remapped to another pointer. May be \a NULL,
* in which case whole \a bmain database of collections is checked.
*/
-void BKE_collections_child_remove_nulls(Main *bmain, Collection *collection)
-{
- if (collection == NULL) {
- /* We need to do the checks in two steps when more than one collection may be involved,
- * otherwise we can miss some cases...
- * Also, master collections are not in bmain, so we also need to loop over scenes.
- */
- for (collection = bmain->collections.first; collection != NULL;
- collection = collection->id.next) {
- collection_null_children_remove(collection);
- }
- for (Scene *scene = bmain->scenes.first; scene != NULL; scene = scene->id.next) {
- collection_null_children_remove(scene->master_collection);
+void BKE_collections_child_remove_nulls(Main *bmain,
+ Collection *parent_collection,
+ Collection *child_collection)
+{
+ if (child_collection == NULL) {
+ if (parent_collection != NULL) {
+ collection_null_children_remove(parent_collection);
+ }
+ else {
+ /* We need to do the checks in two steps when more than one collection may be involved,
+ * otherwise we can miss some cases...
+ * Also, master collections are not in bmain, so we also need to loop over scenes.
+ */
+ for (child_collection = bmain->collections.first; child_collection != NULL;
+ child_collection = child_collection->id.next) {
+ collection_null_children_remove(child_collection);
+ }
+ for (Scene *scene = bmain->scenes.first; scene != NULL; scene = scene->id.next) {
+ collection_null_children_remove(scene->master_collection);
+ }
}
- for (collection = bmain->collections.first; collection != NULL;
- collection = collection->id.next) {
- collection_missing_parents_remove(collection);
+ for (child_collection = bmain->collections.first; child_collection != NULL;
+ child_collection = child_collection->id.next) {
+ collection_missing_parents_remove(child_collection);
}
for (Scene *scene = bmain->scenes.first; scene != NULL; scene = scene->id.next) {
collection_missing_parents_remove(scene->master_collection);
}
}
else {
- for (CollectionParent *parent = collection->parents.first, *parent_next; parent;
+ for (CollectionParent *parent = child_collection->parents.first, *parent_next; parent;
parent = parent_next) {
parent_next = parent->next;
collection_null_children_remove(parent->collection);
- if (!collection_find_child(parent->collection, collection)) {
- BLI_freelinkN(&collection->parents, parent);
+ if (!collection_find_child(parent->collection, child_collection)) {
+ BLI_freelinkN(&child_collection->parents, parent);
}
}
}
diff --git a/source/blender/blenkernel/intern/lib_remap.c b/source/blender/blenkernel/intern/lib_remap.c
index 1f597bbb9a6..b32b97dc250 100644
--- a/source/blender/blenkernel/intern/lib_remap.c
+++ b/source/blender/blenkernel/intern/lib_remap.c
@@ -303,6 +303,7 @@ static void libblock_remap_data_postprocess_object_update(Main *bmain,
/* Can be called with both old_collection and new_collection being NULL,
* this means we have to check whole Main database then. */
static void libblock_remap_data_postprocess_collection_update(Main *bmain,
+ Collection *owner_collection,
Collection *UNUSED(old_collection),
Collection *new_collection)
{
@@ -311,7 +312,7 @@ static void libblock_remap_data_postprocess_collection_update(Main *bmain,
* and BKE_main_collection_sync_remap() does not tolerate any of those, so for now always check
* whole existing collections for NULL pointers.
* I'd consider optimizing that whole collection remapping process a TODO for later. */
- BKE_collections_child_remove_nulls(bmain, NULL /*old_collection*/);
+ BKE_collections_child_remove_nulls(bmain, owner_collection, NULL /*old_collection*/);
}
else {
/* Temp safe fix, but a "tad" brute force... We should probably be able to use parents from
@@ -523,7 +524,7 @@ void BKE_libblock_remap_locked(Main *bmain, void *old_idv, void *new_idv, const
break;
case ID_GR:
libblock_remap_data_postprocess_collection_update(
- bmain, (Collection *)old_id, (Collection *)new_id);
+ bmain, NULL, (Collection *)old_id, (Collection *)new_id);
break;
case ID_ME:
case ID_CU:
@@ -628,6 +629,12 @@ void BKE_libblock_relink_ex(
switch (GS(id->name)) {
case ID_SCE:
case ID_GR: {
+ /* Note: here we know which collection we have affected, so at lest for NULL children
+ * detection we can only process that one.
+ * This is also a required fix in case `id` would not be in Main anymore, which can happen
+ * e.g. when called from `id_delete`. */
+ Collection *owner_collection = (GS(id->name) == ID_GR) ? (Collection *)id :
+ ((Scene *)id)->master_collection;
if (old_id) {
switch (GS(old_id->name)) {
case ID_OB:
@@ -636,7 +643,7 @@ void BKE_libblock_relink_ex(
break;
case ID_GR:
libblock_remap_data_postprocess_collection_update(
- bmain, (Collection *)old_id, (Collection *)new_id);
+ bmain, owner_collection, (Collection *)old_id, (Collection *)new_id);
break;
default:
break;
@@ -644,7 +651,7 @@ void BKE_libblock_relink_ex(
}
else {
/* No choice but to check whole objects/collections. */
- libblock_remap_data_postprocess_collection_update(bmain, NULL, NULL);
+ libblock_remap_data_postprocess_collection_update(bmain, owner_collection, NULL, NULL);
libblock_remap_data_postprocess_object_update(bmain, NULL, NULL);
}
break;
More information about the Bf-blender-cvs
mailing list