Repository navigation
Add delayed implicit actuator support - #8992
AntoineRichard wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Isaac Lab Review Bot
Needs attention: Style consistency (3), Test quality (2). Other reviewed categories look good.
- PR description: The title, scope, and reported validation match the diff; quantify delay-processing overhead with reproducible baseline/candidate revisions, command, workload, hardware, backend, environment count, warmup, step latency, and memory.
- Design and architecture: Architecture looks good.
- Scope and over-engineering: No over-engineering issues.
- API: API changes look good.
- Compatibility and deprecation: No breaking changes identified.
- Implementation: Implementation looks good.
- Style consistency: The new mixin constructor lacks the required return annotation; add
-> Noneto match the contribution guide. - Test quality: The added kitless tests lack explicit CI routing markers, so kitless selection skips their delay and reset coverage; mark the qualifying cases.
Minor fixes needed. Posted 5 inline findings.
Automated review; human maintainers own approval decisions.
There was a problem hiding this comment.
Isaac Lab Review Bot
Needs attention: Style consistency (4), Test quality (2). Other reviewed categories look good.
- PR description: The title and body match the actuator, lifecycle reuse, documentation, and test scope; revision-specific validation is reported, and no measurable performance claim requires benchmark evidence.
- Design and architecture: Architecture looks good.
- Scope and over-engineering: No over-engineering issues.
- API: API changes look good.
- Compatibility and deprecation: No breaking changes identified.
- Implementation: Implementation looks good.
- Style consistency: The extracted constructor lacks the required return annotation; add
-> Noneand remove redundant prose while placing the implicit-model heading above its actual section. - Test quality: The native-conflict case marks an implicit-only group native rather than constructing mixed execution; add a disjoint native explicit group and mark the kitless cases for CI selection.
Minor fixes needed. Posted 6 inline findings.
Automated review; human maintainers own approval decisions.
[Medium impact] The PR appears safe to merge; no actionable issues were found. SummaryAdds
No actionable issues were found. This review checked source code; it did not run tests. Diagram%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Position, velocity, and effort targets] --> B[Shared command history]
B --> C[Targets delayed per environment]
C --> D[Solver-side PD control]
E[Current joint state and gains] --> D
F[Reset selected environments] --> G[Sample delays and clear selected histories]
G --> B
Reviews (1) · Last reviewed commit: "Add delayed implicit actuator support" · Reviewed by Greptile |
|
run-ci |
Description
Adds
DelayedImplicitActuatorandDelayedImplicitActuatorCfgto model command latency while retaining solver-side implicit PD control, as requested by Hexagon. Position, velocity, and feed-forward effort targets share one delay per environment, sampled uniformly from[min_delay, max_delay]on reset in physics steps. Joint-state feedback and solver gains remain current.The implementation shares the existing delayed PD command-history lifecycle, preserves partial-reset history isolation, and documents startup behavior and configuration. Combining delayed implicit groups with native explicit actuator execution in the same articulation raises an actionable error; use
use_newton_actuators=Falsefor that combination. Separate articulations can mix both execution paths: Newton retains the environment-owned physics loop whenever a Lab actuator model needs per-step updates, preventing delays from growing with policy decimation.Type of change
Validation
Tests reuse existing coverage: the native collection test owns the mixed-group rejection, the existing delayed-PD equivalence test owns explicit-delay compatibility, and the mixed-scene timing regression uses the already-live Newton scene. No additional simulation scene builds are introduced. Independent zero/fixed delay, partial reset, per-environment sampling/channel sharing, and invalid configuration boundaries remain covered.
pytest ... -m kitless).Checklist
uv run isaaclab --formatsource/isaaclab/changelog.d/andsource/isaaclab_newton/changelog.d/, with a minor-version marker for the new actuator APICONTRIBUTORS.mdThe delayed explicit actuator regression exercises the existing Lab-execution deprecation warning. Existing Newton adapter tests also emit PyTorch export/deprecation warnings; the documentation build was warning-free.