Code Review for PR #5570: [OVPHYSX] RigidObjectCollection asset

Thanks for this substantial contribution implementing RigidObjectCollection for the OVPhysX backend! I've thoroughly reviewed the changes and have the following observations:

✅ Strengths

1. Well-designed _FusedRigidBodyBinding adapter: The approach of wrapping multiple per-body RIGID_BODY_* bindings as a single (N, B, D) view is elegant and maintains API consistency with the articulation architecture.

2. Comprehensive test coverage: 636 interface tests passing plus 72 backend-specific tests shows good coverage.

3. Proper device handling: The staging buffer management respects CPU vs GPU device boundaries, which is critical for CPU-only quantities like mass and inertia.

4. Good documentation: The docstrings thoroughly explain the architecture decisions (e.g., why articulation bindings can't be used for plain rigid bodies).

🔍 Potential Issues / Questions

1. Memory efficiency in _FusedRigidBodyBinding.write()

In the write method (lines ~148-165 of rigid_object_collection.py), you're using staging[b].assign(wp.from_torch(...)). This creates a temporary warp array from torch on each write. Consider:

# Current (creates intermediate wp.array):
staging[b].assign(wp.from_torch(col, dtype=wp.float32))
binding.write(staging[b], indices=indices, mask=mask)

# Alternative (direct copy into pre-allocated staging):
wp.copy(staging[b], wp.from_torch(col, dtype=wp.float32))

However, if wp.from_torch is already zero-copy via DLPack, this may be optimal. Could you confirm the intention here?

2. Potential race condition with _sim_timestamp

In RigidObjectCollectionData.update() (data file ~line 160), the timestamp is incremented:

self._sim_timestamp += dt

If update() is called concurrently with property accessors that check timestamps, there could be a race. This mirrors the existing pattern in other backends, but worth noting if multi-threaded access is ever considered.

3. Cherry-picked commit dependency

The PR notes it carries a cherry-pick from #5545 for ovphysx_manager.py. Please ensure this gets rebased cleanly once #5545 lands to avoid duplicate history.

💡 Suggestions

1. Consider adding a warning for unimplemented material properties: The 4 xfailed tests are for material properties. A clear log message when users try to access these would improve UX:

   if tensor_type == TT.RIGID_BODY_MATERIAL:
       warnings.warn("Material properties not yet supported on OVPhysX backend")

2. Type stub completeness: The __init__.pyi updates look good, but consider adding return type hints for the @property methods in the data class for better IDE support.

📋 Validation Checklist

• CI passing (labeler ✓)
• Tests: 636 iface + 72 backend-specific passing
• Code follows existing OVPhysX articulation patterns
• Documentation includes architecture rationale
• Changelog fragment present

Verdict

This is solid work that correctly implements the collection abstraction using the binding adapter pattern. The design cleanly handles the OVPhysX constraint that ARTICULATION_LINK_* types don't resolve for plain rigid bodies.

Minor suggestions above are non-blocking. LGTM pending the cherry-pick rebase once #5545 lands.

Code Review: [OVPHYSX] RigidObjectCollection asset

This PR implements RigidObjectCollection and RigidObjectCollectionData for the OVPhysX backend, completing the rigid-body asset surface alongside RigidObject and Articulation. I've reviewed the implementation across all 10 changed files.

✅ Strengths

1. Efficient fused binding approach: The use of ovphysx 0.4.3's create_tensor_binding(prim_paths=[...]) API creates one native binding per tensor type spanning all num_instances * num_bodies prims. This avoids per-body Python fan-out at runtime.

2. Well-designed layout conversion: The strided-view reshape helpers (_reshape_view_to_data_2d/_3d and reshape_data_to_view_2d/_3d) cleanly handle body-major ↔ instance-major layout conversion without requiring Warp kernels.

3. Proper dual-path dispatching: The _binding_write and _read_binding_into_instance_major methods correctly dispatch between articulation-mode mock (for iface tests) and native fused binding layouts based on binding.shape.

4. Comprehensive test coverage: The test file covers initialization, state setting, external forces, material properties (xfailed), and state consistency across COM/link frames.

5. Good kitless test guard: The update to test_rigid_object_collection_iface.py properly handles the ovphysx kitless execution path with appropriate module stubbing.

🔍 Observations

1. Timestamp invalidation consistency (minor)

In the pose/velocity write methods, timestamp invalidation is applied to dependent buffers. The pattern is consistent but worth verifying:

• write_body_link_pose_to_sim_index: Sets _body_link_pose_w.timestamp fresh, invalidates COM-derived buffers
• write_body_com_velocity_to_sim_index: Sets _body_com_vel_w.timestamp fresh, invalidates link-derived buffers

This follows the expected dependency graph, but confirm that _body_link_state_w.timestamp and _body_com_state_w.timestamp (deprecated concat properties) are consistently invalidated when their source buffers change.

2. resolve_view_ids kernel placement

The new resolve_view_ids Warp kernel in kernels.py is cleanly isolated. The docstring accurately describes the body-major ordering (view_id = body_id * num_total_envs + env_id).

3. CPU-only binding staging

The CPU staging buffers (_cpu_body_mass, _cpu_body_coms, _cpu_body_inertia) are correctly pinned when the device is GPU. The _get_cpu_env_ids helper efficiently reuses _cpu_all_env_ids for the common all-env case.

💡 Suggestions (non-blocking)

1. Consider explicit material property warning: Since RIGID_BODY_MATERIAL isn't yet exposed by the wheel, consider adding a clear log warning when users attempt operations that would require it, rather than silently failing via the xfailed test gap.

2. Docstring for _TT_MAP constant: The tensor-type mapping tuple in _initialize_impl could benefit from a brief comment explaining the store-key vs rigid-body-TT relationship.

Verdict

This is a well-structured implementation that correctly extends the OVPhysX backend to support rigid object collections with proper (env, body) dual indexing. The fused binding approach and strided-view reshaping are elegant solutions to the layout differences between backends.

LGTM — pending resolution of the changelog CI failure and rebase once #5545 lands (as noted in the PR description).

📝 Update (920f2a6)

Reviewed SHA changed: 2af401fe → 920f2a62 (force-push/rebase)

✅ Changelog CI now passing — The proper changelog fragment antoiner-feat-ovphysx_rigidobjectcollection.major.rst has been added with a clear feature description.

No concerns from previous review require re-evaluation — the codebase changes appear consistent with the original implementation.

Status: LGTM ✅