Details

Reviewers

Maniphest Tasks

T81545: Organizing Nested Collections Activates Deactivated Children

Commits

rB89491d67d96f: Fix T81545: Moving nested collections changes view layer flags

Summary

The code that restored collection flags after moving them didn't take into account
collection children. So the flag for the active collection was properly restored,
but all of its children would take on the exclude flag of the collection the active
collection was dragged into.

This fix works in my test cases, but I will say that I don't have a very strong
conception of the context of these changes, so they might be the wrong way to
approach this problem.

Diff Detail

Repository: rB Blender

Event Timeline

Hans Goudey (HooglyBoogly) requested review of this revision.Oct 9 2020, 12:10 AM

Hans Goudey (HooglyBoogly) created this revision.

Harbormaster completed remote builds in B10642: Diff 29762.Oct 9 2020, 12:10 AM

Needs more testing

Some cleanup and a fix

Harbormaster completed remote builds in B10645: Diff 29766.Oct 9 2020, 4:36 AM

Generally looks fine, but I think you can improve it on two aspects:

Get rid of that GHash usage, it makes no sense to use that when we are only iterating over its content?
Tweak a bit things and add some asserts to ensure everything is going on as expected.

See comments below for details (hope this is clear enough).

Note that I suggest to put the ViewLayer pointer in the LayerCollectionFlag struct to avoid having to add another struct for the first level of the tree (pointer is just left to NULL for sub-levels then). We could also have two structs here, one for first level with only ViewLayer pointer and list of children… If we keep everything in one struct, both 'sections' should be clearly defined by comments.

source/blender/blenkernel/intern/collection.c
1547	I would also store the `Collection *` pointer itself here, that way you can `BLI_assert` it matches the `layer_collection->collection` one in restore step.
1547	Also, I would add here the `ViewLayer *` pointer (see other comments about the why).
1551	`layer_collection_flags_store_recursive`
1555	Also store here the `Collection` pointer from the `layer_collection` into the `flag`.
1587	Replace this with inserting into a listbase of 'first level' `LayerCollectionFlag` items, that also store the `ViewLayer` pointer.
1592	`layer_collection_flags_restore_recursive`
1595	Here you can assert that `layer_collection->collection == flag->collection`.
1601	Free the listbase item properly: `BLI_freelinkN(&flag->children, child_flag);`
1604	Now here you can assert that `BLI_listbase_is_empty(&flag->children)`.
1614–1615	This is not needed anymore.
1624–1625	To be replaced by `LISTBASE_FOREACH_MUTABLE` over the top `ListBase` of `LayerCollectionFlag`, that now also store their `ViewLayer` pointer.
1692–1693	Replace this by a `ListBase` of top-level `LayerCollectionFlag` storing a pointer to matching `Viewlayer`.
1699–1700	To be replaced by `BLI_freelistN` call.

This revision now requires changes to proceed.Oct 9 2020, 11:54 AM

Remove unecessary use of GHash
Add assert for the correct collection
Rename functions
Change the way the flag structs are freed
Add asserts and const where appropriate

Harbormaster completed remote builds in B10661: Diff 29798.Oct 9 2020, 5:48 PM

Hans Goudey (HooglyBoogly) added inline comments.Oct 9 2020, 5:48 PM

source/blender/blenkernel/intern/collection.c
1601	I thought it made a bit more sense to do it all in one call at the end of the loop, that way it doesn't need to be a mutable loop. I can still do it this way if you like, I think the way it is now is a bit cleaner though.

Thanks for the detailed review! This should be better, I only differed from your comments in one small place.

Bastien Montagne (mont29) added inline comments.Oct 9 2020, 6:00 PM

source/blender/blenkernel/intern/collection.c
1601	The point of doing it item by item here is that you can then assert (once all children `LayerCollection` have been processed in upper level) that you also have processed all of the stored `LayerCollectionFlag`. This gives some basic check against potential discrepancy between the two lists… If you prefer, you can also assert that they have the same length (`BLI_listbase_count()`) before starting iterating on them, might actually be clearer (and we do not really care about some extra looping on those, especially if only for debug builds).

Add assert that child lists have equal count

Hans Goudey (HooglyBoogly) marked an inline comment as done.Oct 9 2020, 6:49 PM

Hans Goudey (HooglyBoogly) added inline comments.

source/blender/blenkernel/intern/collection.c
1601	Okay, yes, that seems clearer to me, I'll add that assert.

Harbormaster completed remote builds in B10663: Diff 29800.Oct 9 2020, 6:49 PM

Almost there, mostly minor picky details in comments below.

source/blender/blenkernel/intern/collection.c
1575	I'd rather have the listbase created in calling code and passed around as a pointer…
1615	Would move that assert at beginning of the function, with the other one.
1639	`isn't`
1641–1643	You assert'ed on that, no need to check it anymore.
1701	should go at the end of `layer_collection_flags_restore`

This revision now requires changes to proceed.Oct 9 2020, 7:33 PM

Review updates

Hans Goudey (HooglyBoogly) marked 4 inline comments as done.Oct 9 2020, 7:51 PM

Hans Goudey (HooglyBoogly) added inline comments.

source/blender/blenkernel/intern/collection.c
1701	I don't think it can though, it still needs to be in the recursive function so that children of children would be freed.

Harbormaster completed remote builds in B10664: Diff 29802.Oct 9 2020, 7:51 PM

Bastien Montagne (mont29) added inline comments.Oct 9 2020, 9:14 PM

source/blender/blenkernel/intern/collection.c
1701	Those have already been freed inf the `_restore_recursive` function… Iḿ talking about that call freeing the main 'viewlayers´ list.

Hans Goudey (HooglyBoogly) added inline comments.Oct 9 2020, 9:26 PM

source/blender/blenkernel/intern/collection.c
1701	Oh sorry, I lost my place and didn't realize which part of this comment applied to.

Move freeing the top level list

Harbormaster completed remote builds in B10665: Diff 29803.Oct 9 2020, 9:27 PM

Hans Goudey (HooglyBoogly) marked 2 inline comments as done.Oct 9 2020, 9:27 PM

Forgot to switch variable name when I moved it!

Harbormaster completed remote builds in B10666: Diff 29804.Oct 9 2020, 9:55 PM

Besides note below, think it's ready now

source/blender/blenkernel/intern/collection.c
1635	Doesn't need to be mutable? You are not removing any item from that collection, but frees it all at once at the end after looping on it.

This revision is now accepted and ready to land.Oct 9 2020, 10:18 PM

Thanks again for the quick review.

source/blender/blenkernel/intern/collection.c
1635	Oops, no it does not!

Closed by commit rB89491d67d96f: Fix T81545: Moving nested collections changes view layer flags (authored by Hans Goudey (HooglyBoogly)). · Explain WhyOct 9 2020, 11:00 PM

This revision was automatically updated to reflect the committed changes.

Hans Goudey (HooglyBoogly) marked an inline comment as done.

Hans Goudey (HooglyBoogly) added a commit: rB89491d67d96f: Fix T81545: Moving nested collections changes view layer flags.

Hans Goudey (HooglyBoogly) mentioned this in D9568: Fix T82439: Crash moving collections between scenes.Nov 14 2020, 8:28 PM

Hans Goudey (HooglyBoogly) mentioned this in rBc645da98d872: Fix T82439: Crash moving collections between scenes.Nov 16 2020, 5:01 PM

Fix T81545: Organizing Nested Collections Activates Deactivated Children
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 29805

source/blender/blenkernel/intern/collection.c

Fix T81545: Organizing Nested Collections Activates Deactivated ChildrenClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 29805

source/blender/blenkernel/intern/collection.c

Fix T81545: Organizing Nested Collections Activates Deactivated Children
ClosedPublic
Actions

Revision Contents
Changeset List