Introduce PlacementEntity by zhx06 · Pull Request #915 · isaac-sim/IsaacLab-Arena

zhx06 · 2026-07-15T17:27:30Z

Summary

Unify object and robot relation placement

Detailed description

Enables robot embodiments to use the existing relation solver.
Adds PlacementEntity and generalizes the placement pipeline from objects to entities.
Preserves existing object placement while adding single-root, bounding-box-based robot placement.

Signed-off-by: zhx06 <zihaox@nvidia.com>

greptile-apps · 2026-07-15T17:44:10Z

Greptile Summary

This PR introduces PlacementEntity as a shared base class for ObjectBase and EmbodimentBase, unifying the placement pipeline so robot embodiments can participate in the existing relation solver alongside scene objects. The key design is that _solve_relations() is called before get_scene_cfg(), allowing the embodiment's lazy scene-config update to pick up the solved pose.

New PlacementEntity base class consolidates initial_pose, relations, collision_mode, and the abstract bounding-box/mesh interface, removing duplication between ObjectBase and EmbodimentBase.
scene_entity_names indirection decouples the placement entity's logical name from its Isaac Lab scene name (e.g. "droid" → "robot"), with a new _apply_static_spawn_pose path for mixed pools where some entities cannot store per-environment poses.
Behavior hardening: layouts with missing non-anchor positions now raise AssertionError instead of printing a warning and silently skipping, matching the updated tests.

Confidence Score: 4/5

Safe 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

Filename	Overview
isaaclab_arena/relations/placement_entity.py	New base class centralising placement state (initial_pose, relations, collision_mode) that previously lived in ObjectBase; well-structured with clean abstract interface.
isaaclab_arena/embodiments/embodiment_base.py	Adds bounding-box and placement interface to EmbodimentBase; set_initial_pose asserts a narrower runtime type than its declared signature, violating the PlacementEntity contract.
isaaclab_arena/assets/object_base.py	Migrates shared placement state to PlacementEntity; adds set_placement_initial_pose that correctly patches object_cfg.init_state without triggering a new reset event.
isaaclab_arena/environments/relation_solver_interface.py	Adds scene_entity_names indirection for robot/embodiment name mapping; new _apply_static_spawn_pose path handles mixed entity pools correctly; scene_entity_names assertion is stricter than needed (exact equality vs subset).
isaaclab_arena/environments/arena_env_builder.py	Correctly adds embodiment relations to the placement pool and builds the scene_entity_names map; _solve_relations is called before get_scene_cfg, ensuring the solved pose is picked up by the embodiment's lazy scene-config update.
isaaclab_arena/relations/placement_events.py	Adds get_pose_from_layout helper and new place_entities_from_layouts reset event; write_layout_to_sim now validates all non-anchor entities are present (hard failure) rather than silently skipping missing positions.
isaaclab_arena/assets/dummy_embodiment.py	New test-only PlacementEntity for embodiment solver tests; correctly implements all abstract methods and returns False for supports_per_env_initial_pose.
isaaclab_arena/tests/test_relation_solver_embodiment.py	New test file covering single-env embodiment placement and expected assertion for batched placement without per-env pose support.
isaaclab_arena/tests/test_relation_solver_interface.py	Updated tests correctly reflect behavior changes: missing-position layouts now raise instead of warn, and new tests cover unique-name and complete-map assertions.

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}

greptile-apps · 2026-07-15T17:44:13Z

+    def set_initial_pose(self, pose: Pose | PoseRange | PosePerEnv) -> None:
+        """Set the embodiment root pose."""
+        assert isinstance(pose, Pose), "Embodiments require one root Pose"
        self.initial_pose = pose


set_initial_pose signature violates PlacementEntity interface contract

PlacementEntity.set_initial_pose declares pose: Pose | PoseRange | PosePerEnv, making all three types valid at the interface level. EmbodimentBase keeps the same overridden signature but immediately asserts isinstance(pose, Pose). A caller holding a PlacementEntity reference — e.g. code that calls entity.set_initial_pose(PoseRange(...)) — gets an unexpected AssertionError at runtime on any embodiment, with no type-checker warning. The method signature should be narrowed to pose: Pose so the constraint is visible at call sites and statically checkable.

greptile-apps · 2026-07-15T17:44:14Z

+    assert len(entity_names) == len(objects), "Placement entity names must be unique"
+    if scene_entity_names is None:
+        scene_entity_names = {obj.name: obj.name for obj in objects}
+    assert set(scene_entity_names) == entity_names, "scene_entity_names must contain every placement entity name"


The assertion requires exact set equality, so a caller supplying a superset scene_entity_names (e.g. passing a shared registry that covers more entities than the current pool) will always fail. A subset check — entity_names.issubset(set(scene_entity_names)) — preserves the required contract (every entity has a mapping) while allowing extra entries.

Suggested change

assert set(scene_entity_names) == entity_names, "scene_entity_names must contain every placement entity name"

assert entity_names.issubset(set(scene_entity_names)), "scene_entity_names must contain every placement entity name"

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

xyao-nv · 2026-07-16T05:27:04Z


    Args:
-        objects: Objects with spatial predicates that should be relation-solved.
+        objects: Entities with spatial predicates that should be relation-solved.


nit, rename objects to entities?

xyao-nv · 2026-07-16T05:28:55Z

+        return bounding_box.rotated_90_around_z(quarters).translated(self.initial_pose.position_xyz)
+
+    def get_collision_mesh(self) -> trimesh.Trimesh | None:
+        """Return no mesh because embodiment placement uses bounds."""


add a TODO as mesh shall also depend on initial joint pos

xyao-nv · 2026-07-16T05:37:51Z

+        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():


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?

xyao-nv · 2026-07-16T05:38:24Z

+        spawn = self.scene_config.robot.spawn
+        assert spawn.usd_path is not None, "scene_config.robot must use a USD spawn for placement"
+        scale = tuple(spawn.scale or (1.0, 1.0, 1.0))
+        # TODO: Compute bounds at configured initial joint positions when joint_pos is not None.


please include your name in the TODO

cvolkcvolk · 2026-07-16T07:02:06Z

This class seems to be used only by tests. Suggestion to move into the test directory

cvolkcvolk · 2026-07-16T07:02:33Z

Suggestion to move to tests/

cvolkcvolk · 2026-07-16T07:10:00Z

+
+    def set_initial_pose(self, pose: Pose | PoseRange | PosePerEnv) -> None:
+        """Set the embodiment root pose."""
+        assert isinstance(pose, Pose), "Embodiments require one root Pose"


pose: Pose | PoseRange | PosePerEnv seems to contradict "assert isinstance(pose, Pose)"?

cvolkcvolk · 2026-07-16T07:12:45Z

+        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:


Could we make this a default implementation on PlacementEntity, then remove the redundant no-op implementations from EmbodimentBase and ObjectBase?

cvolkcvolk · 2026-07-16T07:15:44Z

-            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"


The assert message does not match the actual assert

cvolkcvolk · 2026-07-16T07:15:56Z

-            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"


Suggested change

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"

cvolkcvolk · 2026-07-16T07:16:26Z

-        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"


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

cvolkcvolk · 2026-07-16T07:18:22Z

+    from isaaclab_arena.utils.bounding_box import AxisAlignedBoundingBox
+
+
+class PlacementEntity(Asset, ABC):


alexmillane

High level review #1

alexmillane · 2026-07-16T08:28:15Z

+    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


Why's this required?

Are there places where we want to set a pose that doesn't reset?

alexmillane · 2026-07-16T08:31:10Z

-    def set_initial_pose(self, pose: Pose) -> None:
+    def get_bounding_box(self) -> AxisAlignedBoundingBox:
+        """Return root-relative bounds for the USD-authored articulation pose."""
+        from isaaclab_arena.utils.usd_helpers import compute_local_bounding_box_from_usd


Is this import required to be local?

alexmillane · 2026-07-16T08:33:43Z

+    def get_collision_mesh(self) -> trimesh.Trimesh | None:
+        """Return no mesh because embodiment placement uses bounds."""


Should this print a warning or something? Is this triggered when a user requests that the embodiment is placed via mesh? If this is called on user request, I feel that we should error out. If returning None is just our way of indicating that a bounding box should be used, then it's fine as is.

alexmillane · 2026-07-16T08:38:47Z

          events restore the same layout every time.
        """
-        objects_with_relations = self.arena_env.scene.get_objects_with_relations()
+        placement_entities = list(self.arena_env.scene.get_objects_with_relations())


nit: out of interest why is the proceeding list required here. The function returns a list. I thought maybe the point of the list is to do a copy, but the function actually builds the returned list from scratch. I.e. you don't get a reference to some internal list you don't wanna modify.

alexmillane · 2026-07-16T08:40:54Z

          events restore the same layout every time.
        """
-        objects_with_relations = self.arena_env.scene.get_objects_with_relations()
+        placement_entities = list(self.arena_env.scene.get_objects_with_relations())


One issue with this MR is that we've introduced another term entities. We now have assets, objects and entities.

I feel that we should maybe only have two. In this case both objects and embodiments inherit from the Asset class. So perhaps we should call them assets?

alexmillane · 2026-07-16T08:45:10Z

+    entity_names = {obj.name for obj in objects}
+    assert len(entity_names) == len(objects), "Placement entity names must be unique"
+    if scene_entity_names is None:
+        scene_entity_names = {obj.name: obj.name for obj in objects}
+    assert set(scene_entity_names) == entity_names, "scene_entity_names must contain every placement entity name"


What is going on here?

We construct our own list of names, then check it against the (optionally) passed list of names and check that they're equal? Why bother passing the list of names at all if we extract it inside and expect to get exactly the same thing passed in?

alexmillane · 2026-07-16T08:49:57Z

+    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,
    )


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.

alexmillane · 2026-07-16T08:50:54Z

        params={
            "objects": objects,
            "placement_pool": placement_pool,
+            "scene_entity_names": scene_entity_names,


why is this now required?

alexmillane · 2026-07-16T08:53:09Z

+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,
+        },
+    )


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?

alexmillane · 2026-07-16T08:59:59Z

+    from isaaclab_arena.utils.bounding_box import AxisAlignedBoundingBox
+
+
+class PlacementEntity(Asset, ABC):


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?

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()

introduce PlacementEntity

5d7d1ae

Signed-off-by: zhx06 <zihaox@nvidia.com>

zhx06 requested review from alexmillane, cvolkcvolk, peterd-NV, qianl-nv, viiik-inside and xyao-nv as code owners July 15, 2026 17:27

zhx06 changed the title ~~introduce PlacementEntity~~ Introduce PlacementEntity Jul 15, 2026

greptile-apps Bot reviewed Jul 15, 2026

View reviewed changes

xyao-nv reviewed Jul 16, 2026

View reviewed changes

cvolkcvolk reviewed Jul 16, 2026

View reviewed changes

Comment thread isaaclab_arena/assets/dummy_object.py

cvolkcvolk Jul 16, 2026

Copy link
Copy Markdown

Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion to move to tests/

cvolkcvolk reviewed Jul 16, 2026

View reviewed changes

alexmillane reviewed Jul 16, 2026

View reviewed changes

	assert set(scene_entity_names) == entity_names, "scene_entity_names must contain every placement entity name"
	assert entity_names.issubset(set(scene_entity_names)), "scene_entity_names must contain every placement entity name"

	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"

		from isaaclab_arena.utils.bounding_box import AxisAlignedBoundingBox


		class PlacementEntity(Asset, ABC):

		def get_collision_mesh(self) -> trimesh.Trimesh \| None:
		"""Return no mesh because embodiment placement uses bounds."""

Uh oh!

Conversation

zhx06 commented Jul 15, 2026

Summary

Detailed description

Uh oh!

greptile-apps Bot commented Jul 15, 2026

Greptile Summary

Confidence Score: 4/5

Important Files Changed

Sequence Diagram

Uh oh!

greptile-apps Bot Jul 15, 2026

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Jul 15, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xyao-nv Jul 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexmillane left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

xyao-nv Jul 16, 2026 •

edited

Loading