Page MenuHome

Action: flip action data using pose contents
ClosedPublic

Authored by Campbell Barton (campbellbarton) on Mar 21 2021, 11:11 AM.

Details

Summary

This adds a new RNA method Action.flip_with_pose(ob, frame)
to flip the action channels that control a pose.
The rest-pose it's self is used to properly flip the bones transformation.

This is useful as a way to flip actions used in pose libraries,
so the same action need not be included for each side.

API calls to cache F-curve look-ups have also been added,
supporting a single hash lookup to access all channels controlling an RNA path.


Motivation

This patch adds functionality to flip an entire action on the X axis using a symmetrical rig.

This was written so the asset manager can apply poses flipped, see T86159,

tested with spring.blend.

Alternative Solutions

It's possible to calculate this on the pose directly (see D10766), however operating on the action data makes this more useful as an API function, since the data can be flipped before it's applied to the pose.
Instead of flipping on the pose, then writing back to the action.

Limitations

There is some information not easily available at the time of flipping, for example - it's possible the flipped data-path doesn't exist in all cases, although the ideal behavior isn't obvious when the RNA path only resolves on one side of the pose.

  • Currently the API function flips all channels, we could add an argument so it only operates on some of the channels.

Open Topics

  • When some actions transform channels don't exist, they could be created (for example - if only X & Y rotation are set, it may be necessary to create a Z F-Curve to properly flip the rotation).
  • The key-framing API is currently part of the editors (it would be a bad-level call from the BKE), we could expose some keyframing functionality to BKE, or move this functionality to editors. For now there is a simple keyframe adjusting function.
  • Currently only the armature rest-matrices is used, not the pose. This API could take an armature instead of an object, although we might want to use pose content in the future, it doesn't seem like a big issue either way.
  • This could eventually be exposed as an operator.
  • Currently this re-orders channels which could have other frames keyed. A more comprehensive approach could be to operate on all keyed frames for an action, so an action that includes multiple poses will have them all properly flipped.

Notes

  • The F-Curve lookup cache can be committed separately.
  • This is an example script for testing with the pose object set active:
import bpy
from bpy import context
ob = context.object
action = bpy.data.actions['07_040_A.spring']
action.flip_with_pose(ob, context.scene.frame_current)

Diff Detail

Repository
rB Blender
Branch
TEMP-POSE-FLIP-ACTION
Build Status
Buildable 13735
Build 13735: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
  • De-duplicate macros & correct f-curve handle calculation flag use, additional comments.
  • Rename variables, minor rearrangement of code
Campbell Barton (campbellbarton) edited the summary of this revision. (Show Details)
Sybren A. Stüvel (sybren) requested changes to this revision.Mar 23 2021, 11:37 AM

It works!

What I'm not sure of, though, is what the frame parameter does. What is it for? And what frame is it referring to (action or scene)?

When some actions transform channels don't exist, they could be created (for example - if only X & Y rotation are set, it may be necessary to create a Z F-Curve to properly flip the rotation).

I think that's fine, IMO it's more important to deliver correct results than to avoid keying too much.

The key-framing API is currently part of the editors (it would be a bad-level call from the BKE), we could expose some keyframing functionality to BKE, or move this functionality to editors. For now there is a simple keyframe adjusting function.

As a longer-term project I'm even thinking of moving the animation/rigging code out of BKE and into its own module. The code is now all over the place, spread over various files in BKE and editors, and moving everything into a newly-organized ANIM module may be a nice way to clean things up.

Currently only the armature rest-matrices is used, not the pose. This API could take an armature instead of an object, although we might want to use pose content in the future, it doesn't seem like a big issue either way.

I think this is fine. I mean, if you want to flip-apply a left-foot pose onto the right foot, but the pose is not fully specified, it's going to be a guess for the animator as to the result. Taking the current pose of the left foot won't likely be an improvement.

This could eventually be exposed as an operator.

Maybe. For now I like it as a function, so that operators can use it as a building block.

Currently this re-orders channels which could have other frames keyed. A more comprehensive approach could be to operate on all keyed frames for an action, so an action that includes multiple poses will have them all properly flipped.

Do you mean it only flips the keys at frame frame of the action? Because I tried it with an Action keyed on frame 1 but passing frame=12 to the function, and it still flips. This is also why I'm unsure of the role of the frame parameter.

There are a few places where I think things can be improved, see the inline notes. The fcurve.c files is already getting quite big, how about creating BKE_fcurve_cache.h and fcurve_cache.c and putting the new code there?

source/blender/blenkernel/intern/action.c
2058–2061 ↗(On Diff #35457)

This code is pretty similar to what rna_FKeyframe_ctrlpoint_ui_set() is doing, so moving the code to a common place and using it from both locations would be a good idea. For the future, though.

2070 ↗(On Diff #35457)

This function is too complex, and should be split up into smaller functions.

2070 ↗(On Diff #35457)

I suspect evaltime can be const.

2105 ↗(On Diff #35457)

I'm not a fan of these macros. I see what they're doing, but I'd much rather have regular functions to do the same.

2136 ↗(On Diff #35457)

Oh wow, I would have never thought of keying the rotation mode. Good catch.

2141 ↗(On Diff #35457)

In this case, it should be explicitly documented that BBone properties are not flipped.

source/blender/blenkernel/intern/fcurve.c
298

"Cached Find" is a bit vague. How about "F-Curve cache for finding curves by RNA path"?

302–303

This doesn't convey the meaning of index, only what it points to (which is very good to have as well). I think appending "indicating the start of the span" is enough to clarify this.

304

This could be interpreted incorrectly as "length of fcurve_array". How about "Length of this span"?

308

I'm not a fan of "find cache" as name, because it could be an imperative ("You, find cache, now!"). How about FCurvePathCache?

314

Does it, though? I would expect it tot map to elements of span_table.

322–331

This can be simplified (lower cognitive complexity, and more const):

const int cmp = strcmp(fcu_a->rna_path, fcu_b->rna_path);
if (cmp != 0) {
  return cmp;
}
if (fcu_a->array_index < fcu_b->array_index) {
  return -1;
}
if (fcu_a->array_index > fcu_b->array_index) {
  return 1;
}
return 0;
334

The "cached find create" in the function name is a bit confusing. The struct is named "find cache", and not "cached find", so I think BKE_fcurve_findcache_create is better.

383

Usually such functions are suffixed with _free instead of _destroy. In any case, _destory is a typo for sure.

396

Cognitive complexity: just write

if (span == NULL) {
  return NULL;
}

and un-indent pretty much the entire function by one level.

425–426

I'd just write this as:

if (span == NULL) {
  return 0;
}

as this reduces the cognitive complexity of the rest of the code.

This revision now requires changes to proceed.Mar 23 2021, 11:37 AM
Campbell Barton (campbellbarton) marked 2 inline comments as not done.Mar 23 2021, 12:16 PM
Campbell Barton (campbellbarton) added inline comments.
source/blender/blenkernel/intern/action.c
2058–2061 ↗(On Diff #35457)

I assumed this would be redone, for example - move the API call to editors and use regular keyframing functions.

Although it seems like I'll go the other way and manipulate beztriples directly - since all keyframes will need to be handled (not just evalframe).

2070 ↗(On Diff #35457)

+1

2105 ↗(On Diff #35457)

While some of this could be a function, I'm not convinced we can easily move this into a function with C at least.

I'd rather leave as-is for now. Splitting pose matrix extraction into a separate function well help IMHO.

source/blender/blenkernel/intern/fcurve.c
298

+1

304

+1

308

+1

314

+1

FCurveFindCache_Span is the value type, but can say elements in span_table.

322–331

+1, although I'm not so fussed on this.

334

+1, also wasn't that happy with naming.

383

In quite a few areas we have _create / _destroy, as free is normally paired with alloc, and creating this data structure isn't simply an allocation.

396

+1

425–426

+1

source/blender/blenkernel/intern/fcurve.c
314

FCurveFindCache_Span is the value type

Yes, but its meaning is only truly revealed when you know it's only used in one place. IMO it's clearer to just point to that one place.

383

👍

Campbell Barton (campbellbarton) marked 11 inline comments as done and 4 inline comments as done.
  • Cleanup based on feedback
source/blender/blenkernel/intern/fcurve.c
334

I ended up renaming these functions, BKE_fcurve_pathcache_* since...

  • BKE_fcurve_findcache_find doesn't read well.
  • It fits with the struct name FCurvePathCache.
  • The paths are what is cached as well as used for look-ups.

Functional changes:

  • Support transforming an entire action (all key-frames).
  • Update action group names too.
  • Evaluate F-Curves without modifiers so the values written to the key-frames don't include effects.
  • Match F-curve rounding behavior when applying an f-curve to an integer value.

Non-functional changes:

  • Split into functions.
  • Reduce macro use.
  • Additional comments.
  • Avoid a binary search for each key-frame lookup when writing.

New API's

  • BKE_fcurves_calc_keyed_frames{_ex} to takes an array of f-curves, returns a array of keyed frames, use to detect which fames need to be flipped for each pose channel.
  • Minor edits to comments.
  • Update epsilon limits, assert we don't overflow the integer

LGTM, nice work on the documentation. Just three 'I think this can be const' notes, no need to review again.

source/blender/blenkernel/intern/action.c
2175 ↗(On Diff #35572)

can be const

2177 ↗(On Diff #35572)

can be const

source/blender/blenkernel/intern/fcurve.c
846

can be const

This revision is now accepted and ready to land.Mar 25 2021, 2:25 PM
source/blender/blenkernel/intern/action.c
2445–2446 ↗(On Diff #35572)

I'm not certain about these, by the way. Why do the armature and the action need any tagging? Shouldn't that be up to the code that calls this function?

source/blender/blenkernel/intern/action.c
2445–2446 ↗(On Diff #35572)

Update: I had to remove these calls for the code to work properly with the pose library. That really only needs the flipped Action; the armature must not be tagged for the pose library's interactive blending operator to work properly.

Split into files, use const where possible.

  • Remove depsgraph update

(message deleted, looked at the wrong thing, please ignore)