Split object provenance from rooted transforms - #1112
Conversation
Separate spawned and referenced prim behavior from root-transform state handling while preserving the existing Object and ObjectReference API names. BREAKING CHANGE: - Direct ObjectBase subclasses must implement get_object_pose and set_object_pose, or compose RootedTransform. - ObjectBase.set_prim_path() has been removed. Prim paths must now be supplied during object construction and remain immutable afterward. Signed-off-by: Qian Lin <qianl@nvidia.com>
Greptile SummaryThis PR separates object provenance from shared rooted-transform behavior while preserving the scene-object configuration lifecycle.
Confidence Score: 5/5The PR appears safe to merge, with no concrete changed-code failures identified. The new mixin composition retains concrete configuration initialization and existing parent-relative reference behavior while adding the intended spawner-backed contact-sensor path. Important Files Changed
Class Diagram%%{init: {'theme': 'neutral'}}%%
classDiagram
class ObjectBase {
<<abstract>>
+prim_path
+object_type
+get_object_pose()
+set_object_pose()
}
class RootedTransform {
+set_initial_velocity()
+get_contact_sensor_cfg()
+get_object_pose()
+set_object_pose()
-_build_reset_event()
-_init_object_cfg()
}
class SpawnPrim {
+usd_path
+spawner_cfg
-_get_cfg_source_kwargs()
-_get_contact_sensor_prim_path()
}
class ReferencedPrim {
+parent_asset
-_get_cfg_source_kwargs()
-_resolve_prim_path_in_parent_usd()
}
class Object
class ObjectReference
ObjectBase <|-- Object
RootedTransform <|-- Object
SpawnPrim <|-- Object
ObjectBase <|-- ObjectReference
RootedTransform <|-- ObjectReference
ReferencedPrim <|-- ObjectReference
Reviews (1): Last reviewed commit: "Split object provenance from rooted tran..." | Re-trigger Greptile |
![arena-review-bot[bot]](https://avatars.githubusercontent.com/u/195794629?s=60&v=4){kind=small}
|
|
||
| def set_prim_path(self, prim_path: str) -> None: | ||
| self.prim_path = prim_path | ||
| def resolve_object_cfg(self, physics_preset: object | None = None): |
There was a problem hiding this comment.
🟡 resolve_object_cfg ignores its physics_preset argument
This new method accepts physics_preset but never reads it, and I couldn't find any caller — the scene builder still pulls configs via get_object_cfg(). Is it needed for this PR, or could it wait for the MR that actually consumes a preset? A parameter that's silently dropped is easy to mis-call, and it looks a bit outside the "split provenance from transform" scope.
|
|
||
| def _resolve_prim_path_in_parent_usd(self, parent_stage: Usd.Stage) -> str: | ||
| """Return the cached referenced path, resolving it for partially initialized test objects.""" | ||
| return getattr(self, "_prim_path_in_parent_usd", None) or self.isaaclab_prim_path_to_original_prim_path( |
There was a problem hiding this comment.
🔵 getattr fallback for a constructor-set attribute
_init_referenced_prim always sets _prim_path_in_parent_usd, so for a fully built reference this is just self._prim_path_in_parent_usd. The getattr(..., None) default and the recompute branch exist only for the "partially initialized test objects" the docstring mentions. Could those tests construct the object fully (or __init__ seed the attribute) so production code doesn't carry the fallback?
🤖 Isaac Lab-Arena Review BotSummaryClean refactor that splits object provenance ( Findings🟡 Warning: object_base.py:48 — Test CoverageThe added Phase-1 case (spawner-backed source/destination, VerdictMinor fixes needed |
|
|
||
| def set_prim_path(self, prim_path: str) -> None: | ||
| self.prim_path = prim_path | ||
| def resolve_object_cfg(self, physics_preset: object | None = None): |
There was a problem hiding this comment.
this is a new API introduced in this MR? remove it
| """Return the root prim used for contact sensing by default.""" | ||
| return self.prim_path | ||
|
|
||
| def get_object_cfg(self) -> tuple[str, object]: |
There was a problem hiding this comment.
should we still use the explicit type RigidObjectCfg | ArticulationCfg | AssetBaseCfg here?
| self, contact_against_object: ObjectBase | None = None, usd_path: str | None = None | ||
| ) -> ContactSensorCfg: | ||
| assert self.object_type == ObjectType.RIGID, "Contact sensor is only supported for rigid objects" | ||
| # We override this function from the parent class because in some assets, the rigid body |
There was a problem hiding this comment.
check if comment still applies
| rigid_body_relative_path is not None | ||
| ), f"No rigid body found in {self.name} USD file: {usd_path}. Can't add contact sensor." | ||
| contact_sensor_prim_path = self.prim_path + rigid_body_relative_path | ||
| # There are also cases where the contact against object does not have its rigid body at the root. |
There was a problem hiding this comment.
check if comment still applies
| return extract_trimesh_from_prim(parent_stage, prim_path_in_usd, self._parent_scale) | ||
|
|
||
| def get_contact_sensor_cfg(self, contact_against_object: ObjectBase | None = None) -> ContactSensorCfg: | ||
| # NOTE(alexmillane): Right now this requires that the object |
There was a problem hiding this comment.
check if comment still applies
1 participant
Summary
Separate object provenance and transform behavior.
Detailed description
ObjectandObjectReferencefrom provenance-specific behavior and sharedRootedTransform.ObjectBase.set_prim_path()and require direct subclasses to provide transform behavior.