Page MenuHome

Fix T97262: Crash with specific view layer setup
ClosedPublic

Authored by Sergey Sharybin (sergey) on Apr 12 2022, 12:44 PM.

Details

Summary

Originally was noticed when using a linked background scene and a scene
camera from another (local) scene.

The root issue was that relation from view layer to object's base flags
evaluation was using wrong view layer. This is because the relation was
created between object and currently built view layer, and it only was
happening once (since the object-level relations are only built once).
Depending on order in which build_object was called it was possible
that relation from a wrong view layer was used.

Now the code is better split to indicate which parts of object relations
are built when object comes from a base in the view layer, and which ones
are built on indirect linking of object to the dependency graph.

This patch makes relations correct in the cases when the same object is
used as a base in both active and set scenes. But, the operation which
handles object-level flags might not behave correctly as there is no
known design of what is the proper thing to do in this case. Making a
clear design and implementation of case when object is shared between
active and set scene is outside of the scope of this patch.

Diff Detail

Repository
rB Blender

Event Timeline

Sergey Sharybin (sergey) requested review of this revision.Apr 12 2022, 12:44 PM
Sergey Sharybin (sergey) created this revision.

I can confirm that the patch seems to work, but I think I'm missing some piece of knowledge to understand why it works.
From what I can tell, build_object_from_layer_relations was called once for every object before. Now the function can run multiple times for an object. Issue is, I don't quite get how running it once is different from running it more than once.
It seems like the behavior may only be different when the if (!has_node(object_flags_key)) branch is taken. However, in my test that doesn't happen, unless I'm doing something stupid.

Note that I don't know much about the relation between view layers and depsgraphs yet.

Note that I don't know much about the relation between view layers and depsgraphs yet.

Lets make it clear in the code!

Here is an attempt. It more clearly isolates code which is to built relations on object level only once, and code which needs to build relations between view layer and object.

TL;DR: the "View Layer flags to Object" relation needs to be betweeb view layer which has base with object. Before the relation was created from first view layer which (indirectly) called build_object.

A high level comment on the last change: Can we add a new method like build_object_from_view_layer instead of passing a bool to every call of build_object?

We can/should. Was thinking about it as well.
Is just a doubt was about keeping nodes and relations builder matched as close as possible. Ideally, the traversal logic would even be moved to a base class.

In any case, lets just introduce build_object_from_view_layer to locally improve code.

Introduce build_object_from_view_layer_base.

Somplifies code in a sense that we do not need to pass extra argments from many placex.

source/blender/depsgraph/intern/builder/deg_builder_relations.cc
804

typos (form, has)

805

typo (ob)

806

The last thing I don't understand yet is how the from every view layer works here. It seems like view_layer_done_key does not depend on the view layer, just on scene_. So isn't this just adding the same relation for every view layer?

source/blender/depsgraph/intern/builder/deg_builder_relations.cc
806

Depsgraph is per-scene per-view layer, so it is not possible to have multiple view layers of the same scene in the dependency graph.

This patch will make it so that if multiple view layers from different scenes shares the same object the the relations to the OperationCode::OBJECT_BASE_FLAGS will be correct: they'll be coming from all view layers which are referencing the object.

What I'm noticing now is that OperationCode::OBJECT_BASE_FLAGS operation itself is not really ready to handle this situation, but this can/should be solved separately, as the design behind what is the proper thing to do is not clear. I'll list it as a limitation of this patch.

This revision is now accepted and ready to land.Apr 14 2022, 12:08 PM

LGTM. Just a small nag about the comment, but that's not re-reviewing worthy.

source/blender/depsgraph/intern/builder/deg_builder_relations.cc
793–797

Minor nag: it's unclear in this comment which view layer this depsgraph is built for. Is that "View Layer C"? Or one of the alread-mentioned ones?

800

a base

806

a base

source/blender/depsgraph/intern/builder/deg_builder_relations.cc
793–797

Good point.
This isn't really important which view layer the depsgraph is built for. Whatever causes a situation to happen.
I've re-phrased it in, hopefully, a better way: makes it more clear that existence of Base is what matters, not the view layer.

Feel free to either update comment directly or give suggestions for improvements. I've committed the patch since it is green and since it fixes rather annoying bug.