Page MenuHome

Fix T61296: Crash with animated b-bone segments
ClosedPublic

Authored by Sergey Sharybin (sergey) on Apr 25 2019, 4:56 PM.

Details

Summary

Consider animated/driven bendy bones segments as something what requires
special bendy-bones operation and relation in the dependency graph.

Diff Detail

Repository
rB Blender

Event Timeline

This is awful in a multitude of ways:

  1. This scans all curves for every bone in the armature.
  2. The scan does not in any way check the bone name.
  3. These are not the only places that do this check during graph construction.
  4. I seem to recall that drivers on the Armature B-Bone properties are not linked with relations to the actual pose evaluation nodes, so evaluation won't be reliable.

If depsgraph is going to rely on checking whether properties are animated, it calls for a common system that scans curves once and builds a lookup table that can be checked efficiently in code that needs it.

Brecht Van Lommel (brecht) requested changes to this revision.Apr 25 2019, 5:15 PM
Brecht Van Lommel (brecht) added inline comments.
source/blender/depsgraph/intern/builder/deg_builder.cc
115–117

This seems to test if any bone in the armature has animated bbone segments, not the specific bone?

It seems simpler use rna_get_fcurve, with a PointerRNA created for the pose bone.

This revision now requires changes to proceed.Apr 25 2019, 5:15 PM
This comment has been deleted.
source/blender/depsgraph/intern/builder/deg_builder.cc
115–117

This seems to test if any bone in the armature has animated bbone segments, not the specific bone?

That is a good point.

source/blender/depsgraph/intern/builder/deg_builder.cc
115–117

No, rna_get_fcurve is not appropriate because it does not work with NLA. The callback approach is the correct way, but it has to be optimized re do it once and build a table + track bone names.

I seem to recall that drivers on the Armature B-Bone properties are not linked with relations to the actual pose evaluation nodes, so evaluation won't be reliable.

That is separate issue which is beyond this particular fix.

Why wouldn't rna_get_fcurve work? It has "Special case for NLA Control Curves".

Why wouldn't rna_get_fcurve work? It has "Special case for NLA Control Curves".

NLA control curves are special things that control NLA tracks themselves. That function doesn't see anything touched by actions in the NLA stack.

The most reliable way for all this in my view is to build a lookup table for all such uses once, by doing something like this:

  1. Use BKE_fcurves_id_cb to walk all curves for all ID types that are 'interesting' (e.g. Armatures here).
  2. In the callback, maybe check BLI_str_endswith for 'interesting' properties, purely for performance reasons.
  3. Resolve the RNA path using RNA_path_resolve_property like the animation system itself does in anim_sys.c
  4. Cache resulting PointerRNA and PropertyRNA values in a hash table.

Then provide an interface to this table for the whole of depsgraph builder system.

The whole point: do it once, use the same path parser as the animation system, cache the result for lookup.

Edit: NLA evaluation in anim_sys.c basically works like this, see nlaevalchan_verify, and even animsys_evaluate_nla_domain -> nla_eval_domain_action, so the cost of all this would basically be exactly like evaluating animation once.

Use BKE_fcurves_id_cb to walk all curves for all ID types that are 'interesting' (e.g. Armatures here).

I wouldn't go that route. There could be a lot of "interesting" IDs which are on other view layers, or not needed for the current builder (keep in mind, we need to keep builder able to start from any ID, effectively being able to build graph needed for a specific ID).

By iterating over all IDs and resolving their paths you'll waste a lot of time, which isn't very good.

To me it's more like on-demand initialization of "interesting" things once they are being queried.

To me it's more like on-demand initialization of "interesting" things once they are being queried.

Well, you can initialize data for each ID on demand, of course. What I meant primarily is instead of trying to scan curves for each property instance you are interested in, build and cache the data for the whole ID (once you need it), and use hash lookups after that. And preferrably use the real RNA parser on the paths to avoid possible issues because of discrepancies between it and any ad-hoc "simple" solutions made for specific properties.

Iterate over ID's animation once, and store the result in a cache.
This cache can be re-used by both nodes and relations builders.

Still not entirely happy with the construction of identifier for
query, but i am unaware of a way to construct PropertyRNA faster.

Also changed the way how disabled/animated bases are detected,
now there is no query for animation F-Curves during copy-on-write,
is all being tagged during dependency graph construction.

Still not entirely happy with the construction of identifier for
query, but i am unaware of a way to construct PropertyRNA faster.

Can't you actually resolve PropertyRNA pointers once via just the RNA structure pointer (e.g. &RNA_Bone) and cache them? I.e. aren't those structures basically static and existing unchanged as long as blender is running? Something like:

static PropertyRNA *prop_bbone_segments = RNA_struct_type_find_property(&RNA_Bone, "bbone_segments")

This likely won't work with stuff like properties defined via python, but depsgraph is interested only in the built-in properties.

You probably can also store just the PointerRNA::data pointer and PropertyRNA in the lookup table so you don't have to construct full PointerRNA to check the property?

Using RNA_struct_type_find_property() would work. But instead of caching, think proper way would be to use hash map in the RNA_struct_type_find_property(). Will give same complexity, without need to cache anything outside of RNA (currently it is a linear list lookup).

You probably can also store just the PointerRNA::data pointer and PropertyRNA in the lookup table so you don't have to construct full PointerRNA to check the property?

Think that is possible, especially if the "resolve" think is removed to construct PropertyRNA.

Still not entirely happy with the construction of identifier for
query, but i am unaware of a way to construct PropertyRNA faster.

Can't you actually resolve PropertyRNA pointers once via just the RNA structure pointer (e.g. &RNA_Bone) and cache them? I.e. aren't those structures basically static and existing unchanged as long as blender is running? Something like:

static PropertyRNA *prop_bbone_segments = RNA_struct_type_find_property(&RNA_Bone, "bbone_segments")

You should be able to use an extern without any lookup, see how WM_msg_publish_rna_prop macro uses:

extern PropertyRNA rna_##type##_##prop

Use RNA_struct_type_find_property().

You should be able to use an extern without any lookup, see how WM_msg_publish_rna_prop macro uses:

extern PropertyRNA rna_##type##_##prop;

Why this is part of WM and not RNA itself? I do not want to spread specifics of implementation details all over the place.

Can anyone be helpful and help with regression test and verifying that this change actually solves the initial bug, rather than going deeper and deeper into optimizing something which is not even showing up in a profiler?

There are at least two more places that do the 'is bbone' check in depsgraph: build_constraints and deg_builder_rna - search for BONE_SEGMENTS.

The initial crash can't be reproduced any more due to improved safety checks in code, so this needs to be tested by simply checking that the depsgraph has BONE_SEGMENTS operations with correct links (including drivers on bbone properties and Copy Transforms with/without 'use bbone shape').

source/blender/depsgraph/intern/builder/deg_builder.cc
76

This ensureInitializedAnimatedPropertyStorage + isPropertyAnimated bit is already repeated twice, so I think there should be an isPropertyAnimated(owner_id, key) method in cache that combines these.

Addressed review feedback.

I tested this and fixed some issues:

  • Fixed a broken "data = data" assignment that stopped B-Bone detection from actually working.
  • Swapped the order of checks to match the simple-to-complex ordering better.
  • Fixed missing relations for drivers and animations of armature data properties - it turned out the code was mostly there, but didn't work.

Remaining issues:

  • Relations for drivers and animation are duplicated because build_animdata is called both from build_armature and build_object_data.

I figured just updating this patch would be faster than messing with a separate one.

Harbormaster completed remote builds in B3447: Diff 14967.

Relations for drivers and animation are duplicated because build_animdata is called both from build_armature and build_object_data.

How's that related to this change? This shouldn't have changed?

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

This hack is the most controversial part of my changes - I'm not completely sure myself. It'l likely the better way is to map &RNA_Bone to PARAMETERS/ARMATURE_EVAL in RNANodeQuery::construct_node_identifier but it's a deeper change and I'm sleepy.

Relations for drivers and animation are duplicated because build_animdata is called both from build_armature and build_object_data.

How's that related to this change? This shouldn't have changed?

It's not related, I just never noticed it before. :)

I am not happy with the direction the change is going into: it includes too many things in a single commit. Even mine original change it to be split into the following steps:

  • Generic animation-property cache commit.
  • Move bases check to the generic animation cache.
  • Build bbone operations for bones which segments is > 1 or are animated.

The latter is where this patch should stop. The rest of fixes is to be done separately and not mixed in here.

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

I don't think this change belongs to this patch. See the non-inlined comment.

1322

Same as above.

  • Fixed an issue with drivers and proxies (proxies don't have BONE_SEGMENTS).
  • Changed the way armature animation relation is handled.

Split into more atomic commits:

  • Depsgraph: Expose set type available
  • Depsgraph: Add generic animated properties cache
  • Depsgraph: Use new animation cache for visibility check
  • Depsgraph: build bbone operation if bone segments has animation

Alexander did more fixes on relations in master this morning. so those
are no longer included into this patchset. It is fully focused on
getting animation properties cache in place.

Not sure whether there are more fixes needed to get T61296 fully
resolved.

Alexander Gavrilov (angavrilov) accepted this revision.EditedMon, Apr 29, 2:51 PM

On my small test this seems to work: the graph image contains all the right nodes and relations, and driving the segment count from a custom property produces the expected deformation changes, even via proxy and animated.

This revision is now accepted and ready to land.Mon, Apr 29, 7:29 PM
This revision was automatically updated to reflect the committed changes.