Alembic: F-Curve cache modifier to lookup arbitrary property animations.
Needs ReviewPublic

Authored by Kévin Dietrich (kevindietrich) on Oct 30 2016, 6:39 AM.

Details

Summary

The main goal of this F-Curve modifier is to support cached animations
of camera focal length and is generalized to support looking up
animations of any single float property of the Alembic object pointed by
the modifier. Perhaps in the future we could also support array
properties, in which case the current scene frame would be the index in
the array.

The Alembic Import operator should automatically add a keyframe and F-
Curve cache modifier to imported cameras if they have an animated focal
length.

A cache is used to gather F-Curves modifier using cache files. This is
to make it easier to manage pointers rather than iterating on all possible F-
Curves of a given object when updating a CacheFile data-block to figure
out whether or not it is using said CacheFile. Though it is not the
ideal solution, and should be handled by some depsgraph mechanism.

Diff Detail

Repository
rB Blender
Branch
abc_fcurve
Build Status
Buildable 255
Build 255: arc lint + arc unit
Kévin Dietrich (kevindietrich) retitled this revision from to Alembic: F-Curve cache modifier to lookup arbitrary property animations..Oct 30 2016, 6:39 AM
Kévin Dietrich (kevindietrich) updated this object.
Joshua Leung (aligorith) requested changes to this revision.Oct 30 2016, 7:49 AM

I'm missing a bit of detail here about the nature of the cache. One thing to be very careful about if you do implement a list of pointers to FCurves is that those references can get invalid/not linked correctly after doing:

  1. Undo
  2. Some combination of library linking madness
source/blender/alembic/intern/abc_camera.cc
219

For what you're doing here, it might be more direct to just call: verify_adt_action() followed by verify_fcurve()
So, something like:

bAction *act = verify_adt_action(&bcam->id, true);
FCurve *fcu = verify_fcurve(act, NULL, path, -1, true);

EDIT: Was there any reason why you had it add a keyframe, because it isn't really necessary to have a keyframe to be able to add FModifiers.

253

Better to use a constant or sizeof(cache_mod->property) instead of hardcoding this here?

source/blender/alembic/intern/abc_object.h
168

Er... what's going on here with the comments? Is this to get around some complier warnings about unused vars?

I'm not sure about the C++ side of things, but usually we use the UNUSED() macros around these kinds of vars.

source/blender/alembic/intern/alembic_capi.cc
386

Hardcoded sizes again

1095

So does this cache span across datablocks, or is it just a per-datablock kind of setup?

If cross datablock, you have to be careful about issues to undo/linking stuff if you store pointers to those FCurves anywhere...

source/blender/editors/animation/fmodifier_ui.c
576

This label seems vague: What is it that the object path/properties fields set?

I'm guessing something like: "Source Data:" might make more sense in this case then?

586

You could use BLI_findstring() here too

source/blender/makesrna/intern/rna_fcurve.c
906

You could use BLI_findstring here instead of doing this loop (it does it for you). So something like:

AlembicObjectPath *path = BLI_findstring(&cache_file->object_paths, data->object_path, offsetof(AlembicObjectPath , path));
This revision now requires changes to proceed.Oct 30 2016, 7:49 AM
Kévin Dietrich (kevindietrich) marked 6 inline comments as done.
  • Handle review points:

As said in inline comments, there is no global cache, but each CacheFile data-block has its own cache. So there shouldn't be too much issues regarding undo and library linking? It's really here to fill a dependency graph gap, where you don't know which F-Curve modifier depends on which CacheFile.

source/blender/alembic/intern/abc_camera.cc
219

I coded that like I would do it in the UI, that is first adding a keyframe and then going to the graph editor to add the F-Curve modifier. Thus there is a keyframe. (And I didn't know you could do it without one.)

source/blender/alembic/intern/abc_object.h
168

The comments were indeed for quieting warnings, but those variables were only necessary to add a keyframe, they are removed now.

source/blender/alembic/intern/alembic_capi.cc
1095

Each CacheFile data-block has its own little cache, only internal stuff, no sharing. I'll clarify the comment about this.

source/blender/editors/animation/fmodifier_ui.c
576

This label is to align with the Mesh Sequence Cache modifier and Transform Cache constraint UI. Basically since those modifiers and constraints display both the UI for the CacheFile data-block they point to and their own properties, I added these labels (along with boxes) to hint the user about which settings affect the cache files and which ones affect the modifiers and constraints individually.

So "Modifier Properties" are the properties of the F-Curve modifier, not necessarily the source data (from the Alembic file).

Here's some visual:

Ah that clears things up a lot :D

I'll take another look later when I'm back on my computer

@Joshua Leung (aligorith): any news on this?

source/blender/blenkernel/intern/fmodifier.c
1070

This is not so nice. We need the FPS value from the scene here, not sure how to get it from the f-curve modifier (if even possible), so it is hardcoded to 24.0f for now...