Skip to content

Fix gravity distributions for PhysX and OvPhysX - #7272

Merged
ooctipus merged 2 commits into
isaac-sim:developfrom
AntoineRichard:codex/fix-scene-gravity-distribution
Aug 24, 2026
Merged

Fix gravity distributions for PhysX and OvPhysX#7272
ooctipus merged 2 commits into
isaac-sim:developfrom
AntoineRichard:codex/fix-scene-gravity-distribution

Conversation

@AntoineRichard

@AntoineRichard AntoineRichard commented Aug 21, 2026

Copy link
Copy Markdown
Collaborator

Description

Scene-wide gravity randomization accepted uniform, log_uniform, and gaussian, but the PhysX and OvPhysX paths always sampled uniformly. This change forwards the validated configured distribution to both scene-wide backend paths. Newton behavior is unchanged.

The focused regression exercises full event dispatch with deterministic Gaussian sampling on both affected backends.

Tested with:

  • uv run --extra test -m pytest source/isaaclab/test/envs/test_gravity_randomization.py source/isaaclab/test/envs/test_mdp_event_selectors.py -q (7 passed)
  • uv run --extra test --extra ovphysx -m pytest source/isaaclab_ov/test/physics/test_ovphysx_gravity.py -q (1 passed)
  • uv run isaaclab -f
  • uv run python tools/changelog/cli.py check develop --include-worktree

Type of change

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

Screenshots

Not applicable.

Checklist

  • I have read and understood the contribution guidelines
  • I have run the pre-commit checks with uv run isaaclab -f
  • I have made corresponding changes to the documentation (not applicable; no public API change)
  • 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

@AntoineRichard
AntoineRichard requested a review from a team August 21, 2026 14:52
@github-actions github-actions Bot added bug Something isn't working isaac-lab Related to Isaac Lab team labels Aug 21, 2026

@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

The change correctly forwards the configured gravity distribution to the PhysX and OvPhysX scene-wide paths, with aligned documentation, regression coverage, and a package changelog fragment.

  • Design and architecture: Distribution validation remains centralized in initialization, while the existing _randomize_prop_by_op abstraction handles sampling for both scene-wide backends. The Newton per-environment path and backend selection remain unchanged.
  • API: The event term retains its existing signature, defaults, and units. PhysX and OvPhysX now honor the documented non-uniform distribution configurations rather than silently sampling uniformly, which is a compatible bug fix.
  • Implementation: Both _call_physx and _call_ovphysx now receive the cached distribution after initialization has rejected unsupported values. The focused test covers all three supported distributions on both backend dispatch paths; it uses injected private backend attributes, creating only a minor future test-maintenance tradeoff if those internals are restructured.

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 Aug 21, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR forwards the configured gravity distribution to the PhysX and OvPhysX scene-wide randomization paths, updates the parameter documentation, and adds focused backend tests.

  • Caches the configured distribution for stateful gravity events.
  • Uses that distribution when randomizing PhysX and OvPhysX gravity.
  • Adds regression coverage for uniform, log-uniform, and Gaussian dispatch.
  • The newly enabled log-uniform path does not reject ordinary non-positive gravity components and can send NaN gravity to either backend.

Confidence Score: 4/5

The PR should not merge until log-uniform gravity parameters are validated or handled without passing non-finite gravity to PhysX and OvPhysX.

Forwarding log-uniform sampling exposes conventional zero and negative gravity components to logarithms, producing NaN values that are then installed as scene gravity.

Files Needing Attention: source/isaaclab/isaaclab/envs/mdp/events.py and source/isaaclab/test/envs/test_gravity_randomization.py

Important Files Changed

Filename Overview
source/isaaclab/isaaclab/envs/mdp/events.py Correctly forwards configured distributions, but enables log-uniform sampling without validating that all component bounds are positive.
source/isaaclab/test/envs/test_gravity_randomization.py Verifies distribution dispatch for both backends, but mocked samplers conceal the real log-uniform domain failure.
source/isaaclab/changelog.d/antoiner-scene-gravity-distribution.rst Accurately records the intended backend distribution fix.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart LR
    C[Event configuration] --> D{Distribution}
    D -->|uniform| U[sample_uniform]
    D -->|gaussian| G[sample_gaussian]
    D -->|log_uniform| L[sample_log_uniform]
    L --> X[log component bounds]
    X -->|zero or negative bound| N[Non-finite sampled gravity]
    U --> B[PhysX or OvPhysX set_gravity]
    G --> B
    N --> B
Loading

Reviews (1): Last reviewed commit: "Honor gravity distributions on scene-wid..." | Re-trigger Greptile

Comment on lines 1354 to +1357
None,
slice(None),
operation=operation,
distribution="uniform",
distribution=self._distribution,

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 Log-uniform produces invalid gravity

When log_uniform is configured with conventional zero or negative gravity-component bounds, this newly forwarded distribution takes their logarithms, producing non-finite values that are passed to PhysX or OvPhysX as scene gravity. Validate that log-uniform bounds are strictly positive or otherwise handle these components before calling the sampler.

@ooctipus
ooctipus merged commit 0081477 into isaac-sim:develop Aug 24, 2026
47 of 48 checks passed
ooctipus added a commit that referenced this pull request Aug 24, 2026
…#7303 (#7313)

# Description

Backports six merged PRs to `release/3.0.0` as separate
provenance-preserving cherry-picks:

| Source PR | Source commit | Backport commit | Scope |
| --- | --- | --- | --- |
| #7272 | `0081477fea2` | `c9fc1997476` | Fix scene-wide gravity
distributions for PhysX and OvPhysX. |
| #7121 | `9f65e3d4fd2` | `cce8acd84e1` | Fix backend-factory fallback
before simulator initialization. |
| #7183 | `3fcc9c6b824` | `99d58d3d3c4` | Streamline the getting-started
documentation. |
| #7306 | `6aad90be0ae` | `9a786cc028a` | Optimize Newton test runtime.
|
| #7305 | `1c9ba908cf1` | `a2dc8ce948e` | Disable Warp autodiff in the
test suite. |
| #7303 | `393fc37d2b0` | `249a5cb97d8` | Remove post-Hydra preset
resolution. |

#7272 forwards the validated `uniform`, `log_uniform`, or `gaussian`
distribution through both scene-wide backend paths. Newton behavior is
unchanged.

#7121 makes factory resolution fall back to Newton when no
`SimulationContext` exists while retaining the visualizer contract of
reporting no active backend before context creation.

#7183 is documentation-only. #7306 and #7305 change test infrastructure
only; they do not change runtime behavior.

#7303 makes `resolve_task_config` and `parse_env_cfg` the
task-composition boundary. Runtime consumers now require concrete
physics, renderer, and camera configurations and no longer attempt late
preset fallback after Hydra composition.

The branch was synchronized with the current `release/3.0.0` tip
containing #7301 before #7303 was applied. All six source cherry-picks
applied without conflicts. A file-by-file existence, mode, and blob
audit confirmed that every #7303 path matches its merged source commit
exactly.

## Validation

### #7272

- Regression verification against the unpatched `release/3.0.0` tip —
both PhysX and OvPhysX cases failed as expected.
- `uv run --extra test --frozen python -m pytest -q
source/isaaclab/test/envs/test_gravity_randomization.py
source/isaaclab/test/envs/test_mdp_event_selectors.py` — 7 passed.
- `uv run --extra test --extra ovphysx --frozen python -m pytest -q
source/isaaclab_ov/test/physics/test_ovphysx_gravity.py` — 1 passed.

### #7121

- Regression verification before the #7121 cherry-pick reproduced the
original `NoneType.physics_manager` failure.
- `uv run --extra test --frozen python -m pytest -q
source/isaaclab/test/utils/test_backend_utils.py
source/isaaclab/test/visualizers/test_visualizer.py` — 18 passed.

### #7183

- `uv run --frozen --extra test python -m pytest --noconftest -q
tools/test/test_environ_docs.py` — 26 passed.

### #7306

- All 9 affected paths match the merged source PR exactly.
- The full simulator-backed Newton suite is left to backport CI because
this local worktree does not include the full Isaac Sim runtime.

### #7305

- Loaded the root `conftest.py` successfully with Warp unavailable.
- Loaded it with Warp installed and confirmed `wp.config.enable_backward
is False`.

### #7303

- Hydra and Shadow Hand camera suites — 129 passed.
- Benchmark capture and RL entrypoint suites — 31 passed.
- Experimental frontend and custom-coupling suites — 74 passed, 1
skipped.
- Ruff and Ruff-format passed for every changed Python path.
- Source/backport audit — 54 paths checked, 0 mismatches.
- The Isaac Sim-backed integration case is left to CI because this local
worktree is kitless.

### Repository gates

- `git diff --check upstream/release/3.0.0...HEAD` — passed.

---------

Signed-off-by: Sylvester Kaczmarek <16242628+sylvesterkaczmarek@users.noreply.github.com>
Co-authored-by: Antoine RICHARD <antoiner@nvidia.com>
Co-authored-by: Sylvester Kaczmarek <16242628+sylvesterkaczmarek@users.noreply.github.com>
Co-authored-by: Mustafa H <34825877+StafaH@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working isaac-lab Related to Isaac Lab team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants