Introduce PlacementAsset#915
Conversation
Greptile SummaryThis PR introduces
Confidence Score: 4/5Safe to merge; the core placement ordering (solve before get_scene_cfg) is correct and well-tested. One method override has a narrower runtime type than its declared signature. The placement pipeline is well-structured and the ordering guarantee (solve before scene build) correctly handles the embodiment's lazy config update. The one concrete defect is that EmbodimentBase.set_initial_pose asserts isinstance(pose, Pose) but keeps the broader Pose | PoseRange | PosePerEnv signature from its parent, so any caller using the PlacementEntity interface with a PoseRange or PosePerEnv argument would get a silent runtime AssertionError on embodiments. The current placement pipeline avoids this via supports_per_env_initial_pose(), but the type contract is broken and future callers could hit it unexpectedly. isaaclab_arena/embodiments/embodiment_base.py — the set_initial_pose override Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Builder as ArenaEnvBuilder
participant RSI as relation_solver_interface
participant Pool as PooledObjectPlacer
participant PE as PlacementEntity
participant Events as placement_events
Builder->>RSI: solve_and_apply_relation_placement(entities, scene_entity_names)
RSI->>Pool: sample_with_replacement(num_envs)
Pool-->>RSI: layouts[]
RSI->>PE: set_placement_initial_pose(pose) [env 0 seed]
Note over PE: ObjectBase patches object_cfg.init_state<br/>EmbodimentBase updates self.initial_pose only
RSI-->>Builder: EventTermCfg (reset event)
Builder->>PE: embodiment.get_scene_cfg()
Note over PE: Lazily applies self.initial_pose<br/>to scene_config.robot.init_state
Note over Builder: Scene built with solved poses
loop On every reset
Events->>Events: solve_and_place_objects / place_entities_from_layouts
Events->>PE: write_root_pose_to_sim(pose, env_ids)
end
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant Builder as ArenaEnvBuilder
participant RSI as relation_solver_interface
participant Pool as PooledObjectPlacer
participant PE as PlacementEntity
participant Events as placement_events
Builder->>RSI: solve_and_apply_relation_placement(entities, scene_entity_names)
RSI->>Pool: sample_with_replacement(num_envs)
Pool-->>RSI: layouts[]
RSI->>PE: set_placement_initial_pose(pose) [env 0 seed]
Note over PE: ObjectBase patches object_cfg.init_state<br/>EmbodimentBase updates self.initial_pose only
RSI-->>Builder: EventTermCfg (reset event)
Builder->>PE: embodiment.get_scene_cfg()
Note over PE: Lazily applies self.initial_pose<br/>to scene_config.robot.init_state
Note over Builder: Scene built with solved poses
loop On every reset
Events->>Events: solve_and_place_objects / place_entities_from_layouts
Events->>PE: write_root_pose_to_sim(pose, env_ids)
end
Reviews (1): Last reviewed commit: "introduce PlacementEntity" | Re-trigger Greptile |
|
|
||
| Args: | ||
| objects: Objects with spatial predicates that should be relation-solved. | ||
| objects: Entities with spatial predicates that should be relation-solved. |
There was a problem hiding this comment.
nit, rename objects to entities?
| placement_entities = list(self.arena_env.scene.get_objects_with_relations()) | ||
| scene_entity_names = {entity.name: entity.name for entity in placement_entities} | ||
| embodiment = self.arena_env.embodiment | ||
| if embodiment is not None and embodiment.get_relations(): |
There was a problem hiding this comment.
If we are modeling single-arm manipulator on a stand as a 3D bbox, does it mean it will put the robot far away from the table that may be even hard to reach? E.g. the stand footprint of droid is pretty large.
How's nextTo(Robot) being defined here? Using a boxy AABB edge?
On(Robot)? a footprint on the top face of the robot?
Shall we have some restrictions/definitions of embodiment-related relations?
|
|
||
| def set_initial_pose(self, pose: Pose | PoseRange | PosePerEnv) -> None: | ||
| """Set the embodiment root pose.""" | ||
| assert isinstance(pose, Pose), "Embodiments require one root Pose" |
There was a problem hiding this comment.
pose: Pose | PoseRange | PosePerEnv seems to contradict "assert isinstance(pose, Pose)"?
| quarters = quaternion_to_90_deg_z_quarters(self.initial_pose.rotation_xyzw) | ||
| return bounding_box.rotated_90_around_z(quarters).translated(self.initial_pose.position_xyz) | ||
|
|
||
| def get_collision_mesh(self) -> trimesh.Trimesh | None: |
There was a problem hiding this comment.
Could we make this a default implementation on PlacementEntity, then remove the redundant no-op implementations from EmbodimentBase and ObjectBase?
| raise RuntimeError("scene_config must be populated with a `robot` before calling `set_robot_initial_pose`.") | ||
| scene_config.robot.init_state.pos = pose.position_xyz | ||
| scene_config.robot.init_state.rot = pose.rotation_xyzw | ||
| assert scene_config is not None, "scene_config.robot must be populated before setting the root pose" |
There was a problem hiding this comment.
The assert message does not match the actual assert
| raise RuntimeError("scene_config must be populated with a `robot` before calling `set_robot_initial_pose`.") | ||
| scene_config.robot.init_state.pos = pose.position_xyz | ||
| scene_config.robot.init_state.rot = pose.rotation_xyzw | ||
| assert scene_config is not None, "scene_config.robot must be populated before setting the root pose" |
There was a problem hiding this comment.
| assert scene_config is not None, "scene_config.robot must be populated before setting the root pose" | |
| assert scene_config is not None, "scene_config must be populated before setting the root pose" |
| scene_config.robot.init_state.rot = pose.rotation_xyzw | ||
| assert scene_config is not None, "scene_config.robot must be populated before setting the root pose" | ||
| robot = scene_config.robot | ||
| assert robot is not None, "scene_config.robot must be populated before setting the root pose" |
There was a problem hiding this comment.
Nit, can directly do assert scene_config.robot is not None, "scene_config.robot must be populated before setting the root pose",as the declared variable is not directly used
alexmillane
left a comment
There was a problem hiding this comment.
High level review #1
| def set_placement_initial_pose(self, pose: Pose) -> None: | ||
| """Set a solved root pose without configuring an independent reset.""" | ||
| self.initial_pose = pose | ||
| if self.object_cfg is not None: | ||
| self.object_cfg.init_state.pos = pose.position_xyz | ||
| self.object_cfg.init_state.rot = pose.rotation_xyzw |
There was a problem hiding this comment.
Why's this required?
Are there places where we want to set a pose that doesn't reset?
There was a problem hiding this comment.
It is designed for robots that does not have object_cfg
| if all(obj.supports_per_env_initial_pose() for obj in objects): | ||
| _apply_static_initial_poses( | ||
| objects=objects, | ||
| placement_pool=placement_pool, | ||
| anchor_objects_set=anchor_objects_set, | ||
| num_envs=num_envs, | ||
| ) | ||
| return None | ||
| return _apply_static_spawn_pose( | ||
| objects=objects, | ||
| placement_pool=placement_pool, | ||
| anchor_objects_set=anchor_objects_set, | ||
| num_envs=num_envs, | ||
| scene_entity_names=scene_entity_names, | ||
| ) |
There was a problem hiding this comment.
What's the difference in behaviour between these two branches?
Like, how does the generated scene differ? In the second case, do we, for example, lose different poses between the environments? That would be a big change in behaviour... Probably something that we don't want to do.
Could you explain what's going on here. I would add such an explanation as a (lengthy) comment. However, let's agree here on what's going on and what should happen first.
There was a problem hiding this comment.
Both branches preserve per-environment variation. The reason is that objects and embodiments currently use different reset mechanisms.
The first branch handles object-only scenes, where each object’s reset event stores its per-environment pose.
The second handles scenes containing an embodiment. Robot configs store only one construction pose, so a reset event writes each environment’s saved layout at reset.
There was a problem hiding this comment.
if we need to handle object vs embodiment differently at relation_solver_interface.py level, it feels like we are missing some abstraction layers at PlacementAsset/EmbodimentBase class so we can unify the interface with ObjectBase. The root cause appears to be embodiment's rejecting PosePerEnv. Is this something we can extend the EmbodimentBase to support, so we provide one unified the per-env & reset pose behavior for object and embodiment?
There was a problem hiding this comment.
Addressed. The new commit unify these 2 classes. One remaining thing is how to deal with the stand of the driod.
| def _apply_static_spawn_pose( | ||
| objects: list[PlacementEntity], | ||
| placement_pool: PooledObjectPlacer, | ||
| anchor_objects_set: set[PlacementEntity], | ||
| num_envs: int, | ||
| scene_entity_names: Mapping[str, str], | ||
| ) -> EventTermCfg: | ||
| """Return a reset event that restores one fixed layout per environment.""" | ||
| from isaaclab.managers import EventTermCfg | ||
|
|
||
| layouts = placement_pool.sample_with_replacement(num_envs) | ||
| _set_placement_initial_poses(objects, anchor_objects_set, layouts[0]) | ||
| return EventTermCfg( | ||
| func=place_entities_from_layouts, | ||
| mode="reset", | ||
| params={ | ||
| "objects": objects, | ||
| "layouts": layouts, | ||
| "scene_entity_names": scene_entity_names, | ||
| }, | ||
| ) |
There was a problem hiding this comment.
Ok this is related to my comment above. Seems like we're introducing new behaviour that is degraded from what was there in some circumstances.
It seems like now, we may end up with the same poses per environment, rather than the varied poses we had before. That is a big change in behaviour no?
Is that coming because we can't set the robot pose per-env?
There was a problem hiding this comment.
The first layout is used only to initialize the single-pose scene configuration during construction. The poses will be applied separately for each environment.
| from isaaclab_arena.utils.bounding_box import AxisAlignedBoundingBox | ||
|
|
||
|
|
||
| class PlacementEntity(Asset, ABC): |
There was a problem hiding this comment.
As I commented above, I'm not sure we should introduce the term Entity, because then we'll have three overlapping things Assets Objects and Entities.
This class is always intended to be combined with something deriving from Asset no? In that case this class is a "mixin" to add functionality for placement no? What do you think about calling this class Placeable. That indicates that this class adds place-ability to another object?
There was a problem hiding this comment.
Agree with the naming. Entitiy is quite vague as well. Placeable would be a good fit, however already taken by one of our affordances IsaacLab-Arena/isaaclab_arena/affordances/placeable.py
Arguable Placeable would be a better fit for this use case here as the current Placeable affordance is just checking for orientation via place_upright()
There was a problem hiding this comment.
I have renamed the class to be PlacementAsset.
bfdfee6 to
62d990d
Compare
62d990d to
cc32f4e
Compare
f856cda to
ce534d7
Compare
alexmillane
left a comment
There was a problem hiding this comment.
Looks pretty much ready to go. A few small comments and questions about pose-setting.
| def set_spawn_pose(self, pose: Pose) -> None: | ||
| """Set the scene-construction pose without rebuilding the reset event.""" | ||
| assert self.object_cfg is not None, "object_cfg must be initialized before setting the spawn pose" | ||
| self._set_pose_state(pose) |
There was a problem hiding this comment.
Can we return this to a single function with a flag for create_reset_event=True
Right now, these three names: set_initial_pose, _set_pose_state, and set_spawn_pose are all basically synonyms. One cannot read the names and know what the difference is.
If we have a single function called set_initial_pose with create_reset_event=True, it's super clear to the reader what is going on.
There was a problem hiding this comment.
Addressed. We now have set_initial_pose(pose, create_reset_event=True). _set_pose_state, and set_spawn_pose are gone and set_initial_pose is the public interface.
| def set_initial_pose(self, pose: Pose | PosePerEnv) -> None: | ||
| """Store the requested base pose(s), lifted by the stand-height offset to match the spawned base.""" | ||
| if isinstance(pose, PosePerEnv): | ||
| super().set_initial_pose(PosePerEnv(poses=[p.translate(self._robot_base_offset) for p in pose.poses])) | ||
| else: | ||
| super().set_initial_pose(pose.translate(self._robot_base_offset)) | ||
|
|
||
| def set_spawn_pose(self, pose: Pose) -> None: | ||
| """Set the scene-construction base pose, lifted by the stand-height offset like ``set_initial_pose``.""" | ||
| super().set_spawn_pose(pose.translate(self._robot_base_offset)) |
There was a problem hiding this comment.
Am I correct that this function just calls the underlying function, except that it first translates the pose by the self._robot_base_offset?
I think it probably makes sense we eventually put the stand into the USD with the robot to skip this backflipping here.
For the time being, let's go with this but:
def _translate_pose(pose: Pose | PosePerEnv):
if isinstance(pose, PosePerEnv):
translated_pose = PosePerEnv(poses=[p.translate(self._robot_base_offset) for p in pose.poses])
else:
translated_pose = pose.translate(self._robot_base_offset)
return translated_pose
def set_initial_pose(self, pose: Pose | PosePerEnv)
pose = _translate_pose(pose)
super().
There was a problem hiding this comment.
Yes, that's correct. We now have _translate_pose and set_initial_pose for droid in droid.py, and add a TODO for adding stand into the USD.
| * **False** — assigns one fixed layout per environment. Object-only | ||
| scenes use per-object reset events; scenes with an embodiment use one | ||
| coordinated reset event. |
There was a problem hiding this comment.
"Object-only scenes use per-object reset events; scenes with an embodiment use one coordinated reset event."
Is there any way we can unify this behaviour?
There was a problem hiding this comment.
Yes, their behaviors are unified now. Each non-anchor asset stores its per-env pose and owns its own reset event.
| def get_collision_mesh(self) -> trimesh.Trimesh | None: | ||
| """Return this asset's collision mesh, or ``None`` if it has none.""" |
There was a problem hiding this comment.
No, it is not. None here means "No collision mesh, fall back to the AABB." It is supposed to stay concrete.
| def get_initial_pose(self) -> Pose | PoseRange | PosePerEnv | None: | ||
| """Return the configured root pose.""" | ||
| return self.initial_pose | ||
|
|
||
| def set_initial_pose(self, pose: Pose | PoseRange | PosePerEnv) -> None: | ||
| """Set the configured root pose. | ||
|
|
||
| Accepts a fixed ``Pose``, a ``PoseRange`` (randomized on reset), or a ``PosePerEnv`` | ||
| (a distinct pose per environment); the interpretation is subclass-specific. | ||
| """ | ||
| self.initial_pose = pose | ||
|
|
||
| def set_spawn_pose(self, pose: Pose) -> None: | ||
| """Set the root pose used when constructing the scene.""" | ||
| self.set_initial_pose(pose) |
There was a problem hiding this comment.
These functions appear to me to be duplicated (overridden) in the classes that this class is mixed into.
Is there a reason for that? It would be amazing if we could centralize the treatment of poses here.
Also there appears to me to be a bit of duplication between ObjectBase and EmbodimentBase. Another reason to centralize the implementation here.
Is there anything preventing us from doing that?
There was a problem hiding this comment.
Yeah, there was some duplicated code here. In the newest commit, all pose-related functions share the same interface in PlaceableAsset. ObjectBase/EmbodimentBase only override the two different methods according to the actual needs.
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>
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>
ee9f659 to
dedf07a
Compare
Signed-off-by: zhx06 <zihaox@nvidia.com>
Signed-off-by: zhx06 <zihaox@nvidia.com>
Summary
Unify object and robot relation placement
Detailed description
PlacementEntityand generalizes the placement pipeline from objects to entities.