Adds velocity_limit_sim and effort_limit_sim to actuator #1654
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jtigue-bdai
requested review from
Mayankm96,
jsmith-bdai and
kellyguo11
as code owners
Contributor
|
I like this! and enjoyed the insightful discussion in PR1509 I think previous commit that write the velocity limit in actuator cfg to simulation-level velocity limit broke some of my quadrupet training, and I only figured it out because of the commit before that one worked. |
Collaborator
renezurbruegg
left a comment
renezurbruegg
left a comment
There was a problem hiding this comment.
Nice, Thank you!
I think this fix/feature is much needed.
source/extensions/omni.isaac.lab/omni/isaac/lab/assets/articulation/articulation.py
Outdated
Show resolved
Hide resolved
jtigue-bdai
force-pushed
the
jat/fix/velocity_limit_sim
branch
from
c7fef1e to
b08422e
Compare
pascal-roth
reviewed
Feb 12, 2025
pascal-roth
reviewed
Feb 12, 2025
Co-authored-by: Pascal Roth <[email protected]> Signed-off-by: James Tigue <[email protected]>
pascal-roth
reviewed
Feb 12, 2025
pascal-roth
reviewed
Feb 12, 2025
jtigue-bdai
commented
Feb 12, 2025
Signed-off-by: James Tigue <[email protected]>
pascal-roth
approved these changes
Feb 12, 2025
Collaborator
pascal-roth
left a comment
pascal-roth
left a comment
There was a problem hiding this comment.
Some smaller comments, otherwise LGTM, lets get this merged soon
Mayankm96
reviewed
Feb 13, 2025
kellyguo11
approved these changes
Feb 14, 2025
kellyguo11
changed the title
pascal-roth
approved these changes
Feb 14, 2025
Collaborator
pascal-roth
left a comment
pascal-roth
left a comment
There was a problem hiding this comment.
Important fix, LGTM, lets get it merged
6 tasks
Mayankm96
added a commit
that referenced
this pull request
Mar 2, 2025
…1873) # 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 #1509.  ## Checklist - [x] I have run the [`pre-commit` checks](https://pre-commit.com/) with `./isaaclab.sh --format` - [x] 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 - [x] I have updated the changelog and the corresponding version in the extension's `config/extension.toml` file - [x] I have added my name to the `CONTRIBUTORS.md` or my name already exists there --------- Signed-off-by: Mayank Mittal <[email protected]> Co-authored-by: James Tigue <[email protected]>
jtigue-bdai
added a commit
that referenced
this pull request
Apr 14, 2025
# Description <!-- Thank you for your interest in sending a pull request. Please make sure to check the contribution guidelines. Link: https://isaac-sim.github.io/IsaacLab/main/source/refs/contributing.html --> This PR follows up on [Issue 1384](#1384) and [PR 1509](#1509) by seperating actuator limits for calculating computed torques from physics solver limits. Fixes # (#1384) <!-- As a practice, it is recommended to open an issue to have discussions on the proposed pull request. This makes it easier for the community to keep track of what is being developed or added, and if a given feature is demanded by more than one party. --> ## Type of change <!-- As you go through the list, delete the ones that are not applicable. --> - Bug fix (non-breaking change which fixes an issue) - New feature (non-breaking change which adds functionality) ## Screenshots Please attach before and after screenshots of the change if applicable. <!-- Example: | Before | After | | ------ | ----- | | _gif/png before_ | _gif/png after_ | To upload images to a PR -- simply drag and drop an image while in edit mode and it should upload the image directly. You can then paste that source into the above before/after sections. --> ## Checklist - [x] I have run the [`pre-commit` checks](https://pre-commit.com/) with `./isaaclab.sh --format` - [x] 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 - [x] I have updated the changelog and the corresponding version in the extension's `config/extension.toml` file - [x] I have added my name to the `CONTRIBUTORS.md` or my name already exists there <!-- As you go through the checklist above, you can mark something as done by putting an x character in it For example, - [x] I have done this task - [ ] I have not done this task --> --------- Signed-off-by: James Tigue <[email protected]> Co-authored-by: Pascal Roth <[email protected]>
jtigue-bdai
added a commit
that referenced
this pull request
Apr 14, 2025
…1873) # 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 #1509.  ## Checklist - [x] I have run the [`pre-commit` checks](https://pre-commit.com/) with `./isaaclab.sh --format` - [x] 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 - [x] I have updated the changelog and the corresponding version in the extension's `config/extension.toml` file - [x] I have added my name to the `CONTRIBUTORS.md` or my name already exists there --------- Signed-off-by: Mayank Mittal <[email protected]> Co-authored-by: James Tigue <[email protected]>
ToxicNS
pushed a commit
to ToxicNS/IsaacLab
that referenced
this pull request
Apr 24, 2025
) # Description <!-- Thank you for your interest in sending a pull request. Please make sure to check the contribution guidelines. Link: https://isaac-sim.github.io/IsaacLab/main/source/refs/contributing.html --> This PR follows up on [Issue 1384](isaac-sim#1384) and [PR 1509](isaac-sim#1509) by seperating actuator limits for calculating computed torques from physics solver limits. Fixes # (isaac-sim#1384) <!-- As a practice, it is recommended to open an issue to have discussions on the proposed pull request. This makes it easier for the community to keep track of what is being developed or added, and if a given feature is demanded by more than one party. --> ## Type of change <!-- As you go through the list, delete the ones that are not applicable. --> - Bug fix (non-breaking change which fixes an issue) - New feature (non-breaking change which adds functionality) ## Screenshots Please attach before and after screenshots of the change if applicable. <!-- Example: | Before | After | | ------ | ----- | | _gif/png before_ | _gif/png after_ | To upload images to a PR -- simply drag and drop an image while in edit mode and it should upload the image directly. You can then paste that source into the above before/after sections. --> ## Checklist - [x] I have run the [`pre-commit` checks](https://pre-commit.com/) with `./isaaclab.sh --format` - [x] 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 - [x] I have updated the changelog and the corresponding version in the extension's `config/extension.toml` file - [x] I have added my name to the `CONTRIBUTORS.md` or my name already exists there <!-- As you go through the checklist above, you can mark something as done by putting an x character in it For example, - [x] I have done this task - [ ] I have not done this task --> --------- Signed-off-by: James Tigue <[email protected]> Co-authored-by: Pascal Roth <[email protected]>
ToxicNS
pushed a commit
to ToxicNS/IsaacLab
that referenced
this pull request
Apr 24, 2025
…saac-sim#1873) # Description The previous fix in isaac-sim#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 isaac-sim#1509. Before that change, the parameter `velocity_limit` was set in the configurations but not getting passed through. The MR isaac-sim#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 isaac-sim#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 78bc07e * Black: This MR * Orange: Commenting out the setting of `write_joint_velocity_to_sim` which was introduced in isaac-sim#1509.  ## Checklist - [x] I have run the [`pre-commit` checks](https://pre-commit.com/) with `./isaaclab.sh --format` - [x] 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 - [x] I have updated the changelog and the corresponding version in the extension's `config/extension.toml` file - [x] I have added my name to the `CONTRIBUTORS.md` or my name already exists there --------- Signed-off-by: Mayank Mittal <[email protected]> Co-authored-by: James Tigue <[email protected]>
SevenFo
pushed a commit
to SevenFo/IsaacLab
that referenced
this pull request
May 19, 2025
) # Description <!-- Thank you for your interest in sending a pull request. Please make sure to check the contribution guidelines. Link: https://isaac-sim.github.io/IsaacLab/main/source/refs/contributing.html --> This PR follows up on [Issue 1384](isaac-sim#1384) and [PR 1509](isaac-sim#1509) by seperating actuator limits for calculating computed torques from physics solver limits. Fixes # (isaac-sim#1384) <!-- As a practice, it is recommended to open an issue to have discussions on the proposed pull request. This makes it easier for the community to keep track of what is being developed or added, and if a given feature is demanded by more than one party. --> ## Type of change <!-- As you go through the list, delete the ones that are not applicable. --> - Bug fix (non-breaking change which fixes an issue) - New feature (non-breaking change which adds functionality) ## Screenshots Please attach before and after screenshots of the change if applicable. <!-- Example: | Before | After | | ------ | ----- | | _gif/png before_ | _gif/png after_ | To upload images to a PR -- simply drag and drop an image while in edit mode and it should upload the image directly. You can then paste that source into the above before/after sections. --> ## Checklist - [x] I have run the [`pre-commit` checks](https://pre-commit.com/) with `./isaaclab.sh --format` - [x] 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 - [x] I have updated the changelog and the corresponding version in the extension's `config/extension.toml` file - [x] I have added my name to the `CONTRIBUTORS.md` or my name already exists there <!-- As you go through the checklist above, you can mark something as done by putting an x character in it For example, - [x] I have done this task - [ ] I have not done this task --> --------- Signed-off-by: James Tigue <[email protected]> Co-authored-by: Pascal Roth <[email protected]>
6 tasks
kellyguo11
added a commit
that referenced
this pull request
Jul 29, 2025
# Description As discussed by several earlier commit and issues #2135 #1654 #1384 #1509, there is a distinction between motor hardware velocity limit(velocity_limit), effort limit(effort_limit), simulation velocity limit(velocity_limit_sim) and effort limit(effort_limit_sim). ImplicitActuator, lacking the motor model, is inherently non-attributable to velocity_limit or effort_limit, and should be using velocity_limit_sim and effort_limit_sim instead if such limits should be set. However, since most of environment with `ImplicitActuatorCfg` was written before v1.4, when velocity_limit was basically ignored, and velocity_limit_sim did not exist. To not break those environments training, we remove all `velocity_limit` attribute from existing `ImplicitActuatorCfg`, change all `effort_limit` to `effort_limit_sim`, and added documentation to articulate this point . However, even with removing velocity_limit, effort_limit, there could be subtitles interacting with default USD value. USD may have joints that comes with velocity_limit_sim and effort_limit_sim unnoticed by user. Thus, user may thinking sim_limits are uncaped by not specifying limits in Cfg, but is silently set in USD. To make that more clear, this PR added flag: `actuator_value_resolution_debug_print(default to false)` in `ArticulationCfg` that has following behavior: case 1: if USD has default, ActuatorCfg has limits >if limits is same -> we are all good, no warning. >if limits is different -> we warn user we used cfg value. case 2: USD has default, ActuatorCfg no limits -> We warn user saying the USD defaults is used Note that his logging can apply to all other joint attributes where there could be USD-ArticulationCfg conflicts, not limited to `velocity_limit_si,` or `effort_limit_sim` -> such as : stiffness, damping, armature ..... Note this section is also documented in articulation.rst This PR added actuator discrepancy logging into the :class:`ActuatorBase`. ## Type of change <!-- As you go through the list, delete the ones that are not applicable. --> - Bug fix (non-breaking change which fixes an issue) - This change requires a documentation update ## Screenshots Please attach before and after screenshots of the change if applicable. <!-- Example: | Before | After | | ------ | ----- | | _gif/png before_ | _gif/png after_ | To upload images to a PR -- simply drag and drop an image while in edit mode and it should upload the image directly. You can then paste that source into the above before/after sections. --> ## Checklist - [x] I have run the [`pre-commit` checks](https://pre-commit.com/) with `./isaaclab.sh --format` - [x] I have made corresponding changes to the documentation - [x] My changes generate no new warnings - [ ] I have added tests that prove my fix is effective or that my feature works - [x] I have updated the changelog and the corresponding version in the extension's `config/extension.toml` file - [x] I have added my name to the `CONTRIBUTORS.md` or my name already exists there <!-- As you go through the checklist above, you can mark something as done by putting an x character in it For example, - [x] I have done this task - [ ] I have not done this task --> --------- Co-authored-by: Kelly Guo <[email protected]>
zuxinrui
pushed a commit
to zuxinrui/IsaacLab
that referenced
this pull request
Jul 31, 2025
# Description As discussed by several earlier commit and issues isaac-sim#2135 isaac-sim#1654 isaac-sim#1384 isaac-sim#1509, there is a distinction between motor hardware velocity limit(velocity_limit), effort limit(effort_limit), simulation velocity limit(velocity_limit_sim) and effort limit(effort_limit_sim). ImplicitActuator, lacking the motor model, is inherently non-attributable to velocity_limit or effort_limit, and should be using velocity_limit_sim and effort_limit_sim instead if such limits should be set. However, since most of environment with `ImplicitActuatorCfg` was written before v1.4, when velocity_limit was basically ignored, and velocity_limit_sim did not exist. To not break those environments training, we remove all `velocity_limit` attribute from existing `ImplicitActuatorCfg`, change all `effort_limit` to `effort_limit_sim`, and added documentation to articulate this point . However, even with removing velocity_limit, effort_limit, there could be subtitles interacting with default USD value. USD may have joints that comes with velocity_limit_sim and effort_limit_sim unnoticed by user. Thus, user may thinking sim_limits are uncaped by not specifying limits in Cfg, but is silently set in USD. To make that more clear, this PR added flag: `actuator_value_resolution_debug_print(default to false)` in `ArticulationCfg` that has following behavior: case 1: if USD has default, ActuatorCfg has limits >if limits is same -> we are all good, no warning. >if limits is different -> we warn user we used cfg value. case 2: USD has default, ActuatorCfg no limits -> We warn user saying the USD defaults is used Note that his logging can apply to all other joint attributes where there could be USD-ArticulationCfg conflicts, not limited to `velocity_limit_si,` or `effort_limit_sim` -> such as : stiffness, damping, armature ..... Note this section is also documented in articulation.rst This PR added actuator discrepancy logging into the :class:`ActuatorBase`. ## Type of change <!-- As you go through the list, delete the ones that are not applicable. --> - Bug fix (non-breaking change which fixes an issue) - This change requires a documentation update ## Screenshots Please attach before and after screenshots of the change if applicable. <!-- Example: | Before | After | | ------ | ----- | | _gif/png before_ | _gif/png after_ | To upload images to a PR -- simply drag and drop an image while in edit mode and it should upload the image directly. You can then paste that source into the above before/after sections. --> ## Checklist - [x] I have run the [`pre-commit` checks](https://pre-commit.com/) with `./isaaclab.sh --format` - [x] I have made corresponding changes to the documentation - [x] My changes generate no new warnings - [ ] I have added tests that prove my fix is effective or that my feature works - [x] I have updated the changelog and the corresponding version in the extension's `config/extension.toml` file - [x] I have added my name to the `CONTRIBUTORS.md` or my name already exists there <!-- As you go through the checklist above, you can mark something as done by putting an x character in it For example, - [x] I have done this task - [ ] I have not done this task --> --------- Co-authored-by: Kelly Guo <[email protected]>
kellyguo11
added a commit
that referenced
this pull request
Aug 7, 2025
As discussed by several earlier commit and issues #2135 #1654 #1384 limit(velocity_limit), effort limit(effort_limit), simulation velocity limit(velocity_limit_sim) and effort limit(effort_limit_sim). ImplicitActuator, lacking the motor model, is inherently non-attributable to velocity_limit or effort_limit, and should be using velocity_limit_sim and effort_limit_sim instead if such limits should be set. However, since most of environment with `ImplicitActuatorCfg` was written before v1.4, when velocity_limit was basically ignored, and velocity_limit_sim did not exist. To not break those environments training, we remove all `velocity_limit` attribute from existing `ImplicitActuatorCfg`, change all `effort_limit` to `effort_limit_sim`, and added documentation to articulate this point . However, even with removing velocity_limit, effort_limit, there could be subtitles interacting with default USD value. USD may have joints that comes with velocity_limit_sim and effort_limit_sim unnoticed by user. Thus, user may thinking sim_limits are uncaped by not specifying limits in Cfg, but is silently set in USD. To make that more clear, this PR added flag: `actuator_value_resolution_debug_print(default to false)` in `ArticulationCfg` that has following behavior: case 1: if USD has default, ActuatorCfg has limits >if limits is same -> we are all good, no warning. >if limits is different -> we warn user we used cfg value. case 2: USD has default, ActuatorCfg no limits -> We warn user saying the USD defaults is used Note that his logging can apply to all other joint attributes where there could be USD-ArticulationCfg conflicts, not limited to `velocity_limit_si,` or `effort_limit_sim` -> such as : stiffness, damping, armature ..... Note this section is also documented in articulation.rst This PR added actuator discrepancy logging into the :class:`ActuatorBase`. <!-- As you go through the list, delete the ones that are not applicable. --> - Bug fix (non-breaking change which fixes an issue) - This change requires a documentation update Please attach before and after screenshots of the change if applicable. <!-- Example: | Before | After | | ------ | ----- | | _gif/png before_ | _gif/png after_ | To upload images to a PR -- simply drag and drop an image while in edit mode and it should upload the image directly. You can then paste that source into the above before/after sections. --> - [x] I have run the [`pre-commit` checks](https://pre-commit.com/) with `./isaaclab.sh --format` - [x] I have made corresponding changes to the documentation - [x] My changes generate no new warnings - [ ] I have added tests that prove my fix is effective or that my feature works - [x] I have updated the changelog and the corresponding version in the extension's `config/extension.toml` file - [x] I have added my name to the `CONTRIBUTORS.md` or my name already exists there <!-- As you go through the checklist above, you can mark something as done by putting an x character in it For example, - [x] I have done this task - [ ] I have not done this task --> --------- Co-authored-by: Kelly Guo <[email protected]>
StephenWelch
pushed a commit
to StephenWelch/IsaacLab
that referenced
this pull request
Aug 17, 2025
# Description As discussed by several earlier commit and issues isaac-sim#2135 isaac-sim#1654 isaac-sim#1384 isaac-sim#1509, there is a distinction between motor hardware velocity limit(velocity_limit), effort limit(effort_limit), simulation velocity limit(velocity_limit_sim) and effort limit(effort_limit_sim). ImplicitActuator, lacking the motor model, is inherently non-attributable to velocity_limit or effort_limit, and should be using velocity_limit_sim and effort_limit_sim instead if such limits should be set. However, since most of environment with `ImplicitActuatorCfg` was written before v1.4, when velocity_limit was basically ignored, and velocity_limit_sim did not exist. To not break those environments training, we remove all `velocity_limit` attribute from existing `ImplicitActuatorCfg`, change all `effort_limit` to `effort_limit_sim`, and added documentation to articulate this point . However, even with removing velocity_limit, effort_limit, there could be subtitles interacting with default USD value. USD may have joints that comes with velocity_limit_sim and effort_limit_sim unnoticed by user. Thus, user may thinking sim_limits are uncaped by not specifying limits in Cfg, but is silently set in USD. To make that more clear, this PR added flag: `actuator_value_resolution_debug_print(default to false)` in `ArticulationCfg` that has following behavior: case 1: if USD has default, ActuatorCfg has limits >if limits is same -> we are all good, no warning. >if limits is different -> we warn user we used cfg value. case 2: USD has default, ActuatorCfg no limits -> We warn user saying the USD defaults is used Note that his logging can apply to all other joint attributes where there could be USD-ArticulationCfg conflicts, not limited to `velocity_limit_si,` or `effort_limit_sim` -> such as : stiffness, damping, armature ..... Note this section is also documented in articulation.rst This PR added actuator discrepancy logging into the :class:`ActuatorBase`. ## Type of change <!-- As you go through the list, delete the ones that are not applicable. --> - Bug fix (non-breaking change which fixes an issue) - This change requires a documentation update ## Screenshots Please attach before and after screenshots of the change if applicable. <!-- Example: | Before | After | | ------ | ----- | | _gif/png before_ | _gif/png after_ | To upload images to a PR -- simply drag and drop an image while in edit mode and it should upload the image directly. You can then paste that source into the above before/after sections. --> ## Checklist - [x] I have run the [`pre-commit` checks](https://pre-commit.com/) with `./isaaclab.sh --format` - [x] I have made corresponding changes to the documentation - [x] My changes generate no new warnings - [ ] I have added tests that prove my fix is effective or that my feature works - [x] I have updated the changelog and the corresponding version in the extension's `config/extension.toml` file - [x] I have added my name to the `CONTRIBUTORS.md` or my name already exists there <!-- As you go through the checklist above, you can mark something as done by putting an x character in it For example, - [x] I have done this task - [ ] I have not done this task --> --------- Co-authored-by: Kelly Guo <[email protected]>
StephenWelch
pushed a commit
to StephenWelch/IsaacLab
that referenced
this pull request
Aug 17, 2025
As discussed by several earlier commit and issues isaac-sim#2135 isaac-sim#1654 isaac-sim#1384 limit(velocity_limit), effort limit(effort_limit), simulation velocity limit(velocity_limit_sim) and effort limit(effort_limit_sim). ImplicitActuator, lacking the motor model, is inherently non-attributable to velocity_limit or effort_limit, and should be using velocity_limit_sim and effort_limit_sim instead if such limits should be set. However, since most of environment with `ImplicitActuatorCfg` was written before v1.4, when velocity_limit was basically ignored, and velocity_limit_sim did not exist. To not break those environments training, we remove all `velocity_limit` attribute from existing `ImplicitActuatorCfg`, change all `effort_limit` to `effort_limit_sim`, and added documentation to articulate this point . However, even with removing velocity_limit, effort_limit, there could be subtitles interacting with default USD value. USD may have joints that comes with velocity_limit_sim and effort_limit_sim unnoticed by user. Thus, user may thinking sim_limits are uncaped by not specifying limits in Cfg, but is silently set in USD. To make that more clear, this PR added flag: `actuator_value_resolution_debug_print(default to false)` in `ArticulationCfg` that has following behavior: case 1: if USD has default, ActuatorCfg has limits >if limits is same -> we are all good, no warning. >if limits is different -> we warn user we used cfg value. case 2: USD has default, ActuatorCfg no limits -> We warn user saying the USD defaults is used Note that his logging can apply to all other joint attributes where there could be USD-ArticulationCfg conflicts, not limited to `velocity_limit_si,` or `effort_limit_sim` -> such as : stiffness, damping, armature ..... Note this section is also documented in articulation.rst This PR added actuator discrepancy logging into the :class:`ActuatorBase`. <!-- As you go through the list, delete the ones that are not applicable. --> - Bug fix (non-breaking change which fixes an issue) - This change requires a documentation update Please attach before and after screenshots of the change if applicable. <!-- Example: | Before | After | | ------ | ----- | | _gif/png before_ | _gif/png after_ | To upload images to a PR -- simply drag and drop an image while in edit mode and it should upload the image directly. You can then paste that source into the above before/after sections. --> - [x] I have run the [`pre-commit` checks](https://pre-commit.com/) with `./isaaclab.sh --format` - [x] I have made corresponding changes to the documentation - [x] My changes generate no new warnings - [ ] I have added tests that prove my fix is effective or that my feature works - [x] I have updated the changelog and the corresponding version in the extension's `config/extension.toml` file - [x] I have added my name to the `CONTRIBUTORS.md` or my name already exists there <!-- As you go through the checklist above, you can mark something as done by putting an x character in it For example, - [x] I have done this task - [ ] I have not done this task --> --------- Co-authored-by: Kelly Guo <[email protected]>
george-nehma
pushed a commit
to george-nehma/IsaacLab-Dreamerv3
that referenced
this pull request
Oct 24, 2025
# Description As discussed by several earlier commit and issues isaac-sim#2135 isaac-sim#1654 isaac-sim#1384 isaac-sim#1509, there is a distinction between motor hardware velocity limit(velocity_limit), effort limit(effort_limit), simulation velocity limit(velocity_limit_sim) and effort limit(effort_limit_sim). ImplicitActuator, lacking the motor model, is inherently non-attributable to velocity_limit or effort_limit, and should be using velocity_limit_sim and effort_limit_sim instead if such limits should be set. However, since most of environment with `ImplicitActuatorCfg` was written before v1.4, when velocity_limit was basically ignored, and velocity_limit_sim did not exist. To not break those environments training, we remove all `velocity_limit` attribute from existing `ImplicitActuatorCfg`, change all `effort_limit` to `effort_limit_sim`, and added documentation to articulate this point . However, even with removing velocity_limit, effort_limit, there could be subtitles interacting with default USD value. USD may have joints that comes with velocity_limit_sim and effort_limit_sim unnoticed by user. Thus, user may thinking sim_limits are uncaped by not specifying limits in Cfg, but is silently set in USD. To make that more clear, this PR added flag: `actuator_value_resolution_debug_print(default to false)` in `ArticulationCfg` that has following behavior: case 1: if USD has default, ActuatorCfg has limits >if limits is same -> we are all good, no warning. >if limits is different -> we warn user we used cfg value. case 2: USD has default, ActuatorCfg no limits -> We warn user saying the USD defaults is used Note that his logging can apply to all other joint attributes where there could be USD-ArticulationCfg conflicts, not limited to `velocity_limit_si,` or `effort_limit_sim` -> such as : stiffness, damping, armature ..... Note this section is also documented in articulation.rst This PR added actuator discrepancy logging into the :class:`ActuatorBase`. ## Type of change <!-- As you go through the list, delete the ones that are not applicable. --> - Bug fix (non-breaking change which fixes an issue) - This change requires a documentation update ## Screenshots Please attach before and after screenshots of the change if applicable. <!-- Example: | Before | After | | ------ | ----- | | _gif/png before_ | _gif/png after_ | To upload images to a PR -- simply drag and drop an image while in edit mode and it should upload the image directly. You can then paste that source into the above before/after sections. --> ## Checklist - [x] I have run the [`pre-commit` checks](https://pre-commit.com/) with `./isaaclab.sh --format` - [x] I have made corresponding changes to the documentation - [x] My changes generate no new warnings - [ ] I have added tests that prove my fix is effective or that my feature works - [x] I have updated the changelog and the corresponding version in the extension's `config/extension.toml` file - [x] I have added my name to the `CONTRIBUTORS.md` or my name already exists there <!-- As you go through the checklist above, you can mark something as done by putting an x character in it For example, - [x] I have done this task - [ ] I have not done this task --> --------- Co-authored-by: Kelly Guo <[email protected]>
george-nehma
pushed a commit
to george-nehma/IsaacLab-Dreamerv3
that referenced
this pull request
Oct 24, 2025
As discussed by several earlier commit and issues isaac-sim#2135 isaac-sim#1654 isaac-sim#1384 limit(velocity_limit), effort limit(effort_limit), simulation velocity limit(velocity_limit_sim) and effort limit(effort_limit_sim). ImplicitActuator, lacking the motor model, is inherently non-attributable to velocity_limit or effort_limit, and should be using velocity_limit_sim and effort_limit_sim instead if such limits should be set. However, since most of environment with `ImplicitActuatorCfg` was written before v1.4, when velocity_limit was basically ignored, and velocity_limit_sim did not exist. To not break those environments training, we remove all `velocity_limit` attribute from existing `ImplicitActuatorCfg`, change all `effort_limit` to `effort_limit_sim`, and added documentation to articulate this point . However, even with removing velocity_limit, effort_limit, there could be subtitles interacting with default USD value. USD may have joints that comes with velocity_limit_sim and effort_limit_sim unnoticed by user. Thus, user may thinking sim_limits are uncaped by not specifying limits in Cfg, but is silently set in USD. To make that more clear, this PR added flag: `actuator_value_resolution_debug_print(default to false)` in `ArticulationCfg` that has following behavior: case 1: if USD has default, ActuatorCfg has limits >if limits is same -> we are all good, no warning. >if limits is different -> we warn user we used cfg value. case 2: USD has default, ActuatorCfg no limits -> We warn user saying the USD defaults is used Note that his logging can apply to all other joint attributes where there could be USD-ArticulationCfg conflicts, not limited to `velocity_limit_si,` or `effort_limit_sim` -> such as : stiffness, damping, armature ..... Note this section is also documented in articulation.rst This PR added actuator discrepancy logging into the :class:`ActuatorBase`. <!-- As you go through the list, delete the ones that are not applicable. --> - Bug fix (non-breaking change which fixes an issue) - This change requires a documentation update Please attach before and after screenshots of the change if applicable. <!-- Example: | Before | After | | ------ | ----- | | _gif/png before_ | _gif/png after_ | To upload images to a PR -- simply drag and drop an image while in edit mode and it should upload the image directly. You can then paste that source into the above before/after sections. --> - [x] I have run the [`pre-commit` checks](https://pre-commit.com/) with `./isaaclab.sh --format` - [x] I have made corresponding changes to the documentation - [x] My changes generate no new warnings - [ ] I have added tests that prove my fix is effective or that my feature works - [x] I have updated the changelog and the corresponding version in the extension's `config/extension.toml` file - [x] I have added my name to the `CONTRIBUTORS.md` or my name already exists there <!-- As you go through the checklist above, you can mark something as done by putting an x character in it For example, - [x] I have done this task - [ ] I have not done this task --> --------- Co-authored-by: Kelly Guo <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR follows up on Issue 1384 and PR 1509 by seperating actuator limits for calculating computed torques from physics solver limits.
Fixes # (#1384)
Type of change
Screenshots
Please attach before and after screenshots of the change if applicable.
Checklist
pre-commitchecks with./isaaclab.sh --formatconfig/extension.tomlfileCONTRIBUTORS.mdor my name already exists there