Anim: Action bake custom properties #113208

Nate Rupsis · 2023-10-03T15:16:11+02:00

Nate Rupsis commented

2023-10-03 15:16:11 +02:00

With the new ability to select channel types while baking an action (#111997), we would like the ability to bake custom properties as well.

@BClark gives a good overview of the issue here, but in short, without the ability to bake down custom properties (IK/FK switching, etc), a custom rig is difficult to work with in the NLA.

This change adds custom properties as an option for baking an action:

For:

objects: this will bake all animatable custom properties.
Armatures: this will bake all bone custom properties, as well as object (armature) custom properties.

With the new ability to select channel types while baking an action (#111997), we would like the ability to bake custom properties as well. @BClark gives a good overview of the issue [here](https://vimeo.com/503261365), but in short, without the ability to bake down custom properties (IK/FK switching, etc), a custom rig is difficult to work with in the NLA. This change adds custom properties as an option for baking an action: ![image](/attachments/977b062f-d41d-4b45-9691-21cf156d2c3b) For: * objects: this will bake all animatable custom properties. * Armatures: this will bake all bone custom properties, as well as object (armature) custom properties.

image.png

270 KiB

Nate Rupsis added the

Module

Animation & Rigging

label 2023-10-03 15:16:11 +02:00

Nate Rupsis added 3 commits 2023-10-03 15:16:25 +02:00

43992710c9 initia; code chagnes

8978fc8a99 syncing with main

buildbot/vexp-code-patch-coordinator Build done. Details

d09eac2af7 Merge branch 'main' into Action-bake-custom-properties

Nate Rupsis commented

2023-10-03 15:16:37 +02:00

@blender-bot package

Blender Bot commented

2023-10-03 15:16:40 +02:00

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR113208) when ready.

Nate Rupsis added 6 commits 2023-10-20 23:04:03 +02:00

58f90e43f3 Merge branch 'main' into Action-bake-custom-properties

fd947fed90 Merge branch 'main' into Action-bake-custom-properties

74da6fc70c Merge branch 'main' into Action-bake-custom-properties

28fe644e4c fixing custom properties for bones

dd951e6c4a Merge branch 'main' into Action-bake-custom-properties

buildbot/vexp-code-patch-coordinator Build done. Details

1e85d5f61d fixing custom properties for bones

Nate Rupsis requested review from Brad Clark 2023-10-20 23:04:11 +02:00

Nate Rupsis commented

2023-10-20 23:04:28 +02:00

@blender-bot package

Blender Bot commented

2023-10-20 23:04:32 +02:00

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR113208) when ready.

Nate Rupsis added 1 commit 2023-10-21 04:48:38 +02:00

dc8ca0305c adding in armature level custom props

Nate Rupsis changed title from ~~WIP: Action-bake-custom-properties~~ to Anim: Action bake custom properties

2023-10-31 16:22:08 +01:00

Nate Rupsis requested review from Sybren A. Stüvel 2023-10-31 16:22:22 +01:00

Nate Rupsis added 1 commit 2023-10-31 16:44:45 +01:00

buildbot/vexp-code-patch-coordinator Build done. Details

d724844494 Merge branch 'main' into Action-bake-custom-properties

Nate Rupsis commented

2023-10-31 16:44:54 +01:00

@blender-bot package

Blender Bot commented

2023-10-31 16:44:56 +01:00

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR113208) when ready.

Sybren A. Stüvel requested changes 2023-11-02 12:45:03 +01:00

Sybren A. Stüvel left a comment

👍 for the functionality!

About the implementation: the current approach, of calling thing.id_properties_ensure().to_dict(), modifies the data by ensuring there is an (empty) IDProperties struct allocated. This means that it unnecessarily increases the memory usage & potentially even blendfile size (I don't expect these empty structs to be removed automatically). I think it's better to use dict(thing.items()), as the .items() function doesn't seem to do this allocation.

It's probably better to move that to a separate function, though. That can then do some more checks on the data, to see if it's really worth baking at all. Dictionaries, lists, and ID pointers cannot, and thus could be skipped from the to-be-baked dictionary. Maybe calling id_properties_ui(key) is a nice way to check this, but also it could have a negative impact on baking performance if this has to be called for every key (and have a catch for the TypeError it can throw).

The calls to pbone.keyframe_insert(f'[\"{key}\"]', ... need some extra escaping, as the key itself can contain quotes. Use bpy.utils.escape_identifier(key) for that.

Also double quotes in single-quoted strings don't need escaping, so f'["{escaped_key}"]' should work.

The code for applying the custom properties could be moved to a small function, as apart from the receiving end (pbone or obj) it seems to do pretty much the same thing everywhere.

:+1: for the functionality! About the implementation: the current approach, of calling `thing.id_properties_ensure().to_dict()`, modifies the data by ensuring there is an (empty) `IDProperties` struct allocated. This means that it unnecessarily increases the memory usage & potentially even blendfile size (I don't expect these empty structs to be removed automatically). I think it's better to use `dict(thing.items())`, as the `.items()` function doesn't seem to do this allocation. It's probably better to move that to a separate function, though. That can then do some more checks on the data, to see if it's really worth baking at all. Dictionaries, lists, and ID pointers cannot, and thus could be skipped from the to-be-baked dictionary. Maybe calling `id_properties_ui(key)` is a nice way to check this, but also it could have a negative impact on baking performance if this has to be called for every key (and have a catch for the `TypeError` it can throw). The calls to `pbone.keyframe_insert(f'[\"{key}\"]', ...` need some extra escaping, as the key itself can contain quotes. Use `bpy.utils.escape_identifier(key)` for that. Also double quotes in single-quoted strings don't need escaping, so `f'["{escaped_key}"]'` should work. The code for applying the custom properties could be moved to a small function, as apart from the receiving end (`pbone` or `obj`) it seems to do pretty much the same thing everywhere.

scripts/modules/bpy_extras/anim_utils.py Outdated

						
				@ -381,0 +409,4 @@

				                            pbone[key] = value

				                            pbone.keyframe_insert(f'[\"{key}\"]', index=-1, frame=f, group=name)

				                        except TypeError:

				                            # Non animatable properties (datablocks, etc) cannot be keyed.

Sybren A. Stüvel commented

2023-11-02 12:39:16 +01:00

The explanation is good, and also means that pbone[key] = value should be taken out of the try block. Same below.

The explanation is good, and also means that `pbone[key] = value` should be taken out of the `try` block. Same below.

nrupsis marked this conversation as resolved

Sybren A. Stüvel commented

2023-11-03 14:27:33 +01:00

Here's an example file that fails because of quotes in the custom property name.

113208-anim-customprop-baking.blend

864 KiB

👍 1

Nate Rupsis added 2 commits 2023-11-09 03:35:38 +01:00

f969aa3652 using dict(*.items())

buildbot/vexp-code-patch-coordinator Build done. Details

7cd7a3cff5 cleaning custom proerties to be python types for baking.

Nate Rupsis commented

2023-11-09 03:38:58 +01:00

Maybe calling id_properties_ui(key) is a nice way to check this, but also it could have a negative impact on baking performance if this has to be called for every key (and have a catch for the TypeError it can throw).

Gave that a try, but for non animatable properties, it still returns an obj. So not very useful.

>Maybe calling `id_properties_ui(key)` is a nice way to check this, but also it could have a negative impact on baking performance if this has to be called for every key (and have a catch for the `TypeError` it can throw). Gave that a try, but for non animatable properties, it still returns an obj. So not very useful.

Nate Rupsis requested review from Nathan Vegdahl 2023-11-09 18:16:17 +01:00

Nate Rupsis commented

2023-11-09 18:16:36 +01:00

@blender-bot package

Blender Bot commented

2023-11-09 18:16:39 +01:00

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR113208) when ready.

Nathan Vegdahl requested changes 2023-11-30 12:17:44 +01:00

Nathan Vegdahl left a comment

Just one point of confusion, but otherwise looks good to me.

scripts/modules/bpy_extras/anim_utils.py Outdated

						
				@ -198,0 +210,4 @@

				        return clean_props

				    def bake_custom_properties(obj, custom_props={}, frame=None, name=""):

				        if (f is None or custom_props is None):

Nathan Vegdahl commented

2023-11-30 12:14:15 +01:00

Where is f defined? Is this supposed to be frame?

Where is `f` defined? Is this supposed to be `frame`?

Nate Rupsis commented

2023-12-01 23:36:35 +01:00

Oops, looks like that was a miss on a refactoring pass. I'm surprised Python didn't complain about it 🤷‍♂️

Nathan Vegdahl commented

2023-12-05 11:20:11 +01:00

Yeah, that's just something that happens sometimes in dynamic languages like Python. Glad I could help! :-)

nrupsis marked this conversation as resolved

Nate Rupsis added 2 commits 2023-12-02 00:15:56 +01:00

9894cc28e5 Merge branch 'main' into Action-bake-custom-properties

buildbot/vexp-code-patch-coordinator Build done. Details

ecf6683eab fixing f -> frame issue

Nate Rupsis requested review from Sybren A. Stüvel 2023-12-02 00:16:46 +01:00

Nate Rupsis requested review from Nathan Vegdahl 2023-12-02 00:16:50 +01:00

Nate Rupsis commented

2023-12-02 00:17:01 +01:00

@blender-bot package

Blender Bot commented

2023-12-02 00:17:04 +01:00

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR113208) when ready.

Nathan Vegdahl approved these changes 2023-12-05 11:34:34 +01:00

Nathan Vegdahl left a comment

Took a quick look at the code again, and tested it out as well. Looks good to me!

But maybe @dr.sybren would like one more look before merging?

Took a quick look at the code again, and tested it out as well. Looks good to me! But maybe @dr.sybren would like one more look before merging?

Sybren A. Stüvel requested changes 2023-12-08 11:36:21 +01:00

Sybren A. Stüvel left a comment

Just a few smaller things.

scripts/modules/bpy_extras/anim_utils.py Outdated

						
				@ -198,0 +206,4 @@

				    def clean_custom_properties(obj):

				        clean_props = {}

				        for key, value in obj.items():

				            clean_props.update({key: rna_idprop_value_to_python(value)})

Sybren A. Stüvel commented

2023-12-08 11:28:51 +01:00

This can be written as dict comprehension:

        clean_props = {
            key: rna_idprop_value_to_python(value)
            for key, value in obj.items()
        }

which I think is easier to read in this case.

This can be written as dict comprehension: ```python clean_props = { key: rna_idprop_value_to_python(value) for key, value in obj.items() } ``` which I think is easier to read in this case.

scripts/modules/bpy_extras/anim_utils.py Outdated

						
				@ -198,0 +210,4 @@

				        return clean_props

				    def bake_custom_properties(obj, custom_props={}, frame=None, name=""):

				        if (frame is None or custom_props is None):

Sybren A. Stüvel commented

2023-12-08 11:29:26 +01:00

No parentheses, this isn't C ;)

Also, given that the for-loop doesn't do anything when custom_props is empty, you can reduce the condition to:

if frame is None or not custom_props:
    return

Having said that, why does frame have a default value that causes the function to not run? That doesn't make much sense to me. The same goes for custom_props by the way; that default value makes the function a no-op too.

If you want to force frame to be passed as keyword argument, this is not the way to do that. Instead, write:

def bake_custom_properties(obj, *, custom_props, frame, name=""):

Finally, the name parameter is passed as group to keyframe_insert(). It's probably better to rename the parameter to group_name so that it's clear what's getting named.

No parentheses, this isn't C ;) Also, given that the for-loop doesn't do anything when `custom_props` is empty, you can reduce the condition to: ```python if frame is None or not custom_props: return ``` Having said that, why does `frame` have a default value that causes the function to not run? That doesn't make much sense to me. The same goes for `custom_props` by the way; that default value makes the function a no-op too. If you want to force `frame` to be passed as keyword argument, this is not the way to do that. Instead, write: ```python def bake_custom_properties(obj, *, custom_props, frame, name=""): ``` Finally, the `name` parameter is passed as `group` to `keyframe_insert()`. It's probably better to rename the parameter to `group_name` so that it's clear what's getting named.

Nate Rupsis commented

2023-12-11 20:57:10 +01:00

No parentheses, this isn't C ;)

But with a little more Syntactical structure, and type checking it could be 😄

Having said that, why does frame have a default value that causes the function to not run? That doesn't make much sense to me. The same goes for custom_props by the way; that default value makes the function a no-op too.

That's a great question, and I wish I knew what last month me was thinking. I remember running into an edge case where the frame was None that caused an issues. Most likely a result of incoherent debugging sessions 🤷‍♂️

> No parentheses, this isn't C ;) But with a little more Syntactical structure, and type checking it _could_ be 😄 > Having said that, why does frame have a default value that causes the function to not run? That doesn't make much sense to me. The same goes for custom_props by the way; that default value makes the function a no-op too. That's a great question, and I wish I knew what last month me was thinking. I remember running into an edge case where the frame was `None` that caused an issues. Most likely a result of incoherent debugging sessions 🤷‍♂️

scripts/modules/bpy_extras/anim_utils.py Outdated

						
				@ -213,0 +243,4 @@

				    def armature_frame_info(obj):

				        custom_props = {}

				        if obj.type == 'ARMATURE':

Sybren A. Stüvel commented

2023-12-08 11:34:40 +01:00

I'd swap this around, and make this a proper precondition check:

    def armature_frame_info(obj):
        if obj.type != 'ARMATURE':
            return {}
        return clean_custom_properties(obj)

I'd swap this around, and make this a proper precondition check: ```python def armature_frame_info(obj): if obj.type != 'ARMATURE': return {} return clean_custom_properties(obj) ```

Nate Rupsis added 2 commits 2023-12-12 18:54:07 +01:00

8703319e21 Merge branch 'main' into Action-bake-custom-properties

3a2d1aedce PR comment cleanup

Sybren A. Stüvel approved these changes 2023-12-21 16:50:22 +01:00

Sybren A. Stüvel left a comment

LGTM!

Nate Rupsis added 2 commits 2023-12-29 17:56:07 +01:00

98fa442f34 resolve merge conflict

079d016c37 Merge branch 'main' into Action-bake-custom-properties

Nate Rupsis merged commit 4ddb52a775 into main

2023-12-29 17:59:36 +01:00

Nate Rupsis referenced this issue from a commit