Reverts the setting of joint velocity limits for implicit actuators

[Mayankm96]

Description

The previous fix in #1654 was checking if effort_limits_sim is None instead of checking cfg.effort_limits_sim for explicit actuators. This did NOT work as effort limits sim is a tensor that gets assigned the value on initialization.

The new fix now adds an implicit/explicit model attribute to the actuator model to ensure the right defaults are getting set. It moves all the implicit actuator warning code to its class to keep the articulation class cleaner.

We also revert the behavior of setting velocity limits for implicit actuators to the state before #1509. Before that change, the parameter velocity_limit was set in the configurations but not getting passed through. The MR #1509 allowed these values to be set which caused many of the assets to not train anymore or behave differently between IsaacLab versions. We now revert this behavior with a warning. If users want to set the limits, they should use the effort_limit_sim and velocity_limit_sim quantities.

Fixes #1837

Type of change

• Bug fix (non-breaking change which fixes an issue)

Screenshot

The training of Allegro hand environment:

• Green: The current main at 6a415df
• Black: This MR
• Orange: Commenting out the setting of write_joint_velocity_to_sim which was introduced in Fixes actuator velocity limits propagation down the articulation root_physx_view #1509.

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

[Mayankm96]

I will run the training and deploying for a sample set of our environments before merging this in. We really should make this a practice if we change something so burried into our architecture. Otherwise, it breaks so much for everyone.

On the other hand, we also need to add tests for actuators that are non-existent at this point and also the source of all the issues we are having with the recent changes.

[Mayankm96]

I see the issues for implicit models not working still remain. The source of that problem is that in the past the users specified the velocity limit but it wasn't getting set. Now that we are setting them, those limits are causing the physics engine to behave differently.

Any suggestions on how to fix that? @jtigue-bdai

Maybe we should throw a warning in implicit if they specified velocity_limit and acknowledge the above and not set anything for velocity limit into the simulation till users actually use velocity_limit_sim?

[jtigue-bdai]

What environments are you running to check if things are working? Do those with implicit actuator environments have velocity_limit set? If so can we not remove that because they weren't doing anything before?

I'm not against warning the user if they set velocity_limit for implicit and only allow setting via velocity_limit_sim.

The tests I wrote are checking that the "xor" logic of setting limit vs limit_sim is propagating down to the physx level. But if there are still rogue unintended velocity_limit settings then those would get passed down.

My only other thought would be to revert the old change (i.e. don't pass velocity limit down) and then check to make sure that still works.

[Mayankm96]

These are the results of the training runs.

Task Name	Reward Curve	Current Main	This branch
Isaac-Lift-Cube-Franka-v0		Purple	Green
Isaac-Repose-Cube-Allegro-v0		Green	Orange
Isaac-Velocity-Rough-Anymal-D-v0		Grey	Blue
Isaac-Velocity-Rough-G1-v0		Blue	Pink