Actuators integration#5455
Conversation
![isaaclab-review-bot[bot]](https://avatars.githubusercontent.com/u/157846462?s=60&v=4){kind=small}
There was a problem hiding this comment.
🤖 Isaac Lab Review Bot
Summary
This PR integrates Newton-native actuators into Isaac Lab, allowing actuator configs (IdealPD, DCMotor, DelayedPD, etc.) to be translated into USD NewtonActuator prims and stepped by the physics engine. The implementation spans both Newton and PhysX backends, with comprehensive test coverage. The architecture is well-designed with a shared adapter pattern, but there are several correctness issues and missing error handling that need attention before merging.
Architecture Impact
High cross-module impact. Changes touch:
- Core base class (
base_articulation.py): USD authoring hook in__init__ - All environment types (
direct_rl_env.py,direct_marl_env.py,manager_based_env.py,manager_based_rl_env.py): New decimation-handling code path - Newton backend (
newton_manager.py,articulation.py): Adapter registration, CUDA graph capture with actuators - PhysX backend (
articulation.py): NewPhysxActuatorWrapperstepping path - Domain randomization (
events.py): Newwrite_actuator_*_to_simmethods
The NewtonActuatorAdapter is correctly shared between backends. The decimation loop folding is a significant performance optimization but introduces complexity in the stepping logic.
Implementation Verdict
Significant concerns. The core design is sound, but there are correctness issues in the stepping logic, missing imports, and potential state corruption scenarios that need addressing.
Test Coverage
Good coverage for the happy path. The PR includes 1766 lines of tests covering:
- Lab vs Newton equivalence for IdealPD, DCMotor, DelayedPD, RemotizedPD
- Mixed actuator configurations (including implicit)
- Decimation with CUDA graphs
- Partial environment reset
- Neural network actuators (MLP/LSTM)
Missing: Tests for error cases (malformed configs, missing joints), PhysX backend equivalence tests don't verify the PhysxActuatorWrapper internal state after reset.
CI Status
No CI checks available yet.
Findings
🔴 Critical: source/isaaclab_newton/isaaclab_newton/physics/newton_manager.py:365-367 — Actuator step uses wrong dt when not all graphable
if cls._adapter is not None:
cls._adapter.step(cls._state_0, cls._control, physics_dt)When actuators are stepped eagerly (non-graphable path), they're called once with physics_dt = solver_dt * num_substeps, but then _simulate_physics_only() runs the full solver substep loop. The actuators should be stepped per-substep, not once with the full physics_dt. This will produce incorrect torques when num_substeps > 1.
🔴 Critical: source/isaaclab_physx/isaaclab_physx/assets/articulation/articulation.py:253-259 — Missing import for _HAS_NEWTON_ACTUATORS guard
The code imports NewtonActuatorAdapter and PhysxActuatorWrapper conditionally but then uses self._physx_actuator_wrapper without checking if the import succeeded:
if self._physx_actuator_wrapper is not None:If isaaclab_newton is not installed, _HAS_NEWTON_ACTUATORS will be False, but _physx_actuator_wrapper is never initialized in the class, causing an AttributeError. Need to initialize self._physx_actuator_wrapper = None in __init__ or _initialize_impl.
🔴 Critical: source/isaaclab_newton/isaaclab_newton/assets/articulation/articulation.py:3463-3465 — Adapter registered but finalized without model actuators
adapter = NewtonActuatorAdapter(
model.actuators, self.num_instances, self.num_joints, dof_offset, self.device,
)If model.actuators is empty (no Newton actuators were authored), the adapter is still created and registered with SimulationManager, but is_all_graphable returns False (line 309 in newton_actuator_utils.py), causing the manager to take the eager-actuator path unnecessarily.
🟡 Warning: source/isaaclab_newton/isaaclab_newton/actuators/newton_actuator_utils.py:304-305 — Potential device mismatch in reset
mask = wp.zeros(self._num_envs, dtype=wp.bool, device=self._device)
# ...
idx = wp.array(list(env_ids), dtype=wp.int32, device=self._device)When env_ids is a torch tensor on a different device than self._device, the contiguous().to(torch.int32) call doesn't move it to the correct device before wp.from_torch. This could cause a silent device mismatch.
🟡 Warning: source/isaaclab/isaaclab/envs/manager_based_rl_env.py:213-214 — recorder_manager only called once when physics handles decimation
if self._physics_handles_decimation:
# ...
self.recorder_manager.record_post_physics_decimation_step() # Called onceIn the original code, record_post_physics_decimation_step() was called for each decimation step. Now it's called once, which may break recording semantics if the recorder expects per-substep data.
🟡 Warning: source/isaaclab_newton/isaaclab_newton/physics/newton_manager.py:1121-1124 — _simulate_full zeros and steps actuators per decimation iteration
for _ in range(cls._decimation):
if cls._adapter is not None:
cls._adapter.step(cls._state_0, cls._control, physics_dt)The adapter's step() method zeros joint_f at actuated indices before stepping. However, if there are external forces added to joint_f between decimation steps (e.g., from wrench composers), they will be zeroed out. This differs from the Lab actuator path which doesn't zero.
🔵 Improvement: source/isaaclab_newton/isaaclab_newton/actuators/newton_actuator_utils.py:591-593 — Silent fallthrough for unsupported actuator types
if is_neural:
schemas.append("NewtonNeuralControlAPI")
else:
schemas.append("NewtonPDControlAPI")If a new actuator type is added that isn't explicitly handled (not IdealPD, DCMotor, Delayed, Remotized, or Neural), it silently falls through to PD control. Consider raising a warning or error for unknown actuator types.
🔵 Improvement: source/isaaclab_newton/isaaclab_newton/assets/articulation/articulation.py:2043-2054 — write_actuator_stiffness_to_sim missing joint_ids filtering
The method writes to all joints via _ALL_JOINT_INDICES but the adapter's joint_indices may be a subset. Should filter to only write actuated joints:
joint_ids = adapter.joint_indices if adapter.joint_indices != slice(None) else self._ALL_JOINT_INDICES
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
AntoineRichard
left a comment
AntoineRichard
left a comment
There was a problem hiding this comment.
The PR is doing what it should, I'm not sure about the general shape, where things live and so on. But in general it looks functional.
I still think we should strip the actuator logic from the articulation completely and have them live in their own class. That would make the articulation code more readable, but also make it easier to extent support to new actuators.
Address three small review comments on PR isaac-sim#5455: - PhysicsManager.set_decimation now has an explicit ``pass`` body to match the other no-op classmethods in the base. - _decimation declaration moves next to _num_substeps in NewtonManager for consistency with related solver-stepping configuration. - Reword FF-routing comments in PhysX Articulation to talk about actuated DOFs rather than implicit DOFs — the surrounding kernel operates on the full actuated DOF set.
AntoineRichard
requested review from
Mayankm96,
jtigue-bdai,
kellyguo11,
ooctipus and
ossamaAhmed
as code owners
Greptile SummaryThis PR integrates Newton-native actuators into both the Newton and PhysX backends, adding a new
Confidence Score: 4/5Core physics and actuator logic is sound; the main risks are edge-case behavioral mismatches rather than correctness failures on the happy path. The decimation fast path is a large surface-area change touching all four env base classes. The render_interval guard is silently dropped in the fast path, so users who configure render_interval larger than decimation will see unexpectedly frequent rendering. The sensor update frequency diverges between the all-graphable and eager paths, which could produce different IMU/frame-transform readings for the same scene depending on whether the graph path is active. The _collect_joint_prims restriction to revolute and prismatic joints will silently drop actuator prims for articulations that include spherical joints. These are all correctable without touching the central CUDA-graph or adapter logic. schemas_actuators.py (joint type coverage and temp file lifetime), all four env step methods (render_interval guard), and newton_manager.py (sensor update frequency in the two stepping paths). Important Files Changed
Sequence DiagramsequenceDiagram
participant Env as Environment (step)
participant SM as NewtonManager
participant Adapter as NewtonActuatorAdapter
participant Graph as CUDA Graph
participant Solver as Newton Solver
Env->>SM: handles_decimation()?
SM-->>Env: True (all-graphable)
Env->>Env: _apply_action()
Env->>Env: scene.write_data_to_sim()
Env->>SM: step()
alt All-graphable path (_simulate_full in graph)
SM->>Graph: wp.capture_launch(graph)
Note over Graph: for _ in range(decimation):<br/> collide()<br/> adapter.step() → joint_f<br/> post_actuator_callbacks()<br/> solver.step() × num_substeps
Graph-->>SM: done
SM->>SM: "sim_time += physics_dt × decimation"
else Eager-actuator fallback
SM->>Adapter: step(state, control, dt)
Adapter->>Adapter: zero actuated DOFs
Adapter->>Adapter: act.step() × N actuators
Adapter-->>SM: joint_f written
SM->>SM: post_actuator_callbacks()
SM->>Graph: wp.capture_launch(physics_only graph)
Note over Graph: collide() + solver.step() × num_substeps
Graph-->>SM: done
SM->>SM: "sim_time += physics_dt"
end
SM-->>Env: done
Env->>Env: "scene.update(dt=step_dt)"
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
The actuator integration refactor in this PR split NewtonManager._simulate into _simulate_full (decimation x actuators + solver) and _simulate_physics_only (solver substeps + sensors, no actuators). The NewtonKaminoManager override of step() and the CUDA graph capture helper still called the removed _simulate, which raised AttributeError on every Kamino step(). Route both call sites to _simulate_physics_only (the semantic equivalent of the prior _simulate -- no actuator stepping, no decimation loop) and consolidate _mark_transforms_dirty() to run unconditionally after both branches of step(), matching the new parent step() idiom. Fixes the kamino_internal_contacts and kamino_newton_pipeline cases of test_initialize_solver_populates_canonical_state.
…rs-integration Restore write_joint_state_to_sim_index on PhysX Articulation: the method was unintentionally dropped during the earlier develop merge (commit 59eb3a6), and develop has continued to depend on it for the fused position+velocity write path. Take develop's definition verbatim. No other conflicts.
There was a problem hiding this comment.
🤖 Automated Code Review
This is a substantial PR (29 files changed) integrating Newton-native actuators into Isaac Lab. The changes touch core infrastructure including physics management, environment stepping, articulation handling, and USD schema authoring.
Overall Assessment
Architecture: The design cleanly separates concerns - USD schema authoring lives in schemas_actuators.py, adapter logic in isaaclab_newton/actuators/, and backend-specific stepping remains in each Articulation class. The _post_spawn hook pattern is elegant for late-bound schema authoring.
Testing: Excellent test coverage with ~2500 lines of new equivalence tests covering IdealPD, DCMotor, DelayedPD, RemotizedPD, and neural actuators on both Newton and PhysX backends.
🔴 Critical Issues
1. Potential Uninitialized Attribute Access in direct_marl_env.py
File: source/isaaclab/isaaclab/envs/direct_marl_env.py (lines 437-460)
The _physics_handles_decimation attribute is set in _init_sim(), but step() may be called before full initialization in some edge cases.
# In step():
if self._physics_handles_decimation: # Could raise AttributeErrorSuggestion: Initialize _physics_handles_decimation = False in __init__ as a defensive default.
2. Missing Type Annotation for _adapter Class Variable
File: source/isaaclab_newton/isaaclab_newton/physics/newton_manager.py (line 200-202)
_adapter: NewtonActuatorAdapter | None = NoneThe type annotation references NewtonActuatorAdapter but it's only imported inside TYPE_CHECKING. While this works at runtime, static type checkers may flag this.
Suggestion: Move the import or add a forward reference string annotation.
🟡 Potential Issues
3. Duplicated Stepping Logic Across 4 Environment Files
Files: direct_rl_env.py, direct_marl_env.py, manager_based_env.py, manager_based_rl_env.py
The decimation-aware stepping logic is copy-pasted across all four environment files with only minor variations (e.g., self._apply_action() vs self.action_manager.apply_action()). This creates maintenance burden.
Suggestion: Consider extracting the decimation stepping logic into a shared helper method in a base class or mixin to reduce duplication.
4. Silent No-Op in define_actuator_properties
File: source/isaaclab/isaaclab/sim/schemas/schemas_actuators.py (lines 69-80)
if sim_cfg is None or not getattr(sim_cfg, "use_newton_actuators", False):
returnWhen use_newton_actuators is False, the function silently returns. This is correct behavior but could surprise users debugging actuator issues.
Suggestion: Add a debug-level log statement when early-returning to aid troubleshooting.
5. Hardcoded Temporary File Path in _resave_checkpoint_with_metadata
File: source/isaaclab/isaaclab/sim/schemas/schemas_actuators.py (lines 347-389)
The function creates temporary checkpoint files that persist for the process lifetime but are never cleaned up:
with tempfile.NamedTemporaryFile(suffix=".pt", delete=False) as tmp:
tmp_path = tmp.nameSuggestion: Consider using atexit to register cleanup or document the expected cleanup behavior.
🔵 Suggestions
6. Consider Lazy Import Pattern for Performance
File: source/isaaclab_newton/isaaclab_newton/assets/articulation/articulation.py
The importlib.util.find_spec("isaaclab_newton.actuators") check at module load adds overhead for all users, even those not using Newton actuators.
_HAS_NEWTON_ACTUATORS = importlib.util.find_spec("isaaclab_newton.actuators") is not NoneSuggestion: This is fine as-is given the check is lightweight, but if more Newton-specific imports are added, consider a lazy evaluation pattern.
7. Improved Error Message for Unsupported Actuator Configs
File: source/isaaclab/isaaclab/sim/schemas/schemas_actuators.py (lines 169-176)
The warning message when an unsupported config type is encountered could be more actionable:
logger.warning(
"Actuator group '%s' uses config type '%s' which is not supported by Newton-native"
" actuator authoring. The group will be skipped.",
group_name,
type(cfg).__name__,
)Suggestion: Include the list of supported types in the warning message to help users fix the issue.
✅ Strengths
-
Comprehensive Equivalence Tests: The test suite verifies Lab-vs-Newton actuator equivalence across multiple actuator types and both physics backends.
-
Clean Hook Pattern: The
_post_spawnhook inAssetBaseCfgenables extensible USD authoring without tight coupling. -
Proper State Reset: The
NewtonActuatorAdapter.reset()correctly handles per-env actuator state isolation using per-DOF masks. -
Backward Compatibility: The
use_newton_actuatorsflag defaults toFalse, ensuring existing code continues to work. -
Good Documentation: Changelog entries clearly explain what was added/changed.
Questions for the Author
-
Performance: Have benchmarks been run comparing the Newton actuator path vs Lab actuators for training scenarios?
-
CUDA Graph Compatibility: The code carefully handles graph capture/re-capture during
set_decimation. Has this been tested with long training runs where graphs might be re-captured multiple times? -
Mixed Actuator Scenes: For scenes with multiple articulations where some use Newton actuators and others don't, is the behavior well-defined?
This review was generated automatically. Please verify all suggestions against your specific use case.
Update (a48d3bd): Reviewed incremental changes since f215bbd.
✅ Critical Issue #1 (Uninitialized _physics_handles_decimation) - FIXED: The new commits now initialize self._physics_handles_decimation = False in __init__ for direct_marl_env.py, direct_rl_env.py, and manager_based_env.py. This properly addresses the potential AttributeError concern.
Other changes reviewed:
- h5py lazy import fix (Windows DLL crash) — good fix, defers import to method scope
- Version bumps and changelog cleanup
- Flexiv Rizon 4s ROS inference config simplification
No new issues found in the incremental changes. Remaining suggestions from initial review still apply to existing code.
|
|
||
| def _post_spawn(self, stage: Any) -> None: | ||
| """Author ``NewtonActuator`` USD prims from :attr:`actuators` after spawn. | ||
|
|
||
| Invoked by :class:`~isaaclab.assets.AssetBase` once the articulation's prims | ||
| exist on the stage. Delegates to | ||
| :func:`~isaaclab.sim.schemas.define_actuator_properties`, which gates itself | ||
| on ``sim_cfg.use_newton_actuators`` and silently no-ops when the simulation | ||
| is not configured for Newton-native actuators. | ||
| """ | ||
| if self.actuators is MISSING: | ||
| return | ||
| from isaaclab.sim.schemas.schemas_actuators import define_actuator_properties # noqa: PLC0415 | ||
|
|
||
| # In InteractiveScene, articulated assets are often spawned first under | ||
| # a template path (for example ``/World/template/Robot``) and cloned | ||
| # into ``{ENV_REGEX_NS}`` later. Author NewtonActuator prims on the | ||
| # actual spawned source prim so clones inherit them. | ||
| author_prim_path = ( | ||
| self.spawn.spawn_path if self.spawn is not None and self.spawn.spawn_path is not None else self.prim_path | ||
| ) | ||
| define_actuator_properties(author_prim_path, self.actuators, stage=stage) |
There was a problem hiding this comment.
Is this still needed?
| sim_ctx = SimulationContext.instance() | ||
| self._sim_cfg = sim_ctx.cfg if sim_ctx is not None else None |
There was a problem hiding this comment.
Is this still needed?
| import torch | ||
| import warp as wp | ||
|
|
||
| from ...sim import SimulationContext |
There was a problem hiding this comment.
Is this still needed?
| from pxr import Usd | ||
|
|
There was a problem hiding this comment.
Is this still needed?
| self._is_initialized = False | ||
| # get stage handle | ||
| self.stage = get_current_stage() | ||
| self.stage: Usd.Stage = get_current_stage() |
There was a problem hiding this comment.
Is this still needed?
| # schema-side post-spawn hook (e.g. ArticulationCfg authors NewtonActuator prims here) | ||
| self.cfg._post_spawn(self.stage) |
There was a problem hiding this comment.
I don't think this should be required. We could do this upon parsing the schema.
|
|
||
| def _post_spawn(self, stage: Any) -> None: | ||
| """Hook invoked by :class:`~isaaclab.assets.AssetBase` after the asset's prims are | ||
| spawned and verified to exist on the stage. | ||
|
|
||
| The default implementation is a no-op. Subclasses that need to author additional | ||
| USD schemas tied to this asset (for example, :class:`~isaaclab.assets.ArticulationCfg` | ||
| which authors ``NewtonActuator`` prims from its ``actuators`` mapping) should | ||
| override this method. | ||
|
|
||
| Args: | ||
| stage: The USD stage on which the asset was spawned. | ||
| """ | ||
| pass |
|
|
||
| from dataclasses import MISSING | ||
| from typing import Literal | ||
| from typing import Any, Literal |
AntoineRichard
left a comment
AntoineRichard
left a comment
There was a problem hiding this comment.
Quick comment for follow-up
There was a problem hiding this comment.
Should use lazy imports.
3 participants
Description
Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context.
List any dependencies that are required for this change.
Fixes # (issue)
Type of change
Screenshots
Please attach before and after screenshots of the change if applicable.
Checklist
pre-commitchecks with./isaaclab.sh --formatconfig/extension.tomlfileCONTRIBUTORS.mdor my name already exists there