Skip to content

Move external wrench frame selection into WrenchComposer - #7536

Draft
AntoineRichard wants to merge 8 commits into
isaac-sim:developfrom
AntoineRichard:antoiner/wrench-submission-plan
Draft

Move external wrench frame selection into WrenchComposer#7536
AntoineRichard wants to merge 8 commits into
isaac-sim:developfrom
AntoineRichard:antoiner/wrench-submission-plan

Conversation

@AntoineRichard

Copy link
Copy Markdown
Collaborator

Description

WrenchComposer.compose_to_body_frame() was called unconditionally by every backend writer on every physics step. It reads body_com_pos_w and body_link_quat_w, which on PhysX chains to _ensure_fk_fresh()update_articulations_kinematic() (a scene-global articulation FK pass) plus root_view.get_link_transforms(), then launches the composition kernel.

That work is unnecessary whenever the buffered wrench is already in a frame the consumer accepts. This PR moves the frame decision into the composer, which hands each writer the cheapest valid representation:

  • all-local content → the local buffers, untouched. An all-local wrench composes to exactly itself, because the pose data is multiplied by zero.
  • all-global-at-CoM content → the global buffers, untouched, submitted in the world frame by backends that accept one.
  • anything else → composed as before.

Writers declare what they can consume once at construction (supports_world_at_com) and call resolve_submission() per step. All frame logic stays inside the composer; no writer touches a raw buffer or knows a frame rule.

New public API

class WrenchComposer:
    class Frame(IntEnum):
        BODY = 0          # body frame, force at the body's CoM
        WORLD_AT_COM = 1  # world frame, force at the body's CoM

    def __init__(self, asset, *, supports_world_at_com: bool = False) -> None: ...
    def resolve_submission(self) -> tuple[wp.array, wp.array, Frame]: ...

Additive only. compose_to_body_frame(), out_force_b, and out_torque_b remain public and behaviorally unchanged, so no deprecation cycle is needed. supports_world_at_com defaults to False, which is exactly the previous behavior.

Eligibility is tracked with a private mask of which input buffer families hold contributions (LOCAL, GLOBAL_AT_COM, GLOBAL_POSITIONED), classified from the same routing the add_forces_to_dual_buffers_* kernels use. The mask is sticky across partial resets and cleared only by a full reset() — proving a partial reset removed every contribution of a family would require scanning the buffers. That can forgo the fast path but never changes the resulting wrench.

Equivalence

All three branches are exact, from compose_wrench_to_body_frame:

  • local only: the three global buffers are zero, so both quat_rotate_inv terms vanish and the output is identically (local_force_b, local_torque_b). Note a positioned local force also qualifies — the kernel folds cross(P_b, F_b) straight into local_torque_b.
  • global-at-CoM only: global_force_w and the local buffers are zero, so corrected_torque_w == global_torque_w. Submitting the world buffers is the same wrench, provided the consumer applies force at the CoM — verified against the vendored omni.physics.tensors API, where position_data=None means "at the link transform" for both is_global values, so the flag only reinterprets the vectors' frame and never moves the application point.
  • anything else: unchanged.

Backend coverage

backend supports_world_at_com change
PhysX True writer maps the returned frame onto is_global
OvPhysX True writer maps it onto a new wrench_is_world kernel flag that skips the inline rotate
Newton False (default) writer swaps to resolve_submission(); no constructor change

OvPhysX is the largest win. Its wrench binding wants a world-frame wrench, so a global wrench previously round-tripped: the composer rotated world→body, then the packing kernel rotated body→world. Measured on this branch with a body rotated 90° about +z, a local force (1,0,0) emits (0,1,0) while a global force (1,0,0) emits (1,0,0) — an exact round trip, so skipping both rotations is output-preserving. The packed [6:9] link position is still written unconditionally on both paths.

Newton keeps the body frame because it binds a body-frame array to the solver. Worth noting for a follow-up: its writer kernel already performs the same body→world rotation OvPhysX's does, so Newton could plausibly take the same flag and win the global-at-CoM path too. That is deliberately out of scope here.

Validation

Environment: OMNI_KIT_ACCEPT_EULA=YES uv run --extra isaacsim --extra test --extra ovphysx, which resolves BACKENDS == ['physx', 'newton', 'ovphysx'] with CUDA. Note EXP_PATH must be exported or _iface_test_boot.py takes its kitless path and physx silently drops out, making backend tests skip green.

  • test_wrench_composer.py plus test/assets/: 6760 passed, 65 skipped, 68 xfailed.
  • 5 failures in test_articulation_ordering.py during full-suite runs are pre-existing and unrelated: the identical 5 fail at the branch base with none of this branch present, and pass in isolation. Test counts reconcile — 6350 passing at base, 6370 at head for the same selection, and the +20 is exactly this branch's new tests.
  • uv run isaaclab -f clean.

New coverage: composer unit tests for every content case and the mask lifecycle; per-backend submission tests asserting the chosen frame and that no body-pose read occurs on a fast path; an OvPhysX articulation test driving the ordered kernel's world path under a non-identity body ordering; and a cross-backend equivalence test over all three backends × four content combinations, comparing the submitted wrench against the composed one.

Two regressions were confirmed to be caught rather than assumed: injecting the eligibility bug the design guards against (_content & LOCAL instead of ==) fails the mixed-content test, and perturbing the expected world-frame torque fails exactly the two WORLD_AT_COM cases and no others.

Performance

Real PhysX, RTX 5090, ANYMAL-C (17 bodies) × 4096 envs = 69,632 links. Same-process paired A/B with the arm order alternated per iteration, 400–500 pairs, 95% CI on paired differences. Baseline is compose_to_body_frame() — what the writers previously called unconditionally.

scope content baseline patched paired delta 95% CI
composition path only global-at-CoM 0.2194 ms 0.0191 ms −0.2003 ms (−91.3%) [0.1915, 0.2091]
composition path only local 0.1926 ms 0.0159 ms −0.1767 ms (−91.8%) [0.1712, 0.1822]
full write_data_to_sim global-at-CoM 1.0799 ms 0.9897 ms −0.0902 ms (−8.4%) [0.0797, 0.1007]
full write_data_to_sim local 1.0072 ms 0.9305 ms −0.0767 ms (−7.6%) [0.0719, 0.0815]

Read the end-to-end row as the honest headline: ~0.08 ms per asset per physics step. Roughly half the isolated saving does not reach the caller, because other work in write_data_to_sim touches body poses anyway and pays part of that cost regardless. A cuboid rigid-object scene at the same env count saved only ~6.5 µs, as expected — there is no articulation FK pass to skip.

Scope caveat: no in-tree task sets is_global=True today (both shipped apply_external_force_torque events use the local default), so the local path is what delivers value now, and it is the one that reaches every backend.

Supersedes

This replaces two open PRs, both of which found one half of this problem:

  • Optimize PhysX global external wrench submission #7431 (@NeoZng) identified the global-at-CoM case and the cost of the pose read. Its mechanism is correct and its writer-level measurement is corroborated here. It is superseded on placement rather than substance: it put the frame decision in three PhysX writers behind a public eligibility flag, leaving Newton and OvPhysX unserved. The credit for the world-at-CoM half of this mechanism is theirs.
  • [Performance] Cache unchanged local wrench composition #7362 cached composition for unchanged local wrenches. Superseded because not composing at all is strictly stronger — the cache does nothing when the wrench is rewritten every step, which is the common case for a per-step randomized push.

Type of change

  • New feature (non-breaking change which adds functionality)

Checklist

  • I have read and understood the contribution guidelines
  • I have run the pre-commit checks with ./isaaclab.sh --format
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • I have added a changelog fragment under source/<pkg>/changelog.d/ for every touched package
  • I have added my name to the CONTRIBUTORS.md or my name already exists there

🤖 Generated with Claude Code

Strengthen the resolve_submission composer tests to assert composed
values instead of just the frame, tautology-proof the Newton frame
pin, cover the OvPhysX ordered-world articulation path under a
nonidentity body ordering, align the PhysX benchmark fixture with
production composer construction, and drop a dead field setup left
over from a deleted RigidObject attribute.
@github-actions github-actions Bot added the isaac-lab Related to Isaac Lab team label Sep 3, 2026
@AntoineRichard

Copy link
Copy Markdown
Collaborator Author

@NeoZng could you see if this works for you. The bulk of the changes are tests.

@NeoZng

NeoZng commented Sep 4, 2026

Copy link
Copy Markdown

Thanks @AntoineRichard — I tested the current patch cleanly on the latest develop. It works for our recovery-assist use case, and the architecture is cleaner.

Our reset-mode MDP event writes a persistent [0, 0, 300] N world-frame force at the torso CoM using permanent_wrench_composer.set_forces_and_torques_index(..., is_global=True) without positions; the wrench is then submitted on every physics step. Startup CoM randomization was up to +/-3 cm.

On Isaac Sim 6.0.1 with an RTX 5090, 388 WrenchComposer tests and 14 focused PhysX/Newton interface tests passed. A learner-free profile of the actual MDP term and full ManagerBasedRLEnv.step() path (5 physics substeps, 200 alternating pairs) gave:

Environments Full MDP step Paired saving (95% CI)
4096 67.1557 -> 66.6808 ms (-0.71%) 0.4749 ms [0.3446, 0.6052]
16384 123.2129 -> 122.3709 ms (-0.68%) 0.8420 ms [0.6322, 1.0519]

The patch is correct and ready for our use case.

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

Labels

isaac-lab Related to Isaac Lab team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants