Replicates fk invalidation on other assets#5367
Replicates fk invalidation on other assets#5367AntoineRichard wants to merge 2 commits intoisaac-sim:developfrom
Conversation
github-actions
Bot
added
bug
![isaaclab-review-bot[bot]](https://avatars.githubusercontent.com/u/157846462?s=60&v=4){kind=small}
There was a problem hiding this comment.
🤖 Isaac Lab Review Bot
Summary
This PR replicates the forward kinematics (FK) invalidation pattern from Articulation to RigidObject and RigidObjectCollection assets in the Newton backend. When poses are written to simulation, the FK cache must be invalidated so that subsequent reads of body_link_pose_w trigger a kinematic update. The implementation mirrors the existing pattern but has a critical bug in the data class initialization and is missing regression tests.
Architecture Impact
This change affects the Newton backend asset system. The FK invalidation pattern is already established in Articulation (as referenced in issue #5359), so this PR extends consistency across asset types. The SimulationManager.invalidate_fk() and SimulationManager.forward() calls are existing APIs. The blast radius is contained to Newton-backend rigid object assets — any code that writes poses and then immediately reads body_link_pose_w would previously get stale data and will now get correct data.
Implementation Verdict
Minor fixes needed — The core logic is correct and follows the established pattern, but there's a sequencing issue with _fk_timestamp initialization in RigidObjectData and the PR lacks regression tests for a bug fix.
Test Coverage
🔴 Missing regression tests. This is a bug fix PR (fixes #5359) but includes no tests demonstrating the bug or verifying the fix. A minimal regression test should:
- Create a
RigidObjectorRigidObjectCollection - Write a pose via
write_root_link_pose_to_sim_index - Immediately read
body_link_pose_wand verify it reflects the written pose
The checklist item "I have added tests that prove my fix is effective" is unchecked, which is a red flag for a bug fix PR.
CI Status
No CI checks available yet — unable to verify the changes pass existing tests.
Findings
🟡 Warning: rigid_object_data.py:67-70 — _fk_timestamp initialized after it may be needed
The _fk_timestamp is initialized at line 70, but _create_simulation_bindings() and _create_buffers() are called later (lines 83-84). If any of those methods access body_link_pose_w (which checks _fk_timestamp), it would fail. While current code appears safe, this ordering is fragile. Consider initializing _fk_timestamp earlier or documenting the dependency.
self._sim_timestamp = 0.0
self._is_primed = False
self._fk_timestamp = 0.0 # Must be set before any property access that uses it🟡 Warning: rigid_object_data.py:284-287 — Accessing body_link_pose_w has side effects
The body_link_pose_w property now calls SimulationManager.forward() which has global side effects (runs FK for all articulations). This is the intended behavior matching Articulation, but consumers should be aware that reading this property can be expensive. The docstring should note this:
"""Body link pose...
.. note::
Accessing this property may trigger a forward kinematics computation
if poses have been modified since the last simulation step.
"""🔵 Improvement: rigid_object.py:333-334, 381-382, 437-438, 490-491 — Duplicated invalidation pattern
The same two-line invalidation pattern is repeated 4 times in rigid_object.py:
self.data._fk_timestamp = -1.0
SimulationManager.invalidate_fk()Consider extracting this to a helper method _invalidate_fk_cache() on the data class or asset class to reduce duplication and ensure consistency. This would also make future modifications (e.g., adding logging) easier.
🔵 Improvement: rigid_object_collection.py:429-430, 522-523 — Same pattern repeated
The rigid object collection has the same duplicated invalidation code. A shared utility or base class method would improve maintainability.
🟡 Warning: rigid_object_data.py:113-116 — Subtle edge case in timestamp sync logic
if self._fk_timestamp >= 0.0:
self._fk_timestamp = self._sim_timestampThis logic means if FK was invalidated (set to -1.0), it stays invalidated until explicitly updated by accessing body_link_pose_w. This is correct, but the comment "keep fk_timestamp in sync unless it was explicitly invalidated" could be clearer: "If FK was not invalidated between steps, mark it as current; otherwise leave it stale to trigger recomputation on next access."
🔵 Improvement: PR Description lacks detail
The description "replicating fk invalidation on other assets" is minimal. It should explain:
- What FK invalidation is
- Why it's needed for RigidObject/RigidObjectCollection
- How users were affected by the bug (stale pose data after writes)
This helps reviewers and future maintainers understand the change's purpose.
Greptile SummaryThis PR extends the FK-invalidation fix (originally applied to articulations in #5359) to Confidence Score: 5/5Safe to merge — straightforward replication of an already-proven pattern with no new logic. All four changed files apply the exact same No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant RigidObject
participant Data as RigidObjectData
participant SM as SimulationManager
User->>RigidObject: write_root_link_pose_to_sim_index(pose)
RigidObject->>SM: wp.launch(set_root_link_pose_to_sim_index)
RigidObject->>Data: _fk_timestamp = -1.0
RigidObject->>SM: invalidate_fk()
User->>Data: body_link_pose_w (property access)
Data->>Data: _fk_timestamp(-1.0) < _sim_timestamp? YES
Data->>SM: forward()
Data->>Data: _fk_timestamp = _sim_timestamp
Data-->>User: _sim_bind_body_link_pose_w (up-to-date)
SM->>Data: update(dt)
Data->>Data: _sim_timestamp += dt
Data->>Data: _fk_timestamp >= 0? sync _fk_timestamp = _sim_timestamp
Reviews (1): Last reviewed commit: "replicating fk invalidation on other ass..." | Re-trigger Greptile |
AntoineRichard
changed the title
Bump isaaclab_newton to 0.5.21 and document the body_link_pose_w staleness fix in the changelog. Add regression tests in two styles: * test_rigid_object.py: behavioural test that writes a root pose and asserts body_link_pose_w reflects it without an intervening sim step. Exercises the property's forward() path: without the fix, body_q is stale and the read returns the pre-write pose. * test_rigid_object_collection.py: state test that asserts SimulationManager._fk_dirty is set and data._fk_timestamp is reset to the invalid sentinel after a pose write. A behavioural assertion would not distinguish fixed from unfixed in the collection because _sim_bind_body_link_pose_w is bound directly to root_transforms, so the read returns the kernel's write regardless of FK state. The downstream effect still matters for the collision pipeline's body_q, which is what _fk_dirty signals. Red-green verified on CPU and CUDA for all 4 writer parametrisations per file.
1 participant
Description
Replicates fk invalidation on other assets in Newton.
Fixes #5359
Type of change
Checklist
pre-commitchecks with./isaaclab.sh --formatconfig/extension.tomlfileCONTRIBUTORS.mdor my name already exists there