Heterogenous Object Placement#676
Conversation
Extends the existing PooledObjectPlacer with per-variant sub-pools for heterogeneous objects (e.g. RigidObjectSet with varying bboxes). Keeps the peer-reviewed API from placement-on-reset unchanged: sample_without_replacement() gains an optional env_ids parameter, sample_with_replacement() routes to variant-aware sampling internally. Made-with: Cursor
- Assert random_choice=False when heterogeneous_bbox=True in RigidObjectSet - Add fallback in heterogeneous pool when no candidates pass validation - Add PooledObjectPlacer heterogeneous mode tests - Remove unnecessary list() defensive copy Made-with: Cursor
zhx06
requested review from
cvolkcvolk,
peterd-NV,
qianl-nv and
viiik-inside
as code owners
![isaaclab-review-bot[bot]](https://avatars.githubusercontent.com/u/157846462?s=60&v=4){kind=small}
There was a problem hiding this comment.
🤖 Isaac Lab Review Bot — PR #676 Review
Summary
This PR adds heterogeneous object placement support, enabling per-environment bounding boxes for RigidObjectSet with varying object variants. This is a well-structured change that properly separates homogeneous and heterogeneous placement paths.
Overall Assessment: ✅ Approve with minor suggestions
Architecture & Design ✅
The two-path approach (_place_homogeneous / _place_heterogeneous) is clean and maintains backward compatibility. Key design decisions:
- Per-env pool storage replaces the single layout list — correct for env-specific geometry
- Variant indices are sampled once and cached — avoids spawning/bbox mismatches
has_env_specific_bboxesflag enables O(1) path detection
Strengths 💪
-
Comprehensive test coverage (568 lines) covering edge cases like:
- Mixed heterogeneous/homogeneous scenes
- Multi-set with different variant counts
- Pool refill behavior
Invalid layout rejection→ Now fallback behavior with warnings
-
Proper On-pair exclusion from no-overlap loss — fixes false positives when child sits on parent
-
Backward compatible — existing homogeneous code paths unchanged
-
Clean type annotations throughout
🆕 Update: e97df23 Changes Reviewed
Key behavioral change: Invalid layouts now fall back to best-loss results instead of raising exceptions. This improves robustness for environments with imperfect validation.
Changes in this push:
object_set.py: Fixed contact sensor cfg to use canonical first member USD path (bugfix)placement_events.py: Added env count assertion, removed pre-pass validation check (deferred to pool)pooled_object_placer.py: Addedhad_fallbacksproperty and graceful fallback path when no candidates pass strict validationtest_*.py: Updated tests to match new fallback behavior (rejects → accepts with warning)gr1_table_multi_object_no_collision_environment.py: Added TODO for residual bouncing issue
Assessment: These are sensible refinements. The fallback approach is more forgiving for existing environments while still logging warnings. The num_envs assertion provides better error messages for mismatched pool/scene configurations.
Minor Suggestions 📝
See inline comments below.
CI Status
⏳ Pre-commit checks pending
Reviewed by Isaac Lab Review Bot — Updated for e97df23
| self._layout_pools[cur_env] = self._layout_pools[cur_env][idx:] | ||
| self._layout_cursors[cur_env] = 0 | ||
|
|
||
| def _solve_and_store(self, num_layouts: int) -> None: |
There was a problem hiding this comment.
Suggestion: Consider adding a docstring to _solve_and_store explaining the bounded-refill loop behavior. The max_solve_batches limit is important for preventing unbounded loops but the reasoning isn't immediately clear:
def _solve_and_store(self, num_layouts: int) -> None:
"""Solve layouts until every env has sufficient unread layouts.
Bounded by max_placement_attempts to avoid infinite loops when
the solver consistently produces invalid placements for certain envs.
"""| finite_losses = [candidate.loss for candidate in all_candidates if math.isfinite(candidate.loss)] | ||
| mean_loss = sum(finite_losses) / len(finite_losses) if finite_losses else float("inf") | ||
| n_valid = sum(1 for candidate in selected if candidate.is_valid) | ||
| print( |
There was a problem hiding this comment.
Minor: The candidate_bboxes construction could benefit from a brief comment explaining the repeat_interleave pattern:
# Tile each env's bbox max_attempts times so candidates[env*max_attempts : (env+1)*max_attempts]
# all share the same geometry.This makes the indexing relationship clearer for future maintainers.
|
|
||
| Unlike ``get_bounding_box()`` (which uses a max-z heuristic), this | ||
| returns the real local bbox of the variant assigned to each env, | ||
| enabling correct collision-free placement for heterogeneous scenes. |
There was a problem hiding this comment.
Good defensive check: The length mismatch validation here is helpful. Consider also validating that variant_indices_by_env elements are actually integers (though the type hint suggests this, runtime data from configs could violate it).
| results_by_env = dict(zip(reset_env_ids, reset_results)) | ||
|
|
||
| # PooledObjectPlacer stores only successful layouts; this pre-pass keeps | ||
| # reset writes atomic if a custom pool or future regression violates that invariant. |
There was a problem hiding this comment.
Good safety check: The pre-pass validation that raises for invalid layouts is an important safety net. This prevents partial state corruption if the pool somehow returns bad data.
| @@ -0,0 +1,568 @@ | |||
| # Copyright (c) 2025-2026, The Isaac Lab Arena Project Developers (https://github.com/isaac-sim/IsaacLab-Arena/blob/main/CONTRIBUTORS.md). | |||
There was a problem hiding this comment.
Excellent test coverage 👍 This 568-line test file covers:
- Basic bbox-per-env expansion
- Solver with env_bboxes
- Heterogeneous ObjectPlacer path
- Mixed hetero/homo scenes
- Pool sampling strategies
- Multi-set with different variant counts
- Refill triggers
- Complete env round consumption
The HeterogeneousDummyObject test double is clean and well-documented.
| @@ -134,6 +164,7 @@ def _compute_no_overlap_loss(self, state: RelationSolverState, debug: bool = Fal | |||
| Args: | |||
There was a problem hiding this comment.
Good fix: Skipping On-pairs in the no-overlap loss prevents the solver from fighting against valid child-on-parent placements. The on_pairs set with bidirectional entries handles this cleanly.
| @@ -37,24 +51,61 @@ | |||
| # objects. Better initialization strategies and constraining unchanged pose dimensions | |||
| # are needed in the near future. | |||
|
|
|||
There was a problem hiding this comment.
Documentation: The HETERO_VARIANT_SETS and HETERO_FIXED_OBJECTS constants are well-organized. Consider adding a brief comment explaining that these are sourced from the het-viz branch for reproducibility.
| scale = (1.0, 1.0, 0.7) | ||
| spawn_cfg_addon = { | ||
| "rigid_props": sim_utils.RigidBodyPropertiesCfg(kinematic_enabled=True), | ||
| } |
There was a problem hiding this comment.
Good approach: Moving the office table to Background class addresses the bouncing objects issue mentioned in the PR description. The kinematic_enabled=True setting prevents physics interactions.
Greptile SummaryThis PR adds heterogeneous object placement support — where each simulation environment can be assigned a different variant of a
Confidence Score: 5/5The change is safe to merge — the solver, placer, and pool refactors are well-structured and the heterogeneous path is exercised by a thorough new test suite. All findings are non-blocking quality notes. The core mechanics — per-env bbox tiling, the two sampling strategies, on-pair skipping in the no-overlap loss, and the placement event's env-indexed branch — are logically correct.
Important Files Changed
Reviews (2): Last reviewed commit: "address comments" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if placement_pool.requires_env_indexed_layouts: | ||
| all_results = placement_pool.sample_without_replacement(env.scene.env_origins.shape[0]) | ||
| results_by_env = {cur_env: all_results[cur_env] for cur_env in reset_env_ids} |
There was a problem hiding this comment.
When requires_env_indexed_layouts=True, every reset event calls sample_without_replacement(total_envs), advancing every env pool cursor by one regardless of how many envs are actually resetting. In RL with staggered episode endings (e.g., 1 env resetting out of 100), 99 layouts are wasted per reset, accelerating pool exhaustion. The reusable else branch (line 56) correctly draws only len(reset_env_ids) layouts.
A sample_for_env_ids(reset_env_ids) method on PooledObjectPlacer that draws only from the specific env pools being reset would remove the forced full-round coupling.
| self._compact() | ||
| self._discard_consumed_layouts() | ||
| target_per_env = max(1, (num_layouts + self._num_envs - 1) // self._num_envs) | ||
| max_solve_batches = max(1, self._placer.params.max_placement_attempts) |
There was a problem hiding this comment.
max_placement_attempts controls two independent knobs simultaneously
max_placement_attempts is already consumed inside each place() call as the per-result candidate multiplier (num_candidates = max_attempts * num_results). Reusing it here as the outer refill-batch retry cap means a user who sets max_placement_attempts=1 to reduce per-call solver cost also silently caps the pool refill loop to a single batch. A valid but geometrically constrained scene that normally needs 2-3 retry batches to fill will raise RuntimeError, even though the scene is solvable.
Consider a dedicated max_refill_batches parameter on PooledObjectPlacer rather than reusing max_placement_attempts.
| for cur_env, pool in self._layout_pools.items(): | ||
| if not pool: | ||
| raise RuntimeError( | ||
| f"Placement pool failed to produce any valid layouts for env {cur_env} " | ||
| f"from {pool_size} attempts. Check object relations and constraints." | ||
| ) |
There was a problem hiding this comment.
_solve_and_store
_solve_and_store either raises RuntimeError before returning (when it cannot reach target_per_env layouts after all retry batches) or returns only once every env pool has ≥ target_per_env ≥ 1 layouts. The if not pool check on line 72 is therefore unreachable in the success path and should be removed or replaced with an assert.
| print( | ||
| "Warning: --objects with --mode heterogeneous wraps each object as a " | ||
| "single-variant set (no per-env variance). Use default sets for true heterogeneity." | ||
| ) |
There was a problem hiding this comment.
warnings.warn() instead of print() so callers can filter, suppress, or escalate the message using Python's standard warnings machinery.
| print( | |
| "Warning: --objects with --mode heterogeneous wraps each object as a " | |
| "single-variant set (no per-env variance). Use default sets for true heterogeneity." | |
| ) | |
| import warnings | |
| warnings.warn( | |
| "--objects with --mode heterogeneous wraps each object as a " | |
| "single-variant set (no per-env variance). Use default sets for true heterogeneity.", | |
| UserWarning, | |
| stacklevel=2, | |
| ) |
1 participant
Summary
Add heterogeneous object placement support
Detailed description
RigidObjectSet.ObjectPlacer,RelationSolver, andPooledObjectPlacerto solve, validate, pool, and reset with env-specific bounding boxes.--mode heterogeneousto the GR1 table example with stable Robolab object sets.