Details

Reviewers

Joshua Leung (aligorith)
Sergey Sharybin (sergey)
Sybren A. Stüvel (sybren)
Dalai Felinto (dfelinto)

Summary

This patch allows animation of the collection visibility buttons
(the monitor or camera icon in the outliner).

Basically concludes the work initiated by @Joshua Leung (aligorith) on
rBd1e3ba22a03c: Collections: Initial support for animating/driving collection properties….

In that commit (rBd1e3ba22a03c) all necessary data for support to
animation of the ID were implemented. But the depsgraph support was
incomplete.

So this patch only focused on the depsgraph changes (and trusted the
original work).

Issue (what hinders the implementation in the depsgraph area):

1. Deg nodes and relations are not created for invisible collections.
Every time a collection flag is changed in rna, depsgraph relations are
recreated which "remove" nodes from hidden collections. This means that
if animation tries to unhide a collection, there are no nodes to update
the scene.

2. The function that updates a Collection is not thread-safe and adds a lot of overhead.
As it is not thread-safe, the function is unsuitable as an operand for
the depsgraph node.

Also it adds a lot of overhead. When a collection flag is changed in rna,
the BKE_main_collection_sync(bmain) function is called. This function
deletes lists, Ghash, arrays from all Blender ViewLayers and Collections
and recreates them no matter if an object was deleted or if a Base flag
was just changed.

Solution:

The solution is similar to what is seen for object visibility animation
(with some changes).

1. Deg nodes and relations to Collections are always created if the collection is animated.
Like Objects, relations of hidden IDs are only created if the datablock
is animated. This solves the issue of operands not being called if the
collection is hidden.

2. Implement an update function that updates only the Base flags of a ViewLayer.
The lightweight BKE_layer_collection_sync_base_flags function was implemented
to be called in the operand. This avoids having to call BKE_main_collection_sync.

To Do

The BKE_layer_collection_sync_base_flags function, while working fine
in its current state, does not follow the depsgraph convention as it
directly reads bmain which may not be thread-safe

Diff Detail

Repository

rB Blender

Branch

arcpatch-D12469 (branched from master)

Build Status

Buildable 17114
Build 17114: arc lint + arc unit

Event Timeline

Germano Cavalcante (mano-wii) requested review of this revision.Sep 13 2021, 2:51 PM

Germano Cavalcante (mano-wii) created this revision.

Harbormaster completed remote builds in B16963: Diff 41742.Sep 13 2021, 2:51 PM

RedMser (RedMser) added a subscriber: RedMser (RedMser).Sep 13 2021, 3:27 PM

Can you do a pass to clean up the style? Right now the comments are all over the place (always use Caps for the 1st letter to start the comment and end with a full-stop). Also I recommend removing all the dead code (the commented out or #idef 0 code).

Germano Cavalcante (mano-wii) mentioned this in D12525: RNA Collections: Synchronize view layer in the callback to set flags.Sep 16 2021, 8:13 PM

More changes planned due to D12525: RNA Collections: Synchronize view layer in the callback to set flags.

Style
New flags that check if the ViewLayer is dirty

Since it is not desirable to resynchronize the Bases flags on every frame change, the COLLECTION_FLAG_DIRTY and VIEW_BASE_FLAGS_DIRTY flags have been implemented.
COLLECTION_FLAG_DIRTY is set to collection when changing an RNA property.
VIEW_BASE_FLAGS_DIRTY is set in the function of the new node to evaluate collections.

The relation can be seen in this code:

add_relation(
    animation_key, collection_eval_key, "Collection on Frame Change -> Collection Eval");
add_relation(collection_eval_key, view_layer_done_key, "Collection Eval -> View Layer Eval");

BKE_layer_eval_view_layer_indexed synchronizes the flags of both the evaluated and the original ViewLayer.

Harbormaster completed remote builds in B17112: Diff 42009.Sep 17 2021, 9:16 PM

Germano Cavalcante (mano-wii) edited the summary of this revision. (Show Details)Sep 17 2021, 9:18 PM

Cleanup
Fix object with animated visibility not in the depsgraph (new BASE_FROM_COLLECTION_ANIMATED flag)

Harbormaster completed remote builds in B17114: Diff 42012.Sep 17 2021, 11:14 PM

Fix base synchronization in the evaluated viewlayer (the object in CollectionObject is not always the evaluated one)

Harbormaster completed remote builds in B17115: Diff 42015.Sep 17 2021, 11:51 PM

Ryan Inch (Imaginer) added a subscriber: Ryan Inch (Imaginer).Sep 18 2021, 5:35 AM

There are different ways to solve this problem (like calling DEG_relations_tag_update in the callback to set the rna value instead of the update

This is not a solution, since this violates the design (can not tamper with graph topology in any way while the graph is being evaluated.
Always building IDs and relations for objects from "animated collections" is the correct solution.

source/blender/blenkernel/intern/collection.c
2109	Evaluation happens on an evaluated datablocks. If the assert does not trigger it indicates a deeper design problem of the new code.
source/blender/blenkernel/intern/layer.c
2392	The `BKE_layer_eval_view_layer_indexed` is supposed to be an evaluation callback, operating on the evaluated datablock. If very rare when you need to do an explicit check with original/evaluated ID. Every time it is needed there should be a comment about it.
source/blender/depsgraph/intern/builder/deg_builder_nodes_view_layer.cc
172	`bmain` is meaningless in the evaluated domain. You can only trust its subset, which is stored in form of the graph. In other words, it seems to be a wrong organization of evaluation callback if you need to have bmain in it.

This revision now requires changes to proceed.Sep 20 2021, 11:39 AM

Silas Opel (Schamph) awarded a token.Sep 20 2021, 12:21 PM

Erick Tukuniata (erickblender) awarded a token.Sep 20 2021, 12:54 PM

Jack Grzes (jacek007g) added a subscriber: Jack Grzes (jacek007g).Sep 20 2021, 3:19 PM

Jack Grzes (jacek007g) awarded a token.Sep 20 2021, 3:23 PM

Jack Grzes (jacek007g) removed a subscriber: Jack Grzes (jacek007g).

Workaround for scene->master_collection to point to evaluated objects
Pass evaluated collection to BKE_collection_eval
Cleanup: remove bool is_evaluated_view_layer
Remove bmain parameter

Harbormaster completed remote builds in B17160: Diff 42105.Sep 20 2021, 6:29 PM

Germano Cavalcante (mano-wii) planned changes to this revision.Sep 20 2021, 6:29 PM

Germano Cavalcante (mano-wii) added inline comments.

source/blender/blenkernel/intern/layer.c
2392	This follows the same seen in `BKE_object_eval_eval_base_flags`. I am not sure why this is done there. I think it could just assert that it is the evaluated `view_layer`.
source/blender/depsgraph/intern/builder/deg_builder_nodes_view_layer.cc
172	For now I just removed the parameter and got bmain via `DEG_get_bmain`. I'm still not sure how I should use the subsets. I thought of two solutions: Pass the list of screens evaluated: `ListBase screens = (ListBase )get_cow_datablock(((bScreen )bmain_->screens.first));` or Create 2 new graph nodes, 1 to be linked to each evaluated `View3D` to call `BKE_layer_collection_local_sync(ViewLayer view_layer, const View3D *v3d)` and another for the `ViewLayer` to `SYNCHRONIZE_TO_ORIGINAL`. In my mind it seems trivial, I just fear that this 2nd option can bring too many changes and unpredictable results. (The 1st seems to be safer and simpler).