[SIM-950] Actuator Drive Model Improvements #3942
Open
+550
−171
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}
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
Comment on lines
300
to
302
| dm_velocity_dependent_resistance=dm_velocity_dependent_resistance, | ||
| dm_max_actuator_velocity=dm_max_actuator_velocity, | ||
| dm_speed_effort_gradient=dm_speed_effort_gradient, |
There was a problem hiding this comment.
syntax: Test uses non-existent properties dm_velocity_dependent_resistance, dm_max_actuator_velocity, dm_speed_effort_gradient. These don't exist in ActuatorBaseCfg.
Use drive_model parameter instead:
Suggested change
| dm_velocity_dependent_resistance=dm_velocity_dependent_resistance, | |
| dm_max_actuator_velocity=dm_max_actuator_velocity, | |
| dm_speed_effort_gradient=dm_speed_effort_gradient, | |
| drive_model=IdealPDActuatorCfg.DriveModelCfg( | |
| speed_effort_gradient=dm_speed_effort_gradient or 0.0, | |
| max_actuator_velocity=dm_max_actuator_velocity or float('inf'), | |
| velocity_dependent_resistance=dm_velocity_dependent_resistance or 0.0 | |
| ) if any([dm_speed_effort_gradient, dm_max_actuator_velocity, dm_velocity_dependent_resistance]) else None, |
| raise TypeError( | ||
| f"Invalid type for parameter value: {type(cfg_value)} for parameter {param_name}" | ||
| + f"actuator on joints {self.joint_names}. Expected float or dict, got tuple" | ||
| ) |
There was a problem hiding this comment.
style: Check uses is instead of == for integer comparison, which works but isn't idiomatic
Suggested change
| ) | |
| if len(cfg_value) == expected_shape[2]: |
| f"Invalid default type for parameter value: {type(default_value)} for " | ||
| + f"actuator on joints {self.joint_names}. Expected float or dict, got tuple" | ||
| ) | ||
| # ensure the tuple is the correct length, and assign to the last tensor dimensions across all joints |
There was a problem hiding this comment.
style: Check uses is instead of == for integer comparison
Suggested change
| # ensure the tuple is the correct length, and assign to the last tensor dimensions across all joints | |
| if len(default_value) == expected_shape[2]: |
| # otherwise, we expect tuples | ||
| else: | ||
| # We can't directly assign tuples to tensors, so iterate through them | ||
| for i, v in enumerate(values): |
There was a problem hiding this comment.
style: Check uses is instead of == for integer comparison
Suggested change
| for i, v in enumerate(values): | |
| if len(v) != expected_shape[2]: |
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: |
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"
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), |
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
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 |
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
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.
| + f"actuator on joints {self.joint_names}. Expected float or dict, got tuple" | ||
| ) | ||
| # ensure the tuple is the correct length, and assign to the last tensor dimensions across all joints | ||
| if len(cfg_value) is expected_shape[2]: |
There was a problem hiding this comment.
could invert this if and avoid the nesting:
if not len(cfg_value) ... :
raise ValueError(...
for i, v in enumerate(cfg_value):
...
| 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): |
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?
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.
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.
| + f"actuator on joints {self.joint_names}. Expected float or dict, got tuple" | ||
| ) | ||
| # ensure the tuple is the correct length, and assign to the last tensor dimensions across all joints | ||
| if len(default_value) is expected_shape[2]: |
There was a problem hiding this comment.
same as above, consider inverting this if
| self._data.joint_pos_limits = self._data.default_joint_pos_limits.clone() | ||
| self._data.joint_vel_limits = self.root_physx_view.get_dof_max_velocities().to(self.device).clone() | ||
| self._data.joint_effort_limits = self.root_physx_view.get_dof_max_forces().to(self.device).clone() | ||
| if int(get_version()[2]) >= 5: |
There was a problem hiding this comment.
please use a constant instead of magic numbers
There was a problem hiding this comment.
I was mirroring the pattern from line 1621.
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
Comment on lines
+375
to
+377
| + f"actuator on joints {self.joint_names}. Expected float or dict, got tuple" | ||
| ) |
There was a problem hiding this comment.
syntax: Missing space before "actuator" in error message
Suggested change
| + f"actuator on joints {self.joint_names}. Expected float or dict, got tuple" | |
| ) | |
| f"Invalid type for parameter value: {type(cfg_value)} for parameter {param_name} " | |
| + f"actuator on joints {self.joint_names}. Expected float or dict, got tuple" |
Comment on lines
399
to
402
| f"Invalid tuple length for parameter {param_name} on joint {_[i]} at index" | ||
| f" {indices[i]}," | ||
| + f" expected {expected_shape[2]} got {len(v)}." | ||
| ) |
There was a problem hiding this comment.
syntax: Missing comma in error message between expected and got values
Suggested change
| f"Invalid tuple length for parameter {param_name} on joint {_[i]} at index" | |
| f" {indices[i]}," | |
| + f" expected {expected_shape[2]} got {len(v)}." | |
| ) | |
| raise ValueError( | |
| f"Invalid tuple length for parameter {param_name} on joint {_[i]} at index" | |
| f" {indices[i]}," | |
| + f" expected {expected_shape[2]}, got {len(v)}." | |
| ) |
Comment on lines
+425
to
+428
| f"Invalid tuple length for parameter {param_name}, got {len(default_value)}, expected" | ||
| + f" {expected_shape[2]}" | ||
| ) |
There was a problem hiding this comment.
syntax: Missing comma in error message
Suggested change
| f"Invalid tuple length for parameter {param_name}, got {len(default_value)}, expected" | |
| + f" {expected_shape[2]}" | |
| ) | |
| raise ValueError( | |
| f"Invalid tuple length for parameter {param_name}, got {len(default_value)}, expected" | |
| + f" {expected_shape[2]}" | |
| ) |
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
Comment on lines
323
to
406
| 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) | ||
| # if the expected shape has two dimensions, we expect floats | ||
| if len(expected_shape) < 3: | ||
| # 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) | ||
| # otherwise, we expect tuples | ||
| else: | ||
| # We can't directly assign tuples to tensors, so iterate through them | ||
| for i, v in enumerate(values): | ||
| # 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 {_[i]} at index" | ||
| f" {indices[i]}," | ||
| + f" expected {expected_shape[2]} got {len(v)}." | ||
| ) | ||
| # Otherwise iterate through the tuple, and assign the values in order. | ||
| for i2, v2 in enumerate(v): | ||
| param[:, indices[i], i2] = float(v2) |
There was a problem hiding this comment.
logic: Variable _ discarded on line 387 but referenced on line 399, causing undefined variable error when dict config with tuples used
Line 387 discards matched joint names with _, but line 399 tries to use _[i] in error message. This will cause runtime error when using dict-based drive_model config with invalid tuple lengths.
Suggested change
| 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) | |
| # if the expected shape has two dimensions, we expect floats | |
| if len(expected_shape) < 3: | |
| # 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) | |
| # otherwise, we expect tuples | |
| else: | |
| # We can't directly assign tuples to tensors, so iterate through them | |
| for i, v in enumerate(values): | |
| # 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 {_[i]} at index" | |
| f" {indices[i]}," | |
| + f" expected {expected_shape[2]} got {len(v)}." | |
| ) | |
| # Otherwise iterate through the tuple, and assign the values in order. | |
| for i2, v2 in enumerate(v): | |
| param[:, indices[i], i2] = float(v2) | |
| indices, names, values = string_utils.resolve_matching_names_values(cfg_value, self.joint_names) | |
| # if the expected shape has two dimensions, we expect floats | |
| if len(expected_shape) < 3: | |
| # 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) | |
| # otherwise, we expect tuples | |
| else: | |
| # We can't directly assign tuples to tensors, so iterate through them | |
| for i, v in enumerate(values): | |
| # 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 {names[i]} at index" | |
| f" {indices[i]}," | |
| + f" expected {expected_shape[2]} got {len(v)}." | |
| ) | |
| # Otherwise iterate through the tuple, and assign the values in order. | |
| for i2, v2 in enumerate(v): | |
| param[:, indices[i], i2] = float(v2) |
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
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
| viscous_friction=self._data.default_joint_viscous_friction_coeff[:, joint_ids], | ||
| effort_limit=self._data.joint_effort_limits[:, joint_ids], | ||
| velocity_limit=self._data.joint_vel_limits[:, joint_ids], | ||
| drive_model=self._data.joint_drive_model_parameters[:, joint_ids], |
There was a problem hiding this comment.
logic: Code will crash with TypeError: 'NoneType' object is not subscriptable when IsaacSim version < 5
joint_drive_model_parameters is only initialized on line 1640 when version >= 5, but here it's always accessed regardless of version. For version < 5, it remains None (initialized on articulation_data.py:388).
Similar parameters like default_joint_dynamic_friction_coeff are initialized with zero tensors for version < 5 (see lines 1625-1626). Apply the same pattern here.
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
| viscous_friction=self._data.default_joint_viscous_friction_coeff[:, joint_ids], | ||
| effort_limit=self._data.joint_effort_limits[:, joint_ids], | ||
| velocity_limit=self._data.joint_vel_limits[:, joint_ids], | ||
| drive_model=self._data.joint_drive_model_parameters[:, joint_ids], |
There was a problem hiding this comment.
logic: Will crash with TypeError: 'NoneType' object is not subscriptable for IsaacSim version < 5
joint_drive_model_parameters is only initialized on lines 1636-1640 when version >= 5, but here it's accessed unconditionally. For version < 5, it remains None (initialized in articulation_data.py:388).
Similar to how default_joint_dynamic_friction_coeff is initialized with zero tensors for version < 5 (lines 1625-1626), you need to either:
- Initialize
joint_drive_model_parameterswith default values for version < 5, OR - Conditionally pass this parameter only when version >= 5 (using
ActuatorBaseCfg.DriveModelCfg()as default)
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
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) |
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) |
| # 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" |
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" |
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