Details

Reviewers

Arun Sojan Parolikkal (actuday6418)
Brecht Van Lommel (brecht)

Maniphest Tasks

T76346: Local collection visibility doesn't update when an item is moved between collections

Commits

rBa5db981b0ea0: Fix T76346: Moving objects in outliner doesn't update local collections

Summary

The fix involves going over ALL the possible combinations of viewlayers and viewports and re-sync.
I tested this with multiple windows, multiple scenes and multiple viewlayers.

Since (for now?) the operation of syncing the local layer collections is not too expensive, this is not so bad. In theory we could improve this by checking if the collection the object was moved to and from is in the scene before iterating over it. I don't think it is worthy though.

Diff Detail

Repository

rB Blender

Branch

fix-local-collection-sync (branched from master)

Build Status

Buildable 10082
Build 10082: arc lint + arc unit

Event Timeline

Arun Sojan Parolikkal (actuday6418) requested review of this revision.Jul 18 2020, 10:58 AM

Arun Sojan Parolikkal (actuday6418) created this revision.

Harbormaster completed remote builds in B9096: Diff 26949.Jul 18 2020, 10:58 AM

Set flags only if the collections are edited
Remove unnecessary updates and flags.

Harbormaster completed remote builds in B9097: Diff 26950.Jul 18 2020, 1:13 PM

Ankit Meel (ankitm) retitled this revision from Commit solves T76346. to Fix T76346: Moving objects in outlines doesn't update local collections.Jul 18 2020, 1:14 PM

Ankit Meel (ankitm) edited the summary of this revision. (Show Details)

Ankit Meel (ankitm) added a task: T76346: Local collection visibility doesn't update when an item is moved between collections.

Ankit Meel (ankitm) retitled this revision from Fix T76346: Moving objects in outlines doesn't update local collections to Fix T76346: Moving objects in outliner doesn't update local collections.

Arun Sojan Parolikkal (actuday6418) abandoned this revision.Jul 18 2020, 1:22 PM

Arun Sojan Parolikkal (actuday6418) reclaimed this revision.Jul 18 2020, 1:26 PM

Ankit Meel (ankitm) added a reviewer: Dalai Felinto (dfelinto).Jul 18 2020, 10:39 PM

Hi Arun, thanks for the patch. However the patch is not the way to go.

There is an advanced scenario where a collection can be linked twice in the the same view layer, which your patch wouldn't be able to handle (it would enable all of them):

multi-collection.blend99 KBDownload

Besides, the performance impact in this approach is really bad. We shouldn't need to run BKE_layer_collection_sync twice.

This revision now requires changes to proceed.Jul 21 2020, 12:45 PM

This fix, as far as my limited understanding goes, iterates through each 3D viewport, and syncs the visibility of local collections.
Hello Dalai Felinto, I'm not entirely sure I understand the scenario you mentioned about collections being linked twice in the same view layer. I did take the blend file you graciously provided out for a spin, but didn't notice anything that seemed unexpected.
As such, I couldn't really check if this patch has got the same issue.

Harbormaster completed remote builds in B9259: Diff 27287.Jul 30 2020, 1:28 PM

Ankit Meel (ankitm) added inline comments.Jul 30 2020, 1:32 PM

source/blender/blenkernel/intern/collection.c
1119 ↗	(On Diff #27287)	Use `LISTBASE_FOREACH`. Same for wmWindow *window = wm->windows.first; window != NULL; window = window->next

Ankit Meel (ankitm) added inline comments.Jul 30 2020, 1:36 PM

source/blender/blenkernel/intern/collection.c
33 ↗	(On Diff #27287)	Run `make format` or use extensions (VSCode, Sublime Text) for `clang-format`. Alphabetical order for headers. Same for #include "DNA_windowmanager_types.h" https://wiki.blender.org/wiki/Tools/ClangFormat

Changed

for

loops to use

LISTBASE_FOREACH

instead

Harbormaster completed remote builds in B9260: Diff 27288.Jul 30 2020, 2:02 PM

Arun Sojan Parolikkal (actuday6418) marked an inline comment as done.Jul 30 2020, 2:02 PM

Ran clang-format on

collection.c

Harbormaster completed remote builds in B9262: Diff 27290.Jul 30 2020, 2:23 PM

Arun Sojan Parolikkal (actuday6418) marked an inline comment as done.Jul 30 2020, 2:30 PM

Ankit Meel (ankitm) awarded a token.Jul 30 2020, 2:31 PM

Dalai Felinto (dfelinto) added inline comments.Aug 13 2020, 5:16 PM

source/blender/blenkernel/intern/collection.c
1115 ↗	(On Diff #27290)	The first view layer is not special, it may not be the active one

update to D8342: Fix T76346: Moving objects in outliner doesn't update local collections

Only the active view layer is affected now.

Harbormaster completed remote builds in B9542: Diff 27759.Aug 15 2020, 8:59 PM

Arun Sojan Parolikkal (actuday6418) marked an inline comment as done.Aug 16 2020, 8:46 AM

Proper fix for local layer collection syncing

Harbormaster completed remote builds in B10075: Diff 28676.Sep 9 2020, 12:08 PM

Dalai Felinto (dfelinto) commandeered this revision.Sep 9 2020, 12:10 PM

Dalai Felinto (dfelinto) edited the summary of this revision. (Show Details)

Dalai Felinto (dfelinto) edited reviewers, added: Arun Sojan Parolikkal (actuday6418); removed: Dalai Felinto (dfelinto).

Dalai Felinto (dfelinto) edited reviewers, added: Brecht Van Lommel (brecht); removed: Dalai Felinto (dfelinto).

I commandeered the patch to do the required changes. The original patch was failing in too many cases. Thanks for the original patch though Arun. It was a good start point.

This doesn't seem like the right place to put this. Why only in BKE_collection_object_move, and not e.g. BKE_collection_object_add? Also note that BKE_collection_object_move already relies on BKE_collection_object_add and BKE_collection_object_remove to call BKE_main_collection_sync.

Maybe BKE_main_collection_sync should just call BKE_layer_collection_local_sync_all always?

This revision now requires changes to proceed.Sep 9 2020, 3:03 PM

addressing review suggestion

Note: I can find a few cases where collections get out of sync, but are not related to the original issue.
For example:

Two scenes, same collections linked to their respective viewlayers
link and object to a collection in a scene, it doesn't get synced to the other

Harbormaster completed remote builds in B10082: Diff 28690.Sep 9 2020, 4:28 PM

Brecht Van Lommel (brecht) accepted this revision.Sep 9 2020, 4:43 PM

This revision is now accepted and ready to land.Sep 9 2020, 4:43 PM

Dalai Felinto (dfelinto) mentioned this in rBa5db981b0ea0: Fix T76346: Moving objects in outliner doesn't update local collections.Sep 9 2020, 4:58 PM

rBa5db981b0ea044313239c3cc2ee92d19a8f682ea

Fix T76346: Moving objects in outliner doesn't update local collections
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 28690

source/blender/blenkernel/BKE_layer.h

source/blender/blenkernel/intern/layer.c

Fix T76346: Moving objects in outliner doesn't update local collectionsClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 28690

source/blender/blenkernel/BKE_layer.h

source/blender/blenkernel/intern/layer.c

Fix T76346: Moving objects in outliner doesn't update local collections
ClosedPublic
Actions

Revision Contents
Changeset List