Anim: Unit tests for inserting keyframes #114465

Christoph Lendenfeld · 2023-11-03T17:55:08+01:00

2023-11-03 17:55:08 +01:00

Cover the current behavior of keyframe insertion with unit tests,
so the changes planned in #113278: Anim: Keyframing Rework are less likely to break things.

Unit tests added:

InsertKeyTest - very basic test to ensure keying things by name or with a keying set adds the right keys
VisualKeyingTest - check if visual keying produces the correct keyframe values
CycleAwareKeyingTest - check if cycle aware keying remaps the keyframes correctly and adds fcurve modifiers

Cover the current behavior of keyframe insertion with unit tests, so the changes planned in [#113278: Anim: Keyframing Rework](https://projects.blender.org/blender/blender/issues/113278) are less likely to break things. Unit tests added: * InsertKeyTest - very basic test to ensure keying things by name or with a keying set adds the right keys * VisualKeyingTest - check if visual keying produces the correct keyframe values * CycleAwareKeyingTest - check if cycle aware keying remaps the keyframes correctly and adds fcurve modifiers

Christoph Lendenfeld added the

Module

Animation & Rigging

label 2023-11-03 17:55:08 +01:00

Christoph Lendenfeld added 3 commits 2023-11-03 17:55:21 +01:00

b51e853183 insertion test

9112af5750 reformat

3a16b8dba6 cycle aware keying test

Christoph Lendenfeld requested review from Nathan Vegdahl 2023-11-03 17:57:38 +01:00

Christoph Lendenfeld added 1 commit 2023-11-03 18:01:59 +01:00

47ce414fe2 add class docstrings

Nathan Vegdahl requested changes 2023-11-07 12:08:31 +01:00

Nathan Vegdahl left a comment

I'm not super familiar with the Python unit testing framework, but things basically look good to me. Just some minor comments.

tests/python/bl_animation_keyframing.py

						
				@ -0,0 +14,4 @@

				def _fcurve_paths_match(fcurves: list, expected_paths: list) -> bool:

				    data_paths = list(set([fcurve.data_path for fcurve in fcurves]))

Nathan Vegdahl commented

2023-11-07 11:43:33 +01:00

I might be misunderstanding something, but the list(set()) looks redundant to me. Can't it just be [fcurve.data_path for fcurve in fcurves] stand-alone?

I might be misunderstanding something, but the `list(set())` looks redundant to me. Can't it just be `[fcurve.data_path for fcurve in fcurves]` stand-alone?

Christoph Lendenfeld commented

2023-11-07 12:23:35 +01:00

the data_path for fcurves is e.g. "location"
so when you have fcurves on location x, y and z you would get "location" 3 times within data_paths

the data_path for fcurves is e.g. "location" so when you have fcurves on location x, y and z you would get "location" 3 times within data_paths

Nathan Vegdahl commented

2023-11-07 15:33:03 +01:00

Ah, got it. Thanks for the explanation!

nathanvegdahl marked this conversation as resolved

tests/python/bl_animation_keyframing.py

						
				@ -0,0 +34,4 @@

				def _insert_by_name_test(insert_key: str, expected_paths: list):

				    bpy.ops.mesh.primitive_monkey_add()

Nathan Vegdahl commented

2023-11-07 11:54:37 +01:00

I don't feel strongly about this, so feel free to ignore if your disagree. But since we're not testing the the add object operator, I would prefer to add objects via the Python API rather than by operator, and explicitly link it to the scene collection and make it selected+active. It's more verbose, but doesn't rely on implicit side effects of operators.

And since you do this multiple times in the code, just making a function out of it would make sense. And then you can include setting the rotation mode as well.

I don't feel strongly about this, so feel free to ignore if your disagree. But since we're not testing the the add object operator, I would prefer to add objects via the Python API rather than by operator, and explicitly link it to the scene collection and make it selected+active. It's more verbose, but doesn't rely on implicit side effects of operators. And since you do this multiple times in the code, just making a function out of it would make sense. And then you can include setting the rotation mode as well.

Christoph Lendenfeld commented

2023-11-07 12:24:19 +01:00

that's a good point. will do

❤️ 1

nathanvegdahl marked this conversation as resolved

tests/python/bl_animation_keyframing.py Outdated

						
				@ -0,0 +41,4 @@

				    with bpy.context.temp_override(**_get_view3d_context()):

				        bpy.ops.anim.keyframe_insert_by_name(type=insert_key)

				    match = _fcurve_paths_match(keyed_object.animation_data.action.fcurves, expected_paths)

				    bpy.ops.object.delete(use_global=False)

Nathan Vegdahl commented

2023-11-07 11:58:32 +01:00

Similarly, I would prefer deleting the object via the Python API rather than by operator.

nathanvegdahl marked this conversation as resolved

tests/python/bl_animation_keyframing.py Outdated

						
				@ -0,0 +185,4 @@

				            self.assertEqual(len(fcurve.keyframe_points), 4)

				            for key in fcurve.keyframe_points:

				                # No keyframe should be outside the cyclic range.

				                self.assertLessEqual(key.co.x, cyclic_range_end)

Nathan Vegdahl commented

2023-11-07 12:06:08 +01:00

I think it would be better to test for the exact frame values you expect here. Additionally, it might be good to add a key at a frame earlier than the action frame range to make sure that gets mapped back in properly as well.

Christoph Lendenfeld commented

2023-11-07 12:54:01 +01:00

Good point, done that :)

nathanvegdahl marked this conversation as resolved

Christoph Lendenfeld added 2 commits 2023-11-07 12:47:02 +01:00

616078de4c Merge branch 'main' into keyframing_unittests

625b1636e9 create object without bpy.ops

Christoph Lendenfeld added 1 commit 2023-11-07 12:53:49 +01:00

689be11fe4 test against exact values for cycle test

Christoph Lendenfeld requested review from Nathan Vegdahl 2023-11-07 12:54:10 +01:00

Nathan Vegdahl requested changes 2023-11-07 15:37:37 +01:00

tests/python/bl_animation_keyframing.py Outdated

						
				@ -0,0 +94,4 @@

				    def test_visual_location_keying_set(self):

				        t_value = 1

				        bpy.ops.object.empty_add(type="PLAIN_AXES", align="WORLD", location=(t_value, t_value, t_value))