Anim: Add channel type options to the Bake Action operator #111997

Nate Rupsis · 2023-09-05T22:08:59+02:00

Nate Rupsis commented

2023-09-05 22:08:59 +02:00

Helping land this patch for @cmbasnett & @BClark

Currently, we're limited to the type of Bake Data, without any control over the channels that get baked.

With this change, the user now has a fine degree of control as to which types of f-curve data will be written to the baked action (i.e., location, rotation, scale & b-bone channels).

Recovered from: https://archive.blender.org/developer/D16481

Helping land this patch for @cmbasnett & @BClark --- Currently, we're limited to the type of Bake Data, without any control over the channels that get baked. With this change, the user now has a fine degree of control as to which types of f-curve data will be written to the baked action (i.e., location, rotation, scale & b-bone channels). ![action_bake_options.png](/attachments/47c163f4-8cc4-4e7f-8541-7995a88638a0) Recovered from: https://archive.blender.org/developer/D16481

action_bake_options.png

50 KiB

Nate Rupsis added the

Module

Animation & Rigging

label 2023-09-05 22:08:59 +02:00

Nate Rupsis added 4 commits 2023-09-05 22:09:13 +02:00

d372fa2a4c applying patch

7aabe777c3 Merge branch 'main' into anim-bake-components

b2a877aadd removing existing add paths

b5554a570d Merge branch 'main' into anim-bake-components

Nate Rupsis requested review from Nathan Vegdahl 2023-09-05 22:10:45 +02:00

Nate Rupsis requested review from Brad Clark 2023-09-05 22:10:46 +02:00

Nate Rupsis requested review from Sybren A. Stüvel 2023-09-05 22:10:59 +02:00

Nate Rupsis referenced this pull request

2023-09-07 19:01:13 +02:00

Animation: Add channel type options to the Bake Action operator #110903

Nathan Vegdahl commented

2023-09-08 11:23:29 +02:00

I'd like to re-raise my question from #110903: do we actually want separate options for location/rotation/scale, or should it just be a single "transforms" option?

To clarify:

I agree that we need to add support for baking custom attributes. But I believe that's unrelated to whether transforms are a single option vs separate location/rotation/scale options.
Further, I agree that it's useful to have fine-grained control over what gets baked, and I don't doubt there are use cases where you want to e.g. bake location and rotation but not scale. However, fully supporting fine-grained control over what gets baked cannot be done with a list of options like this—the list of options would get huge. Case in point: we could also have a full separate list of location/rotation/scale options for both object and pose. Basically, we can always just keep growing the number of options to support more use cases. But I think there needs to be a limit at some point, and an acknowledgement that to fully support fine-grained baking control we'll need to re-think how this works more broadly. Which we should definitely do, IMO, but the timeline for that is unclear.

So my question isn't whether splitting location/rotation/scale is useful or not. My question is where is that line where we stop adding more options and wait for a proper redesign. And the answer may well be that having separate loc/rot/scale options is sufficiently useful to justify adding them right now. But I just want to make sure we consider that in the larger context of option bloat.

I'd like to re-raise my question from #110903: do we actually want separate options for location/rotation/scale, or should it just be a single "transforms" option? To clarify: 1. I agree that we need to add support for baking custom attributes. But I believe that's unrelated to whether transforms are a single option vs separate location/rotation/scale options. 2. Further, I agree that it's useful to have fine-grained control over what gets baked, and I don't doubt there are use cases where you want to e.g. bake location and rotation but not scale. However, *fully* supporting fine-grained control over what gets baked cannot be done with a list of options like this—the list of options would get *huge*. Case in point: we could also have a full separate list of location/rotation/scale options for both object and pose. Basically, we can always just keep growing the number of options to support more use cases. But I think there needs to be a limit at some point, and an acknowledgement that to fully support fine-grained baking control we'll need to re-think how this works more broadly. Which we should definitely do, IMO, but the timeline for that is unclear. So my question isn't whether splitting location/rotation/scale is useful or not. My question is where is that line where we stop adding more options and wait for a proper redesign. And the answer may well be that having separate loc/rot/scale options is sufficiently useful to justify adding them right now. But I just want to make sure we consider that in the larger context of option bloat.

Sybren A. Stüvel requested changes 2023-09-11 11:12:53 +02:00

Sybren A. Stüvel left a comment

I think the function argument count is getting out of hand. Better to do a cleanup pass first, moving the existing options into a class, like below, and pass that instead of the separate arguments.

@dataclass
class BakeOptions:
    only_selected: bool
    do_pose: bool
    do_object: bool
    do_visual_keying: bool
    do_constraint_clear: bool
    do_parents_clear: bool
    do_clean: bool

To keep in mind for that refactor: I don't think it makes much sense to have any default values specified in the bake_action_iter() parameters, as I don't think it's ever called without these arguments. There shouldn't be any need to sync up the UI defaults with this particular function (single responsibility principle: only one thing should be responsible for setting the defaults).

PS: please make sure that the PR description matches the Ingredients of a Pull Request.

I think the function argument count is getting out of hand. Better to do a cleanup pass first, moving the existing options into a class, like below, and pass that instead of the separate arguments. ```python @dataclass class BakeOptions: only_selected: bool do_pose: bool do_object: bool do_visual_keying: bool do_constraint_clear: bool do_parents_clear: bool do_clean: bool ``` To keep in mind for that refactor: I don't think it makes much sense to have any default values specified in the `bake_action_iter()` parameters, as I don't think it's ever called without these arguments. There shouldn't be any need to sync up the UI defaults with this particular function (single responsibility principle: only one thing should be responsible for setting the defaults). PS: please make sure that the PR description matches the [Ingredients of a Pull Request](https://wiki.blender.org/wiki/Process/Contributing_Code#Ingredients_of_a_Pull_Request).

Nate Rupsis commented

2023-09-13 16:56:54 +02:00

I think the function argument count is getting out of hand. Better to do a cleanup pass first, moving the existing options into a class, like below, and pass that instead of the separate arguments.

@dr.sybren for creating data classes (types), where should those live? Obviously we'd be using it anim_utils.py, but bake_action_objects is called from anim.py, and I'd assume we'd want to clean up bake_action_objects's parameters too?

> I think the function argument count is getting out of hand. Better to do a cleanup pass first, moving the existing options into a class, like below, and pass that instead of the separate arguments. @dr.sybren for creating data classes (types), where should those live? Obviously we'd be using it anim_utils.py, but `bake_action_objects` is called from `anim.py`, and I'd assume we'd want to clean up `bake_action_objects`'s parameters too?

Sybren A. Stüvel commented

2023-09-15 10:57:21 +02:00

anim.py imports anim_utils.py, so IMO the class(es) should be in anim_utils.py to avoid circular dependencies.

`anim.py` imports `anim_utils.py`, so IMO the class(es) should be in `anim_utils.py` to avoid circular dependencies.

Nate Rupsis added 4 commits 2023-09-20 00:02:40 +02:00

66d2c975a1 Merge branch 'main' into anim-bake-components

b40d27a436 inital dataclass

50fad770c5 Merge branch 'main' into anim-bake-components

671f78b463 adding in dataclass BakeOptions

Nate Rupsis changed title from ~~Animation: Add channel type options to the Bake Action operator~~ to Anim: Add channel type options to the Bake Action operator

2023-09-20 00:06:02 +02:00

Sybren A. Stüvel requested changes 2023-09-21 17:30:34 +02:00

Sybren A. Stüvel left a comment

Looking much better!

Too bad it doesn't work any more ;-)

Error: Python: Traceback (most recent call last):
  File "build_linux/bin/4.0/scripts/startup/bl_operators/anim.py", line 306, in execute
    actions = anim_utils.bake_action_objects(
  File "build_linux/bin/4.0/scripts/modules/bpy_extras/anim_utils.py", line 96, in bake_action_objects
    iter.send(None)
  File "build_linux/bin/4.0/scripts/modules/bpy_extras/anim_utils.py", line 120, in bake_action_objects_iter
    iter.send(None)
  File "build_linux/bin/4.0/scripts/modules/bpy_extras/anim_utils.py", line 256, in bake_action_iter
    if not (do_pose or bake_options.do_object):
UnboundLocalError: local variable 'do_pose' referenced before assignment

I think it's better to split apart the PR into a non-functional part (extraction of BakeOptions) and a functional part (the actual functional changes in this PR).

Looking much better! Too bad it doesn't work any more ;-) ``` Error: Python: Traceback (most recent call last): File "build_linux/bin/4.0/scripts/startup/bl_operators/anim.py", line 306, in execute actions = anim_utils.bake_action_objects( File "build_linux/bin/4.0/scripts/modules/bpy_extras/anim_utils.py", line 96, in bake_action_objects iter.send(None) File "build_linux/bin/4.0/scripts/modules/bpy_extras/anim_utils.py", line 120, in bake_action_objects_iter iter.send(None) File "build_linux/bin/4.0/scripts/modules/bpy_extras/anim_utils.py", line 256, in bake_action_iter if not (do_pose or bake_options.do_object): UnboundLocalError: local variable 'do_pose' referenced before assignment ``` I think it's better to split apart the PR into a non-functional part (extraction of `BakeOptions`) and a functional part (the actual functional changes in this PR).

👍 1 👀 1

scripts/modules/bpy_extras/anim_utils.py Outdated

													
				@ -126,2 +138,2 @@

				        do_parents_clear=False,

				        do_clean=False

				        bake_options: BakeOptions

				        # only_selected=False,

Sybren A. Stüvel commented

2023-09-21 17:22:49 +02:00

If this is to be kept, please add a comment that explains why.

nrupsis marked this conversation as resolved

scripts/modules/bpy_extras/anim_utils.py Outdated

													
				@ -148,6 +170,14 @@ def bake_action_iter(

				    :type do_parents_clear: bool

				    :arg do_clean: Remove redundant keyframes after baking.

				    :type do_clean: bool

				    :arg do_location: Bake location channels.

Sybren A. Stüvel commented

2023-09-21 17:23:35 +02:00

These arguments no longer exist. Explanation of which option means what could go into BakeOptions.

These arguments no longer exist. Explanation of which option means what could go into `BakeOptions`.

nrupsis marked this conversation as resolved

scripts/startup/bl_operators/anim.py Outdated

													
				@ -257,3 +269,4 @@

				        from bpy_extras import anim_utils

				        do_pose = 'POSE' in self.bake_types

				        do_object = 'OBJECT' in self.bake_types

				        do_location = 'LOCATION' in self.channel_types

Sybren A. Stüvel commented

2023-09-21 17:27:01 +02:00

Because these aren't used here directly, I think it's clearer to just have do_location='LOCATION' in self.channel_types in the anim_utils.BakeOptions(...) call.

Because these aren't used here directly, I think it's clearer to just have `do_location='LOCATION' in self.channel_types` in the `anim_utils.BakeOptions(...)` call.

nrupsis marked this conversation as resolved

Nate Rupsis added 1 commit 2023-09-29 03:12:27 +02:00

bfa588e598 updating with main

Nate Rupsis requested review from Sybren A. Stüvel 2023-09-29 03:13:21 +02:00

Sybren A. Stüvel approved these changes 2023-09-29 11:21:00 +02:00

Sybren A. Stüvel left a comment

LGTM!

Nate Rupsis referenced this issue from a commit

2023-09-29 15:38:32 +02:00

Anim: Add channel type options to the Bake Action operator

Nate Rupsis merged commit dd5b870d15 into main

2023-09-29 15:38:32 +02:00

Nate Rupsis deleted branch anim-bake-components

2023-09-29 15:38:33 +02:00

Sybren A. Stüvel commented

2023-09-29 16:12:20 +02:00

My question is where is that line where we stop adding more options and wait for a proper redesign.

I think this step is pretty reasonable. So let's consider this the last extension of the current code. It's been stretched far enough.

> My question is where is that line where we stop adding more options and wait for a proper redesign. I think this step is pretty reasonable. So let's consider this the last extension of the current code. It's been stretched far enough.

Nate Rupsis referenced this pull request

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

Anim: Action bake custom properties #113208

Vasilis-Milios referenced this issue from a commit

2023-10-04 14:15:43 +02:00

Anim: Add channel type options to the Bake Action operator

Vasilis-Milios referenced this issue from a commit

2023-10-04 14:26:47 +02:00

Anim: Add channel type options to the Bake Action operator

Richard Antalik referenced this issue from a commit

2023-10-04 19:17:52 +02:00

Anim: Add channel type options to the Bake Action operator