Skip to content

Regression test: FrameView staleness under physics integration#5476

Open
ooctipus wants to merge 1 commit into
isaac-sim:developfrom
ooctipus:octi/fabric-frameview-staleness-test
Open

Regression test: FrameView staleness under physics integration#5476
ooctipus wants to merge 1 commit into
isaac-sim:developfrom
ooctipus:octi/fabric-frameview-staleness-test

Conversation

@ooctipus
Copy link
Copy Markdown
Collaborator

@ooctipus ooctipus commented May 3, 2026

Summary

  • Adds a contract test in test_views_xform_prim_fabric.py that drops a RigidBody/ArticulationRoot parent under gravity and asserts the child Xform's FabricFrameView.get_world_poses() follows it.
  • Expected to FAIL on develop today. Drift = 0.0000 m: the sensor pose stays byte-identical to the spawn pose for the entire 1 s of free fall.
  • The fix lives in a follow-up branch.

Why

Every other FrameView contract test asserts pose at init time (before any sim.step). That's why a fabric-write regression introduced in #5179 has been shipping silently — RayCaster / Camera / IMU sensors parented under articulation or rigid bodies return their spawn-time pose forever in headless training. Height-scan obs in rough-terrain locomotion was a frozen patch from spawn; agents have been training on a constant input.

Without this test, no CI signal would have caught the bug. Adding it here so the fix PR has a concrete green/red marker.

Test plan

  • CI for test_world_pose_tracks_physics_body_parent[cuda:0] shows FAIL with the assertion message documenting drift = 0.0000 m.
  • After the companion fix lands, this test goes green.

The existing FabricFrameView contract tests only assert pose at init time
(before any sim.step), so they never exercised the physics → fabric write
path that real callers (RayCaster, Camera, IMU spawned under articulation
or rigid bodies) rely on. This let a fabric-write regression ship silently:
sensors parented under a physics body return their spawn-time pose forever
even as the body moves meters under gravity.

This commit adds a new contract test to the PhysX backend test file that:

- spawns a child Xform under a RigidBody + ArticulationRoot parent at z=5
- lets gravity drop it for ~1 s (100 sim.step calls at dt=0.01)
- asserts FabricFrameView.get_world_poses returns a fresh pose

The test is expected to FAIL on develop today: drift = 0.0000 m, with the
sensor pose byte-identical to the spawn pose for the entire 1 s of free
fall. The companion fix is in a follow-up branch.
@github-actions github-actions Bot added the isaac-lab Related to Isaac Lab team label May 3, 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.

🤖 Isaac Lab Review Bot

Summary

This PR adds a regression test that exposes a known bug where FabricFrameView.get_world_poses() returns stale spawn-time poses instead of tracking physics-integrated parent bodies. The test is intentionally designed to fail on the current develop branch to provide a concrete signal for the companion fix PR. The test logic is sound and the physics setup is correct.

Architecture Impact

Self-contained test addition. No production code changes. The test imports FabricFrameView from isaaclab_physx.sim.views and uses standard sim_utils APIs. The test validates a critical contract: child Xforms under physics bodies must reflect parent motion through the Fabric hierarchy.

Implementation Verdict

Minor fixes needed

Test Coverage

This PR is the test coverage — it adds a regression test for a silent bug affecting sensors (RayCaster, Camera, IMU) parented under articulations/rigid bodies. The test design is good: it creates a physics body at z=5m, simulates 1s of free-fall, and asserts the child Xform dropped. The 0.5m threshold is reasonable (free-fall would yield ~4.9m drop).

CI Status

No CI checks available yet. The author explicitly expects this test to fail on develop, which is the intended behavior for a "canary" test awaiting its companion fix.

Findings

🟡 Warning: test_views_xform_prim_fabric.py:156 — Test relies on default gravity without explicitly setting it
The test assumes default gravity (~9.81 m/s²) but doesn't configure it in SimulationCfg. While the default is likely correct, explicitly setting gravity=(0.0, 0.0, -9.81) would make the test more robust against future default changes and self-documenting:

sim = sim_utils.SimulationContext(sim_utils.SimulationCfg(
    dt=0.01, device=device, use_fabric=True,
    gravity=(0.0, 0.0, -9.81)
))

🟡 Warning: test_views_xform_prim_fabric.py:155 — Missing ground plane means body falls indefinitely; collision cube may be unnecessary
The test creates a collision cube on the body but there's no ground plane. The cube serves no purpose for this test (nothing to collide with). Consider either: (a) removing the collision setup entirely since it's not exercised, or (b) adding a brief comment that collision geometry is intentionally omitted. The current code suggests collision matters when it doesn't.

🔵 Improvement: test_views_xform_prim_fabric.py:122 — Parametrize could include "cpu" with xfail to document the known limitation
The test only parametrizes cuda:0 and skips CPU via _skip_if_unavailable. Consider adding CPU with an xfail marker to document the known Warp fabricarray CPU issues in the test suite itself:

@pytest.mark.parametrize("device", [
    pytest.param("cuda:0"),
    pytest.param("cpu", marks=pytest.mark.xfail(reason="Warp fabricarray CPU issues")),
])

🔵 Improvement: test_views_xform_prim_fabric.py:160-161 — Consider adding intermediate assertion or logging for debuggability
When this test fails (as expected on develop), the failure message shows final positions but doesn't indicate whether the body moved at all. Adding a mid-simulation check or logging the body's direct position (via ArticulationView or RigidPrimView) would help distinguish "Fabric staleness" from "physics not running":

# Optional: verify physics is actually running by checking body position directly
# body_pos = ... 
# assert body_pos[2] < initial_z - 1.0, "Physics body itself isn't falling"

🔵 Improvement: test_views_xform_prim_fabric.py:163-168 — Assertion message could include expected drop distance
The error message explains the bug well but could include the expected physics calculation for completeness:

f"Expected drop ≈{0.5 * 9.81 * 1.0**2:.1f}m under gravity over 1s, got {drift_z:.4f}m."

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 3, 2026

Greptile Summary

This PR adds a single backend-specific regression test to test_views_xform_prim_fabric.py that drops a RigidBody/ArticulationRoot prim under gravity for 1 s and asserts FabricFrameView.get_world_poses() tracks the motion (expected drift_z > 0.5 m). The test is explicitly intended to fail on develop until the companion fix for the PhysX → Fabric write-path regression (#5179) lands.

  • The test is designed to fail but lacks @pytest.mark.xfail(strict=True), meaning CI will report a hard FAIL instead of an expected XFAIL — this could block unrelated PRs depending on CI policy. strict=True gives the same red/green signal while keeping CI clean.

Confidence Score: 4/5

Safe to merge; the only concern is the missing xfail marker, which may cause CI noise but does not affect correctness of the test.

A single P2 finding (missing xfail decorator). No logic bugs in the test itself — the drift direction, threshold, setup/teardown, and Fabric view usage are all correct. Score stays at 4 per P2-only ceiling.

source/isaaclab_physx/test/sim/test_views_xform_prim_fabric.py — missing @pytest.mark.xfail on the new regression test.

Important Files Changed

Filename Overview
source/isaaclab_physx/test/sim/test_views_xform_prim_fabric.py Adds regression test that drops a RigidBody/ArticulationRoot parent under gravity and asserts FabricFrameView.get_world_poses() follows it; intentionally fails on develop. Missing @pytest.mark.xfail is the only notable gap.

Sequence Diagram

sequenceDiagram
    participant T as test_world_pose_tracks_physics_body_parent
    participant USD as USD Stage
    participant PhysX as PhysX Engine
    participant Fabric as Fabric Layer
    participant FV as FabricFrameView

    T->>USD: create_prim("/World/PhysicsParent") at z=5
    T->>USD: Apply RigidBodyAPI + ArticulationRootAPI + MassAPI
    T->>USD: Define CollisionCube child
    T->>USD: create_prim("/World/PhysicsParent/Child", Camera)
    T->>PhysX: SimulationContext(use_fabric=True).reset()
    T->>FV: FrameView(child_path)
    T->>FV: get_world_poses() → pos_before (z≈5)
    loop 100 × sim.step()
        PhysX->>Fabric: write updated body transforms
        Fabric->>FV: IFabricHierarchy propagates to child Xform
    end
    T->>FV: get_world_poses() → pos_after
    T->>T: assert (pos_before.z - pos_after.z) > 0.5 m
Loading

Reviews (1): Last reviewed commit: "Add regression test for FrameView stalen..." | Re-trigger Greptile

Comment on lines +122 to +123
@pytest.mark.parametrize("device", ["cuda:0"])
def test_world_pose_tracks_physics_body_parent(device):
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 Missing xfail marker for intentionally-failing test

The PR description states this test is "Expected to FAIL on develop today," but without @pytest.mark.xfail, the test runner reports it as a hard FAIL rather than an expected failure. The codebase already uses @pytest.mark.xfail for known-broken tests in test_cloner.py. Using strict=True is cleanest here: it records XFAIL (non-blocking) while the bug persists, and automatically flags XPASS once the fix lands — a precise green/red marker without breaking CI for unrelated PRs.

Suggested change
@pytest.mark.parametrize("device", ["cuda:0"])
def test_world_pose_tracks_physics_body_parent(device):
@pytest.mark.xfail(strict=True, reason="PhysX → Fabric write path broken (#5179); fix in follow-up branch")
@pytest.mark.parametrize("device", ["cuda:0"])
def test_world_pose_tracks_physics_body_parent(device):

sim.step(render=False)

pos_after = view.get_world_poses()[0].torch[0]
drift_z = (pos_before[2] - pos_after[2]).item()
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 Drift direction is correct; ground-plane edge case worth noting

drift_z = pos_before[2] - pos_after[2] correctly yields a positive value when the body falls (z decreases). With initial_z = 5.0 and dt * 100 = 1 s, free-fall gives ~4.9 m of drop, so the 0.5 m threshold is very conservative and safe. One edge case: if SimulationCfg adds a default ground plane at z=0, the body stops there and drift_z ≈ 5.0, not "several meters" as the assertion message implies. The assertion still passes correctly, but a small clarification in the failure message would improve debuggability.

@kellyguo11 kellyguo11 moved this to In review in Isaac Lab May 5, 2026
ooctipus added a commit that referenced this pull request May 17, 2026
# Description

Refactors `RayCaster` and `RayCasterCamera` into a backend-agnostic base
+ per-backend implementations under `isaaclab_physx.sensors.ray_caster`
and `isaaclab_newton.sensors.ray_caster`, mirroring the iconic split
pattern already used by `Pva`, `FrameTransformer`, and `ContactSensor`.

The PhysX backend tracks the parent rigid body directly via
`RigidObjectView` instead of going through `FabricFrameView`, which
fixes the staleness regression from #5179: sensors parented under an
articulation / rigid body were returning their spawn-time pose forever
during headless training, silently freezing height-scan observations in
rough-terrain locomotion (and any similar IMU / camera path that read
through `FrameView`).

The Newton backend uses the site-based pattern from `Pva` /
`FrameTransformer`: walk USD to the rigid-body ancestor, register a
body-attached site via `NewtonManager.cl_register_site`, and read
per-step transforms off a `SensorFrameTransform` against a shared
world-origin reference. Static parents bypass the site machinery (a
single `body=-1` global site can't represent per-env world origins) and
serve a cached per-env `wp.transformf` array.

`MultiMeshRayCaster` / `MultiMeshRayCasterCamera` re-parent onto the new
base but keep their `FrameView`-backed body tracker, so the staleness
behavior persists there. Tracked as `xfail` in
`test_ray_caster_sensor.py` — extending the backend split to MultiMesh
is a follow-up.

The cfg surface, `class_type` strings, and runtime semantics are
unchanged for callers; existing user code does not need to migrate.

Fixes #5476 (the `FabricFrameView` contract regression-test PR — the bug
it documents is fixed for the single-mesh path here).

## Type of change

- Bug fix (non-breaking change which fixes an issue)
- New feature (non-breaking change which adds functionality)

## Screenshots

N/A — backend refactor, no UI changes.

## Checklist

- [x] I have read and understood the [contribution
guidelines](https://isaac-sim.github.io/IsaacLab/main/source/refs/contributing.html)
- [x] I have run the [\`pre-commit\` checks](https://pre-commit.com/)
with \`./isaaclab.sh --format\`
- [x] I have made corresponding changes to the documentation
- [x] My changes generate no new warnings
- [x] I have added tests that prove my fix is effective or that my
feature works
- [x] 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)
- [x] I have added my name to the \`CONTRIBUTORS.md\` or my name already
exists there
matthewtrepte pushed a commit to matthewtrepte/IsaacLab that referenced this pull request May 18, 2026
# Description

Refactors `RayCaster` and `RayCasterCamera` into a backend-agnostic base
+ per-backend implementations under `isaaclab_physx.sensors.ray_caster`
and `isaaclab_newton.sensors.ray_caster`, mirroring the iconic split
pattern already used by `Pva`, `FrameTransformer`, and `ContactSensor`.

The PhysX backend tracks the parent rigid body directly via
`RigidObjectView` instead of going through `FabricFrameView`, which
fixes the staleness regression from isaac-sim#5179: sensors parented under an
articulation / rigid body were returning their spawn-time pose forever
during headless training, silently freezing height-scan observations in
rough-terrain locomotion (and any similar IMU / camera path that read
through `FrameView`).

The Newton backend uses the site-based pattern from `Pva` /
`FrameTransformer`: walk USD to the rigid-body ancestor, register a
body-attached site via `NewtonManager.cl_register_site`, and read
per-step transforms off a `SensorFrameTransform` against a shared
world-origin reference. Static parents bypass the site machinery (a
single `body=-1` global site can't represent per-env world origins) and
serve a cached per-env `wp.transformf` array.

`MultiMeshRayCaster` / `MultiMeshRayCasterCamera` re-parent onto the new
base but keep their `FrameView`-backed body tracker, so the staleness
behavior persists there. Tracked as `xfail` in
`test_ray_caster_sensor.py` — extending the backend split to MultiMesh
is a follow-up.

The cfg surface, `class_type` strings, and runtime semantics are
unchanged for callers; existing user code does not need to migrate.

Fixes isaac-sim#5476 (the `FabricFrameView` contract regression-test PR — the bug
it documents is fixed for the single-mesh path here).

## Type of change

- Bug fix (non-breaking change which fixes an issue)
- New feature (non-breaking change which adds functionality)

## Screenshots

N/A — backend refactor, no UI changes.

## Checklist

- [x] I have read and understood the [contribution
guidelines](https://isaac-sim.github.io/IsaacLab/main/source/refs/contributing.html)
- [x] I have run the [\`pre-commit\` checks](https://pre-commit.com/)
with \`./isaaclab.sh --format\`
- [x] I have made corresponding changes to the documentation
- [x] My changes generate no new warnings
- [x] I have added tests that prove my fix is effective or that my
feature works
- [x] 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)
- [x] I have added my name to the \`CONTRIBUTORS.md\` or my name already
exists there
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

Status: In review

Development

Successfully merging this pull request may close these issues.

2 participants