LayerCollection: Refactor of resync-with-Collection-hierarchy process.
ClosedPublic
Actions

Authored by Bastien Montagne (mont29) on Jul 23 2021, 6:43 PM.

Details

Reviewers

Brecht Van Lommel (brecht)
Campbell Barton (campbellbarton)

Commits

rB3db37075f699: Remove the code in `BKE_collection_move` to preserve LayerCollection flags.
rBb18d0244fc07: LayerCollection: Refactor of resync-with-Collection-hierarchy process.

Summary

The goal of this refactor is to improve resync of LayerCollections
hierarchy to match again Collection one.

Current code would destroy and re-create valid layers whenever a parent
collection would be removed, which leads to losing way too often
layer-related settings when editing collection hierarchies.

While this could be partially addressed from operators side, there was
no way to fix those issues from lower level, more generic ID management
code like ID remapping or library override resync processes.

The new code builds a shallow wrapper around existing (aka old) layers
hierarchy, does a set of checks to define the status of all existing
layers, and try to find the closest matching unused layer in cases where
layers and collections hierarchies do not match anymore.

The intent is to both re-use as much as possible existing layers, and
to pick the 'best' possible layer to re-use, following those heuristics:

Prefer layers children of current one first (in old hierarchy), and only use those from other higher-level hierarchies in no (grand-)child is found.
Prefer to use closest layers available in the old hierarchy.

NOTE: The new code is about 12%-15% slower than the previous one, which is expected given the increased complexity. Note that this would not be an issue in practice if this code were not called way too often (needs to be converted to lazy update instead, which is a long known TODO).

NOTE: The LayerCollectionResync code uses its own built-in version of FIFO queue, as performances in this code is currently a critical point (it can get called tens of thousands of times during a single (heavy) ID management operation currently, in a production file e.g.).

This commit also removes the dedicated code in BKE_collection_move to
preserve LayerCollection flags. This code was actually forcefully
re-enabling excluded layers in some cases...

NOTE: The removal of BKE_collection_move code will be comitted as its own separate commit.

Diff Detail

Repository: rB Blender

Event Timeline

Bastien Montagne (mont29) requested review of this revision.Jul 23 2021, 6:43 PM

Bastien Montagne (mont29) created this revision.

Harbormaster completed remote builds in B15998: Diff 39884.Jul 23 2021, 6:43 PM

NOTE: the goal of this patch is essentially the same as D9599: Preserve Layer Collections During Sync. However, proposed solution here is expected to better respect hierarchy changes, by re-using the closest 'available' layer to to point of breakage, instead of just the first one available in the whole ViewLayer.

Great to see this tackled. The logic makes sense to me, I could not spot issues in that.

I get a crash opening older .blend files with this patch. For example lib/tests/render/denoise/denoise_hair.blend, but anything from before 2.80 I guess. Backtrace: P2289.

source/blender/blenkernel/intern/layer.c
763–767	Replace `i.e.` with `:` "i.e." means that the statement is not precisely accurate and there is some detail being omitted, which is not the case.
770	prents -> parents
834	euristics -> heuristics

This revision now requires changes to proceed.Jul 26 2021, 4:18 PM

Updates from review, fix crash with older files.

Harbormaster completed remote builds in B16054: Diff 39996.Jul 27 2021, 11:51 AM

Thanks, for some reason I assumed do_version would handle adding initial LayerCollections...

source/blender/blenkernel/intern/layer.c
763–767	Not sure I agree with that understanding of `i.e.`? It's just a shortcut for `that is (to say)`, it means we are going to add some precision to the first part of the sentence. https://en.wiktionary.org/wiki/i.e.