Skip to content

Add Newton SemiImplicit solver support - #7779

Open
AntoineRichard wants to merge 3 commits into
isaac-sim:developfrom
AntoineRichard:antoiner/newton-semi-implicit
Open

Add Newton SemiImplicit solver support#7779
AntoineRichard wants to merge 3 commits into
isaac-sim:developfrom
AntoineRichard:antoiner/newton-semi-implicit

Conversation

@AntoineRichard

@AntoineRichard AntoineRichard commented Sep 13, 2026

Copy link
Copy Markdown
Collaborator

Description

Expose Newton's SolverSemiImplicit through the normal NewtonCfg(solver_cfg=SemiImplicitSolverCfg()) configuration. The new NewtonSemiImplicitManager uses the shared collision, force, reset, double-buffered stepping, and CUDA graph paths, with the native constructor options exposed by its configuration class.

SemiImplicit advances body coordinates. The manager synchronizes generalized coordinates after each solver substep so Isaac Lab root/joint observations and the next actuator update see the resulting motion. Documentation describes the native solver's supported features and stability limitations.

This branch starts directly from develop at a8b4da3c29ae528b39d4b3c9444d782ce58d886d. It has no dependency on the separate USD importer and model lifecycle PRs and introduces no dependencies.

Dependencies and merge order

The four PRs are independent and can merge into develop in any order:

#7781 contains the complete hard-reset lifecycle change; it does not require a callback-removal API from #7778.

Type of change

  • New feature
  • Documentation update

Validation

  • uv run --no-sync python -m pytest -q source/isaaclab_newton/test/physics/test_newton_manager_abstraction.py -k semi_implicit: 12 passed, 188 unrelated cases deselected, with 3 existing dependency/fixture warnings.
  • Existing solver matrices cover public config resolution, force/collision capabilities, deterministic configuration acceptance, USD joint-property policy, and solver initialization/stepping.
  • Public root pose/velocity matched an independent force/mass reference on CPU, CUDA, and CUDA graph replay; selected-world and hard resets preserved continued stepping.
  • A public articulated-joint test verified updated joint telemetry before the next folded actuator callback.
  • The stale public-root regression failed before synchronization and passed after it.
  • uv run --no-sync isaaclab -f and git diff --check passed.

These are Kit-less tests binding concrete Isaac Lab asset/data implementations to finalized Newton views. They exercise production public read/write methods. Full USD-bound asset lifecycle coverage remains outside this suite.

Release backport

  • Backport this pull request to the active release branch after it merges into develop

Checklist

  • Read the contribution guidelines
  • Updated public documentation
  • Added a package changelog fragment
  • Contributor name is already in CONTRIBUTORS.md
  • Ran repository pre-commit checks with uv run --no-sync isaaclab -f
  • Added regression coverage
  • No new pytest warning types in the affected suite

@AntoineRichard
AntoineRichard marked this pull request as ready for review September 13, 2026 17:08
@AntoineRichard
AntoineRichard requested a review from a team September 13, 2026 17:08

@isaaclab-review-bot isaaclab-review-bot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isaac Lab Review Bot

Adds Newton SolverSemiImplicit support through SemiImplicitSolverCfg and NewtonSemiImplicitManager, including generalized-coordinate synchronization, public exports, documentation, changelog metadata, and focused solver-path coverage. The inline solver-tuning section preceding the existing toctree does not establish a demonstrated documentation failure requiring correction.

  • Design and architecture: The manager follows the existing standalone-solver architecture: it constructs the native solver, assigns canonical state through NewtonManager, reuses shared collision, force, reset, double-buffer, and CUDA graph paths, and confines maximal-to-generalized state reconciliation to its _step_solver override.
  • API: The new configuration and manager are additive public APIs, exported through the package stub and included in generated API documentation. Deterministic-mode validation and collision-pipeline documentation were updated consistently, with no existing public symbols removed or renamed.
  • Implementation: Constructor options flow through the existing solver-argument filtering mechanism, while manager flags select the expected double-buffered collision-pipeline behavior. The output state is reconciled with eval_ik after each substep so public root and joint consumers see current generalized coordinates; tests trace eager CPU, eager CUDA, CUDA graph, reset, force, and folded-actuator paths. Per-substep IK remains an inherent performance tradeoff to monitor for large articulated scenes, but the patch supplies a concrete correctness rationale and no demonstrated defect.

No blocking issues. No inline issue met the actionable-evidence threshold; the assessment above records the review feedback.

Automated review; human maintainers own approval decisions.

@greptile-apps

greptile-apps Bot commented Sep 13, 2026

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 4/5

The PR should not merge until SemiImplicit rejects unsupported actuator and model configurations instead of accepting scenes whose configured controls will not be enforced.

Findings

  1. P1 Unsupported actuator modes accepted
  2. P2 Articulated graph coverage missing

Summary

  • Adds SemiImplicitSolverCfg and NewtonSemiImplicitManager.
  • Reconciles generalized coordinates from integrated body state after every substep.
  • Enables shared collision, force, reset, double-buffering, deterministic-mode, and CUDA-graph paths.
  • Adds API documentation, solver-tuning guidance, a changelog fragment, and focused regression coverage.
  • The solver currently accepts actuator/model features documented as unsupported instead of rejecting them during initialization.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart LR
    A[NewtonCfg with SemiImplicitSolverCfg] --> B[NewtonSemiImplicitManager]
    B --> C[Newton collision pipeline]
    C --> D[SolverSemiImplicit substep]
    D --> E[Maximal body state]
    E --> F[eval_ik reconciliation]
    F --> G[Generalized joint state]
    G --> H[Actuators, assets, sensors, and observations]
    G --> D
Loading

Reviews (1) · Last reviewed commit: "Trim redundant SemiImplicit test coverag..."

consumes rigid-body force input, and steps with separate input/output
states, so the flags are fixed.
"""
NewtonManager._solver = cls._create_solver(model, solver_cfg)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Unsupported actuator modes accepted

Normal Isaac Lab articulations convert actuator gains into joint target modes before model finalization, but _build_solver() accepts these models even though SemiImplicit does not support joint target mode. An articulation using an implicit position or velocity actuator can therefore initialize successfully while its commands are not enforced. Please validate and reject unsupported actuator and model features during solver construction rather than silently accepting them.

Knowledge Base Used: Newton backend

cls._solver.step(state_0, state_1, control, contacts, substep_dt)
# SemiImplicit integrates maximal coordinates only. Public root/joint bindings and
# the next folded actuator iteration read generalized coordinates from this state.
eval_ik(cls._model, state_1, state_1.joint_q, state_1.joint_qd)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Articulated graph coverage missing

The new state reconciliation runs inside every solver substep, including CUDA graph capture, but the CUDA-graph test uses only free rigid bodies with no generalized coordinates. The only articulated-state regression runs on CPU with graphs disabled. Please add an articulated CUDA-graph case that exercises eval_ik; otherwise, capture and replay failures in the new synchronization path can go undetected.

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation isaac-lab Related to Isaac Lab team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant