Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes actuator velocity limits propagation down the articulation root_physx_view #1509

Merged
merged 7 commits into from
Dec 8, 2024

Conversation

jtigue-bdai
Copy link
Collaborator

@jtigue-bdai jtigue-bdai commented Dec 6, 2024

Description

Previously the velocity limits of the the ActuatorBaseCfg do not get used in most actuators. Only the DCMotor actuator uses it but it only uses it for torque-speed limitations.

This PR propagates the velocity_limits of the ActuatorBaseCfg to the articulation root_physx_view.

Fixes #1384

Type of change

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

Screenshots

Please attach before and after screenshots of the change if applicable.

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

@jtigue-bdai jtigue-bdai self-assigned this Dec 6, 2024
@jtigue-bdai jtigue-bdai marked this pull request as ready for review December 6, 2024 19:47
@jtigue-bdai jtigue-bdai changed the title Fix the propagation of actuator velocity limits down the articulation root_physx_view Fix actuator velocity limits propagation down the articulation root_physx_view Dec 6, 2024
@kellyguo11 kellyguo11 changed the title Fix actuator velocity limits propagation down the articulation root_physx_view Fixes actuator velocity limits propagation down the articulation root_physx_view Dec 8, 2024
@kellyguo11 kellyguo11 merged commit 4ee4957 into main Dec 8, 2024
5 checks passed
@kellyguo11 kellyguo11 deleted the jat/fix/articulation_actuator_velocity_limts branch December 8, 2024 02:36
@larsrpe
Copy link

larsrpe commented Dec 10, 2024

This seems like a nice feature for consistency through the different layers. However, I wonder if this can have implications for the simulator performance, depending on how this constraint is implemented in the backend.

I think one potential issue is when using the DCMotor model we want to define a torque cutoff speed to obtain more realistic speed torque relationship, however this does not necessarily mean that we want to have a hard limit on the joint velocities, which might give simulation artefacts.

I think follow up work should be to separate the velocity limit used in the DC motor model and the one used for the hard physics limit. What do you think?

@jtigue-bdai
Copy link
Collaborator Author

jtigue-bdai commented Dec 10, 2024 via email

@larsrpe
Copy link

larsrpe commented Dec 10, 2024

Should we instead use self.noload_speed here:

max_effort = self._saturation_effort * (1.0 - self._joint_vel / self.velocity_limit)

This would only require adding one field to the DCMotorCfg.

@Mayankm96
Copy link
Contributor

I agree with @larsrpe. Solver-level limits are different from the explicit model considerations. For explicit models, we may want to do the clipping inside the model but not have PhysX apply any of that within it (as tight constraints may lead to undefined behaviors).

For most of the shared quantities, we should maybe have their counterpart quantities as "torque_limit_sim" and "velocity_limit_sim". This makes it clear that these are being set at the solver level always. Their default values can be high (as right now).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug Report] Incorrect dof max velocities for explicit actuators
4 participants