Joint Dependent Mesh/BBox for Embodiment - #968
Conversation
| return PlacementGeometrySource( | ||
| usd_path=spawn.usd_path, | ||
| scale=(scale_x, scale_y, scale_z), | ||
| joint_pos=dict(robot.init_state.joint_pos or {}), |
There was a problem hiding this comment.
When a Franka or Droid embodiment receives a non-default initial_joint_pose, the reset event applies that pose while placement geometry still reads robot.init_state.joint_pos, causing relation placement to use a bounding box and collision mesh for the wrong arm configuration.
Knowledge Base Used:
There was a problem hiding this comment.
This is better handle in droid/franka. set_initial_joint_pose currently only updates the reset event's default_pose.
can expand set_initial_joint_pose to also all self.set_joint_initial_pos(...) with a name→value map matching set_default_joint_pose order.
There was a problem hiding this comment.
That's the right fix. Looking at both constructors, set_initial_joint_pose only writes to the event's positional default_pose list but leaves init_state.joint_pos (what get_placement_geometry_source reads) unchanged.
One ordering issue to watch in FrankaEmbodimentBase.__init__: set_initial_joint_pose is called before self.scene_config is assigned:
self.event_config = FrankaEventCfg()
if initial_joint_pose is not None:
self.set_initial_joint_pose(initial_joint_pose) # scene_config is still None here
...
self.scene_config = FrankaSceneCfg() # assigned afterSo if you expand set_initial_joint_pose to also call self.set_joint_initial_pos(...), it'll hit the assert self.scene_config is not None in the base class. Droid doesn't have this problem since self.scene_config = DroidSceneCfg() is assigned before the call.
The simplest fix for Franka is to move scene_config assignment before the joint pose call:
self.scene_config = FrankaSceneCfg()
self.event_config = FrankaEventCfg()
if initial_joint_pose is not None:
self.set_initial_joint_pose(initial_joint_pose)Then the expanded set_initial_joint_pose in both Franka and Droid can safely call self.set_joint_initial_pos(joint_name_map) alongside the existing default_pose update, and placement geometry will reflect the correct configuration.
Greptile SummaryThis PR introduces joint-dependent placement geometry and reusable collision-mesh artifacts.
Confidence Score: 3/5The PR should not merge until Franka and Droid placement geometry honors constructor-supplied initial joint poses; source-aware cache invalidation should also be strengthened. Franka and Droid can reset into a constructor-selected arm pose while their newly introduced bounding box and collision mesh are computed from a different joint mapping, causing incorrect placement geometry on a supported path. Files Needing Attention: isaaclab_arena/embodiments/embodiment_base.py, isaaclab_arena/embodiments/franka/franka.py, isaaclab_arena/embodiments/droid/droid.py, isaaclab_arena/utils/collision_mesh_store.py Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart LR
Config[Embodiment scene config] --> Source[PlacementGeometrySource]
Source --> Pose[USD articulation posing]
Pose --> BBox[Posed bounding box]
Pose --> Extract[Mesh extraction]
Source --> Store{Stored artifact valid?}
Store -->|yes| Mesh[Scaled collision mesh]
Store -->|no| Extract
Extract --> Cache[Local/published mesh store]
Cache --> Mesh
BBox --> Placement[Relation placement]
Mesh --> Placement
Reviews (1): Last reviewed commit: "add joint support for robots" | Re-trigger Greptile |
| what makes a mesh reusable: embodiments spawn at their configured pose rather than at zero, so keying | ||
| on the asset alone would store a mesh nobody asks for. | ||
|
|
||
| Lookup order is the local cache, then the robot's own folder under ``ARENA_ROBOT_LIBRARY_DIR`` on |
There was a problem hiding this comment.
🟡 Does the published robot library need to ship now?
The in-process lru_cache on the posed-geometry helpers already covers the relation-solver hot path (repeated bbox queries within a run), so this disk + Nucleus layer's marginal benefit is the one-time 0.1–2.5 s extraction per fresh process. Weighed against that, it adds a publish pipeline that must be re-run on every USD/joint change, a Nucleus dependency in the placement path, and a 1 GiB ~/.cache cache that is now on by default for every user (a default-path change, not opt-in). Could we ship the in-process cache first — plus a plain local disk cache if the cross-process cost actually bites — and defer the exported-library machinery until it's shown to be a bottleneck?
🤖 Isaac Lab-Arena Review BotSummaryThis PR makes an embodiment's placement bounding box and collision mesh reflect the robot as actually spawned — posed at its configured Design, Boundaries & ScopeMy one real question is scope, raised inline on Boundaries otherwise hold: the new FK/store code is generic USD/IO utility, no robot-specific logic leaks into core, and the embodiment geometry methods stay pure (no live Findings🟡 collision_mesh_store.py — question whether the published-library + disk-LRU persistence needs to ship now, or could be deferred behind the in-process cache (inline). Test CoverageExcellent. New tests follow the inner/outer VerdictMinor fixes needed — essentially ship-ready; please just weigh in on the persistence-layer scope question before merge. |
qianl-nv
left a comment
There was a problem hiding this comment.
I have some general questions on why we need the "local/Omniverse cache for pre-computed mesh" part.
- collecting the mesh for droid is only taking about 1s, it's hardly the bottlenet in the overall pipeline atm. we don't think we need to go done for the perf there using cache. Finding ways the speed up the mesh mode for Background (where we absolutely need it) is more important imo.
- for embodiment, what's blocking is actually the joint-angle-based bounding box collection. unless we are confident of shipping v0.3 with both background and embodiment using mesh mode (so far it has always take forever for solver), we need a working version of background in mesh mode + embodiment in bbox mode.
| return PlacementGeometrySource( | ||
| usd_path=spawn.usd_path, | ||
| scale=(scale_x, scale_y, scale_z), | ||
| joint_pos=dict(robot.init_state.joint_pos or {}), |
There was a problem hiding this comment.
This is better handle in droid/franka. set_initial_joint_pose currently only updates the reset event's default_pose.
can expand set_initial_joint_pose to also all self.set_joint_initial_pos(...) with a name→value map matching set_default_joint_pose order.
4e98441 to
204ab44
Compare
Retire the obsolete serial no-overlap reference and its profiling controls now that the batched implementation is the only supported path. Move serial oracle to unit test and add regression testing Cleanup unused MeshPairCache data
qianl-nv
left a comment
There was a problem hiding this comment.
For droid/franka let's just overwrite get_collision_mesh instead of creating a new get_collsion_meshes. The return should be one mesh, this should remove a lot of downstream changes.
Please rebase the MR onto qianl/feature/mesh-optimization
Update the commit message with solver/validation logging before/after this change for the droid kitchen example
Will do another pass after above modification.
|
|
||
| def set_initial_joint_pose(self, initial_joint_pose: list[float]) -> None: | ||
| self.event_config.init_franka_arm_pose.params["default_pose"] = initial_joint_pose | ||
| def set_initial_joint_pose(self, initial_joint_pose: Mapping[str, float]) -> None: |
There was a problem hiding this comment.
can we keep this initial_joint_pose: list[float] type unchanged, and pre-define the list of joint name constants in matching order? I'd avoid unnecessary modification to existing API interface if possible
There was a problem hiding this comment.
Sure, it still takes initial_joint_pose: list[float]
|
|
||
| def set_initial_joint_pose(self, initial_joint_pose: list[float]) -> None: | ||
| self.event_config.init_franka_arm_pose.params["default_pose"] = initial_joint_pose | ||
| def set_initial_joint_pose(self, initial_joint_pose: Mapping[str, float]) -> None: |
| self, | ||
| enable_cameras: bool = False, | ||
| initial_pose: Pose | None = None, | ||
| initial_joint_pose: list[float] | None = None, |
There was a problem hiding this comment.
why remove this from init?
| """Return root-relative bounds computed from the articulation's USD geometry. | ||
| """Return root-relative bounds of the articulation posed at its configured joint positions. | ||
|
|
||
| Shared and cached across callers, so treat the result as read-only. |
There was a problem hiding this comment.
since _compute_local_bounding_box_from_usd_at_joint_pos results can stored in lru_caches and modification to it persist, it's best to make a copy of it in the compute_local_bounding_box_from_usd_at_joint_pos wrapper before returning, so we don't rely on the caller to not accidentally modify it.
There was a problem hiding this comment.
Done. The public wrapper now constructs a new bounding box, avoiding the chance of modifying it directly.
| return self._collision_mesh | ||
| """Return one mesh containing all posed link boxes.""" | ||
| # Import locally because USD/pxr is available only after simulation initialization. | ||
| from isaaclab_arena.utils.usd_helpers import extract_trimesh_from_usd_at_joint_pos |
There was a problem hiding this comment.
Addressed. It now returns mesh.copy()
| source = self.get_placement_geometry_source() | ||
| return extract_trimesh_from_usd_at_joint_pos(source.usd_path, source.joint_pos, source.scale) | ||
|
|
||
| def get_collision_meshes(self) -> tuple[trimesh.Trimesh, ...]: |
There was a problem hiding this comment.
I suggest we don't return a new API get_collision_meshes. we just overwrite the get_collision_mesh in droid/franka. also can we union the extract_link_bbox_meshes_from_usd_at_joint_pos before returning so it acts as one mesh?
There was a problem hiding this comment.
Done. get_collision_meshes is removed. Droid and Franka now override the existing get_collision_mesh
Signed-off-by: zhx06 <zihaox@nvidia.com>
Signed-off-by: zhx06 <zihaox@nvidia.com>
Signed-off-by: zhx06 <zihaox@nvidia.com>
Signed-off-by: zhx06 <zihaox@nvidia.com>
Signed-off-by: zhx06 <zihaox@nvidia.com>
204ab44 to
fb0fa80
Compare
qianl-nv
left a comment
There was a problem hiding this comment.
Some more nits.
Plz do one more pass in cleaning / simplying the util functions
|
|
||
|
|
||
| @dataclass(frozen=True) | ||
| class PlacementGeometrySource: |
There was a problem hiding this comment.
ArticulationGeometrySpec or EmbodimentGeometrySpec would be more accurate.
There was a problem hiding this comment.
Addressed. Renamed to ArticulationGeometrySpec
|
|
||
| """Forward kinematics over a USD articulation's physics joints. | ||
|
|
||
| Authored USD transforms only describe the one joint configuration an asset happens to be saved in, |
There was a problem hiding this comment.
Instead of explaining "how" the transform are computed, you need to explain why we need to implement this forward kinematics here (instead of re-using lab/curobo ect)
| assert stage is not None, f"could not open USD: {usd_path}" | ||
| default_prim = stage.GetDefaultPrim() or stage.GetPseudoRoot() | ||
| return extract_trimesh_from_prim(stage, default_prim.GetPath().pathString, scale) | ||
|
|
There was a problem hiding this comment.
let's add a code block separation here for better grouping as this file gets long
# -----------------------------------------------------------------------------
# Joint-posed articulation geometry helpers
# -----------------------------------------------------------------------------
There was a problem hiding this comment.
Addressed. Section separator added.
| return extract_trimesh_from_prim(stage, default_prim.GetPath().pathString, scale) | ||
|
|
||
|
|
||
| def extract_link_bbox_meshes_from_usd_at_joint_pos( |
There was a problem hiding this comment.
this wrapper is not used anymore, remove
There was a problem hiding this comment.
Addressed. This wrapper has been removed.
| # NOTE(zihaox, 2026-07-28): Cache here rather than on the asset. Isaac Lab reaches assets through | ||
| # EventTermCfg params, and configclass's validation walk tracks no visited set, so a trimesh held by | ||
| # an asset sends it recursing through trimesh's internal back-references until the stack overflows. | ||
| @functools.lru_cache(maxsize=_POSED_GEOMETRY_CACHE_SIZE) |
There was a problem hiding this comment.
since this is now only called from _extract_trimesh_from_usd_at_joint_pos which is also lru cached, we can remove the cache here.
There was a problem hiding this comment.
Addressed. The redundant inner cache was removed.
| return trimesh.util.concatenate(meshes) | ||
|
|
||
|
|
||
| def extract_trimesh_from_usd_at_joint_pos( |
There was a problem hiding this comment.
let's move the wrapper before _extract_trimesh_from_usd_at_joint_pos for better readibility
There was a problem hiding this comment.
Addressed. Moved the public extract_trimesh_from_usd_at_joint_pos wrapper before the private cached implementation.
|
|
||
|
|
||
| def _joint_edges( | ||
| stage: Usd.Stage, |
| if not edges: | ||
| return {} | ||
|
|
||
| rest_transforms = { |
There was a problem hiding this comment.
can reuse one UsdGeom.XformCache for body transforms to avoid repeatingly construct it in the for loop
can also inline and remove the one-use _local_to_world helper
xform_cache = UsdGeom.XformCache(Usd.TimeCode.Default())
rest_transforms: dict[str, np.ndarray] = {}
for path in {path for edge in edges for path in (edge.parent, edge.child) if path}:
prim = stage.GetPrimAtPath(path)
assert prim, f"Joint references a missing prim: {path}"
rest_transforms[path] = np.array(xform_cache.GetLocalToWorldTransform(prim), dtype=np.float64)
There was a problem hiding this comment.
Addressed. The changes have been applied as suggested.
| values_by_path = {joint_prims[name].GetPath().pathString: value for name, value in joint_pos.items()} | ||
|
|
||
| edges: list[_JointEdge] = [] | ||
| for prim in Usd.PrimRange(root_prim): |
There was a problem hiding this comment.
store root_sdf_path = Sdf.Path(root_path)
outside the for loop
There was a problem hiding this comment.
Addressed. he root path is now stored once as root_sdf_path and stays out of the for loop.
Signed-off-by: zhx06 <zihaox@nvidia.com>
Summary
Joint-dependent mesh/bbox for embodiments
Detailed description
Validation comparison
Droid Lightwheel RoboCasa kitchen — 50 candidates
Before: Droid bbox
After: Droid mesh