[sw-2167] fix inconsistent specification of arm upon upload animation

[gaddison-bdai]

Previously, animation uploads to BD's choreo client would occasionally fail as the animation keyframe protobuf would omit arm joint_angles fields (probably due to value = 0). In this PR, I add a step in AnimationBuilder validate() which fills in missing joint_angle fields in the protobuf, ensuring that this issue won't occur so long as you build() the animation prior to uploading.

One point of note is that in the build() step, I moved the validate() to be prior to the output_animation = copy.deepcopy(self._animation) so that my code that fixes the omitted pb fields actually affects the animation that is outputted by build().

Additionally, while this fixes the Omitted joint angle issue, it does not verify all of the other non-joint angle fields. Though I haven't yet encountered issues with those fields, they may be worth validating further down the road.

[kkarol-bdai]

As written this will probably cause issues when playing animations on a spot without an arm. It would also be good to keep this function as a pure validation check, and do the fix in a separate function like _remove_duplicate_timestamp.

Since the BD requirement is any joint angle that exists in one keyframe must exist in all keyframes, you could grab the set of keys in the first keyframe, and then for every other frame you can fail if there are any missing/extra keys.

To do the automatic fix you could do it in two loops (since validation check is fast no major need to that optimize this to a single loop)

• Loop 1, collect a set of joint_angle fields that exist in the animation (similar to what you have hardcoded here)
• Loop 2, for any fields that are missing in any keyframe, set to near-zero value

Some links that might be relevant:

spot_choreo_utils/spot_choreo_utils/spot_choreo_utils/choreo_creation/choreo_builders/animation_operators.py

Lines 282 to 302 in 105dbd9

	def flatten_keyframe_to_dictionary(keyframe: AnimationKeyframe) -> dict[str, float]:
	"""
	Recursively walks through the keyframe to extract all of the set values
	"""
	keyframe_to_proto_attrs = joint_angle_to_protobuf_attrs_map()
	flattened_map = {}
	for animation_name, nested_proto_attrs in keyframe_to_proto_attrs.items():
	active_proto = keyframe
	extraction_success = True
	for attribute in nested_proto_attrs:
	if check_if_protobuf_field_set(active_proto, attribute):
	active_proto = getattr(active_proto, attribute)
	else:
	extraction_success = False
	if extraction_success:
	flattened_map[animation_name] = active_proto
	else:
	flattened_map[animation_name] = 0
	return flattened_map

https://github.com/bdaiinstitute/spot_choreo_utils/blob/main/spot_choreo_utils/spot_choreo_utils/choreo_creation/choreo_builders/spot_properties.py#L40-L71

spot_choreo_utils/spot_choreo_utils/spot_choreo_utils/choreo_creation/choreo_builders/animation_proto_utils.py

Lines 20 to 26 in 105dbd9

	def protobuf_zero_omission_check(value: float) -> bool:
	"""
	Returns whether the value would be omitted by protobuf when sent over the wire
	"""
	if value == 0.0 and math.copysign(1, value) == 1.0:
	return True
	return False

spot_choreo_utils/spot_choreo_utils/spot_choreo_utils/choreo_creation/choreo_builders/animation_operators.py

Lines 196 to 226 in 105dbd9

	def perform_keyframe_interpolation(
	start: AnimationKeyframe,
	stop: AnimationKeyframe,
	modified: AnimationKeyframe,
	percent: float,
	logger: Optional[Logger] = None,
	) -> bool:
	"""Interpolates between two animation keyframes by the requested percent"""
	is_protobuf = hasattr(start, "ListFields")
	if is_protobuf:
	properties = [descriptor.name for descriptor, _ in start.ListFields()]
	for property in properties:
	start_ref = getattr(start, property, None)
	stop_ref = getattr(stop, property, None)
	modified_ref = getattr(modified, property, None)
	result = perform_keyframe_interpolation(start_ref, stop_ref, modified_ref, percent)
	# If interpolation not performed at nested level, use set attribute at this
	# level to update the interpolation
	if not result:
	if isinstance(stop_ref, float) and isinstance(start_ref, float):
	interpolated_value = (stop_ref - start_ref) * percent + start_ref
	elif isinstance(stop_ref, bool) and isinstance(start_ref, bool):
	interpolated_value = start_ref
	else:
	if logger:
	logger.error(f"Unknown type {type(stop_ref)} for interpolation")
	raise AssertionError()
	setattr(modified, property, interpolated_value)
	return is_protobuf

Also, Gripper values need to be between -1 and 0, so for that field you have to set -1e-6 instead of 1e-6
https://github.com/bdaiinstitute/spot_choreo_utils/blob/main/spot_choreo_utils/spot_choreo_utils/choreo_creation/choreo_builders/animation_builder.py#L467

[gaddison-bdai]

Ok, thanks for this! I am taking a look now.

[gaddison-bdai]

Addressed most of this here @kkarol-bdai
The scope of this issue is just the joint_angles fields of the "arm" field, and the gripper_angle is part of its own "gripper" field. If you think we should include that as well, though, let me know!

[gaddison-bdai]

Alright, I redid the implementation to to find and fill in all fields, not just arm joint_angle fields.

Two things to note:

1. I used flatten_keyframe_to_dictionary as @kkarol-bdai suggested, but had to paste its definition into animation_builder.py to avoid a circular import. If there is a more preferred way of handling this, I am open to suggestions!
2. Currently, the arm fields will always at least be filled in with non-zero protobuf values. We may need to more properly handle the omission of arm fields if we implement a version of web_animator that works for armless spots, but that may not be necessary right now. @kkarol-bdai thoughts?

Any other thoughts and suggestions welcome!

[kkarol-bdai]

Are this and the spot_properties updates to account for naming in the web_animator? Could we update the web animator to use start_time instead?

[gaddison-bdai]

The update to spot_properties I believe was because there previously was no "time" field returned in the attribute map, which would propagate through until joint_angle_to_keyframe_proto() where the lack of a correct "time" field would cause the keyframe to be skipped altogether
# Set the Keyframe time if "start_time" in keyframe_angles.keys(): keyframe_proto.time = keyframe_angles["start_time"] if "time" in keyframe_angles.keys(): keyframe_proto.time = keyframe_angles["time"] elif start_time is not None: keyframe_proto.time = start_time else: print("No start time specified, skipping joint angle keyframe") return None

[gaddison-bdai]

I think the reason I had to include "time" was because that is what spot-sdk's AnimationKeyframe currently uses: https://dev.bostondynamics.com/protos/bosdyn/api/proto_reference.html#bosdyn-api-spot-AnimationKeyframe.
I can try to go through web_animator and choreo_utils and rename all "start_time" to "time", would that work?

[kkarol-bdai]

Alright, I redid the implementation to to find and fill in all fields, not just arm joint_angle fields.

Two things to note:

1. I used flatten_keyframe_to_dictionary as @kkarol-bdai suggested, but had to paste its definition into animation_builder.py to avoid a circular import. If there is a more preferred way of handling this, I am open to suggestions!
2. Currently, the arm fields will always at least be filled in with non-zero protobuf values. We may need to more properly handle the omission of arm fields if we implement a version of web_animator that works for armless spots, but that may not be necessary right now. @kkarol-bdai thoughts?

Any other thoughts and suggestions welcome!

Looking good, added a couple final cleanup questions.

1. To my knowledge the only way to break this cycle in python is to move the code to a new file and import into both places its needed. If there's no clean way to do that, perhaps add a comment about the fact that it's duplicated in both places
2. It looks like the updated implementation will fill the arm with non-zero protobufs for the web-animator, but it wouldn't do that for a non-arm animation from a different source correct? Totally fine that the web animator only supports creating arm animations for now, just need to make sure the core library isn't limited to only animations with arms

[gaddison-bdai]

@kkarol-bdai To answer your questions:

1. Am I clear to delete the original definition of flatten_keyframe_to_dictionary in animation_operators.py? As far as my editor's live grep and language server can tell, the original definition is not actually used at all, so I can essentially just keep it only in animation_builder.py.
2. My understanding is that AnimationBuilder.build() will always fill in arm fields with 1e-06 if necessary, and that the arm fields will always exist, since chore_utils's joint_angle_to_protobuf_attrs_map() always returns arm fields:

   spot_choreo_utils/spot_choreo_utils/spot_choreo_utils/choreo_creation/choreo_builders/spot_properties.py

   Lines 48 to 51 in 63a05d1

   	# Arms
   	arm_angle_names = joint_angle_names["arm"]
   	for arm_joint_name in arm_angle_names:
   	attrs_map[arm_joint_name] = ["arm", "joint_angles", arm_joint_name, "value"]

I will say, the AnimationKeyframe type is defined to always have an arm field. Do you think I should make it so that we fill in the arm fields with 1e-06 iff this is an armed spot? if so, it is not immediately clear to me what is the best way to check if this is an armed spot; perhaps if at least one arm field in one keyframe is non-zero we can fill in, otherwise omit. What do you think?

[kkarol-bdai]

@kkarol-bdai To answer your questions:

1. Am I clear to delete the original definition of flatten_keyframe_to_dictionary in animation_operators.py? As far as my editor's live grep and language server can tell, the original definition is not actually used at all, so I can essentially just keep it only in animation_builder.py.
2. My understanding is that AnimationBuilder.build() will always fill in arm fields with 1e-06 if necessary, and that the arm fields will always exist, since chore_utils's joint_angle_to_protobuf_attrs_map() always returns arm fields:

   spot_choreo_utils/spot_choreo_utils/spot_choreo_utils/choreo_creation/choreo_builders/spot_properties.py

   Lines 48 to 51 in 63a05d1

   	# Arms
   	arm_angle_names = joint_angle_names["arm"]
   	for arm_joint_name in arm_angle_names:
   	attrs_map[arm_joint_name] = ["arm", "joint_angles", arm_joint_name, "value"]

I will say, the AnimationKeyframe type is defined to always have an arm field. Do you think I should make it so that we fill in the arm fields with 1e-06 iff this is an armed spot? if so, it is not immediately clear to me what is the best way to check if this is an armed spot; perhaps if at least one arm field in one keyframe is non-zero we can fill in, otherwise omit. What do you think?

1. Yeah, that's fine by me
2. That's a pre-existing bug with flatten_keyframe then. I think we can just remove the else case where we set the attribute to 0 and then flattened_map will only contain keys that are actually set in the protobuf

[gaddison-bdai]

1. Cool! Done here.
2. Good idea, that seems to have worked! here.