Details

Reviewers

Bastien Montagne (mont29)
Brecht Van Lommel (brecht)
Dalai Felinto (dfelinto)

Summary

Currently layer collections are discarded and recreated when collections are moved to another listbase or when their parents are deleted. This results in extraneous code being required in the move function to handle flags and loss of flag data when parents are deleted. This also prevents storing any kind of persistent data in layer collections in the future, i.e. converting them to ID properties.

To solve this problem I have added code to detect invalid layer collections before syncing and then handle them by either deleting layer collections whose corresponding collections have been deleted, or relinking the invalid collections during the sync. Any layer collections that are invalid but did not get relinked during the sync are assumed to have been unlinked, and are deleted. This allows all layer collections to be preserved, including the layer collections of linked collections, and eliminates the need for workarounds to preserve data.

All other, simpler, attempts at perserving layer collections via syncing, that I could think of, did not work. The only other potential alternative solution for this that I can think of would be to
merge layer collections with collections and store view layer specific data within a hashtable that uses view layers as keys, but this would be a far more extensive change and would cause python api breakage. Barring problems I'm not aware of, this seems like it could be a better long term solution for the future.

Limitations include increased code complexity.

Diff Detail

Repository

rB Blender

Branch

preserve_layer_collections_attempt_2 (branched from master)

Build Status

Buildable 16867
Build 16867: arc lint + arc unit

Event Timeline

Ryan Inch (Imaginer) requested review of this revision.Nov 19 2020, 5:05 AM

Ryan Inch (Imaginer) created this revision.

Harbormaster completed remote builds in B11335: Diff 31204.Nov 19 2020, 5:05 AM

Ryan Inch (Imaginer) edited the summary of this revision. (Show Details)Nov 19 2020, 5:12 AM

Ryan Inch (Imaginer) added reviewers: Bastien Montagne (mont29), Brecht Van Lommel (brecht), Dalai Felinto (dfelinto).Nov 19 2020, 5:15 AM

Ryan Inch (Imaginer) added a project: Pipeline, Assets & I/O.

Ryan Inch (Imaginer) mentioned this in D9146: Attempt at Preserving Layer Collections During Sync.Nov 19 2020, 5:18 AM

Merge branch 'master' into preserve_layer_collections_attempt_2

Harbormaster completed remote builds in B12255: Diff 32884.Jan 18 2021, 10:47 PM

Here's a good blendfile for testing deletion of linked collections:

LinkTest_Master_Gen_Custom_Names.blend758 KBDownload

Hey, thanks for the update, just a note that this is not forgotten and still on my radar (unfortunately I have a lot on my plate currently :( )

You're welcome. It's good to know this hasn't been forgotten.

I don't really have a comment on the general idea, though I wouldn't be surprised if this is a better solution than the flag syncing.

Some smaller comments about else statements here.

source/blender/blenkernel/intern/layer.c
800	No need for `else` here since the `if` ends with `return`.
878–879	Personally I'd rather not have the empty space before the `else`, I think it makes the flow harder to read. I'd prefer it like this, which is also the way I usually see it in Blender: if (...) { ... } else { /* Everything is in sync. */ ... } This comment would apply elsewhere in the patch too.
1045–1049	Here you do an operation in the if statement to check which case to use, and then inside the if statement you do the operation again. Same with the next else if and `retrieve_invalid_layer_collection`. Without looking into it in more detail, can guess a couple ways to improve this: Split these cases to a separate function, return early in the case to avoid needing the `else`. Or use continue Declare the variables before the `if` and assign them in the `else if` statement Something else?

Thanks for the review @Hans Goudey (HooglyBoogly) :)
I definitely prefer preserving layer collections to flag syncing, plus it fixes a bug and lays the groundwork for conversion to ID Properties :)

source/blender/blenkernel/intern/layer.c
800	I did that to be more explicit, this is fairly complex code and I wanted to make it as clear as possible, but if you don't think it helps readability, I'll change it.
878–879	Ah okay. If that's Blender's style then I'll use that.
1045–1049	Yes, this was a redundancy I identified, but chose to ignore for the time being because the code isn't triggered all that often and reducing the redundancy would increase code complexity. That being said, I dislike redundancies and I'm willing to look for a better solution. Without looking into it in more detail, can guess a couple ways to improve this: Split these cases to a separate function, return early in the case to avoid needing the `else`. Or use continue This would definitely work, but it would further abstract things in an already complex section (arguably a good thing) and I think it would introduce the overhead of a function call (I don't think the compiler would be able to inline it). Still this may end up being the clearest option. Declare the variables before the `if` and assign them in the `else if` statement Again, this would work, but those checks/assignments are called relatively infrequently compared to the first condition, so it would definitely be less performant because the function calls would happen every time even when they aren't needed. Something else? I could incorporate this into a do-while loop (or any loop, technically) as if statements and then assign the variable before the corresponding if statement, and use break to exit early. I think this would the most performant of the options, but may be more confusing.

Hans Goudey (HooglyBoogly) added inline comments.Feb 10 2021, 4:39 PM

source/blender/blenkernel/intern/layer.c
800	Yeah, I would change it. We have a clang tidy check for this actually.
1045–1049	Thanks for the thoughts. One other option is to do something like this: LayerCollection *found_layer_collection; if (...) { ... } else if (!deletion && found_layer_collection = BLI_findptr(child->collection, offsetof(LayerCollection, collection))) { BLI_remlink(lb_layer_collections, found_layer_collection); ... } ... Since I haven't really looked into this I don't know what's best, just glad you're considering the options : )

Ryan Inch (Imaginer) added inline comments.Feb 11 2021, 9:20 AM

source/blender/blenkernel/intern/layer.c
1045–1049	Since I haven't really looked into this I don't know what's best, just glad you're considering the options : ) Yeah, no worries! I'm always happy to improve stuff :) That's actually a very interesting solution, I wasn't aware C allowed that. It's certainly closest to what I have currently and what I had hoped to do from the beginning. I'll definitely take a look at that. Thanks!

Bastien Montagne (mont29) edited projects, added Core; removed Pipeline, Assets & I/O.Feb 16 2021, 6:37 PM

Merge branch 'master' into preserve_layer_collections_attempt_2
Merge branch 'master' into preserve_layer_collections_attempt_2
Fix issues pointed out by Hans Goudey, update comments and a function name to be clearer.
make format.

Harbormaster completed remote builds in B13154: Diff 34502.Feb 28 2021, 10:24 AM

Thanks for your time and comments @Hans Goudey (HooglyBoogly) , I've hopefully addressed them all now. :)

Merge branch 'master' into preserve_layer_collections_attempt_2
Remove the corner case workaround in the layercollection sync function that was introduced from merging master as it isn't needed for this patch and causes crashes.

Harbormaster completed remote builds in B14992: Diff 37922.Jun 6 2021, 6:41 AM

Merge branch 'master' into preserve_layer_collections_attempt_2
Remove the corner case workaround in the layercollection sync function that was introduced from merging master as it isn't needed for this patch and causes crashes.

Harbormaster completed remote builds in B14993: Diff 37923.Jun 6 2021, 6:44 AM

On second thought, the recently removed workaround does not provide enough information, for me, on what user action triggers the workaround for me to test that my patch 100% supports the corner case (although it should). Does anyone have a simple way to reproduce so I can test that my patch covers the corner case?

In D9599#294731, @Ryan Inch (Imaginer) wrote:

On second thought, the recently removed workaround does not provide enough information, for me, on what user action triggers the workaround for me to test that my patch 100% supports the corner case (although it should). Does anyone have a simple way to reproduce so I can test that my patch covers the corner case?

@Ryan Inch (Imaginer) not sure what you mean here, can you link to the task or commit you are referring to?

I was referring to the code comment at line 888 in the current master. From further looking now this seems to be from rB56dabfac5cba8d4762579120e6416230af1465d9 which fixes T86741 and T86594.
If there's anything else related it'd be good to know, but I can test with the steps from the tasks.

Bastien Montagne (mont29) mentioned this in D12016: LayerCollection: Refactor of resync-with-Collection-hierarchy process..Jul 23 2021, 6:45 PM

Thanks a lot for working on this topic!

A slightly more advanced process for Layer resync has been committed now (D12016, rBb18d0244fc07 and rB3db37075f699), so think this diff can be archived.

Ryan Inch (Imaginer) reclaimed this revision.Aug 17 2021, 6:15 AM

Merge branch 'master' into preserve_layer_collections_attempt_2
Fix bug where some collections were being missed on the get invalid layer collections search.
Fix ASan errors about layer collection deletion order.
Clang Format.
Add temporary performance timing code.

Harbormaster completed remote builds in B16455: Diff 40765.Aug 17 2021, 6:16 AM

To be more specific, the issue with this patch is that it always re-uses the first 'invalid'/available-for-reuse layer that matches the 'missing' collection. This becomes easy to reproduce in complex hierarchy cases where a single collection is child of several different parents...

Code that was committed from D12016: LayerCollection: Refactor of resync-with-Collection-hierarchy process. instead re-uses the 'closest' (in term of tree topology) available-for-reuse one.

This is important especially if we add custom properties to layers in the future (which is on our radar, and has been requested several times by add-ons authors) .

Regarding performances, current code from D12016 can for sure get improved (especially in the 'thousands of collections' situation, even though this case is absolutely not recommended, for many reasons beyond viewlayer resync process). Help and contribution is much welcome here. As @Ryan Inch (Imaginer) suggested on the chat, search in list becomes very costly with a lot of items, an idea here could be e.g. to use the heuristic that the next collection to find will also be the next in the list (and hence store the current item pointer for the next search).

But the main, massive performance improvement here will be to stop re-syncing viewlayers many times (thousands of times in some cases!) for a single operation, see T73411: Fix ViewLayers cache building.

Lost one hour rebuilding the proper branch and demo case...

In attached file, if you delete Collection 1, the excluded Collection 1 1 1 in Collection tree remains excluded in master, while with this branch it (aka its view layer) would be 'switched' with the one directly under the scene master collection.

D9599_fail_case.blend104 KBDownload

@Dalai Felinto (dfelinto)

Thanks Bastien, I can reproduce the issue here with the patch and your file as well.

Fixed a bug when deleting collections. Thanks to Bastien for reporting.
Merge branch 'master' into preserve_layer_collections_attempt_2

Harbormaster completed remote builds in B16867: Diff 41566.Sep 8 2021, 11:31 AM

Hi @Bastien Montagne (mont29),
Thank you for providing a solid reproducible test case, it nicely illustrated the bug you were talking about and allowed me to fix it. I have updated the patch and will now look into creating unit tests so that hopefully any bugs in layer collection syncing can be more easily caught in the future. Your demo case helped put in perspective the type of unit tests I think will have to be created, but if you have any thoughts on unit tests, I'd be glad to hear them.

Raimund Klink (Raimund58) added a subscriber: Raimund Klink (Raimund58).Sep 8 2021, 2:58 PM

Hi @Ryan Inch (Imaginer), thanks for your updates, and I see that your changes fix the .blend. But there is chance that it still doesn't handle the issue at large (i.e., the sample file Bastien shared is rather simple, how would this work in a more complex setup)?

As it is, as Bastien mentioned before, there is room to prioritize reviewing a patch that is built on top of the existing solution. Not replacing it entirely as you propose here. There are more pressing matters at the moment, and the mental energy to consider reviewing such a different approach is not something the core module can afford at the moment.

Hi @Dalai Felinto (dfelinto), unless there is some other issue at large that you haven't told me about, my fix should handle more complex setups just fine as it uses parent information to supersede order when deleting now.

To look at this from another perspective, had these issues been brought to my attention when Bastien initially found them, and my patch been reviewed then, they likely could have been fixed in less than a week by me, and there would have been plenty of time for testing before release, plus Bastien would have been free to deal with more important things and we wouldn't need to be worrying about performance now.

As it is now clear that this will not be merged into master, I will abandon it so that no more time is lost. However, I still have a vested interest in Blender's development so I hope that the core devs will reach out to me before something like this happens again, instead of after.

Evan Wilson (EAW) added a subscriber: Evan Wilson (EAW).Dec 14 2021, 5:03 PM

Preserve Layer Collections During Sync
AbandonedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 41566

source/blender/blenkernel/intern/layer.c

Preserve Layer Collections During SyncAbandonedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 41566

source/blender/blenkernel/intern/layer.c

Preserve Layer Collections During Sync
AbandonedPublic
Actions

Revision Contents
Changeset List