[SIM-950] Actuator Drive Model Improvements #3942
Open
+561
−200
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
bmccann-bdai
force-pushed
the
bcm/performance_envelope
branch
3 times, most recently
from
bc69c88 to
bc15f89
Compare
bmccann-bdai
requested review from
Mayankm96,
jtigue-bdai,
kellyguo11 and
pascal-roth
as code owners
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This PR implements PhysX 5.0's drive model improvements to enable more realistic motor behavior modeling through velocity and effort dependent constraints.
Key Changes:
- Refactors actuator configuration by splitting
actuator_cfg.pyinto three separate files (actuator_base_cfg.py,actuator_pd_cfg.py,actuator_net_cfg.py) for better organization - Introduces
DriveModelCfgNamedTuple with three parameters:speed_effort_gradient,max_actuator_velocity, andvelocity_dependent_resistance - Adds
drive_modelparameter toActuatorBaseCfgaccepting tuple/dict/float configurations - Extends
_parse_joint_parameterto handle 3D tensors for drive model parameters - Adds
write_joint_drive_model_to_sim()method inArticulationclass to apply drive model to PhysX - Updates
ArticulationDatato store drive model parameters - Updates import paths across robot configurations
Issues Found:
test_ideal_pd_actuator.pyuses non-existent properties (dm_speed_effort_gradient,dm_max_actuator_velocity,dm_velocity_dependent_resistance) instead of thedrive_modelparameter withDriveModelCfg- Several style improvements needed for integer comparisons using
isinstead of==
Confidence Score: 3/5
- PR has a critical test failure but core implementation is sound
- Score reflects a well-architected implementation with comprehensive testing, but one test file (
test_ideal_pd_actuator.py) uses non-existent API properties that will cause runtime failures. The core drive model implementation is correct and follows good patterns. The style issues are minor. - Fix
source/isaaclab/test/actuators/test_ideal_pd_actuator.py- test will fail due to incorrect API usage
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| source/isaaclab/isaaclab/actuators/actuator_base_cfg.py | 5/5 | New file splitting actuator configs - adds DriveModelCfg NamedTuple with drive model parameters |
| source/isaaclab/isaaclab/actuators/actuator_base.py | 4/5 | Adds drive_model parameter support with tuple/tensor parsing logic for 3D parameters |
| source/isaaclab/isaaclab/assets/articulation/articulation.py | 4/5 | Adds write_joint_drive_model_to_sim method and integrates drive model into initialization flow |
| source/isaaclab/test/actuators/test_ideal_pd_actuator.py | 2/5 | Test uses non-existent dm_* properties instead of drive_model - will fail at runtime |
| source/isaaclab/test/actuators/test_drive_model_properties.py | 5/5 | Test correctly uses DriveModelCfg to instantiate and validate drive model parameters |
Sequence Diagram
sequenceDiagram
participant User
participant ArticulationCfg
participant Articulation
participant ActuatorBaseCfg
participant ActuatorBase
participant PhysXView
User->>ArticulationCfg: Configure actuator with drive_model
Note over ArticulationCfg: drive_model = DriveModelCfg(speed_effort_gradient, max_actuator_velocity, velocity_dependent_resistance)
User->>Articulation: Initialize articulation
Articulation->>PhysXView: get_dof_drive_model_properties()
PhysXView-->>Articulation: default_joint_drive_model_parameters
Articulation->>Articulation: Store in ArticulationData
Articulation->>ActuatorBase: Initialize actuator with drive_model
ActuatorBase->>ActuatorBase: _parse_joint_parameter(cfg.drive_model, usd_default, shape=(num_envs, num_joints, 3))
Note over ActuatorBase: Parses tuple/dict/float into tensor
ActuatorBase->>ActuatorBase: Store as self.drive_model tensor
ActuatorBase-->>Articulation: Actuator initialized with drive_model
Articulation->>Articulation: write_joint_drive_model_to_sim(actuator.drive_model)
Articulation->>ArticulationData: Update joint_drive_model_parameters
Articulation->>PhysXView: set_dof_drive_model_properties(parameters)
PhysXView-->>Articulation: Drive model applied
Note over PhysXView: PhysX 5.0+ uses drive model to constrain motor torque-speed envelope
24 files reviewed, 4 comments
Contributor
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This commit removes an incorrectly implemented test (test_ideal_drive_model_parameters) from test_ideal_pd_actuator.py. The test was attempting to use non-existent properties (dm_velocity_dependent_resistance, dm_max_actuator_velocity, dm_speed_effort_gradient) directly on IdealPDActuatorCfg.
The correct implementation uses the drive_model parameter with a DriveModelCfg object (as shown in the separate test_drive_model_properties.py file that was added in the broader PR). The removed test would have failed with attribute errors since those properties don't exist as direct configuration parameters.
Key points:
- Clean removal of 79 lines containing the problematic test
- Existing tests for basic actuator initialization, effort limits, velocity limits, and computation remain intact
- Proper drive model testing exists in
test_drive_model_properties.py
Confidence Score: 5/5
- This PR is safe to merge - it simply removes a broken test
- The change removes a test that would have failed due to using non-existent configuration properties. The test was attempting to directly access drive model parameters as top-level attributes (
dm_*properties) when they should be configured through thedrive_modelparameter with aDriveModelCfgobject. This is a cleanup commit that removes dead/broken code, with proper tests existing elsewhere - No files require special attention
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| source/isaaclab/test/actuators/test_ideal_pd_actuator.py | 5/5 | Removed problematic test that used non-existent drive model properties - clean removal with no issues |
Sequence Diagram
sequenceDiagram
participant User
participant ActuatorCfg
participant Articulation
participant ActuatorBase
participant PhysX
User->>ActuatorCfg: Configure drive_model with DriveModelCfg
Note over ActuatorCfg: DriveModelCfg(speed_effort_gradient,<br/>max_actuator_velocity,<br/>velocity_dependent_resistance)
User->>Articulation: Initialize articulation with actuators
Articulation->>PhysX: Read default drive model parameters
PhysX-->>Articulation: Return default parameters
Articulation->>ActuatorBase: Create actuator with drive_model
ActuatorBase->>ActuatorBase: Parse drive_model tuple/dict config
Note over ActuatorBase: Converts to tensor shape<br/>(num_envs, num_joints, 3)
ActuatorBase-->>Articulation: Return configured actuator
Articulation->>Articulation: Store drive_model in actuator.drive_model
Articulation->>PhysX: write_joint_drive_model_to_sim()
Note over PhysX: set_dof_drive_model_properties()
User->>Articulation: Step simulation
Articulation->>ActuatorBase: compute(actions, pos, vel)
ActuatorBase->>ActuatorBase: Calculate effort
ActuatorBase-->>Articulation: Return effort commands
Articulation->>PhysX: Apply effort with drive model constraints
Note over PhysX: PhysX applies velocity-effort<br/>envelope constraints
1 file reviewed, no comments
gattra-rai
reviewed
Nov 4, 2025
| for param_name, usd_val in to_check: | ||
| for param_name, usd_val, *tuple_len in to_check: | ||
| # check if the parameter requires a tuple or a single float | ||
| if len(tuple_len) > 0: |
Collaborator
There was a problem hiding this comment.
since the only param with a tuple_len in the list is drive_model, it might be simpler to just check if param_name == "drive_model"
Collaborator
Author
There was a problem hiding this comment.
The goal was to implement a more general pattern which other related parameters can use.
| ("friction", friction), | ||
| ("dynamic_friction", dynamic_friction), | ||
| ("viscous_friction", viscous_friction), | ||
| ("drive_model", drive_model, 3), |
Collaborator
There was a problem hiding this comment.
if you choose to create the DriveModelParam enum suggested in another comment, you could do len(DriveModelParam.values) instead of a hardcoded magic number
Collaborator
Author
There was a problem hiding this comment.
There might be a way to extract it from the DriveModelCfg. I know it automatically imposes the tuple[float,float,float] constraint. I agree, the magic number isn't ideal.
| If a tensor, then the shape is (num_envs, num_joints). | ||
| velocity_limit: The default velocity limit. Defaults to infinity. | ||
| If a tensor, then the shape is (num_envs, num_joints). | ||
| drive_model: Drive model for the actuator including speed_effort_gradient, max_actuator_velocity, and |
Collaborator
There was a problem hiding this comment.
a DriveModelParam enum with a mapping of param -> index would avoid requiring the user to remember the indices of each drive model param
Collaborator
Author
There was a problem hiding this comment.
I was trying to do this through the NamedTuple class. At least for the ActuatorBaseCfg.DriveModelCfg, they can treat the cfg either as a tuple, or a dataclass with named attributes.
| if isinstance(default_value, (float, int)): | ||
| # if float, then use the same value for all joints | ||
| param[:] = float(default_value) | ||
| elif isinstance(default_value, tuple): |
Collaborator
There was a problem hiding this comment.
i might be missing context here, but im seeing a lot of checks for isinstance(..., tuple) in this function and elsewhere. is it possible to convert the value to a tensor or tuple upfront and then reduce the branching code downstream?
Collaborator
Author
There was a problem hiding this comment.
Unfortunately not, this is where we parse the configuration file, and a design constraint was that we use simple types in the configuration classes. I originally proposed using tensors directly, but they asked me to stay with vanilla types.
Collaborator
There was a problem hiding this comment.
oh yeah, not suggesting changing anything about the config class itself. my thinking was, if the output/result of the function is the same, just with branching logic based on the type, it might be simpler to convert the tuple to a tensor (or vice versa) at the top of the function and then scrap the branching logic.
Collaborator
Author
There was a problem hiding this comment.
I see. There's an additional complication which is that here isinstance(tensor) is taken as shorthand for the complete tensor (across envs and joints), whereas the tuple is treated as a single entry to be broadcast across envs and joints. So if I were to convert to a tensor, I would still need to branch (maybe with a flag or something) so I can correctly check the shape, and broadcast the values correctly.
What might work is to create a more general base class for tuple parameters. The NamedTuple is a good start, but maybe something that 1) exposes an enum to directly map indices (as you suggest), and 2) has something like a to_tensor() call to support casting to something that can be broadcast like a slice, which can't be done with tuples. At least then the branching logic would be more concise, and look more like a simple assignment than admittedly opaque nested loops.
My concern was creating code that relies on a more complex object, and forces other code to rely on a class when a tuple will do. We could do something like cast the tuple to the tuple parameter class early in the function as you suggest. At least then the exposed api could use either a tuple, or the convenience object depending on the consumer's preference.
Contributor
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This PR implements drive model support for actuators in IsaacLab, introducing velocity and effort dependent constraints on motor performance. The changes add a new drive_model parameter that accepts a tuple of three values: speed_effort_gradient, max_actuator_velocity, and velocity_dependent_resistance.
Key Changes:
- Added
drive_modeltensor attribute toActuatorBasewith shape(num_envs, num_joints, 3) - Extended
_parse_joint_parametermethod to handle tuple-type parameters in addition to floats and dicts - Updated parameter resolution logic to support 3D tensors for drive model configuration
- Modified
_record_actuator_resolutionto log tuple-based parameters - Changed import from conditional TYPE_CHECKING to direct import of
ActuatorBaseCfg
Issues Found:
- Three instances of
is/is notfor integer comparison instead of==/!=(already flagged in previous comments) - Minor formatting issues in error messages (missing spaces/commas)
The implementation correctly guards usage to IsaacSim v5.0+ at the articulation level, and the tuple parameter handling logic is sound.
Confidence Score: 4/5
- This PR is safe to merge with minor syntax fixes needed
- The implementation is logically sound and well-tested. The core functionality correctly handles tuple parameters for drive model configuration. However, there are syntax issues: 3 instances using
is/is notfor integer comparison (non-idiomatic and potentially buggy), and minor formatting issues in error messages. These are easy fixes that don't affect runtime behavior in most cases but should be corrected for code quality. - source/isaaclab/isaaclab/actuators/actuator_base.py needs syntax fixes for integer comparisons and error message formatting
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| source/isaaclab/isaaclab/actuators/actuator_base.py | 4/5 | Adds drive model support with tuple parameter handling. Uses is for integer comparison in 3 places (already flagged), minor formatting issues in error messages. |
Sequence Diagram
sequenceDiagram
participant User
participant ActuatorCfg as ActuatorBaseCfg
participant Actuator as ActuatorBase
participant Articulation
participant PhysX as PhysX View
User->>ActuatorCfg: Configure drive_model<br/>(speed_effort_gradient,<br/>max_actuator_velocity,<br/>velocity_dependent_resistance)
User->>Articulation: Initialize with ActuatorCfg
Articulation->>Actuator: __init__(cfg, joint_names,<br/>joint_ids, drive_model)
Note over Actuator: Parse drive_model parameter
Actuator->>Actuator: _parse_joint_parameter()<br/>Handle tuple/float/dict types<br/>Create (num_envs, num_joints, 3) tensor
Actuator->>Actuator: _record_actuator_resolution()<br/>Log parameter resolution
Actuator-->>Articulation: Return initialized actuator<br/>with drive_model tensor
alt IsaacSim v5.0+
Articulation->>Articulation: write_joint_drive_model_to_sim()<br/>Set drive_model to data buffer
Articulation->>PhysX: set_dof_drive_model_properties()<br/>Apply to physics engine
PhysX-->>Articulation: Drive model constraints active
else IsaacSim v4.x
Note over Articulation: Drive model stored but not<br/>written to simulation
end
1 file reviewed, 3 comments
Contributor
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
Implements PhysX 5.0 drive model configuration for velocity/effort-dependent motor constraints. The changes add a 3-parameter drive model (speed-effort gradient, max actuator velocity, velocity-dependent resistance) to actuators and articulations.
Key Changes:
- Added
drive_modeltensor attribute toActuatorBaseclass with shape(num_envs, num_joints, 3) - Extended
_parse_joint_parameterto handle 3D tensors and tuple values - Added
write_joint_drive_model_to_simmethod inArticulationclass - Created
DriveModelCfgnested configuration class with three parameters - Added comprehensive tests for drive model initialization
Critical Issue Found:
- Line 399 in
actuator_base.pyreferences undefined variable_that was discarded on line 387, causing runtime crash when using dict-based drive model configuration with invalid tuples
Other Issues:
- Multiple uses of
isinstead of==for integer comparisons (non-idiomatic Python) - Missing spaces/commas in error messages
- Tests only cover tuple-based config, not dict-based config (missing test coverage for the critical bug path)
Confidence Score: 2/5
- This PR has a critical bug that will cause runtime crashes when using dict-based drive model configuration
- Score of 2 reflects a critical logic bug on line 399 where an undefined variable
_is referenced, which will cause a NameError when users configure drive_model using dict syntax with invalid tuple lengths. The bug is not caught by tests because test coverage only includes tuple-based config, not dict-based config. Additional minor issues include non-idiomaticiscomparisons and formatting errors in messages. The core feature implementation appears sound, but the critical bug must be fixed before merge. source/isaaclab/isaaclab/actuators/actuator_base.pyrequires immediate attention due to undefined variable bug on line 399
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| source/isaaclab/isaaclab/actuators/actuator_base.py | 2/5 | Adds drive model support with tuple/dict parsing. Critical bug: undefined variable _ on line 399 will crash when using dict-based drive_model config. Multiple style issues with is comparisons and error message formatting. |
Sequence Diagram
sequenceDiagram
participant User
participant ArticulationCfg
participant Articulation
participant ActuatorBase
participant PhysXView
User->>ArticulationCfg: Configure drive_model in actuator config
ArticulationCfg->>Articulation: Initialize with actuator configs
Articulation->>ActuatorBase: __init__(cfg, drive_model=usd_default)
ActuatorBase->>ActuatorBase: _parse_joint_parameter(cfg.drive_model, usd_default, shape=(num_envs, num_joints, 3))
alt cfg_value is tuple
ActuatorBase->>ActuatorBase: Validate tuple length == 3
ActuatorBase->>ActuatorBase: Assign tuple values to tensor[:,:,0:3]
else cfg_value is dict
ActuatorBase->>ActuatorBase: resolve_matching_names_values()
ActuatorBase->>ActuatorBase: For each joint, validate and assign tuple
Note right of ActuatorBase: BUG: Line 399 references undefined variable _
else cfg_value is None
ActuatorBase->>ActuatorBase: Use usd_default values
end
ActuatorBase-->>Articulation: Return actuator with drive_model tensor
Articulation->>Articulation: write_joint_drive_model_to_sim(actuator.drive_model)
Articulation->>PhysXView: set_dof_drive_model_properties(drive_params)
PhysXView-->>Articulation: Drive model applied to simulation
1 file reviewed, 1 comment
Contributor
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
Adds actuator drive model support to configure velocity and effort dependent motor constraints through PhysX 5.0+ APIs. The implementation extends ActuatorBase to handle 3-tuple drive model parameters (speed-effort gradient, max actuator velocity, velocity-dependent resistance) and integrates them into the articulation pipeline via write_joint_drive_model_to_sim().
Key changes:
- Extended
_parse_joint_parameter()to handle tuples in addition to floats and dicts for multi-dimensional parameters - Added
drive_modeltensor attribute (shape:num_envs × num_joints × 3) toActuatorBase - Integrated drive model into
Articulationclass with version-gated PhysX API calls - Updated actuator resolution logging to handle tuple formatting
- Refactored config structure by splitting
actuator_cfg.pyinto separate files per actuator type
Previous reviewer comments about non-existent test properties have been addressed in the current code.
Confidence Score: 4/5
- This PR is safe to merge with minor style improvements recommended
- Code correctly implements drive model feature with proper type handling, tensor operations, and version gating. Previous syntax issues flagged by reviewers have been addressed. The main concerns are non-critical style issues with
isvs==for integer comparison which work correctly but aren't idiomatic Python. - No files require special attention - style improvements are optional
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| source/isaaclab/isaaclab/actuators/actuator_base.py | 4/5 | Adds drive model support with tuple parameter handling. Implementation is solid with minor style issues in integer comparisons. |
Sequence Diagram
sequenceDiagram
participant User
participant Articulation
participant ActuatorBase
participant PhysXView
User->>Articulation: Configure actuators with drive_model
Articulation->>ActuatorBase: __init__(cfg, drive_model=...)
ActuatorBase->>ActuatorBase: _parse_joint_parameter(drive_model)
Note over ActuatorBase: Handles tuple/dict/float<br/>Creates (num_envs, num_joints, 3) tensor
ActuatorBase-->>Articulation: Actuator initialized with drive_model
Articulation->>Articulation: _process_actuators()
Articulation->>Articulation: Store drive_model in _data.joint_drive_model_parameters
Articulation->>Articulation: write_joint_drive_model_to_sim()
Articulation->>PhysXView: set_dof_drive_model_properties()
PhysXView-->>Articulation: Drive model applied
Note over PhysXView: PhysX applies velocity-effort<br/>constraints during simulation
1 file reviewed, no comments
github-actions
bot
added
the
documentation
Contributor
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This PR implements drive model support for actuators to better model motor performance envelopes with velocity-effort constraints. The implementation adds three parameters (speed-effort gradient, max actuator velocity, and velocity-dependent resistance) to the actuator configuration and integrates them throughout the articulation and actuator base classes.
Key Changes:
- New
ActuatorBaseCfgconfiguration class withDriveModelCfgnested tuple for drive model parameters - Extended
ActuatorBase._parse_joint_parameter()to handle 3D tensor parameters (tuples of floats) - Added
write_joint_drive_model_to_sim()method to apply drive model properties to PhysX (IsaacSim v5+ only) - New test file
test_drive_model_properties.pyvalidates drive model initialization - Updated documentation to reflect new configuration structure
Critical Issue Found:
articulation.py:1763attempts to accessjoint_drive_model_parametersregardless of IsaacSim version, but this field is only initialized for v5+, causing runtime crash for v4.x users
Style Issues:
- Multiple uses of
isoperator for integer comparison instead of== - Minor formatting issues in error messages
Confidence Score: 1/5
- This PR has a critical bug that will cause runtime crashes for IsaacSim v4.x users
- The score reflects a critical logical error on line 1763 of
articulation.pywherejoint_drive_model_parametersis accessed unconditionally, but it's only initialized when IsaacSim version >= 5. For v4.x installations, this will crash withTypeError: 'NoneType' object is not subscriptable. The bug must be fixed before merging. - Pay critical attention to
source/isaaclab/isaaclab/assets/articulation/articulation.py- the version check logic forjoint_drive_model_parametersinitialization needs to match its usage patterns
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| source/isaaclab/isaaclab/assets/articulation/articulation.py | 1/5 | Critical bug: joint_drive_model_parameters not initialized for IsaacSim < v5, causing runtime crash when accessing None object on line 1763 |
| source/isaaclab/isaaclab/actuators/actuator_base.py | 3/5 | Multiple style issues with is operator used for integer comparison (lines 378, 397, 423), and missing formatting in error messages (lines 375, 401, 426) |
| source/isaaclab/isaaclab/actuators/actuator_base_cfg.py | 5/5 | New configuration file for actuator base settings including drive model parameters - well documented |
| source/isaaclab/isaaclab/assets/articulation/articulation_data.py | 5/5 | Added joint_drive_model_parameters attributes for storing drive model properties - straightforward addition |
| source/isaaclab/test/actuators/test_drive_model_properties.py | 5/5 | New test file validating drive model parameter initialization - properly tests all three drive model components |
| docs/source/api/lab/isaaclab.actuators.rst | 5/5 | Documentation updated to reflect new actuator base configuration structure |
Sequence Diagram
sequenceDiagram
participant User
participant ArticulationCfg
participant Articulation
participant ArticulationData
participant ActuatorBase
participant PhysX
User->>ArticulationCfg: Configure actuator with drive_model
ArticulationCfg->>Articulation: Initialize articulation
Articulation->>PhysX: get_dof_drive_model_properties() (v5+ only)
PhysX-->>Articulation: Drive model defaults from USD
Articulation->>ArticulationData: Store joint_drive_model_parameters
Articulation->>ActuatorBase: Create actuator with drive_model tensor
ActuatorBase->>ActuatorBase: Parse drive_model (cfg or defaults)
ActuatorBase-->>Articulation: Actuator initialized
Articulation->>PhysX: write_joint_drive_model_to_sim() (v5+ only)
PhysX-->>Articulation: Drive model applied
Articulation-->>User: Articulation ready with drive model
1 file reviewed, 1 comment
Contributor
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This PR implements actuator drive model improvements by adding support for velocity and effort dependent constraints on motor actuation. The implementation includes three new parameters: speed_effort_gradient, max_actuator_velocity, and velocity_dependent_resistance.
Key Changes:
- Refactored actuator configuration by splitting
actuator_cfg.pyintoactuator_base_cfg.py,actuator_pd_cfg.py, andactuator_net_cfg.py - Added
DriveModelCfgas aNamedTuplewith sensible defaults (0.0, inf, 0.0) - Extended
_parse_joint_parameter()to handle 3-dimensional tensors and tuples for drive model parameters - Added
write_joint_drive_model_to_sim()method to articulation for setting drive model properties - Created tests for drive model configuration instantiation
Critical Issue Found:
articulation.py:1763will crash on IsaacSim version < 5 becausejoint_drive_model_parametersis only initialized for version >= 5 but accessed unconditionally when creating actuators
Minor Issues:
- Several uses of
isinstead of==for integer comparisons (lines 379, 398, 424) - works but not idiomatic - Minor formatting issues in error messages (missing spaces/commas)
Confidence Score: 1/5
- This PR has a critical bug that will cause crashes on IsaacSim version < 5
- Score reflects a critical logic error on articulation.py:1763 where
joint_drive_model_parametersis accessed unconditionally but only initialized for IsaacSim v5+. This will causeTypeError: 'NoneType' object is not subscriptablefor any users on version < 5. The fix is straightforward but essential before merging. - source/isaaclab/isaaclab/assets/articulation/articulation.py - requires version check or initialization for v < 5 before line 1763
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| source/isaaclab/isaaclab/actuators/actuator_base.py | 3/5 | Implements tuple parameter parsing for drive model configuration with minor style and formatting issues in error messages |
| source/isaaclab/isaaclab/assets/articulation/articulation.py | 1/5 | Adds drive model support but has critical bug causing crash on IsaacSim version < 5 due to uninitialized parameter access |
| source/isaaclab/isaaclab/actuators/actuator_base_cfg.py | 5/5 | New configuration file defining DriveModelCfg with reasonable defaults for actuator drive model parameters |
Sequence Diagram
sequenceDiagram
participant Config as ActuatorBaseCfg
participant Articulation as Articulation
participant ArticulationData as ArticulationData
participant ActuatorBase as ActuatorBase
participant PhysX as PhysX View
Note over ArticulationData: joint_drive_model_parameters = None
Articulation->>PhysX: get_dof_drive_model_properties() (v5+ only)
PhysX-->>Articulation: drive model tensor
Articulation->>ArticulationData: default_joint_drive_model_parameters = tensor
Articulation->>ArticulationData: joint_drive_model_parameters = clone()
Note over Config: DriveModelCfg(0.0, inf, 0.0)
Articulation->>ActuatorBase: __init__(drive_model=data.joint_drive_model_parameters)
ActuatorBase->>ActuatorBase: _parse_joint_parameter()
Note over ActuatorBase: Handles tuple, dict, float types<br/>Creates (num_envs, num_joints, 3) tensor
ActuatorBase->>ArticulationData: Store actuator.drive_model
Articulation->>PhysX: set_dof_drive_model_properties() (v5+ only)
1 file reviewed, 1 comment
Contributor
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This PR adds documentation improvements for the actuator drive model feature. The changes include:
- Added
:noindex:directives to all actuator configuration classes in the API documentation to prevent duplicate reference warnings during documentation builds - Updated the CHANGELOG.rst with version 0.49.7 release notes documenting the drive model improvements
The implementation of the drive model feature itself (in the actuator and articulation classes) has several critical issues that were identified in previous review comments that need to be addressed.
Confidence Score: 1/5
- This PR has critical runtime errors that will crash for IsaacSim version < 5
- The documentation changes themselves are safe, but the implementation code (articulation.py) has a critical bug where
joint_drive_model_parametersis accessed unconditionally on line 1763 despite only being initialized for version >= 5. This will causeTypeError: 'NoneType' object is not subscriptablecrashes for version < 5. Additionally, there are multiple syntax issues in error messages and a logic error with variable naming that will cause runtime errors. - source/isaaclab/isaaclab/assets/articulation/articulation.py - Line 1763 accesses
joint_drive_model_parameterswithout version check; source/isaaclab/isaaclab/actuators/actuator_base.py - Lines 399, 426 have variable naming issues and syntax errors in error messages
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| docs/source/api/lab/isaaclab.actuators.rst | 5/5 | Added :noindex: directives to prevent duplicate index entries in documentation |
| source/isaaclab/docs/CHANGELOG.rst | 5/5 | Added version 0.49.7 changelog entry documenting drive model improvements |
Sequence Diagram
sequenceDiagram
participant User
participant Articulation
participant PhysX
participant ActuatorBase
participant DriveModelCfg
User->>Articulation: Initialize with ActuatorCfg
Note over Articulation: _initialize_impl() called
alt IsaacSim version >= 5
Articulation->>PhysX: get_dof_drive_model_properties()
PhysX-->>Articulation: drive_model_parameters
Articulation->>Articulation: Store in _data.joint_drive_model_parameters
else IsaacSim version < 5
Note over Articulation: joint_drive_model_parameters remains None
end
Articulation->>Articulation: _process_actuators()
Articulation->>Articulation: Index joint_drive_model_parameters (BUG: crashes if None)
Articulation->>ActuatorBase: Create actuator with drive_model param
ActuatorBase->>ActuatorBase: _parse_joint_parameter(drive_model)
ActuatorBase->>ActuatorBase: Store as self.drive_model tensor
alt Implicit Actuator & version >= 5
Articulation->>Articulation: write_joint_drive_model_to_sim()
Articulation->>PhysX: set_dof_drive_model_properties()
Note over PhysX: Drive model constraints applied
end
User->>Articulation: write_joint_actions()
Articulation->>ActuatorBase: compute(control_action)
ActuatorBase-->>Articulation: Modified control_action
Articulation->>PhysX: Apply control with drive constraints
2 files reviewed, no comments
…actuator classes in order to configure the new actuator drive model including velocity and effort dependent constraints on motor actuation. For details see https://nvidia-omniverse.github.io/PhysX/physx/5.6.1/docs/Articulations.html#articulation-drive-stability and https://docs.omniverse.nvidia.com/kit/docs/omni_physics/latest/extensions/runtime/source/omni.physics.tensors/docs/api/python.html#omni.physics.tensors.impl.api.ArticulationView.set_dof_drive_model_properties
bmccann-bdai
force-pushed
the
bcm/performance_envelope
branch
from
7d83d8d to
9da2352
Compare
Contributor
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This PR implements actuator drive model improvements by adding support for velocity and effort dependent motor actuation constraints. The implementation adds a new DriveModelCfg that defines three parameters: speed_effort_gradient, max_actuator_velocity, and velocity_dependent_resistance to model motor drive performance envelopes.
Key changes:
- Added
DriveModelCfgNamedTuple toActuatorBaseCfgwith drive model parameters - Extended
ActuatorBaseto parse and store drive model configurations (supports float, dict, and tuple inputs) - Added
write_joint_drive_model_to_sim()method inArticulationclass to apply drive model to simulation - Properly gated drive model functionality with IsaacSim version checks (v5.0+)
- Refactored actuator configs into separate files (
actuator_pd_cfg.py,actuator_net_cfg.py) for better organization - Added comprehensive tests for drive model initialization and integration
Issues found:
- Variable naming inconsistency in
actuator_base.py:388(jvsnames) that will cause incorrect error messages - Minor style issues with integer comparison using
isinstead of==
The implementation is well-structured with proper version compatibility handling. The drive model feature is only enabled for IsaacSim 5.0+, with appropriate guards throughout the codebase.
Confidence Score: 4/5
- Safe to merge after fixing variable naming bug that will cause incorrect error messages in dict-based drive_model configs
- Implementation is solid with proper version checking and comprehensive tests. The variable naming bug (line 388/400) needs fixing as it will produce incorrect error messages when using dict-based drive_model configs with invalid tuple lengths. Otherwise, architecture is sound and well-integrated.
- source/isaaclab/isaaclab/actuators/actuator_base.py - Fix variable naming inconsistency on lines 388/400
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| source/isaaclab/isaaclab/actuators/actuator_base.py | 3/5 | Adds drive model parsing with tuple/dict support. Has variable name bug on line 388/400 (j vs names) and uses is instead of == for integer comparison. |
| source/isaaclab/isaaclab/actuators/actuator_base_cfg.py | 5/5 | Adds DriveModelCfg NamedTuple for drive model parameters - clean implementation, well-documented. |
| source/isaaclab/isaaclab/assets/articulation/articulation.py | 5/5 | Adds write_joint_drive_model_to_sim() method and integrates drive model with proper version checking (>=5). Version guards properly protect new functionality. |
| source/isaaclab/isaaclab/assets/articulation/articulation_data.py | 5/5 | Adds joint_drive_model_parameters and default_joint_drive_model_parameters fields - straightforward data structure changes. |
| source/isaaclab/test/actuators/test_drive_model_properties.py | 5/5 | New test file validates drive model initialization - comprehensive parametrized tests cover different configurations. |
Sequence Diagram
sequenceDiagram
participant User
participant ArticulationCfg
participant Articulation
participant ActuatorBase
participant PhysXView
User->>ArticulationCfg: Configure actuator with drive_model
Note over ArticulationCfg: DriveModelCfg(speed_effort_gradient,<br/>max_actuator_velocity,<br/>velocity_dependent_resistance)
User->>Articulation: Initialize articulation
Articulation->>Articulation: _process_actuators()
Articulation->>ActuatorBase: __init__(cfg, drive_model=usd_defaults)
ActuatorBase->>ActuatorBase: _parse_joint_parameter(cfg.drive_model)
Note over ActuatorBase: Parse dict/float/tuple config<br/>into (num_envs, num_joints, 3) tensor
ActuatorBase-->>Articulation: actuator.drive_model
alt IsaacSim v5.0+
Articulation->>Articulation: write_joint_drive_model_to_sim(actuator.drive_model)
Articulation->>PhysXView: set_dof_drive_model_properties()
Note over PhysXView: Apply drive model constraints<br/>to motor simulation
else IsaacSim < v5.0
Note over Articulation: Skip drive model setup<br/>(feature not available)
end
loop Simulation Step
User->>Articulation: set_joint_*_target()
Articulation->>Articulation: write_data_to_sim()
Articulation->>Articulation: _apply_actuator_model()
Articulation->>ActuatorBase: compute(control_action)
Note over ActuatorBase: Drive model parameters<br/>already in PhysX
ActuatorBase-->>Articulation: computed_effort
Articulation->>PhysXView: set_dof_actuation_forces()
Note over PhysXView: Apply effort with<br/>drive model constraints
end
24 files reviewed, 2 comments
| indices, _, values = string_utils.resolve_matching_names_values(cfg_value, self.joint_names) | ||
| # note: need to specify type to be safe (e.g. values are ints, but we want floats) | ||
| param[:, indices] = torch.tensor(values, dtype=torch.float, device=self._device) | ||
| indices, j, values = string_utils.resolve_matching_names_values(cfg_value, self.joint_names) |
Contributor
There was a problem hiding this comment.
syntax: Variable j assigned here but referenced as names on line 400 error message. This is inconsistent - use j throughout for matched joint names.
Suggested change
| indices, j, values = string_utils.resolve_matching_names_values(cfg_value, self.joint_names) | |
| indices, names, values = string_utils.resolve_matching_names_values(cfg_value, self.joint_names) |
Collaborator
Author
There was a problem hiding this comment.
This is probably a useful data point for the Greptile devs.
| # Raise an exception if the tuple is the incorrect length | ||
| if len(v) is not expected_shape[2]: | ||
| raise ValueError( | ||
| f"Invalid tuple length for parameter {param_name} on joint {j[i]} at index" |
Contributor
There was a problem hiding this comment.
syntax: Use names[i] instead of j[i] to match variable name from line 388.
Suggested change
| f"Invalid tuple length for parameter {param_name} on joint {j[i]} at index" | |
| f"Invalid tuple length for parameter {param_name} on joint {names[i]} at index" |
Contributor
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This PR adds support for PhysX 5.0+ drive model properties that enable more accurate motor performance envelope modeling. The implementation adds a new DriveModelCfg with three parameters (speed_effort_gradient, max_actuator_velocity, velocity_dependent_resistance) to constrain motor actuation based on velocity and effort dependencies.
Key Changes:
- Added
DriveModelCfgnested class toActuatorBaseCfgwith motor performance parameters - Extended
ActuatorBaseto parse and store drive model as(num_envs, num_joints, 3)tensor - Added
write_joint_drive_model_to_sim()method toArticulationclass - Refactored actuator configs into separate
actuator_pd_cfg.pyfile - Added initialization of
joint_drive_model_parametersinArticulationData - Version-gated functionality for IsaacSim v5+ using
get_version()[2] >= 5checks
Critical Issue Found:
The PR contains a breaking bug for IsaacSim versions < 5. The joint_drive_model_parameters field is only initialized when version >= 5 (lines 1636-1640), but is unconditionally accessed at lines 1763 and 1808, causing TypeError: 'NoneType' object is not subscriptable for older versions. This needs to be fixed by either initializing with default values for all versions or adding conditional checks.
Confidence Score: 1/5
- Not safe to merge - contains critical runtime bug for IsaacSim < 5
- The version compatibility issue will cause immediate crashes on IsaacSim < 5 when any articulation with actuators is instantiated. While the feature implementation is otherwise solid with good test coverage, this blocking bug prevents safe deployment
source/isaaclab/isaaclab/assets/articulation/articulation.pyrequires immediate fix for version < 5 compatibility at lines 1636-1640, 1763, and 1808
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| source/isaaclab/isaaclab/assets/articulation/articulation.py | 1/5 | Adds drive model API and initialization, but contains critical bug: joint_drive_model_parameters remains None for IsaacSim < 5, causing crashes at lines 1763 and 1808 |
| source/isaaclab/isaaclab/actuators/actuator_base.py | 3/5 | Adds drive_model parsing with tuple support. Some minor issues with variable naming inconsistency (j vs names) and is comparisons instead of == |
| source/isaaclab/isaaclab/actuators/actuator_base_cfg.py | 5/5 | Adds DriveModelCfg nested class with three parameters for motor performance envelope modeling. Clean implementation |
| source/isaaclab/isaaclab/assets/articulation/articulation_data.py | 5/5 | Adds joint_drive_model_parameters field initialized to None. Clean change |
| source/isaaclab/test/actuators/test_drive_model_properties.py | 5/5 | New test file verifying drive model initialization with proper assertions. Tests pass configuration and verify tensor values |
| source/isaaclab/isaaclab/actuators/actuator_pd_cfg.py | 5/5 | New config file extracting actuator configs from __init__.py. Clean refactor with no logic changes |
Sequence Diagram
sequenceDiagram
participant User
participant Articulation
participant ArticulationData
participant ActuatorBase
participant PhysXView
Note over Articulation,PhysXView: Initialization Phase (v5+ only)
User->>Articulation: Create articulation with actuator configs
Articulation->>PhysXView: get_dof_drive_model_properties()
PhysXView-->>Articulation: default drive model params
Articulation->>ArticulationData: Store default_joint_drive_model_parameters
Articulation->>ArticulationData: Clone to joint_drive_model_parameters
Note over Articulation,ActuatorBase: Actuator Setup Phase
loop For each actuator group
Articulation->>ArticulationData: Get joint_drive_model_parameters[joint_ids]
Articulation->>ActuatorBase: __init__(cfg, ..., drive_model=params)
ActuatorBase->>ActuatorBase: _parse_joint_parameter(cfg.drive_model, default)
Note over ActuatorBase: Parses dict/float/tuple/DriveModelCfg<br/>Creates (num_envs, num_joints, 3) tensor
ActuatorBase-->>Articulation: Initialized actuator with drive_model
alt IsaacSim v5+
Articulation->>Articulation: write_joint_drive_model_to_sim(actuator.drive_model)
Articulation->>PhysXView: set_dof_drive_model_properties(params)
Articulation->>ArticulationData: Update default_joint_drive_model_parameters
end
end
Note over Articulation,PhysXView: Runtime Phase
User->>Articulation: write_joint_drive_model_to_sim(params, joint_ids, env_ids)
Articulation->>ArticulationData: Update joint_drive_model_parameters[env_ids, joint_ids]
Articulation->>PhysXView: set_dof_drive_model_properties(params)
PhysXView-->>Articulation: Drive model applied to simulation
24 files reviewed, 2 comments
Collaborator
Author
|
I'm getting a test failure: =================================== FAILURES ===================================
____________________________ test_resolve_prim_pose ____________________________
source/isaaclab/test/sim/test_utils.py:254: in test_resolve_prim_pose
np.testing.assert_allclose(quat, rand_quats[i, 2], atol=1e-3)
/isaac-sim/kit/python/lib/python3.11/contextlib.py:81: in inner
return func(*args, **kwds)
^^^^^^^^^^^^^^^^^^^
E AssertionError:
E Not equal to tolerance rtol=1e-07, atol=0.001
E
E Mismatched elements: 4 / 4 (100%)
E Max absolute difference: 0.00692904
E Max relative difference: 0.03919505
E x: array([ 0.615105, 0.438734, 0.169855, -0.632699])
E y: array([ 0.61288 , 0.432324, 0.176784, -0.637355])
- generated xml file: /workspace/isaaclab/tests/test-reports-test_utils.py.xml -
=========================== short test summary info ============================
FAILED source/isaaclab/test/sim/test_utils.py::test_resolve_prim_pose - Asser...It seems unrelated to my changes. I will rerun, but if it continues to fail, what is the best action to take? My usual approach would be to mark it as expected to fail or to skip the test. Otherwise, I will have to bypass merge the MR. What's the best way forward? |
Contributor
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This PR adds support for actuator drive model performance envelopes, allowing more accurate sim-to-real transfer by modeling velocity-effort constraints on motor actuation through three parameters: speed_effort_gradient, max_actuator_velocity, and velocity_dependent_resistance.
Key Changes:
- Added
DriveModelCfgtoActuatorBaseCfgwith tuple-based configuration support - Extended
ActuatorBaseto parse and store drive model parameters as tensors - Added
write_joint_drive_model_to_sim()method toArticulationclass - Reorganized actuator config modules (renamed
actuator_cfg.pytoactuator_base_cfg.py, addedactuator_pd_cfg.pyandactuator_net_cfg.py) - Added comprehensive tests with proper version guards for IsaacSim 5+ requirement
Critical Issue Found:
- Version compatibility bug:
joint_drive_model_parametersis accessed without version checks but only initialized for IsaacSim v5+, causing crashes on older versions (articulation.py:829, 1763, 1808)
Confidence Score: 2/5
- Critical version compatibility bugs will cause crashes on IsaacSim < 5
- Score reflects critical logic errors that will cause runtime failures when using IsaacSim versions below 5, despite the feature being well-tested for v5+
- Critical:
source/isaaclab/isaaclab/assets/articulation/articulation.py- must fix version compatibility before merge
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| source/isaaclab/isaaclab/assets/articulation/articulation.py | 1/5 | Added drive model support but has critical version compatibility bugs - joint_drive_model_parameters accessed when None for IsaacSim < 5 (lines 829, 1763, 1808) |
| source/isaaclab/isaaclab/actuators/actuator_base.py | 4/5 | Added drive model parsing logic with tuple support - implementation looks correct, handles dict/float/tuple configs properly |
| source/isaaclab/isaaclab/actuators/actuator_base_cfg.py | 5/5 | Added DriveModelCfg namedtuple with 3 parameters for performance envelope - clean config structure |
| source/isaaclab/test/assets/test_articulation.py | 5/5 | Added comprehensive tests for drive model with proper version guards (@pytest.mark.skipif) - tests look thorough |
Sequence Diagram
sequenceDiagram
participant User
participant ArticulationCfg
participant Articulation
participant ActuatorBase
participant PhysXView
User->>ArticulationCfg: Define drive_model in actuator config
Note over ArticulationCfg: DriveModelCfg(speed_effort_gradient,<br/>max_actuator_velocity,<br/>velocity_dependent_resistance)
User->>Articulation: Initialize articulation
Articulation->>PhysXView: get_dof_drive_model_properties()
Note over PhysXView: Only available IsaacSim v5+
PhysXView-->>Articulation: joint_drive_model_parameters tensor
Articulation->>ActuatorBase: Create actuator with drive_model
Note over ActuatorBase: _parse_joint_parameter(cfg.drive_model,<br/>usd_default, expected_shape=(n,m,3))
ActuatorBase->>ActuatorBase: Parse dict/float/tuple configs
ActuatorBase-->>Articulation: actuator.drive_model tensor
Articulation->>Articulation: write_joint_drive_model_to_sim()
Articulation->>PhysXView: set_dof_drive_model_properties()
Note over PhysXView: Applies performance envelope<br/>constraints to motors
User->>Articulation: Step simulation
Articulation->>ActuatorBase: compute(control_action, joint_pos, joint_vel)
ActuatorBase-->>Articulation: desired efforts (constrained by drive model)
Articulation->>PhysXView: Apply computed efforts
24 files reviewed, 3 comments
Contributor
Greptile OverviewUpdated On: 2025-11-06 19:35:03 UTC Greptile SummaryThis PR implements actuator drive model improvements to enable velocity and effort dependent constraints on motor actuation, supporting PhysX's performance envelope modeling. The implementation adds a new Key Changes:
Critical Issue Found:
Style Issues:
Confidence Score: 2/5
Important Files ChangedFile Analysis
Sequence DiagramsequenceDiagram
participant User
participant ArticulationCfg
participant Articulation
participant PhysXView
participant ActuatorBase
participant ArticulationData
User->>ArticulationCfg: Configure with drive_model in actuators
ArticulationCfg->>Articulation: Initialize articulation
Articulation->>PhysXView: get_dof_drive_model_properties() [v5+]
PhysXView-->>Articulation: Return default drive model params
Articulation->>ArticulationData: Store in joint_drive_model_parameters
Articulation->>ActuatorBase: Create actuator with drive_model
ActuatorBase->>ActuatorBase: _parse_joint_parameter(drive_model)
ActuatorBase->>ActuatorBase: Store as tensor (num_envs, num_joints, 3)
Articulation->>Articulation: write_joint_drive_model_to_sim()
Articulation->>PhysXView: set_dof_drive_model_properties() [v5+]
PhysXView-->>Articulation: Drive model applied to simulation
Note over Articulation,PhysXView: Drive model constrains motor<br/>performance envelope in PhysX
|
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com> Signed-off-by: Brian McCann <[email protected]>
…-bdai/IsaacLab into bcm/performance_envelope
Contributor
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This PR implements actuator drive model improvements to support velocity and effort dependent constraints on motor actuation, enabling better sim-to-real transfer through motor performance envelope modeling.
Key Changes:
- Introduced
DriveModelCfgwith three parameters:speed_effort_gradient,max_actuator_velocity, andvelocity_dependent_resistance - Added
drive_modeltensor attribute (shape: num_envs × num_joints × 3) toActuatorBaseclass - Implemented
write_joint_drive_model_to_sim()method inArticulationclass - Properly handles IsaacSim version compatibility (v5+ required for drive model, v<5 uses zero tensors)
- Refactored actuator configuration files: split
actuator_cfg.pyintoactuator_base_cfg.py,actuator_pd_cfg.py, andactuator_net_cfg.py - Added comprehensive tests in
test_drive_model_properties.pyfor drive model initialization
Architecture:
The drive model parameters flow from configuration → ActuatorBaseCfg.DriveModelCfg → ActuatorBase.drive_model tensor → PhysX simulation (v5+). The implementation includes proper version guards ensuring backward compatibility with IsaacSim v4.x.
Issues Found:
- Minor style issue: Variable naming inconsistency in
actuator_base.py:388(usesjinstead ofnames)
Confidence Score: 5/5
- This PR is safe to merge with minimal risk - well-tested, properly version-gated, and includes backward compatibility
- The implementation is solid with proper version compatibility handling for IsaacSim v4 vs v5, comprehensive test coverage for the new drive model feature, and clean refactoring of configuration files. The only issue found is a minor variable naming inconsistency that doesn't affect functionality.
- No files require special attention - the minor style issue in actuator_base.py:388 can be addressed but doesn't impact functionality
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| source/isaaclab/isaaclab/actuators/actuator_base.py | 4/5 | Added drive_model parameter support with 3D tensor shape (num_envs, num_joints, 3) for speed_effort_gradient, max_actuator_velocity, and velocity_dependent_resistance. Minor variable naming inconsistency on line 388 (uses 'j' instead of 'names'). |
| source/isaaclab/isaaclab/actuators/actuator_base_cfg.py | 5/5 | Introduced DriveModelCfg NamedTuple and drive_model configuration attribute. Clean, well-documented changes with appropriate defaults (speed_effort_gradient=0.0, max_actuator_velocity=inf, velocity_dependent_resistance=0.0). |
| source/isaaclab/isaaclab/assets/articulation/articulation.py | 5/5 | Added write_joint_drive_model_to_sim() method and proper version checking for IsaacSim v5. Correctly initializes drive_model parameters with zeros for v<5 compatibility. Passes drive_model to actuators with proper version guards. |
| source/isaaclab/test/actuators/test_drive_model_properties.py | 5/5 | New test file validating drive_model initialization with proper tensor shape verification for all three parameters across different device and env/joint configurations. |
Sequence Diagram
sequenceDiagram
participant User
participant Articulation
participant ActuatorBase
participant PhysX
User->>Articulation: Configure actuator with drive_model
Note over Articulation: Check IsaacSim version
alt IsaacSim >= v5
Articulation->>PhysX: get_dof_drive_model_properties()
PhysX-->>Articulation: drive_model_parameters tensor (N, J, 3)
else IsaacSim < v5
Articulation->>Articulation: Initialize zeros tensor (N, J, 3)
end
Articulation->>ActuatorBase: __init__(drive_model=tensor)
ActuatorBase->>ActuatorBase: _parse_joint_parameter(drive_model)
Note over ActuatorBase: Validates shape (num_envs, num_joints, 3)<br/>for speed_effort_gradient, max_actuator_velocity,<br/>velocity_dependent_resistance
alt IsaacSim >= v5
Articulation->>PhysX: write_joint_drive_model_to_sim()
PhysX->>PhysX: Apply performance envelope constraints
end
User->>ActuatorBase: compute(control_action, joint_pos, joint_vel)
ActuatorBase->>ActuatorBase: Calculate effort with drive_model constraints
ActuatorBase-->>User: Modified control action
24 files reviewed, 1 comment
Contributor
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This PR implements actuator drive model improvements to support velocity and effort dependent constraints on motor actuation, enabling better sim-to-real transfer by modeling motor performance envelopes.
Key Changes
- New DriveModelCfg: Introduces
ActuatorBaseCfg.DriveModelCfgwith three parameters:speed_effort_gradient,max_actuator_velocity, andvelocity_dependent_resistanceto model torque-speed and speed-torque boundaries - Config refactoring: Separated actuator configuration from implementation by creating
actuator_base_cfg.py,actuator_pd_cfg.py, andactuator_net_cfg.pyfiles - Enhanced parameter parsing: Extended
_parse_joint_parameter()to handle tuple values (3-element drive model parameters) in addition to floats and dicts - Version-safe implementation: Properly guards IsaacSim v5+ features with version checks and provides zero-initialized fallbacks for v4.x compatibility
- Comprehensive testing: Added tests for drive model initialization across both implicit and explicit actuators with proper v5+ guards
Issues Found
- Minor syntax issues in error messages (missing space and comma) in
actuator_base.py:376andactuator_base.py:401-402
The implementation correctly handles backward compatibility by initializing drive model parameters to zero tensors for IsaacSim versions < 5, and only calls PhysX drive model APIs when v5+ is detected.
Confidence Score: 4/5
- This PR is safe to merge after fixing minor syntax issues in error messages
- Score of 4 reflects solid implementation with proper version guards and comprehensive testing, but minor syntax errors in error messages need correction. The core logic is sound: version compatibility is handled correctly, tests validate initialization behavior, and the feature is properly gated for v5+.
- Pay attention to
source/isaaclab/isaaclab/actuators/actuator_base.pyfor the syntax fixes in error messages at lines 376 and 401-402
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| source/isaaclab/isaaclab/actuators/actuator_base_cfg.py | 5/5 | New config file introducing DriveModelCfg with velocity-effort constraints |
| source/isaaclab/isaaclab/actuators/actuator_base.py | 4/5 | Extended _parse_joint_parameter to handle tuples for drive model; minor syntax issues in error messages |
| source/isaaclab/isaaclab/assets/articulation/articulation.py | 5/5 | Added write_joint_drive_model_to_sim method and version-safe initialization for v5+ drive model parameters |
| source/isaaclab/test/assets/test_articulation.py | 5/5 | Added comprehensive drive model tests for both implicit and explicit actuators with v5+ guard |
Sequence Diagram
sequenceDiagram
participant User
participant ArticulationCfg
participant Articulation
participant PhysX
participant ActuatorBase
participant ArticulationData
User->>ArticulationCfg: Define drive_model params
ArticulationCfg->>Articulation: Initialize with actuator configs
Articulation->>PhysX: Check IsaacSim version
alt IsaacSim >= v5
PhysX-->>Articulation: get_dof_drive_model_properties()
Articulation->>ArticulationData: Store default_joint_drive_model_parameters
else IsaacSim < v5
Articulation->>ArticulationData: Initialize zeros(num_instances, num_joints, 3)
end
Articulation->>ActuatorBase: Initialize actuator with drive_model
ActuatorBase->>ActuatorBase: _parse_joint_parameter(drive_model)
ActuatorBase->>ActuatorBase: Store as tensor (num_envs, num_joints, 3)
alt IsaacSim >= v5
Articulation->>PhysX: write_joint_drive_model_to_sim()
PhysX->>PhysX: set_dof_drive_model_properties()
end
Articulation->>ArticulationData: Update default_joint_drive_model_parameters
24 files reviewed, 2 comments
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com> Signed-off-by: Brian McCann <[email protected]>
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com> Signed-off-by: Brian McCann <[email protected]>
Contributor
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This PR implements support for the new actuator drive model in IsaacSim v5+, enabling velocity and effort dependent constraints on motor actuation through three key parameters: speed_effort_gradient, max_actuator_velocity, and velocity_dependent_resistance.
Key Changes
-
Configuration Refactoring: Split
actuator_cfg.pyintoactuator_base_cfg.py,actuator_pd_cfg.py, andactuator_net_cfg.pyfor better organization. AddedDriveModelCfgNamedTuple toActuatorBaseCfg. -
Drive Model Support: Extended
ActuatorBaseto parse and store 3D drive model tensors (num_envs × num_joints × 3). Supports float, tuple, and dict-based configuration with per-joint customization. -
Version Compatibility: Properly handles IsaacSim version differences - initializes drive model parameters from PhysX for v5+, uses zero tensors for v<5. All drive model writes are conditionally executed only for v5+.
-
Integration:
Articulationclass reads default drive model parameters from PhysX, passes them to actuators during initialization, and writes configured values back to simulation. -
Test Coverage: Added dedicated test file
test_drive_model_properties.pyand extendedtest_articulation.pywith tests for both implicit and explicit actuators, properly guarded with version checks.
Issues Found
- Line 401 in actuator_base.py: Duplicate error message line needs removal (syntax error).
Confidence Score: 4/5
- This PR is safe to merge after fixing the duplicate error message on line 401.
- The implementation is solid with proper version compatibility handling, comprehensive test coverage, and clean refactoring. The only issue is a minor syntax error (duplicate line) in an error message that needs removal. The code properly handles both IsaacSim v5+ (with drive model support) and v<5 (without support) through version checks and default initialization.
- source/isaaclab/isaaclab/actuators/actuator_base.py - Remove duplicate error message line 401
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| source/isaaclab/isaaclab/actuators/actuator_base.py | 3/5 | Adds drive model support with 3D tensor parameter parsing. Has duplicate error message on line 401 that needs removal. |
| source/isaaclab/isaaclab/actuators/actuator_base_cfg.py | 5/5 | Defines DriveModelCfg with speed_effort_gradient, max_actuator_velocity, velocity_dependent_resistance parameters. Clean implementation. |
| source/isaaclab/isaaclab/assets/articulation/articulation.py | 5/5 | Properly handles drive model parameters with version compatibility checks for IsaacSim v5. Initializes with zeros for v<5. |
| source/isaaclab/test/actuators/test_drive_model_properties.py | 5/5 | New test file validating drive model parameter initialization across different configurations. Good test coverage. |
| source/isaaclab/test/assets/test_articulation.py | 5/5 | Added comprehensive tests for drive model for both implicit and explicit actuators with version guards. Good coverage. |
Sequence Diagram
sequenceDiagram
participant User
participant ActuatorCfg as ActuatorBaseCfg
participant Articulation
participant ActuatorBase
participant PhysXView
User->>ActuatorCfg: Configure drive_model<br/>(speed_effort_gradient, max_actuator_velocity,<br/>velocity_dependent_resistance)
User->>Articulation: Initialize with actuator configs
Articulation->>PhysXView: get_dof_drive_model_properties()
PhysXView-->>Articulation: Default drive model tensors (v5+)<br/>or None (v<5)
alt IsaacSim v5+
Articulation->>Articulation: Store default_joint_drive_model_parameters
else IsaacSim v<5
Articulation->>Articulation: Initialize with zeros<br/>(num_instances, num_joints, 3)
end
Articulation->>ActuatorBase: __init__(..., drive_model)
ActuatorBase->>ActuatorBase: _parse_joint_parameter()<br/>Parse drive_model config
alt drive_model is float
ActuatorBase->>ActuatorBase: Apply same value to all joints
else drive_model is tuple
ActuatorBase->>ActuatorBase: Validate tuple length == 3<br/>Assign to [:,:,0], [:,:,1], [:,:,2]
else drive_model is dict
ActuatorBase->>ActuatorBase: Match joint names<br/>Assign per-joint tuples
end
ActuatorBase-->>Articulation: actuator with drive_model tensor
alt IsaacSim v5+
Articulation->>PhysXView: write_joint_drive_model_to_sim()
PhysXView->>PhysXView: set_dof_drive_model_properties()
else IsaacSim v<5
Articulation->>Articulation: Skip write (not supported)
end
Articulation->>Articulation: Store in default_joint_drive_model_parameters
Note over Articulation,PhysXView: Drive model affects motor performance<br/>during simulation with velocity-effort<br/>constraints
24 files reviewed, 1 comment
… recognizing other PRs are landing features simultaneously.
Contributor
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
Adds actuator drive model support to configure velocity and effort-dependent constraints on motor actuation, enabling more accurate sim-to-real transfer by modeling motor performance envelopes.
Key Changes:
- Introduces
DriveModelCfgwith 3 parameters:speed_effort_gradient,max_actuator_velocity, andvelocity_dependent_resistance - Adds drive model parameter parsing in
ActuatorBasesupporting float, tuple, and dict configurations - Integrates drive model with PhysX for IsaacSim v5+, with graceful fallback for v<5 (stores parameters but doesn't apply to simulation)
- Refactors actuator configs: splits
actuator_cfg.py→actuator_base_cfg.py,actuator_pd_cfg.py,actuator_net_cfg.py - Adds comprehensive tests for drive model initialization and integration
Issues Found:
- Critical: Duplicate error message line in
actuator_base.py:400-401that needs removal - Version compatibility properly handled with v5+ checks throughout
Confidence Score: 4/5
- Safe to merge after fixing the duplicate line bug in actuator_base.py:400-401
- Score reflects well-designed feature with proper version checks and comprehensive tests, but has one syntax bug (duplicate error message line) that must be fixed before merge. The implementation correctly handles backward compatibility with v<5, includes good test coverage, and follows clean architecture patterns with config separation.
- source/isaaclab/isaaclab/actuators/actuator_base.py requires fixing duplicate line 400-401
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| source/isaaclab/isaaclab/actuators/actuator_base.py | 3/5 | Added drive_model support with parameter parsing for 3-tuple values. Has duplicate error message line (400-401) that must be fixed. |
| source/isaaclab/isaaclab/actuators/actuator_base_cfg.py | 5/5 | Added DriveModelCfg NamedTuple for drive model parameters (speed_effort_gradient, max_actuator_velocity, velocity_dependent_resistance). Clean configuration addition. |
| source/isaaclab/isaaclab/assets/articulation/articulation.py | 4/5 | Added drive model support with v5+ version checking. Properly initializes parameters with zeros for v<5 compatibility. Includes new write_joint_drive_model_to_sim method. |
| source/isaaclab/test/actuators/test_drive_model_properties.py | 5/5 | New test validating drive model parameter initialization across different configurations (num_envs, num_joints, devices). |
| source/isaaclab/test/assets/test_articulation.py | 5/5 | Added tests for drive model with both implicit and explicit actuators. Tests properly skip for IsaacSim v<5. |
Sequence Diagram
sequenceDiagram
participant User
participant ArticulationCfg
participant Articulation
participant ActuatorBase
participant PhysXView
User->>ArticulationCfg: Configure drive_model params
Note over ArticulationCfg: DriveModelCfg(speed_effort_gradient,<br/>max_actuator_velocity,<br/>velocity_dependent_resistance)
User->>Articulation: Initialize with config
Articulation->>PhysXView: Check IsaacSim version
alt IsaacSim v5+
Articulation->>PhysXView: get_dof_drive_model_properties()
PhysXView-->>Articulation: Return drive model parameters
Articulation->>Articulation: Store default_joint_drive_model_parameters
else IsaacSim v<5
Articulation->>Articulation: Initialize with zeros (3-tuple per joint)
end
Articulation->>ActuatorBase: Create actuator with drive_model
ActuatorBase->>ActuatorBase: _parse_joint_parameter(drive_model)
Note over ActuatorBase: Parse float/tuple/dict configs<br/>into tensor [num_envs, num_joints, 3]
alt IsaacSim v5+
Articulation->>PhysXView: write_joint_drive_model_to_sim()
PhysXView->>PhysXView: set_dof_drive_model_properties()
Note over PhysXView: Apply velocity/effort constraints<br/>to motor actuation
else IsaacSim v<5
Note over Articulation: Drive model stored but not<br/>applied to simulation
end
User->>Articulation: Step simulation
Articulation->>ActuatorBase: compute(action, pos, vel)
ActuatorBase->>ActuatorBase: Use drive_model constraints
ActuatorBase-->>Articulation: Return computed efforts
24 files reviewed, 1 comment
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com> Signed-off-by: Brian McCann <[email protected]>
…-bdai/IsaacLab into bcm/performance_envelope
Contributor
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This PR implements actuator drive model improvements for IsaacSim v5.0+, adding velocity and effort dependent constraints on motor actuation through three new parameters: speed_effort_gradient, max_actuator_velocity, and velocity_dependent_resistance.
Key Changes:
- Introduced
ActuatorBaseCfg.DriveModelCfgas aNamedTupleto organize drive model parameters - Extended
ActuatorBase._parse_joint_parameterto handle 3D tensors for tuple-based configurations - Added
write_joint_drive_model_to_simmethod toArticulationclass with proper version checks - Reorganized actuator config files (
actuator_base_cfg.py) to avoid circular dependencies - Properly handles backward compatibility for IsaacSim < v5 by initializing drive model parameters with zero tensors
Test Coverage:
- New test file
test_drive_model_properties.pyvalidates drive model parameter initialization - All existing tests pass according to the PR description
Minor Issues:
- Two minor string formatting issues in error messages (missing spaces/commas) that don't affect functionality
Confidence Score: 4/5
- This PR is safe to merge with only minor style improvements needed
- Score reflects well-structured implementation with proper version compatibility checks. The code correctly handles IsaacSim v5+ features while maintaining backward compatibility. Only minor formatting issues in error messages were found that don't affect functionality.
- source/isaaclab/isaaclab/actuators/actuator_base.py - Minor string formatting improvements in error messages
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| source/isaaclab/isaaclab/actuators/actuator_base.py | 4/5 | Extended _parse_joint_parameter to handle 3D tensors for drive model parameters with tuple configs; minor string formatting issues in error messages |
| source/isaaclab/isaaclab/actuators/actuator_base_cfg.py | 5/5 | New config file with ActuatorBaseCfg and nested DriveModelCfg NamedTuple - clean separation to avoid circular dependencies |
| source/isaaclab/isaaclab/assets/articulation/articulation.py | 5/5 | Added write_joint_drive_model_to_sim method with proper IsaacSim v5+ version checks; correctly initializes drive model params for backward compatibility |
| source/isaaclab/test/actuators/test_drive_model_properties.py | 5/5 | New test verifying drive model initialization with all three parameters (speed_effort_gradient, max_actuator_velocity, velocity_dependent_resistance) |
Sequence Diagram
sequenceDiagram
participant User
participant ArticulationCfg
participant Articulation
participant ActuatorBase
participant PhysX
User->>ArticulationCfg: Configure actuator with drive_model
ArticulationCfg->>Articulation: Initialize articulation
Articulation->>Articulation: _post_initialize()
alt IsaacSim >= v5
Articulation->>PhysX: get_dof_drive_model_properties()
PhysX-->>Articulation: default drive model params
else IsaacSim < v5
Articulation->>Articulation: Initialize zeros tensor (backward compat)
end
Articulation->>Articulation: _process_actuators()
Articulation->>ActuatorBase: Create actuator instance
ActuatorBase->>ActuatorBase: _parse_joint_parameter(drive_model)
ActuatorBase->>ActuatorBase: Store drive_model tensor (num_envs, num_joints, 3)
Articulation->>Articulation: write_joint_drive_model_to_sim()
alt IsaacSim >= v5
Articulation->>PhysX: set_dof_drive_model_properties()
Note over PhysX: Applies speed_effort_gradient,<br/>max_actuator_velocity,<br/>velocity_dependent_resistance
else IsaacSim < v5
Note over Articulation: Skip (feature not supported)
end
24 files reviewed, 2 comments
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
This change implements the necessary changes to the articulation and actuator classes in order to configure the new actuator drive model including velocity and effort dependent constraints on motor actuation.
I have written tests against instantiation, and no other tests fail. I began looking into testing whether the drive model is being applied by physX. Doing so will require getting an isaacsim rather than isaaclab or physx view into the articulation in order to access the measured effort. @jtigue-bdai suggested we are trying to minimize dependence on the isaacsim api, preferring direct access to the physx layer. I decided to PR these changes without testing the physX level behavior. If we decide we want to introduce a dependency on the IsaacSim ArticulationView within the tests, I can either modify this PR, or create a follow on PR.
Type of change
Checklist
pre-commitchecks with./isaaclab.sh --formatconfig/extension.tomlfileCONTRIBUTORS.mdor my name already exists there