Replace cubric ctypes shim with IFabricHierarchy GPU API - #6834
Conversation
Greptile SummaryThis PR replaces Newton's private cubric ctypes adapter with the public Fabric hierarchy GPU update API.
Confidence Score: 4/5The GPU hierarchy error path should be fixed before merging because a failed update can leave rendering stale while synchronization is reported complete. The new path clears its retry state before the fallible hierarchy operation and never falls back after invocation errors; it also overwrites pre-existing shared tracking settings on exit. Files Needing Attention: source/isaaclab_newton/isaaclab_newton/physics/newton_manager.py Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Newton state becomes dirty] --> B[Write body transforms to Fabric]
B --> C{GPU hierarchy API available?}
C -->|Yes| D[Pause transform tracking]
D --> E[GPU update with RIGID_BODY and FORCE_UPDATE]
E --> F[Resume transform tracking]
C -->|No| G[CPU update_world_xforms]
F --> H[Viewport and RTX consume propagated transforms]
G --> H
Reviews (1): Last reviewed commit: "Replace cubric ctypes shim with IFabricH..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| NewtonManager._newton_fabric_ready = True | ||
| NewtonManager._transforms_dirty = False | ||
|
|
||
| if use_cubric and fabric_hierarchy is not None: | ||
| fabric_id = cls._usdrt_stage.GetFabricId().id | ||
| if fabric_id != cls._cubric_bound_fabric_id: | ||
| cls._cubric.bind_to_stage(cls._cubric_adapter, fabric_id) | ||
| NewtonManager._cubric_bound_fabric_id = fabric_id | ||
| cls._cubric.compute(cls._cubric_adapter) | ||
| if use_gpu_hierarchy: | ||
| # RIGID_BODY: inverse-propagate on PhysicsRigidBodyAPI buckets | ||
| # (keep Newton world matrices, derive local). FORCE_UPDATE: | ||
| # bypass the change-listener dirty check after tracking pause. | ||
| fabric_hierarchy.update_world_xforms_gpu_with_options( | ||
| gpu_opts_cls.RIGID_BODY | gpu_opts_cls.FORCE_UPDATE |
There was a problem hiding this comment.
When update_world_xforms_gpu_with_options raises, the code has already cleared _transforms_dirty and set _newton_fabric_ready, and the exception handler does not run update_world_xforms. Subsequent renders therefore report readiness while displaying stale transforms, and later physics steps retry the same failing GPU path instead of falling back to the CPU update.
| finally: | ||
| if use_cubric and fabric_hierarchy is not None: | ||
| if use_gpu_hierarchy: | ||
| fabric_hierarchy.track_world_xform_changes(True) | ||
| fabric_hierarchy.track_local_xform_changes(True) |
There was a problem hiding this comment.
The GPU synchronization scope unconditionally enables both shared Fabric tracking modes on exit. When either mode was already disabled, this overwrites the owning scope's state and causes later Fabric writes to be tracked even though tracking was intended to remain suspended; preserve and restore each prior value as the sibling Fabric writer does.
![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
The patch replaces the private cubric ctypes bridge with Kit’s IFabricHierarchy.update_world_xforms_gpu_with_options API while retaining the CPU fallback. The implementation paths are coherent, but the Newton changelog should not document removal of a private implementation module or reference a deleted module target.
- Design and architecture: Using Kit’s supported Fabric hierarchy API instead of a hand-maintained Carbonite vtable binding simplifies lifecycle management and removes ABI-sensitive code. The existing hierarchy tracking controls, rigid-body tagging, fallback path, and session cache reset remain aligned with the prior design.
- API: The removed Python symbols are private and their in-tree consumers were updated. However, the
Removedchangelog entry exposes the privateisaaclab_newton.physics._cubricmodule and creates a Sphinx module reference to a deleted target, contrary to the repository guidance to omit internal implementation details. Keep the changelog focused on the user-visible transform-sync change. - Implementation: The transform synchronization path was traced through capability detection, hierarchy acquisition, tracking suspension and restoration, GPU option selection, CPU fallback, and lifecycle reset. No concrete implementation defect is established by the supplied patch.
Minor fixes needed. Posted 1 actionable finding inline.
Automated review; human maintainers own approval decisions.
| Removed | ||
| ^^^^^^^ | ||
|
|
||
| * Removed :mod:`isaaclab_newton.physics._cubric` ctypes bindings for |
There was a problem hiding this comment.
🔵 Suggestion · Api — Changelog exposes private module removal
The Removed entry announces :mod:isaaclab_newton.physics._cubric``, a private module that was never user-facing, and the :mod: role now points at a deleted target. Repository changelog guidance asks to avoid internal implementation details users would not understand; fold the user-visible behavior into the existing `Changed` entry and drop the private-module removal.
53242c3 to
82e6474
Compare
Kit now exposes update_world_xforms_gpu_with_options with FabricHierarchyGpuUpdateOptions, so Newton can drive the same RIGID_BODY | FORCE_UPDATE hierarchy update through Python and drop the fragile IAdapter ctypes bindings.
82e6474 to
8e42186
Compare
3 participants
Description
Kit now exposes update_world_xforms_gpu_with_options with FabricHierarchyGpuUpdateOptions, so Newton can drive the same RIGID_BODY | FORCE_UPDATE hierarchy update through Python and drop the fragile IAdapter ctypes bindings.
Fixes # (issue)
Type of change
Screenshots
Please attach before and after screenshots of the change if applicable.
Checklist
pre-commitchecks with./isaaclab.sh --formatsource/<pkg>/changelog.d/for every touched package (do not editCHANGELOG.rstor bumpextension.toml— CI handles that)CONTRIBUTORS.mdor my name already exists there