Page MenuHome

Fix T78071: Drivers reading object visibility not updating automatically
ClosedPublic

Authored by Sybren A. Stüvel (sybren) on Thu, Jun 25, 5:56 PM.

Details

Summary

This code is pretty much doing what we discussed, and it works in the example file attached to T78071. However, it breaks the EEVEE Spring rig; with this patch applied, Spring's hair is gone.

@Sergey Sharybin (sergey) I'm just uploading it here so that maybe you can take a quick look to see if something seems iffy to you. Otherwise I'll just continue tomorrow and figure out why it's failing.

Diff Detail

Repository
rB Blender

Event Timeline

Sybren A. Stüvel (sybren) requested review of this revision.Thu, Jun 25, 5:56 PM
source/blender/depsgraph/intern/builder/deg_builder_relations.cc
765

The base pointer is no longer used to determine whether there is an OBJECT_BASE_FLAGS node; the has_node() function is now used to do this. This allows the code to create the relations properly the first time the object is seen, rather than waiting until it is (maybe) seen with a non-nullptr base pointer.

An alternative would be to always create the Entry → Exit relation, and create the Entry → OBJECT_BASE_FLAGS → Exit relations only when necessary. This would result in redundant relations, though, which the current approach avoids.

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

This is a bit confusing terminology. In the nodes builder the build_object_from_layer()is used from the loop which iterates bases.

Here you can read this as "build relations inside of object's FROM_LAYER component", but then the lac of "symmetry" with nodes builder requires extra brain cells.

Maybe build_object_from_layer_relations would solve the mystery? What do you think?

771–773

FROM_LAYER -> SYNCHRONIZATION seems to only happen when the object is build from base, and before this code was always run. Is this an intentional change?

source/blender/depsgraph/intern/builder/deg_builder_rna.cc
297–299 ↗(On Diff #26286)

Two things here.

First, I think we should stick to COMPONENT level communication as much as possible, leave picking up exact operation to the graph. Kind of hides specifics of topology from other areas. Additionally, this allows depsgraph to automatically pick exit operation for source of relation, and will pick up entry operation for destination of relation.

Second, is maybe this is the reason why object is invisible? If there is a driver on hide_viewport this code wires relation to the EXIT, which is too late since then FROM_LAYER function will not have driver evaluated yet.

source/blender/depsgraph/intern/depsgraph_tag.cc
112

Think we should tag OBJECT_BASE_FLAGS here, as this is what is being modified.

The fact that there is ENTRY/EXIT is kind of details. The way how update flush works is that the entire component is tagged anyway. Sticking to OBJECT_BASE_FLAGS allows us:

  • Hide details of topology
  • Allow to eliminate no-op ENTRY_EXIT nodes (here I mean have graph "minimization" pass)

To elaborate a bit on the latter point. Having all those explicit ENTRY/EXIT and some other "utility" operations allows to simplify graph building process since you don't need to worry too much about "hey, if there is no FOO used in BAR, i should use node BAZ, otherwise i need to use node DOOMOP". However, these nodes are a burden for evaluation. So one might imagine that it's totally possible to write graph minimizer which will rewire some relations and will eliminate the no-op nodes. Exact algorithm is yet to be formalized, but I feel it's totally doable and is something we should implement eventually.

139

Same as above.

Sybren A. Stüvel (sybren) marked 4 inline comments as done.Fri, Jun 26, 11:56 AM
Sybren A. Stüvel (sybren) added inline comments.
source/blender/depsgraph/intern/builder/deg_builder_relations.cc
644

Even though it makes sense given the FROM_LAYER component, my brain keeps reading it as build_X_from_Y, and we're not building an object from a layer.

I agree that naming the function build_object_from_layer_relations is an improvement, so I'll do that and leave the overall naming for a future improvement.

771–773

This was an intentional change.

The code didn't always run. It was only run when base != nullptr. In other words, this happened:

  • The objects in the view layer are iterated, let's say they're some_object and parent_object.
  • The view layer iterator calls build_object(some_base, some_object) is called.
  • That function calls build_object(nullptr, some_object->parent). In this case the build_object_flags() function wouldn't know that there would be a base, so it didn't add the FROM_LAYERSYNCHRONIZATION relation yet.
  • The view layer iterator calls build_object(parent_base, parent_object). Now build_object_flags() is called again, this time with a known base, so now it does add that relation.

So, in the old code the Base * is used to determine whether that FROM_LAYERSYNCHRONIZATION needs to be added. Because of this, it needs to be called every time build_object() is called.

My new approach uses has_node(object_flags_key) to determine whether that relation needs to be added or not. This means that it can do the right thing right away, without having to rely on future calls that may or may not pass a Base *.

source/blender/depsgraph/intern/builder/deg_builder_rna.cc
297–299 ↗(On Diff #26286)

This was the cause of Spring loosing her hair. I didn't take into account the source parameter, so it would indeed always relate to the EXIT node. By sticking to component level communication, this is picked up properly automatically.

source/blender/depsgraph/intern/depsgraph_tag.cc
112

I was in doubt what to do here, so thanks for the clarification.

And yes, it would be fun to think of an algorithm to reduce the no-op nodes further.

Sybren A. Stüvel (sybren) marked 3 inline comments as done.Fri, Jun 26, 12:27 PM
Sybren A. Stüvel (sybren) edited the summary of this revision. (Show Details)
  • Feedback from Sergey. Now things work with the Spring rig.
This revision was not accepted when it landed; it landed in state Needs Review.Fri, Jun 26, 12:39 PM
This revision was automatically updated to reflect the committed changes.