Refactor: Move NLA anim bake paremeters into dataclass #112865

Nate Rupsis · 2023-09-25T19:03:56+02:00

Nate Rupsis commented

2023-09-25 19:03:56 +02:00

The bake_action (and subsequent methods) for NLA action baking have a long parameter lists. Refactoring to move the bake options into a data class.

The `bake_action` (and subsequent methods) for NLA action baking have a long parameter lists. Refactoring to move the bake options into a data class.

Nate Rupsis added 2 commits 2023-09-25 19:04:10 +02:00

b694acf22b refactoring anim bake_options

3a4e6a443d Merge branch 'main' into refactor-anim-bake-components

Nate Rupsis requested review from Sybren A. Stüvel 2023-09-25 19:04:18 +02:00

Sybren A. Stüvel requested changes 2023-09-26 10:07:32 +02:00

Sybren A. Stüvel left a comment

I guess you missed moving an option:

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

It's this code:

    if obj.pose is None:
        do_pose = False

    if not (do_pose or bake_options.do_object):
        raise Exception("Pose and object baking is disabled, no action needed")

which is missing an initialisation:

do_pose = bake_options.do_pose

I guess you missed moving an option: ```py Error: Python: Traceback (most recent call last): File "build_linux/bin/4.0/scripts/startup/bl_operators/anim.py", line 284, in execute actions = anim_utils.bake_action_objects( File "build_linux/bin/4.0/scripts/modules/bpy_extras/anim_utils.py", line 109, in bake_action_objects iter.send(None) File "build_linux/bin/4.0/scripts/modules/bpy_extras/anim_utils.py", line 133, in bake_action_objects_iter iter.send(None) File "build_linux/bin/4.0/scripts/modules/bpy_extras/anim_utils.py", line 238, in bake_action_iter if not (do_pose or bake_options.do_object): UnboundLocalError: local variable 'do_pose' referenced before assignment ``` It's this code: ```py if obj.pose is None: do_pose = False if not (do_pose or bake_options.do_object): raise Exception("Pose and object baking is disabled, no action needed") ``` which is missing an initialisation: ``` do_pose = bake_options.do_pose ```

scripts/modules/bpy_extras/anim_utils.py

						
				@ -32,2 +33,4 @@

				"""

				:arg only_selected: Only bake selected bones.

Sybren A. Stüvel commented

2023-09-26 10:04:13 +02:00

This style of documentation isn't necessary any more. You can document like this:

@dataclass
class BakeOptions:
    only_selected: bool
    """Only bake selected bones."""

    do_pose: bool
    """Bake pose channels."""

+1 for using a dataclass for this!

This style of documentation isn't necessary any more. You can document like this: ```py @dataclass class BakeOptions: only_selected: bool """Only bake selected bones.""" do_pose: bool """Bake pose channels.""" ``` +1 for using a dataclass for this!

👍 1

dr.sybren marked this conversation as resolved

Nate Rupsis commented

2023-09-26 18:41:36 +02:00

@dr.sybren

I guess you missed moving an option:
...

Hmm, did you do anything special to get that stack trace? I double checked the output & python console, and didn't see that.
Obviously I missed this, but just curious why I missed it. I appreciate you checking!

@dr.sybren > I guess you missed moving an option: > ... Hmm, did you do anything special to get that stack trace? I double checked the output & python console, and didn't see that. Obviously I missed this, but just curious _why_ I missed it. I appreciate you checking!

Nate Rupsis added 2 commits 2023-09-26 19:07:52 +02:00

d51931a34a Merge branch 'main' into refactor-anim-bake-components

5f1a8c0368 missed do_pose from bakeOptions. Cleanup documentation onf BakeOptions Dataclass

Sybren A. Stüvel commented

2023-09-28 14:59:53 +02:00

Hmm, did you do anything special to get that stack trace? I double checked the output & python console, and didn't see that.
Obviously I missed this, but just curious why I missed it. I appreciate you checking!

No idea what I did, just disabled some options and enabled some others 😆

> Hmm, did you do anything special to get that stack trace? I double checked the output & python console, and didn't see that. > Obviously I missed this, but just curious _why_ I missed it. I appreciate you checking! No idea what I did, just disabled some options and enabled some others 😆

😆 1

Sybren A. Stüvel approved these changes 2023-09-28 15:06:31 +02:00

Sybren A. Stüvel left a comment

Looking good!

Nate Rupsis merged commit 5024c0ef97 into main

2023-09-28 16:17:36 +02:00

Nate Rupsis referenced this issue from a commit

2023-09-28 16:17:38 +02:00

Refactor: Move NLA anim bake paremeters into dataclass

Nate Rupsis deleted branch refactor-anim-bake-components

2023-09-28 16:17:38 +02:00

Vasilis-Milios referenced this issue from a commit

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

Refactor: Move NLA anim bake paremeters into dataclass

Vasilis-Milios referenced this issue from a commit

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

Refactor: Move NLA anim bake paremeters into dataclass

Richard Antalik referenced this issue from a commit

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

Refactor: Move NLA anim bake paremeters into dataclass

Colin Basnett referenced this issue from a commit

2023-10-08 23:38:51 +02:00

Refactor: Move NLA anim bake paremeters into dataclass

Damien Picard commented

2023-11-20 22:24:13 +01:00

@nrupsis Hi, just stumbled upon this change. I think it would make sense to specify a default value for each option in the BakeOptions class:

@dataclass
class BakeOptions:
    only_selected: bool = False
    """Only bake selected bones."""

    do_pose: bool = True
    """Bake pose channels"""

    do_object: bool = True
    """Bake objects."""

    do_visual_keying: bool = True
    """Use the final transformations for baking ('visual keying')."""

    do_constraint_clear: bool = False
    """Remove constraints after baking."""

    ...

That way you wouldn’t have to specify every single option on each invocation, and the API wouldn’t break if any option is added. We’d get:

bake_action(
    bpy.context.object, action=bpy.data.actions["test"],
    frames=list(range(10)), bake_options=BakeOptions(do_location=True)
)

instead of:

bake_action(
    bpy.context.object, action=bpy.data.actions["test"],
    frames=list(range(10)), bake_options=BakeOptions(only_selected=False,
                                                     do_pose=False, do_object=True, do_visual_keying=False,
                                                     do_constraint_clear=False, do_parents_clear=False, do_clean=False,
                                                     do_location=True, do_rotation=False, do_scale=False, do_bbone=False)
)

What do you think?

@nrupsis Hi, just stumbled upon this change. I think it would make sense to specify a default value for each option in the BakeOptions class: ```python @dataclass class BakeOptions: only_selected: bool = False """Only bake selected bones.""" do_pose: bool = True """Bake pose channels""" do_object: bool = True """Bake objects.""" do_visual_keying: bool = True """Use the final transformations for baking ('visual keying').""" do_constraint_clear: bool = False """Remove constraints after baking.""" ... ``` That way you wouldn’t have to specify every single option on each invocation, and the API wouldn’t break if any option is added. We’d get: ```python bake_action( bpy.context.object, action=bpy.data.actions["test"], frames=list(range(10)), bake_options=BakeOptions(do_location=True) ) ``` instead of: ```python bake_action( bpy.context.object, action=bpy.data.actions["test"], frames=list(range(10)), bake_options=BakeOptions(only_selected=False, do_pose=False, do_object=True, do_visual_keying=False, do_constraint_clear=False, do_parents_clear=False, do_clean=False, do_location=True, do_rotation=False, do_scale=False, do_bbone=False) ) ``` What do you think?

Nate Rupsis commented

2023-11-27 18:44:13 +01:00

@nrupsis Hi, just stumbled upon this change. I think it would make sense to specify a default value for each option in the BakeOptions class:

...

What do you think?

My preference is to keep data classes clean of any logic, but in this instance it might make sense. @dr.sybren do you have any preferences?

> @nrupsis Hi, just stumbled upon this change. I think it would make sense to specify a default value for each option in the BakeOptions class: > > ... > > What do you think? My preference is to keep data classes clean of any logic, but in this instance it might make sense. @dr.sybren do you have any preferences?

👍 1

Sybren A. Stüvel commented

2023-12-15 14:47:28 +01:00

I think in principle this is fine. The issue that I do see is that, with this proposal, there are two sets of defaults: the ones in this dataclass, and the default values of the actual properties shown in the GUI.

If these dataclass defaults are only used in Python code, and their only purpose is to keep the code short (and readable, which is important), then we might want to choose different defaults here. I think it makes sense to default most-if-not-all options to False, so that the ones that are active are all explicitly specified. Otherwise, in order to fully understand what code is doing, you'd have to know all the defaults by heart.

So that would mean passing bake_options=BakeOptions() causes nothing to be baked, and BakeOptions(do_object=True, do_location=True) only bakes object locations normally (i.e. non-visually). This in contrast to the proposed example, which in the latter case would create visual keys.

I think in principle this is fine. The issue that I do see is that, with this proposal, there are two sets of defaults: the ones in this dataclass, and the default values of the actual properties shown in the GUI. If these dataclass defaults are only used in Python code, and their only purpose is to keep the code short (and readable, which is important), then we might want to choose different defaults here. I think it makes sense to default most-if-not-all options to `False`, so that the ones that are active are all explicitly specified. Otherwise, in order to fully understand what code is doing, you'd have to know all the defaults by heart. So that would mean passing `bake_options=BakeOptions()` causes nothing to be baked, and `BakeOptions(do_object=True, do_location=True)` **only** bakes object locations normally (i.e. non-visually). This in contrast to the proposed example, which in the latter case would create visual keys.

Damien Picard commented

2023-12-15 14:55:24 +01:00

This in contrast to the proposed example, which in the latter case would create visual keys.

I proposed these simply because they were the default before this commit, but in fact I agree that the defaults should not being opinionated.

if their only purpose is to keep the code short (and readable, which is important), then we might want to choose different defaults here.

It also avoids script breakage in case new options are added, but your point stands.

> This in contrast to the proposed example, which in the latter case would create visual keys. I proposed these simply because they were the default before this commit, but in fact I agree that the defaults should not being opinionated. > if their only purpose is to keep the code short (and readable, which is important), then we might want to choose different defaults here. It also avoids script breakage in case new options are added, but your point stands.

Zdenek Dolezal referenced this pull request

2024-04-24 17:10:09 +02:00

Change of params of `anim_utils.bake_action` to datacass introduced in 4.1 isn't documented as breaking change #121036

Sign in to join this conversation.

No reviewers