Add open-top object containment predicate

[cvolkcvolk]

Summary

Prevent false-positive object placements

Detailed description

• Why: Contact plus low velocity could previously report success while a pickup was touching the outside or side of its destination.
• Geometry: Require the pickup bounding-box centroid inside the destination-local horizontal bounds and above its bottom while leaving the top open.
• Support: Require filtered destination contact to point upward within a configurable cone and the pickup linear speed to remain below its threshold.
• Impact: Pick-and-place and sorting now use the stricter success criterion, including rotated references and per-environment object-set bounds.

[greptile-apps]

Greptile Summary

Adds open-top containment semantics to pick-and-place and sorting success predicates.

• Assigns per-environment object-set variants before relation solving so runtime predicates can use matching cached bounds.
• Preserves Arena asset identity through copied manager configurations with lightweight asset handles.
• Requires the pickup bounding-box centroid to lie within destination-local horizontal bounds and above its bottom.
• Requires destination-filtered contact force to provide upward support within a configurable cone while the object remains below the velocity threshold.
• Adds predicate coverage and updates sorting simulations for the new containment behavior.

Confidence Score: 5/5

The PR appears safe to merge, with no concrete correctness or security failures identified in the changed paths.

The new predicate consistently combines per-environment bounds, destination-local centroid containment, filtered upward support, and velocity checks, while task configurations provide the required asset identities and tests cover the principal geometry and force cases.

Important Files Changed

Filename	Overview
isaaclab_arena/environments/arena_env_builder.py	Assigns object-set variants before relation solving and scene configuration materialization.
isaaclab_arena/tasks/predicates/predicate_utils.py	Adds identity-preserving Arena asset handles and world-pose resolution for registered and nested assets.
isaaclab_arena/tasks/predicates/spatial.py	Implements open-top centroid containment, upward-support filtering, and the composed destination predicate.
isaaclab_arena/tasks/pick_and_place_task.py	Wires asset handles and configurable support-cone behavior into standard and Mimic success predicates.
isaaclab_arena/tasks/sorting_task.py	Supplies per-object and per-destination asset handles to multi-object sorting success evaluation.
isaaclab_arena/tests/test_object_in_container_predicate.py	Covers copied asset handles, transformed containment bounds, support-force cones, and combined success conditions.
isaaclab_arena/tests/test_sorting_task.py	Updates simulated object poses and quaternion ordering to exercise containment inside destination bounds.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  A[Assign per-environment asset variants] --> B[Read matching cached bounds]
  B --> C[Transform pickup centroid into destination-local frame]
  C --> D{Inside X/Y and above bottom?}
  E[Destination-filtered contact forces] --> F{Upward force within support cone?}
  G[Pickup linear velocity] --> H{Below threshold?}
  D --> I[Containment success]
  F --> I
  H --> I

Loading

_{Reviews (1): Last reviewed commit: "Add open-top object containment predicat..." | Re-trigger Greptile}

[arena-review-bot]

🔵 ArenaAssetHandle smuggles a live asset through config copies

This wrapper exists only to hold a live PlaceableAsset in a termination term's params and defeat deepcopy so the asset's cached bounds/variants survive config copying. Predicates elsewhere reference scene entities by name (SceneEntityCfg) and resolve them from env.scene at eval time, keeping params as plain data.

Is there a way to resolve the Arena asset (and its bounds) by name from the env/scene at eval time instead, so the term params stay plain data? If holding the object by identity is genuinely required, a short NOTE explaining why it must survive deepcopy would help the next reader.

[arena-review-bot]

🤖 Isaac Lab-Arena Review Bot

Summary

This PR replaces the old contact+velocity object_on_destination success check with an open-top containment predicate: the pickup object's bounding-box centroid must sit inside the destination's local X/Y and lower-Z bounds, filtered contact must push upward within a configurable support cone, and linear velocity must be low. The geometry is well factored and the new test_object_in_container_predicate covers the centroid transform, the ObjectReference parent-frame case, the support cone, and the combined predicate. Comments below are design questions, not blockers.

Note this changes the success criteria for all pick-and-place and sorting tasks (that is the point of the PR, and the sorting test was updated accordingly) — worth a heads-up for downstream eval baselines.

Findings

🟡 predicate_utils.py:75 — asset-type branching via getattr. get_asset_pose_w accepts a PlaceableAsset then switches on subtype with getattr(asset, "object_type"/"parent_asset"/"initial_pose_relative_to_parent", None) (repeated in spatial._get_bounding_box_pose_w). These are known names the asset classes own — capability detection for a missing interface. A polymorphic get_pose_w(env) on the asset types would move this logic to its owner and drop the getattr switching.

🔵 predicate_utils.py:24 — ArenaAssetHandle smuggles a live asset through config deepcopy. The wrapper only exists to keep a live PlaceableAsset in a term's params and defeat deepcopy. Could the asset (and its bounds) be resolved by name from the env/scene at eval time, keeping params plain data? If identity really must survive deepcopy, a NOTE on why would help.

🔵 spatial.py:227 — object_in_contact appears unused. No caller after the refactor; drop it or confirm it's an intended public building block.

Test Coverage

Good. The new test uses the inner/outer run_simulation_app_function pattern with deferred imports, asserts concrete boolean vectors across meaningful geometry cases, and verifies the ArenaAssetHandle deepcopy behavior. The sorting-task tests were updated to match the new containment semantics. New file carries a correct single-year (2026) header.

Verdict

Minor fixes needed — the design questions are worth a look, but nothing here blocks.