Skip to content

Skips apply_external_force_torque when both ranges are zero#5688

Merged
kellyguo11 merged 3 commits into
isaac-sim:developfrom
AntoineRichard:antoiner/fix/perf_composer_initial
May 19, 2026
Merged

Skips apply_external_force_torque when both ranges are zero#5688
kellyguo11 merged 3 commits into
isaac-sim:developfrom
AntoineRichard:antoiner/fix/perf_composer_initial

Conversation

@AntoineRichard
Copy link
Copy Markdown
Collaborator

Description

Fast-path early-return in :func:~isaaclab.envs.mdp.events.apply_external_force_torque when both force_range and torque_range are exactly zero — a common configuration for tasks that declare the event term but apply no disturbance.

Before this change, zero-wrench configurations were still sampled, written into the dual-buffer WrenchComposer introduced in #5265, and pushed through the per-step compose-and-apply path in Articulation.write_data_to_sim, paying the full per-step cost for what is semantically a no-op. Applying a zero wrench is equivalent to not applying one at all, so the function now returns immediately when both ranges are zero.

This restores the H1, G1, and Anymal-C Velocity-Rough throughput observed prior to #5265, as recorded in the OmniPerf DB regression flagged in isaac-sim/IsaacLab-Internal#906.

Scope limitation. This only addresses the zero-force case. Tasks that apply non-zero external forces (curriculum disturbances, push events, domain-randomized wrenches) still pay the per-step body-frame recompose cost under the new dual-buffer architecture. That broader optimization (compose caching / kernel fusion) is tracked separately in isaac-sim/IsaacLab-Internal#911 and is out of scope here.

Correctness. The dual-buffer WrenchComposer architecture from #5265 is untouched; this fix sits one layer above it in the event term. For any non-zero force_range or torque_range, the early-return predicate is false and behavior is unchanged.

Fixes isaac-sim/IsaacLab-Internal#906
Follow-up: isaac-sim/IsaacLab-Internal#911

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Screenshots

N/A — performance fix, no user-visible behavior change.

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 (do not edit CHANGELOG.rst or bump extension.toml — CI handles that)
  • I have added my name to the CONTRIBUTORS.md or my name already exists there

Documents the early-return added to apply_external_force_torque when
force_range and torque_range are both zero, restoring H1/G1/Anymal-C
Velocity-Rough throughput regressed by the WrenchComposer rework in
isaac-sim#5265.

Refs: isaac-sim/IsaacLab-Internal#906, isaac-sim/IsaacLab-Internal#911
@github-actions github-actions Bot added bug Something isn't working documentation Improvements or additions to documentation asset New asset feature or request isaac-sim Related to Isaac Sim team isaac-mimic Related to Isaac Mimic team infrastructure labels May 19, 2026
Copy link
Copy Markdown

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

Choose a reason for hiding this comment

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

Review Summary

TL;DR: Clean performance fix — the early-return logic is correct and well-documented. The change restores throughput for zero-wrench configurations by avoiding unnecessary WrenchComposer overhead.


⚠️ Base Branch Note

This PR targets main but shows 2214 files changed due to develop divergence. The actual meaningful change is isolated to:

  • source/isaaclab/isaaclab/envs/mdp/events.py — 4 lines added
  • source/isaaclab/changelog.d/fix-zero-wrench-fast-path.rst — changelog fragment

Please consider rebasing onto main or develop to clean up the diff.


Code Review

The Change (events.py):

# Skip force application if the wrench ranges are zero
if force_range[0] == 0.0 and force_range[1] == 0.0 and torque_range[0] == 0.0 and torque_range[1] == 0.0:
    return

Analysis:

  1. Correctness ✅ — Applying a zero wrench is semantically equivalent to not applying one. The early-return correctly short-circuits when both force_range and torque_range are (0.0, 0.0).

  2. Edge cases ✅ — The comparison uses exact floating-point equality (== 0.0), which is appropriate here since these are user-provided configuration values, not computed results subject to floating-point error.

  3. Backward compatibility ✅ — Non-zero ranges behave identically to before. The WrenchComposer dual-buffer architecture (PR #5265) is untouched.

  4. Performance rationale ✅ — The PR description explains the regression: zero-wrench configs were paying full per-step compose-and-apply costs for a no-op. This fix restores throughput for H1, G1, and Anymal-C Velocity-Rough tasks.


Minor Suggestions

  1. Consider tuple comparison for readability:

    if force_range == (0.0, 0.0) and torque_range == (0.0, 0.0):
        return

    This is functionally equivalent but slightly more readable.

  2. Changelog fragment looks good — correctly documents the fix with links to the relevant PRs (#5265) and describes the regression context.


Checklist

  • Builds successfully (CI labeler passed)
  • Code logic is correct
  • Changelog fragment included
  • No breaking changes
  • CI: Full test suite not yet run (only labeler check visible — base branch issue may be blocking)

Verdict: LGTM pending rebase to clean up the base branch diff.


Update (67e0b1d): ✅ The base branch issue has been resolved — the PR now correctly targets develop. The rebase introduced significant unrelated changes in the commit comparison (contact sensors, Newton actuators, Isaac Teleop MCAP support, etc.) but the actual functional change in this PR remains unchanged: the early-return optimization in apply_external_force_torque when both ranges are zero. No further action needed — this PR is ready for merge.

@AntoineRichard AntoineRichard changed the title perf: skip apply_external_force_torque when both ranges are zero Skips apply_external_force_torque when both ranges are zero May 19, 2026
@AntoineRichard AntoineRichard changed the base branch from main to develop May 19, 2026 12:15
@kellyguo11 kellyguo11 moved this to Ready to merge in Isaac Lab May 19, 2026
@github-actions github-actions Bot added the isaac-lab Related to Isaac Lab team label May 19, 2026
@kellyguo11 kellyguo11 merged commit a9b6210 into isaac-sim:develop May 19, 2026
63 of 65 checks passed
@github-project-automation github-project-automation Bot moved this from Ready to merge to Done in Isaac Lab May 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

asset New asset feature or request bug Something isn't working documentation Improvements or additions to documentation infrastructure isaac-lab Related to Isaac Lab team isaac-mimic Related to Isaac Mimic team isaac-sim Related to Isaac Sim team

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants