Add maximal-coordinate solver hooks to core env and actuators#6026
Add maximal-coordinate solver hooks to core env and actuators#6026aserifi wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Thanks for this PR! The Kamino integration work looks useful. I have reviewed the changes and have some observations and suggestions.
Summary
This PR adds two hooks for maximal-coordinate solver (Kamino) integration:
- A
route_torque_toclass variable onActuatorBasefor choosing betweenjoint_f(semi-explicit) andjoint_act(backward-Euler) torque routing - Post-reset kinematics refresh in
ManagerBasedRLEnv.step()to ensure observations reflect reset poses for maximal-coordinate solvers
What Looks Good
- The changes are minimal and targeted, with clear "no-op for other backends" semantics
- The changelog entry is well-written and documents both additions
- The
route_torque_todocstring clearly explains the behavioral difference between the two options - The pattern in
step()mirrors the existing pattern inManagerBasedEnv.reset()(lines 423-424)
Questions / Suggestions
-
Testing: There are no test changes in this PR. While the changes are designed to be no-ops for existing backends, it would strengthen confidence to have:
- Unit tests verifying
route_torque_todefaults correctly - Integration tests demonstrating the post-reset refresh works correctly for Kamino (if Kamino support exists in CI)
- Unit tests verifying
-
Performance consideration for
step()changes: The addedwrite_data_to_sim()→forward()→scene.update()sequence runs for all envs that reset, not just on Kamino backends. The comment says "no-op for others" - could you clarify whether this is:- A literal no-op (these calls early-return when not needed)?
- Or a cheap operation that's negligible even if it does work?
If it's the latter, have you benchmarked the overhead for high-frequency resets on non-Kamino backends?
-
route_torque_toas ClassVar: Since this is aClassVar, all instances of a given actuator class share the same routing. Is this intentional? If users want different routing for different actuator groups on the same robot (e.g., arms vs legs), they would need separate subclasses. Worth documenting this limitation or considering an instance-level override. -
Missing type export: The
Literalimport is added butroute_torque_tousesLiteral["joint_f", "joint_act"]. Should this type alias be exported in__all__or defined as a module-level type for downstream subclasses that might want to type-annotate their own overrides?
Overall this is a clean, focused PR. The suggestions above are mostly about completeness rather than correctness issues.
Greptile SummaryThis PR adds two lightweight integration hooks for the Kamino maximal-coordinate solver without changing behavior for existing PhysX / MuJoCo-Warp backends. The changes are small and well-scoped.
Confidence Score: 4/5The changes are minimal and isolated; existing PhysX and MuJoCo-Warp training loops are unaffected except for a small overhead on steps that include resets. The
Important Files Changed
Sequence DiagramsequenceDiagram
participant RL as ManagerBasedRLEnv.step
participant AM as ActionManager
participant SC as Scene
participant SIM as sim
participant RM as ResetManager
RL->>AM: process_action / apply_action
RL->>SC: write_data_to_sim()
RL->>SIM: step()
RL->>SC: "update(dt=physics_dt)"
RL->>RL: "compute terminations & rewards"
RL->>RM: _reset_idx(reset_env_ids)
Note over RM: resets joint states,<br/>randomizes poses
rect rgb(200, 230, 255)
Note over RL,SIM: NEW – Kamino kinematics refresh
RL->>SC: write_data_to_sim()
RL->>SIM: forward() [FK only, no dynamics]
RL->>SC: "update(dt=physics_dt)"
end
RL->>SC: render (RTX re-renders)
RL->>AM: compute observations
Reviews (1): Last reviewed commit: "Add maximal-coordinate solver hooks to c..." | Re-trigger Greptile |
| route_torque_to: ClassVar[Literal["joint_f", "joint_act"]] = "joint_f" | ||
| """Where the per-step actuator-computed torque is written into Newton's ``Control``. | ||
|
|
||
| - ``"joint_f"`` (default): torque is written to ``Control.joint_f`` and enters the | ||
| body wrench, integrated semi-explicitly. This matches PhysX/MuJoCo semantics. | ||
| - ``"joint_act"``: torque is written to ``Control.joint_act`` instead. For the Kamino | ||
| solver this enters the joint dynamic-constraint row of the PADMM solve, getting | ||
| backward-Euler treatment of joint armature and damping. | ||
| """ |
There was a problem hiding this comment.
route_torque_to is declared but has no consumer in this PR
The class variable is added to ActuatorBase and documented, but searching the entire codebase reveals no code that reads route_torque_to and acts on it — nothing in write_data_to_sim, the Newton articulation's actuator path, or anywhere else. A subclass that sets route_torque_to = "joint_act" today would have no effect on simulation behavior. The consumer (the code that branches on this value to write either joint_f or joint_act) appears to be missing from this PR. Is this intentional (i.e., a follow-up PR adds the routing logic), and if so, is there a tracking issue?
| # Refresh kinematics and derived buffers for the just-reset envs so the | ||
| # observations / terminations below reflect the reset pose. Maximal-coordinate | ||
| # solvers (e.g. Kamino) apply reset FK one step late; for others this is a no-op. | ||
| self.scene.write_data_to_sim() | ||
| self.sim.forward() | ||
| self.scene.update(dt=self.physics_dt) |
There was a problem hiding this comment.
scene.update() call after reset deviates from the established pattern
Every other reset site in the codebase (manager_based_env.reset() line 422-424, direct_rl_env.reset() line 366-367, reset_to() line 485-486) uses write_data_to_sim() + sim.forward() and then goes straight to computing observations — none of them call scene.update() afterwards. Adding scene.update(dt=self.physics_dt) here means all backends (not only Kamino) execute an extra scene update on every in-step reset. For backends that maintain sensor history or accumulate state across update() calls, this extra invocation could produce stale or doubled velocity estimates. The dt used is physics_dt, which is also inconsistent with the step_dt used in the _physics_handles_decimation branch of the main loop. If this call is genuinely a no-op for PhysX and MuJoCo-Warp, it may be worth adding a brief inline note explaining why.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Description
This is part of the Kamino integration into IsaacLab work.
Two small core additions that let maximal-coordinate solvers (Kamino) integrate cleanly. No behavior change for existing PhysX / MuJoCo-Warp setups.
ActuatorBase.route_torque_toto choose whether actuator torque is written to Newton'sControl.joint_f(default, semi-explicit, PhysX/MuJoCo-like) orControl.joint_act(backward-Euler joint treatment for the Kamino solver).ManagerBasedRLEnv, so post-reset observations and terminations reflect the reset pose for maximal-coordinate solvers (no-op for other backends).