Page MenuHome

Dragging a sub-collection out of a scene-linked collection into the "Scene Collection" crashes reliably
Closed, ResolvedPublicBUG

Description

System Information
Operating system: Kubuntu 20.04
Graphics card: nvidia TU106 GeForce RTX 2060 SUPER

Blender Version
Broken: 2.91 flatpak on Kubuntu
Worked: 2.82 (I guess 2.90 too)

Short description of error

Dragging a sub-collection out of a scene-linked collection into the "Scene Collection" crashes reliably.

This happens only in the original scene, not in the new scene.

Exact steps for others to reproduce the error

  • Factory startup
  • create a sub-collection ("collection 1") in the default collection
  • create a linked copy of the scene
  • return to the first scene
  • drag and drop collection 1 into the scene collection

In the blend, only the last step.

Event Timeline

Robert Guetzkow (rjg) changed the task status from Needs Triage to Confirmed.Nov 26 2020, 2:05 PM
Robert Guetzkow (rjg) triaged this task as High priority.

I can confirm this crash. @Hans Goudey (HooglyBoogly) This seems very similar to T82439 that was supposed to be fixed by rBc645da98d8727e636c14383f4c10706c6cc5ead2.

_BLI_assert_abort() Line 51	C
layer_collection_flags_restore(ListBase * flags, const Collection * collection) Line 1818	C
BKE_collection_move(Main * bmain, Collection * to_parent, Collection * from_parent, Collection * relative, bool relative_after, Collection * collection) Line 1886	C
collection_drop_invoke(bContext * C, wmOperator * UNUSED_op, const wmEvent * event) Line 1290	C
wm_operator_invoke(bContext * C, wmOperatorType * ot, wmEvent * event, PointerRNA * properties, ReportList * reports, const bool poll_only, bool use_last_properties) Line 1296	C
wm_operator_call_internal(bContext * C, wmOperatorType * ot, PointerRNA * properties, ReportList * reports, const short context, const bool poll_only, wmEvent * event) Line 1542	C
wm_handlers_do_intern(bContext * C, wmEvent * event, ListBase * handlers) Line 2789	C
wm_handlers_do(bContext * C, wmEvent * event, ListBase * handlers) Line 2872	C
wm_event_do_handlers(bContext * C) Line 3368	C
WM_main(bContext * C) Line 638	C
main(int argc, const unsigned char * * UNUSED_argv_c) Line 526	C

Oh no.. I can reproduce this too. Should have released with a simple NULL check after that assert. Thanks for the report anyway.

Bastien Montagne (mont29) changed the subtype of this task from "Report" to "Bug".Nov 26 2020, 4:02 PM
Bastien Montagne (mont29) moved this task from Backlog to bcon3: Bugs on the BF Blender (2.91) board.

@Hans Goudey (HooglyBoogly) are you on this? that kind of issues should be fixed sooner than later...

@Bastien Montagne (mont29) Yes, sorry for the delay, I'll get to this today.

So I've discovered why this happens, it's basically because moving collections that are also in linked scenes just doesn't work properly. Or at least that's how it seems, maybe that's expected.

Here's a short summary. The second scene, "Scene.001", was created with the "Linked Copy" option:

Before MoveMoveAfter Move

The following patch just removes the assert and the NULL de-referencing in this situation. In the scope of this situation I think it's the right thing to do, the whole behavior of moving collections when there are linked scenes is far in "undefined behavior" territory for me.

diff --git a/source/blender/blenkernel/intern/collection.c b/source/blender/blenkernel/intern/collection.c
index 5eec788255d..69fd682203f 100644
--- a/source/blender/blenkernel/intern/collection.c
+++ b/source/blender/blenkernel/intern/collection.c
@@ -1773,6 +1773,15 @@ static void layer_collection_flags_store(Main *bmain,
   }
 }
 
+static void layer_collection_flags_free_recursive(LayerCollectionFlag *flag)
+{
+  LISTBASE_FOREACH (LayerCollectionFlag *, child, &flag->children) {
+    layer_collection_flags_free_recursive(child);
+  }
+
+  BLI_freelistN(&flag->children);
+}
+
 static void layer_collection_flags_restore_recursive(LayerCollection *layer_collection,
                                                      LayerCollectionFlag *flag)
 {
@@ -1788,7 +1797,6 @@ static void layer_collection_flags_restore_recursive(LayerCollection *layer_coll
 
     child_flag = child_flag->next;
   }
-  BLI_freelistN(&flag->children);
 
   /* We treat exclude as a special case.
    *
@@ -1814,10 +1822,13 @@ static void layer_collection_flags_restore(ListBase *flags, const Collection *co
 
     LayerCollection *layer_collection = BKE_layer_collection_first_from_scene_collection(
         view_layer, collection);
-    /* The flags should only be added if the collection is in the view layer. */
-    BLI_assert(layer_collection != NULL);
-
-    layer_collection_flags_restore_recursive(layer_collection, flag);
+    /* The flags should only be added if the collection is the scene in the first place, which
+     * means that it should also be in the view layer. However, in "Linked Copy" scenes where the
+     * moved collection is completely removed from the heirarchy. */
+    if (layer_collection != NULL) {
+      layer_collection_flags_restore_recursive(layer_collection, flag);
+    }
+    layer_collection_flags_free_recursive(flag);
   }
 
   BLI_freelistN(flags);

Actually, when I wrote that last comment I forgot about they key information that this only happens when moving to the scene collection, moving the collection anywhere else works fine. In this case I think it actually makes sense to look into fixing that instead.

Maybe that patch makes sense for a possible 2.91 corrective release though, since it may be simpler.

Please keep 'linked'/'link' terminology for actually linked data from libraries. 'Linked copy' is a bit unfortunate naming we inherit from history, but resulting data is not linked at all, it's just sharing some data-blocks between origin and copy items (scene or any other ID), that's all.

Behavior described in pictures looks correct and expected to me, since even when two scenes share their sub-collections, their main one (the master collection) is always local to each scene and never shared.

Can you submit your patch as a proper review, so that we can comment on it?

Hmm, thanks for the explanation. I wonder what a better name for "Linked Copy" would be.