Page MenuHome

Fix T88433: no greaspencil depsgraph evaluation with certain drivers
ClosedPublic

Authored by Philipp Oeser (lichtwerk) on Aug 26 2021, 4:00 PM.

Details

Summary

When the same stroke was used as a driver variable, this could make this
stroke already tagged as built in the course of building driver
variables (via build_gpencil), but then important stuff from
build_object_data_geometry_datablock could be missed later on (because
both of these funtions use checkIsBuiltAndTag). Most importantly,
setting up operations such as GEOMETRY_EVAL would be skipped entirely.

build_object_data_geometry_datablock seems to cover greasepencil just
fine (does the same as build_gpencil and more). Proposed solution is to remove build_gpencil entirely. In build_id it would then also call build_object_data_geometry_datablock for ID_GD IDs. Now the covered types that _call_ build_object_data_geometry_datablock match exactly to what is covered _inside_ build_object_data_geometry_datablock.

Think this "duplication" of functionality was just overseen in rB66da2f537ae8 [build_gpencil existed long before and said commit made greasepencil a real object with geometry and such].

thx @Jacques Lucke (JacquesLucke) for additional input!

Diff Detail

Repository
rB Blender
Branch
T88433 (branched from master)
Build Status
Buildable 16637
Build 16637: arc lint + arc unit

Event Timeline

Philipp Oeser (lichtwerk) requested review of this revision.Aug 26 2021, 4:00 PM
Philipp Oeser (lichtwerk) created this revision.

Think it does make sense, but having second pair of eyes will be great here.

Based on the description and looking at the code I'd say that the real problem is that build_object_data_geometry_datablock and build_gpencil both check for the same flag with checkIsBuiltAndTag, but are actually doing different things after the check.

build_object_data_geometry_datablock seems to be doing the same as build_gpencil and more. Maybe the solution is to remove build_gpencil. In build_id it would then also call build_object_data_geometry_datablock for ID_GD ids.

Alternatively, there should probably be two separate flags for the different cases.

  • remove 'build_gpencil' entirely ('build_object_data_geometry_datablock' covers greasepencil just fine)
Philipp Oeser (lichtwerk) retitled this revision from Depsgraph: dont build self while building driver variables to Fix T88433: no greaspencil depsgraph evaluation with certain drivers.Aug 27 2021, 9:01 AM
Philipp Oeser (lichtwerk) edited the summary of this revision. (Show Details)

@Philipp Oeser (lichtwerk) I don't know the depsgraph code, but be sure to check you don't break the animation in annotations that are not using Object... maybe it's not a problem, but better check it.

@Philipp Oeser (lichtwerk) I don't know the depsgraph code, but be sure to check you don't break the animation in annotations that are not using Object... maybe it's not a problem, but better check it.

That should not be a problem, since both build_animdata & build_parameters is done in build_object_data_geometry_datablock.
Simple test is also fine, as well as files from https://cloud.blender.org/p/gallery/5822feae1f474232daf12570 [they work the same as in master].
Go another file I should test?

I'm going to test the patch with a simple annotation keyframing.

@Philipp Oeser (lichtwerk) Annotations tested and works as expected, but I think I have found a new bug (also in 2.93)...try to keyframe the opacity of the annotations. You will see the opacity frame is created but nothing change when frame change.

... but I think I have found a new bug (also in 2.93)...

mind reporting that separately?

Hi!
I'm not seeing this fixed in the latest nightly, is that normal?
Thanks!
Piotr

Hi!
I'm not seeing this fixed in the latest nightly, is that normal?
Thanks!
Piotr

Review is still in progress (so patch has not landed yet).
You'll get notified automatically when it does.

Anyone dares to give green light :) ?

Looks good to me, but would obviously be good if @Sergey Sharybin (sergey) approves it as well now, since he also basically approved the original patch.

This revision is now accepted and ready to land.Aug 31 2021, 2:51 PM