Skip to content

Fix ovphysx reset-velocity cache staleness and slow ASCII warmup export#6628

Closed
ClemensSchwarke wants to merge 1 commit into
isaac-sim:developfrom
ClemensSchwarke:clemens/ovphysx-fixes
Closed

Fix ovphysx reset-velocity cache staleness and slow ASCII warmup export#6628
ClemensSchwarke wants to merge 1 commit into
isaac-sim:developfrom
ClemensSchwarke:clemens/ovphysx-fixes

Conversation

@ClemensSchwarke

Copy link
Copy Markdown
Collaborator

Description

Fixes two issues I encountered in the velocity env cleanup.

  1. The velocity cache was stale after reset, which caused huge velocities to enter the MDP terms. ArticulationData._reset_velocity() invalidated the world-frame velocity buffers but not the body-frame ones.
  2. OvPhysxManager exported the warmup stage as ASCII USD (.usda). Serializing large robot/terrain geometry to text is extremely slow (It took 38 min to load the rough Digit environment).

Type of change

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

@ClemensSchwarke
ClemensSchwarke requested a review from a team July 20, 2026 12:43
@github-actions github-actions Bot added bug Something isn't working isaac-lab Related to Isaac Lab team labels Jul 20, 2026
@greptile-apps

greptile-apps Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR fixes two independent bugs in the isaaclab_ovphysx extension: a velocity cache staleness issue after reset, and a pathologically slow ASCII USD export during warmup.

  • Cache invalidation fix: ArticulationData._reset_velocity() now also resets timestamps for the four body-frame root velocity buffers (_root_link_lin_vel_b, _root_link_ang_vel_b, _root_com_lin_vel_b, _root_com_ang_vel_b). Previously these were guarded only by the simulation timestamp, which a reset/write never advances, so the first observation after a cold reset returned the pre-reset cached values — manifesting as ~1e5 base-velocity spikes for closed-loop articulations like Cassie.
  • Binary USD export: The OvPhysX warmup stage is now written as .usdc (binary crate) instead of .usda (ASCII), cutting environment-creation time by ~10× for large scenes (e.g. Digit on rough terrain dropped from ~38 min to ~4 min at 4096 envs).

Confidence Score: 4/5

Both fixes are minimal and targeted — the cache invalidation change mirrors an identical pattern already in _reset_body_com_pose_b_dependents, and the .usda.usdc switch is a one-line filename change with no logic impact.

The core changes are straightforward and well-understood. The regression test covers the angular velocity path but leaves the linear and com-frame counterparts untested; a future regression on those three buffers would go undetected by this test suite.

The regression test in test_articulation.py would benefit from additional assertions covering root_lin_vel_b, root_com_lin_vel_b, and root_com_ang_vel_b to fully exercise all four newly-invalidated buffers.

Important Files Changed

Filename Overview
source/isaaclab_ovphysx/isaaclab_ovphysx/assets/articulation/articulation_data.py Adds the four body-frame root velocity buffers to the _reset_velocity invalidation list, exactly mirroring how they are already handled in _reset_body_com_pose_b_dependents. The fix is minimal and correct.
source/isaaclab_ovphysx/isaaclab_ovphysx/physics/ovphysx_manager.py Changes the warmup USD export from .usda (ASCII) to .usdc (binary crate) in both the docstring and the filename. USD's layer API determines format from the extension, so all layer operations handle .usdc correctly without any other changes.
source/isaaclab_ovphysx/test/assets/test_articulation.py Adds a regression test that writes a distinctive root velocity without advancing the sim timestamp and checks that root_ang_vel_b reflects the new value. Missing analogous assertions for root_lin_vel_b and the two root_com_* body-frame velocity properties.
source/isaaclab_ovphysx/changelog.d/fix-reset-root-velocity-body-frame-cache.rst New changelog entry accurately describes the body-frame cache invalidation bug and its symptom.
source/isaaclab_ovphysx/changelog.d/fix-warmup-export-usdc.rst New changelog entry accurately describes the ASCII-vs-binary USD export issue and the impact on large-scene startup time.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant Caller
    participant Articulation
    participant ArticulationData
    participant TimestampedBuffer

    Note over Caller,TimestampedBuffer: Velocity write path (after fix)
    Caller->>Articulation: write_root_velocity_to_sim_index(root_velocity)
    Articulation->>ArticulationData: "_reset_velocity(from_com=True)"
    ArticulationData->>TimestampedBuffer: reset_timestamps([..., _root_link_lin_vel_b, _root_link_ang_vel_b, _root_com_lin_vel_b, _root_com_ang_vel_b, ...])
    Note over TimestampedBuffer: timestamps set below sim_timestamp
    Caller->>ArticulationData: root_ang_vel_b (property read)
    ArticulationData->>TimestampedBuffer: "check _root_link_ang_vel_b.timestamp < sim_timestamp"
    TimestampedBuffer-->>ArticulationData: stale, recompute
    ArticulationData->>ArticulationData: wp.launch(_world_vel_to_body_ang, root_link_pose_w, root_link_vel_w)
    ArticulationData-->>Caller: fresh body-frame angular velocity
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
    participant Caller
    participant Articulation
    participant ArticulationData
    participant TimestampedBuffer

    Note over Caller,TimestampedBuffer: Velocity write path (after fix)
    Caller->>Articulation: write_root_velocity_to_sim_index(root_velocity)
    Articulation->>ArticulationData: "_reset_velocity(from_com=True)"
    ArticulationData->>TimestampedBuffer: reset_timestamps([..., _root_link_lin_vel_b, _root_link_ang_vel_b, _root_com_lin_vel_b, _root_com_ang_vel_b, ...])
    Note over TimestampedBuffer: timestamps set below sim_timestamp
    Caller->>ArticulationData: root_ang_vel_b (property read)
    ArticulationData->>TimestampedBuffer: "check _root_link_ang_vel_b.timestamp < sim_timestamp"
    TimestampedBuffer-->>ArticulationData: stale, recompute
    ArticulationData->>ArticulationData: wp.launch(_world_vel_to_body_ang, root_link_pose_w, root_link_vel_w)
    ArticulationData-->>Caller: fresh body-frame angular velocity
Loading

Reviews (1): Last reviewed commit: "fix ovphysx reset-velocity cache stalene..." | Re-trigger Greptile

Comment on lines +1815 to +1821
# the body-frame angular velocity must reflect the write; its norm is rotation-invariant (= 5)
ang_after = articulation.data.root_ang_vel_b.torch
torch.testing.assert_close(
ang_after.norm(dim=-1), torch.full((num_articulations,), 5.0, device=device), atol=1e-3, rtol=1e-3
)
# and it must actually differ from the stale pre-write value
assert not torch.allclose(ang_after, ang_before)

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 root_lin_vel_b and root_com_* cache invalidations not exercised

The test verifies root_ang_vel_b after the velocity write but skips root_lin_vel_b, root_com_lin_vel_b, and root_com_ang_vel_b, all of which received matching invalidation entries in _reset_velocity. Linear velocity norm is equally rotation-invariant (3.0 in the test setup), so adding assert_close(lin_after.norm(dim=-1), torch.full(..., 3.0, ...)) and the analogous root_com_* assertions would give full coverage of the four newly-invalidated buffers.

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!

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

If we do this tests here we should probably add it to the other backends.

@AntoineRichard

Copy link
Copy Markdown
Collaborator

Working on a PR that will fix it on all backend.

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.

2 participants