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

New Issue

Dirk Wallenstein · 2020-11-26T13:19:31+01:00

Dirk Wallenstein commented

2020-11-26 13:19:31 +01:00

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.

minimal.blend

**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. [minimal.blend](https://archive.blender.org/developer/F9380526/minimal.blend)

Dirk Wallenstein commented

2020-11-26 13:19:31 +01:00

Added subscriber: @d-egg

Robert Guetzkow commented

2020-11-26 14:05:49 +01:00

Added subscribers: @HooglyBoogly, @rjg

Robert Guetzkow commented

2020-11-26 14:05:49 +01:00

Changed status from 'Needs Triage' to: 'Confirmed'

Robert Guetzkow commented

2020-11-26 14:05:49 +01:00

I can confirm this crash. @HooglyBoogly This seems very similar to #82439 that was supposed to be fixed by c645da98d8.

_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

I can confirm this crash. @HooglyBoogly This seems very similar to #82439 that was supposed to be fixed by c645da98d8. ``` _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 ```

Raimund Klink commented

2020-11-26 14:55:37 +01:00

Added subscriber: @Raimund58

Hans Goudey self-assigned this 2020-11-26 14:59:50 +01:00

Hans Goudey commented

2020-11-26 14:59:50 +01:00

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

Joachim Przibyllla commented

2020-11-26 18:09:45 +01:00

Added subscriber: @Blender_Render

Bastien Montagne commented

2020-12-04 16:23:07 +01:00

Added subscriber: @mont29

Bastien Montagne commented

2020-12-04 16:23:07 +01:00

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

Hans Goudey commented

2020-12-04 20:33:42 +01:00

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

Hans Goudey commented

2020-12-07 01:54:13 +01:00

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 Move	Move	After 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);

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 Move | Move | After Move | | -- | -- | -- | | ![image.png](https://archive.blender.org/developer/F9482804/image.png) | ![Screenshot from 2020-12-06 18-49-49.png](https://archive.blender.org/developer/F9482847/Screenshot_from_2020-12-06_18-49-49.png) | ![image.png](https://archive.blender.org/developer/F9482856/image.png) | | ![image.png](https://archive.blender.org/developer/F9482806/image.png) | | ![image.png](https://archive.blender.org/developer/F9482861/image.png) | 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. ```lines=10 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); ```

Hans Goudey commented

2020-12-07 02:01:35 +01:00

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.

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.

Bastien Montagne commented

2020-12-08 10:09:32 +01:00

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?

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?

Hans Goudey commented

2020-12-08 16:00:49 +01:00

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