Antoiner/action clipping

[AntoineRichard]

Description

Improves the support for clipping in the action terms. The clip parameter was removed from the ActionTermCfg, and per action term clipping is implemented where relevant. For instance, the Binary action no longer supports clipping. However, the JointActions remain unchanged. Where the biggest changes occurs is on the Holonomic, DifferentialIK, and OSC controllers. These controllers get unique clip configurations tailored to their use-cases.

@ozhanozen I know it's been a while since you posted your initial PR, though if you have time, could you check that this PR does what you'd like it to?

Fixes #1548 #1732

Type of change

• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• This change requires a documentation update

Checklist

• I have run the pre-commit checks with ./isaaclab.sh --format
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works
• I have updated the changelog and the corresponding version in the extension's config/extension.toml file
• I have added my name to the CONTRIBUTORS.md or my name already exists there

[ozhanozen]

Hi @AntoineRichard,

I’ve reviewed the OSC-related parts. They’re largely similar, but this version differs from #1856 in a few places. Below are the items I think deserve another look:

1. Axis-wise clipping
   When any type of clipping is enabled you currently require limits for all six axes. In OSC, though, the user can decide which axes are under motion or force control (motion_control_axes_task and contact_wrench_control_axes_task). A common case is controlling position in x / y while applying only a force in z. In that scenario a z-position clip is unnecessary, and even misleading: because it is ignored. Perhaps the validation should depend on those two axis-selection arguments instead.

2. pose_rel orientation indices
   In pose_rel mode the orientation delta is an axis–angle vector, not a quaternion. Therefore, you should access only
   self._pose_rel_idx + 3 : self._pose_rel_idx + 6, not ... + 7. The downstream transformations are invalid as a result.

3. Quaternion normalisation in pose_abs
   In pose_abs you call euler_xyz_from_quat, which assumes a normalised quaternion. Because you scale the quaternion just beforehand, you should re-normalise it first.

4. Frame convention for RPY limits
   The roll/pitch/yaw clip values are implied to be world/extrinsic RPY in the action class documentation. If I’m not mistaken, euler_xyz_from_quat returns extrinsic YPR, equivalently intrinsic/body RPY (which is, btw, not clearly documented in the function documentation). That mismatch might confuse users. We should either clarify in the action-config docs that these angles are body rotations or provide a helper that converts to extrinsic RPY.

5. Euler-angle parameterisation
   Euler-angle clipping can be tricky in general, because the meaning changes with rotation order. Would a delta axis-angle parameterization: clipping each component or the norm, be clearer?

6. Runtime adjustment of OSC scales/clips
   I previously added helpers to change OSC scale/clip values on-the-fly. They’re handy for curriculum learning. Could we bring them back?

Let me know what you think!

[AntoineRichard]

Thanks a lot for the feedback @ozhanozen

I factored in your comments:
1: the users now only need to specify the axes that they use.
2: Yes that was an oupsy...
3: True, should be correct now.
4: Agreed the documentation is not super clear on that. The world frame / body frame is also true of the position clipping. In relative mode the position deltas are clipped in the body frame? When the clipping is done in the world frame if using the absolute mode. I don't really like it.
5: Yes, it might be simpler.
6: It added that back in. Does that look like what you'd expect?

[ozhanozen]

Thanks a lot for the feedback @ozhanozen

I factored in your comments:

1: the users now only need to specify the axes that they use.

2: Yes that was an oupsy...

3: True, should be correct now.

4: Agreed the documentation is not super clear on that. The world frame / body frame is also true of the position clipping. In relative mode the position deltas are clipped in the body frame? When the clipping is done in the world frame if using the absolute mode. I don't really like it.

5: Yes, it might be simpler.

6: It added that back in. Does that look like what you'd expect?

Hi @AntoineRichard, thank you for your consideration and the changes.

4. I believe all the clipping in the relative mode happens in the robot base frame by default (unless you explicit set the controller to use another task frame, which is sometimes quite useful). Absolute mode could do the same, only exception being if we use Euler-angle representation and interpret the consecutive rotations to be in intrinsic frames. We can, to be consistent, enforce the clip_rotation to represent extrinsic YPR (first yaw, then pitch, then roll, all around robot base: alternatively creating a new function for extrinsic RPY).

5. Related to also 4, my issue with the orientation clipping using Euler is that the same numerical clipping results in different physical orientations depending on which Euler parameterization is used (xyz, zyx, zyz, etc). It is not unique, hence, prone to misinterpretation. Angle-axis does not have this issue as it is a single rotation, not three consecutive rotations. That being said, it is also not intuitive to come up with meaningful angle limit knowing the physical limits of the robot. Maybe we should simply drop to option to clip the orientation if the mode is absolute? I had added that earlier because there was an attempt to enforce action clipping that is inherited from the base class, but if this is no longer the case, we can also keep only the clipping that make sense and even rename them explicitly such as position_abs_clip, position_rel_clip, orientation_rel_clip, etc, so they dont function differently based on the mode. What do you think?

6. Yes, I think so. However, I haven't had the chance to test the PR in a workflow yet.

[jtigue-bdai]

@AntoineRichard thanks for taking this on. I have a few line specific comments, but one thought I had was that for the more complex actions that require very specific formulation of the clip dict(), we may want to switch to useing subclass config (similar to how we use OffsetCfgs) to help define the API and reduce the checking logic you need to do.

In general things look good a functional. I think the biggest thing will be to define some unit tests for this. I know we don't currently have mdp tests. But I think it would be good to start adding them specifically for the actions without integration to the managers.

[jtigue-bdai]

Suggested change

	"""Clip range for the action (dict of regex expressions). Defaults to None."""
	"""Clip range for the action (dict of regex expressions). Defaults to None.
	Dictionary keys should be regex expressions of joint names and values should be tuples of (lower, upper) bounds.
	"""

[jtigue-bdai]

Suggested change

	"""Clip range for the action (dict of regex expressions). Defaults to None."""
	"""Clip range for the action (dict of regex expressions). Defaults to None.
	Dictionary keys should be regex expressions of joint names and values should be tuples of (lower, upper) bounds.
	"""

[jtigue-bdai]

Suggested change

	The expected keys are "v", and "yaw". Defaults to None for no clipping.
	The expected keys are "v", and "yaw". Values are tuples of (lower, upper) bounds. Defaults to None for no clipping.

[jtigue-bdai]

or something like this. min/max or lower/upper is fine whatever is consistent through the actions.

[jtigue-bdai]

Make sure in the class/cfg documentation that we explain the order in which scaling an clipping get handled.

[jtigue-bdai]

Suggested change

	if self._clip_orientation is not None:
	if self._clip_orientation is not None:

[jtigue-bdai]

just for readability