Heterogenous Object Placement#676
Conversation
zhx06
requested review from
alexmillane,
cvolkcvolk,
peterd-NV,
qianl-nv,
viiik-inside and
xyao-nv
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 Updated 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_heterogeneous_objectsflag enables O(1) path detection- On-pair exclusion from no-overlap loss prevents false positives
Strengths 💪
-
Comprehensive test coverage (600+ lines) covering edge cases like:
- Mixed heterogeneous/homogeneous scenes
- Multi-set with different variant counts
- Pool refill behavior
- 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: Commits Since Previous Review
Commits reviewed: 0ebd50ca → 569a1e91 (9 commits)
Changes in Recent Commits:
1. 594587a0 - address new feedback
- Minor refinements to comments and code style
2. 8eeab82e - add bbox guard
- Added guard condition for bounding box access safety
3. a4450e84 - separate anchor behavior
- Improved separation of anchor object handling in placement logic
4. 2af2ae43 - address comments
- Documentation and comment improvements
5. f668f514 - add bbox helper, edit comments
- Extracted bbox helper functions for cleaner code organization
- Updated comments for clarity
6. 389acab6 - edit comments style
- Documentation style consistency pass
7. beaef723 - fix bug and change names
- Bug fix in placement logic
- Variable renaming for better readability
8. ae97f6dc - revert redundant changes
- Removed unnecessary changes that were causing issues
9. 569a1e91 - address comments
- Final round of comment/documentation improvements
Assessment of Recent Changes ✅
The recent commits show a pattern of iterative refinement based on review feedback:
- Code organization has improved (bbox helpers extracted)
- Comments and documentation are now more consistent
- Bug fixes have been applied appropriately
- No new architectural concerns introduced
CI Status
⏳ Pre-commit checks pending
Previous Review Comments Status
The main bot review and incremental updates (through ecec090) covered:
- Per-env bbox architecture
- Pool split implementation
- No-overlap solver changes
- Fallback behavior for invalid layouts
- Mutable default argument fixes
All prior concerns have been addressed in subsequent commits.
Minor Observations (Non-blocking)
-
Test coverage is strong — The 600+ lines of tests in
test_heterogeneous_placement.pyand related files provide good confidence in the implementation. -
Documentation quality — The docstrings and comments have been progressively improved through the commit history.
-
Kinematic table background — The
OfficeTableBackgroundwithkinematic_enabled=Trueis a good fix for object/table bouncing issues.
Reviewed by Isaac Lab Review Bot — Updated for commit 569a1e91
| 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.
Greptile SummaryThis PR introduces heterogeneous object placement support, allowing each Isaac Lab environment to receive a different variant of a
Confidence Score: 4/5The core heterogeneous placement machinery is well-structured and well-tested. The changes that touch all existing On-relation environments (on-pair skipping in the no-collision loss) are intentional fixes for solver oscillation but alter convergence characteristics silently. The redesigned per-env pool queues, sample_for_envs, and _store_env_matched_results target-aware capping together address several issues raised in the previous review round. The main remaining concerns are: (1) a misleading total_valid counter in _store_env_matched_results that can report inflated valid counts when some env pools are already at capacity; (2) the removal of no-collision loss between On-related pairs is a behavioral change that affects every existing environment using On relations, with no changelog or migration note. Neither is a runtime crash, but the solver behavior change could surface as unexpected convergence differences in downstream training runs. isaaclab_arena/relations/pooled_object_placer.py (_store_env_matched_results total_valid accounting) and isaaclab_arena/relations/relation_solver.py (on-pair skipping behavioral change for existing environments) Important Files Changed
Reviews (15): Last reviewed commit: "move rigidobject import" | 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, | |
| ) |
| teleop_device: TeleopDeviceBase | None = None, | ||
| env_cfg_callback: Callable[IsaacLabArenaManagerBasedRLEnvCfg] | None = None, | ||
| env_cfg_callback: ( | ||
| Callable[[IsaacLabArenaManagerBasedRLEnvCfg], IsaacLabArenaManagerBasedRLEnvCfg] | None |
There was a problem hiding this comment.
Is this change needed?
There was a problem hiding this comment.
No it is not, will revert it
There was a problem hiding this comment.
Sounds good, thanks for confirming! 👍
| keeping only those that pass validation. The pool is refilled automatically | ||
| when consumed layouts run out. | ||
| Storage: ``num_envs`` independent layout pools, each with its own read | ||
| cursor (this replaces the single ``_layouts`` list + ``_next_idx`` cursor |
There was a problem hiding this comment.
Suggestion to remove: "(this replaces the single _layouts list + _next_idx cursor used before heterogeneous placement)" Ai leftover
There was a problem hiding this comment.
I reviewed the comments and docstring and removed the leftover or confusing statement.
There was a problem hiding this comment.
Thanks for cleaning that up! The docstring reads clearly now. 👍
| """Get bounding box in world coordinates (local bbox rotated and translated).""" | ||
| ... | ||
|
|
||
| def get_bounding_box_per_env(self, num_envs: int) -> AxisAlignedBoundingBox: |
There was a problem hiding this comment.
I feel like this should not live in object_base.py. It's a runtime detail of the placement/simulation &num_envs is a placer/scene concept, not an object concept.
Same for the introduced variable has_env_specific_bboxes: It only matters from the placer's perspective.
Could we move this to a relation side helper?
There was a problem hiding this comment.
Thanks! We now have a new filebbox_helpers.py where we introduced environment-specific variable to bboxes if necessary, if not just keep the same as in the original bbox.
There was a problem hiding this comment.
Got it, that makes sense! Separating the environment-specific bounding box logic into bbox_helpers.py is a clean approach — keeps the base interface simple while allowing per-env customization when needed. Thanks for clarifying!
| self, | ||
| state: RelationSolverState, | ||
| debug: bool = False, | ||
| env_bboxes: dict[ObjectBase, AxisAlignedBoundingBox] | None = None, |
There was a problem hiding this comment.
Is there another place where this could be stored and accessed within this function to not need to thread it trough here?
Maybe store env_bboxes on RelationSolverState alongside positions as state.get_position(obj) and state.get_bbox(obj)?
There was a problem hiding this comment.
Done. Now RelationSolverState stores bbox, will return env_bboxs if it is used, so users can get the env-specific variables from the solver states.
There was a problem hiding this comment.
Nice, looks good! The get_bbox() accessor on RelationSolverState cleanly handles both the per-env override case and the default fallback. Thanks for the clean implementation! 👍
zhx06
force-pushed
the
zxiao/heter-support
branch
from
e97df23 to
97c4c45
Compare
alexmillane
left a comment
alexmillane
left a comment
There was a problem hiding this comment.
Hi @zhx06.
Thank you for implementing this.
I have given it a (very) partial review that focused on the RigidObjectSet. It's becoming quite complicated to follow what's going on in that class.
I think that if we make some additional assumptions and simplifications we'll get something more easily understood.
| self.object_usd_paths.append(obj.usd_path) | ||
|
|
||
| self.objects: list[Object] = objects | ||
| self._member_object_usd_paths: list[str] = list(self.object_usd_paths) |
There was a problem hiding this comment.
@cvolkcvolk . This relates to our discussion on your MR. Basically, agents are starting to insert leading underscores into our code. Honestly, probably correctly. The problem is that we haven't done this consistently.
We need to make a decision as a team about our coding style here and try to adhere to it (and put in the agent.md our decision.
@zhx06 For now I would avoid the leading underscore in this class. Basically all these members are intended to be private (even though they don't have a leading underscore). Let's aim for class-level consistency until we make a call.
There was a problem hiding this comment.
Just to confirm, we don't want to add new underscored members, but underscored helper methods are still fine?
| if n == 1: | ||
| return [0 for _ in range(num_envs)] | ||
| if not self.random_choice: | ||
| return [env_idx % n for env_idx in range(num_envs)] |
There was a problem hiding this comment.
I think we don't need the extra branch here for n == 1?
The following two options generate a list of zeros.
| ), | ||
| ) | ||
| object_cfg = self._add_initial_pose_to_cfg(object_cfg) | ||
| assert isinstance(object_cfg, RigidObjectCfg) |
There was a problem hiding this comment.
What's the meaning of this check? Above we create the RigidObjectCfg and here we check it? Are we making sure we dont get weird behaviour in _add_initial_pose_to_cfg?
| if any(idx < 0 or idx >= n for idx in variant_indices_by_env): | ||
| raise ValueError( | ||
| f"RigidObjectSet '{self.name}' variant indices must be in [0, {n}); got {variant_indices_by_env}." | ||
| ) |
There was a problem hiding this comment.
We should probably add this to the AGENTS.md that we prefer assert rather than if...raise
| f"RigidObjectSet '{self.name}' variant indices must be in [0, {n}); got {variant_indices_by_env}." | ||
| ) | ||
|
|
||
| self.variant_indices_by_env = list(variant_indices_by_env) |
There was a problem hiding this comment.
why list. The type hint say that this thing should already be a list?
| self.object_usd_paths = [] | ||
| for obj in objects: | ||
| assert obj.usd_path is not None | ||
| self.object_usd_paths.append(obj.usd_path) | ||
|
|
||
| self.objects: list[Object] = objects | ||
| self._member_object_usd_paths: list[str] = list(self.object_usd_paths) |
There was a problem hiding this comment.
What's the reason for having self.object_usd_paths AND self._member_object_usd_paths
They are at least initialized to being the same thing? Perhaps one is shuffled with respect to the other?
Can we get away with one of these members? It's making things very difficult to interpret what's going on.
| self._member_object_usd_paths: list[str] = list(self.object_usd_paths) | ||
| self.random_choice = random_choice | ||
| self.variant_indices_by_env: list[int] | None = None | ||
| self.has_env_specific_bboxes = len(objects) > 1 |
There was a problem hiding this comment.
Why do we have this? I would assume that a user who is using the RigidObjectSet, has more than a single object in the set. I think that that's a safe assumption. I would prefer that we just assert len(objects) > 1 and then use that assumption to simplify everything.
| def is_heterogeneous(obj: ObjectBase) -> bool: | ||
| """True if obj has different bounding boxes per environment (e.g. multi-variant RigidObjectSet).""" | ||
| return getattr(obj, "has_env_specific_bboxes", False) |
There was a problem hiding this comment.
Suggestion to just substitute with isinstance(obj, RigidObjectSet)
| if self.variant_indices_by_env is None: | ||
| self._set_variant_indices_by_env(self._generate_variant_indices(num_envs)) |
There was a problem hiding this comment.
Here we appear to be setting variant_indices_by_env, in a function called get_variant_indices. That is quite confusing behaviour, and is making it quite hard to tell what's going on in this class.
Let's have a larger discussion about the function of this class.
| spawn=sim_utils.MultiUsdFileCfg( | ||
| usd_path=self.object_usd_paths, | ||
| random_choice=self.random_choice, | ||
| random_choice=self.random_choice if self.variant_indices_by_env is None else False, |
There was a problem hiding this comment.
It's quite confusing what's going on in this class.
It appears to me that if you request bounding boxes, the random choice being handled by Isaac Lab is overridden and the randomness is handled by us? Is that correct?
That makes sense to me - we need to handle handle the obj -> env mapping, such that we have access to which bounding box ends up where.
The dataflow in this class is very confusing. I think because we're trying to maintain the option that randomness is handled by Isaac Lab.
Am I correct that we could simplify this class substantially by just always handling randomization in the Arena layer?
zhx06
force-pushed
the
zxiao/heter-support
branch
from
ecec090 to
569a1e9
Compare
There was a problem hiding this comment.
📝 Incremental Review Update (569a1e9)
Commits reviewed: 0ebd50ca → 569a1e91
✅ Final Changes Look Good
This update consists of iterative refinements addressing reviewer feedback:
| Commit | Summary |
|---|---|
594587a0 |
Address new feedback |
8eeab82e |
Add bbox guard for safety |
a4450e84 |
Separate anchor behavior |
2af2ae43 |
Address comments |
f668f514 |
Add bbox helper, edit comments |
389acab6 |
Edit comments style |
beaef723 |
Fix bug and change names |
ae97f6dc |
Revert redundant changes |
569a1e91 |
Address comments |
Key Improvements in This Push:
- Code organization — Bbox helper functions extracted for cleaner code
- Anchor behavior separation — Clearer handling of anchor vs non-anchor objects
- Bug fixes — Issues identified in previous reviews have been addressed
- Documentation — Comments and docstrings improved for consistency
Assessment: ✅ No new issues introduced
The heterogeneous placement implementation is now mature and well-tested. The changes since the last review are primarily refinements and documentation improvements.
CI Status: ⏳ Pre-commit checks pending
Reviewed commits: 0ebd50c...569a1e9
| assign_variants_for_envs(objects, num_envs) | ||
| if env_bboxes is None: | ||
| env_bboxes = {obj: get_bounding_box_per_env(obj, num_envs) for obj in objects} |
There was a problem hiding this comment.
assign_variants_for_envs is called unconditionally with num_envs = total_layouts even when env_bboxes has already been pre-built by PooledObjectPlacer._solve_layouts_with_env_bboxes, which previously called assign_variants_for_envs(objects, self._num_envs) (the real env count). When pool_size > num_envs (e.g., default pool_size=100, num_envs=16), total_layouts = layouts_per_env * num_envs > num_envs, so RigidObjectSet.assign_variants(total_layouts) sees an existing assignment of length num_envs != total_layouts and raises ValueError. This makes PooledObjectPlacer entirely unusable with RigidObjectSet at any typical pool size.
| assign_variants_for_envs(objects, num_envs) | |
| if env_bboxes is None: | |
| env_bboxes = {obj: get_bounding_box_per_env(obj, num_envs) for obj in objects} | |
| if env_bboxes is None: | |
| assign_variants_for_envs(objects, num_envs) | |
| env_bboxes = {obj: get_bounding_box_per_env(obj, num_envs) for obj in objects} |
alexmillane
left a comment
alexmillane
left a comment
There was a problem hiding this comment.
Another partial review. My main suggestion is that we try to do some unification between the heterogeneous and homogeneous paths in the placer and solver.
Is there any reason that we can't treat a homogeneous solve as a homogeneous solve, except that the bounding boxes happen to be the same over each environment? Is there a reason we can't do that?
One potential reason I can see is that right now (I think) we have batching behaviour on the homogeneous code path, and we don't on the heterogeneous code path. Is my reading correct? Could we also do some unification there? I.e. both code paths have batching behaviour?
My central fear is that with all this branching, the solver is going to become quite difficult to maintain over time, because each path has to be separately tested, maintained, and fixed.
| usd_path="", | ||
| prim_path=prim_path, | ||
| scale=(1.0, 1.0, 1.0), # We rewrite the USDs to handle scaling | ||
| scale=(1.0, 1.0, 1.0), |
There was a problem hiding this comment.
Suggestion to add the comment back in.
There was a problem hiding this comment.
This is reverted now.
| def get_bounding_box_per_env(self, num_envs: int) -> AxisAlignedBoundingBox: | ||
| """Get the actual bounding box for each env's variant. | ||
|
|
||
| Unlike ``get_bounding_box()`` (which uses a max-z heuristic), this |
There was a problem hiding this comment.
Do we still use the max-z heuristic somewhere? Is there a reason to still have that around?
There was a problem hiding this comment.
Heterogeneous placement uses get_bounding_box_per_env() and does not use this max-z heuristic.
There was a problem hiding this comment.
-> bounding_box_helpers (avoid abbreviations)
| env_bboxes: Pre-computed per-env bounding boxes (shape (num_envs, 3) | ||
| per object). When provided, _place_heterogeneous uses these | ||
| instead of calling get_bounding_box_per_env(obj, num_envs). This | ||
| allows the caller to tile real-env bboxes for pooled solving. |
There was a problem hiding this comment.
This seems a little strange to me. Is there a reason to have the bounding boxes computed outside?
Is this some requirement coming from the pooling?
| # - homogeneous: any solved layout serves any env; pick the top num_results. | ||
| # - heterogeneous: some objects vary per env, so each env owns a fixed slice | ||
| # of candidates and we pick the best within that slice. | ||
| uses_env_specific_bboxes = result_per_env and has_heterogeneous_objects(objects) |
There was a problem hiding this comment.
Is it true that in the hetrogeneous case we ALWAYS want a result per env?
In the past, we had heterogeneous environments which got the same placement per env (using an approximation of the bounding box). However, this was always a stepping stone to the true solution, which is the subject of this MR.
I would suggest removing the old behaviour. That is, if you heterogeneous objects, always compute a solution per env. (unless you have a good reason for maintaining this behaviour?)
| if num_results == 1: | ||
| return results_per_env[0] | ||
| return MultiEnvPlacementResult(results=results_per_env) |
There was a problem hiding this comment.
Could we simplify things by wrapping these two into a single placement result object, that is either multi-env or single env?
There was a problem hiding this comment.
This was discussed in the MR for homogeneous object-placement. Lots of code outside use single placement result. I would suggest having a separate MR for this
| """Per-env placement: each candidate is tied to its env's object variants. | ||
|
|
||
| Candidates [cur_env * max_attempts : (cur_env + 1) * max_attempts] | ||
| belong to cur_env and use that env's variant geometry. We solve all | ||
| num_envs * max_attempts candidates in one batched solver.solve() | ||
| call and pick the best candidate within each env's slice. |
There was a problem hiding this comment.
Suggestion to describe the shape of the return value.
| """Per-env placement: each candidate is tied to its env's object variants. | ||
|
|
||
| Candidates [cur_env * max_attempts : (cur_env + 1) * max_attempts] | ||
| belong to cur_env and use that env's variant geometry. We solve all | ||
| num_envs * max_attempts candidates in one batched solver.solve() | ||
| call and pick the best candidate within each env's slice. |
There was a problem hiding this comment.
So there's a bit of a difference between homogeneous and heterogeneous.
In homogeneous, we return ranked results, in heterogenous we return a single result per env? Is that correct?
Do we then loose the ability to do pooling? A clearer equivalent to me would be to have ranked results per env.
What's your thinking there?
There was a problem hiding this comment.
I changed the design so both homogeneous and heterogeneous placement share the same ranked placement designed. The differences lie in the ranking mode, whether it is shared geometry ranks from one common pool, or env-specific geometry ranks for each single environment.
There was a problem hiding this comment.
📝 Incremental Review Update (be699ec)
Commits reviewed: 569a1e91 → be699ecd
✅ Final Review Comments Addressed
This update consists of two additional commits addressing reviewer comments:
| Commit | Summary |
|---|---|
30a8d552 |
Address comments |
be699ecd |
Address review comments |
Summary of PR #676: Heterogeneous Object Placement
This PR introduces heterogeneous object placement support for Isaac Lab Arena, enabling different object variants per simulation environment.
🏗️ Architecture Changes
-
RigidObjectSet Enhancements (
object_set.py)- Per-environment variant assignment via
assign_variants(num_envs) - New
get_bounding_box_per_env()for env-specific geometry variant_indices_by_envtracking for consistent spawning
- Per-environment variant assignment via
-
Placer Refactoring (
object_placer.py,pooled_object_placer.py)- Dual placement paths: homogeneous (shared) vs heterogeneous (per-env)
- New
place_ranked_per_env()API for pool filling - Per-env layout pools with independent cursors
-
Solver Updates (
relation_solver.py,relation_solver_state.py)env_bboxesparameter for batched per-env geometry- State management via
get_bbox()accessor
-
New Helpers (
bounding_box_helpers.py)has_heterogeneous_objects()detectionassign_variants_for_envs()coordinationget_bounding_box_per_env()abstraction
✅ Strengths
- Comprehensive test coverage — 648 lines of new tests in
test_heterogeneous_placement.py - Backward compatibility — Homogeneous path unchanged; existing environments unaffected
- Clean API boundaries — Helper module keeps num_envs logic out of ObjectBase
- Robust pool management — Per-env pools with fallback behavior for validation failures
⚠️ Minor Observations (Non-blocking)
- CI Status — Pre-commit checks still pending at time of review
- Documentation — Consider adding a user-facing guide for heterogeneous mode usage
📊 Final Assessment
| Category | Status |
|---|---|
| Architecture | ✅ Clean separation of concerns |
| Test Coverage | ✅ Comprehensive (648 lines new tests) |
| Error Handling | ✅ Fallbacks for validation failures |
| Backward Compat | ✅ Homogeneous path preserved |
| Code Quality | ✅ Well-documented, consistent style |
Verdict: ✅ Ready for merge pending CI completion
Reviewed commits: 569a1e9...be699ec
📝 Update (09557d4)
Commits reviewed: be699ecd → 09557d4c (42 commits)
Summary
Major updates include GR00T CI integration, Agent Skills library, and continued heterogeneous placement refinements.
✅ Previous Feedback Addressed
| Issue | Status |
|---|---|
Dead-code guard after _solve_and_store |
✅ Fixed — constructor validates each pool individually |
warnings.warn() suggestion |
✅ Simplified — warning removed when --objects provided in hetero mode |
| Code clarity improvements | ✅ Docstrings, comments, and naming refined |
⚠️ Outstanding (Acceptable to Defer)
| Issue | Notes |
|---|---|
| P1: Partial reset consumes full env round | Performance optimization — can address if staggered resets become problematic |
P2: max_placement_attempts double duty |
Noted in comments — can separate if it causes issues in practice |
📊 Assessment
The heterogeneous placement implementation is solid with comprehensive test coverage. The new GR00T CI infrastructure is a welcome addition.
Verdict: ✅ Ready for merge
Reviewed commits: be699ec...09557d4
📝 Update (7969e98)
Commits reviewed: 09557d4c → 7969e984 (46 commits)
Summary
This update brings substantial new features alongside continued heterogeneous placement refinements addressing review feedback.
🆕 New Features
-
HDF5 Demo Merge Script (
merge_demos.py)- Combines multiple
record_demosHDF5 files into one training-ready dataset - Preserves
format_version(critical for quaternion convention) - Schema validation across inputs, dry-run mode, atomic writes
- Comprehensive 470-line test suite (
test_merge_demos.py)
- Combines multiple
-
EnvGraphSpec & YAML Parser (
arena_env_graph_spec.py)- Structured environment specification via YAML
- Node types: embodiment, background, object, object_reference
- Spatial constraints:
on,in,is_anchor,position_limits,at_pose - Task constraints with state spec references
- Full test coverage (
test_arena_env_graph_spec.py)
-
G1 Static Pick-and-Place Improvements
- Waist locking (
--lock_waist) for upper-body-only tasks - High-friction finger material binding via
apply_high_friction_to_g1_fingers - Open-arm initial posture configuration
- Apple spawn XY range set to 0 for deterministic demos
- Waist locking (
-
G1 Agile Policy Multi-Env Support (
g1_agile_policy.py)- Auto-converts static-batch ONNX to dynamic-batch for
num_envs > 1 - Cached model with
_dynamic_batchsuffix - Eliminates previous
num_envs == 1assertion
- Auto-converts static-batch ONNX to dynamic-batch for
✅ Previous Feedback Addressed
| Issue | Status |
|---|---|
| Heterogeneous placement refinements | ✅ 20+ commits addressing review feedback |
| Pool fallback behavior | ✅ Cleaner separation between reusable and env-specific pools |
| On-pair no-overlap skip | ✅ Added to prevent On loss vs overlap loss fighting |
| Partial reset optimization | ✅ Reusable pools now consume only reset envs, not full rounds |
📁 Key File Changes
| File | Changes |
|---|---|
placement_events.py |
Env-indexed vs reusable pool routing for partial resets |
pooled_object_placer.py |
requires_env_indexed_layouts, total_remaining properties |
relation_solver.py |
On-pair exclusion from no-overlap loss |
object_set.py |
Refined variant assignment with seed support |
record_demos.py |
--disable_full_sim_buffer_reset flag, manual reset guards |
📊 Test Coverage Additions
| New Test File | Lines | Coverage |
|---|---|---|
test_merge_demos.py |
~470 | HDF5 merging edge cases |
test_arena_env_graph_spec.py |
~260 | Graph spec parsing/validation |
test_heterogeneous_placement.py |
~720 | Heterogeneous placement paths |
⚠️ Minor Observations (Non-blocking)
- G1 ONNX auto-conversion — The dynamic-batch model caching is clever; ensure the
.tmpsuffix does not collide in parallel builds. - EnvGraphSpec validation — Good defensive assertions; consider adding schema versioning for future-proofing.
📊 Assessment
| Category | Status |
|---|---|
| New Features | ✅ Well-designed merge script and env graph spec |
| Heterogeneous Fixes | ✅ Comprehensive feedback addressed |
| Test Coverage | ✅ Excellent (1400+ new test lines total) |
| Code Quality | ✅ Consistent style, good documentation |
Verdict: ✅ Ready for merge
Reviewed commits: 09557d4...7969e98
📝 Update (c779958)
Commits reviewed: 7969e984 → c779958f
Summary
Minor cleanup commit addressing a code simplification in the variant indices handling.
📁 Changes
| File | Change |
|---|---|
object_set.py |
Removed unnecessary list() wrapper from variant_indices_by_env assignment |
Details
In _set_variant_indices_by_env(), the assignment was changed from:
self.variant_indices_by_env = list(variant_indices_by_env)to:
self.variant_indices_by_env = variant_indices_by_envThis is a valid simplification since:
- The input is already validated via the assertion as a sequence of valid indices
- The caller already provides a list, so the defensive copy was redundant
- Reduces one unnecessary list allocation per call
📊 Assessment
| Category | Status |
|---|---|
| Risk | ✅ Trivial — single line simplification |
| Behavior | ✅ Unchanged — no semantic difference |
| Code Quality | ✅ Cleaner, less redundant |
Verdict: ✅ LGTM — Ready for merge
Reviewed commits: 7969e98...c779958
📝 Update (29f4076)
Commits reviewed: c779958f → 29f4076
Summary
Documentation and style cleanup commit — removes RST-style backtick formatting from docstrings.
📁 Changes
| File | Change |
|---|---|
object_set.py |
Docstring cleanup: removed backticks around parameter/method references |
arena_env_builder.py |
Docstring cleanup: simplified phrasing and removed backticks |
bounding_box_helpers.py |
Docstring cleanup: removed VARIANT_SEED_STRIDE constant, simplified seed description |
object_placer.py |
Docstring cleanup: removed backticks from return type descriptions |
pooled_object_placer.py |
Docstring cleanup: removed backticks, changed exception to assertion |
test_heterogeneous_placement.py |
Comment cleanup: removed backticks from test fixture description |
Details
This commit standardizes docstring formatting across the heterogeneous placement codebase:
- Backtick removal — References like
variant_indices_by_env→variant_indices_by_env - Phrasing simplification — e.g., "via
set_initial_pose" → "as per-env initial poses" - Exception → Assertion —
EnvLayoutPool.next()now usesassertinstead of raisingIndexError(consistent with other pool methods) - Constant removal —
VARIANT_SEED_STRIDE(1,000,003) replaced with simpler per-set index offset
📊 Assessment
| Category | Status |
|---|---|
| Risk | ✅ Zero — documentation/style only |
| Behavior | ✅ Unchanged — no functional changes |
| Code Quality | ✅ More consistent docstring style |
Verdict: ✅ LGTM — Ready for merge
Reviewed commits: c779958...29f4076
| if len(objects) < 2: | ||
| raise ValueError(f"Object set {name} must contain at least 2 objects.") |
There was a problem hiding this comment.
The previous guard was len(objects) == 0 (minimum 1). Any existing RigidObjectSet constructed with a single object will now throw ValueError at init. A 1-member set degenerates to a homogeneous object so it isn't useful for heterogeneous placement, but this is still a silently breaking API change for callers that do exist. At minimum, the PR description should note this as a breaking change, and the test_empty_object_set helper may need a companion test that confirms a 1-element set raises.
zhx06
force-pushed
the
zxiao/heter-support
branch
from
be699ec to
09557d4
Compare
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
force-pushed
the
zxiao/heter-support
branch
from
09557d4 to
7969e98
Compare
| if pool.requires_env_indexed_layouts: | ||
| print("Warning: Skipping static init_state seeding for env-indexed placement layouts.") | ||
| return |
There was a problem hiding this comment.
For env-indexed (heterogeneous) layouts, _set_init_state_from_pool returns early without writing any init_state.pos to the object configs. Isaac Lab will spawn each object at (0, 0, 0) — likely clipping through the floor or table — until the first reset event overrides poses. In most RL loops the first reset fires quickly, but a few physics ticks at an invalid pose can trigger contact forces that destabilize the scene at episode start. The simpler fix is to draw one sample_without_replacement(num_envs) round here, write each env's layout to init_state.pos, and advance the pool cursor by one full round so the placement event's first draw gets a fresh layout.
| else: | ||
| self._env_pools[cur_env].extend(env_results) | ||
| fallback_envs.append(cur_env) | ||
| self._had_fallbacks = True |
There was a problem hiding this comment.
When valid_results is empty, _store_env_matched_results stores all layouts_per_env fallback entries (line 253). Because layouts_per_env == max_missing == target_per_env - min(available), that single call pushes every env to ≥ target_per_env entries. The check in _solve_and_store (if min(self._available_per_env()) >= target_per_env: return) then exits immediately, so the remaining max_solve_batches - 1 retry iterations are never reached.
A scene that would have found valid placements with different random seeds never gets a second attempt — every env is permanently served fallback-only layouts even though max_solve_batches > 1 was intended to provide retries.
The fix is to skip counting fallback-only entries toward the termination condition. One approach: track a separate fallback_count_per_env and only allow the loop to exit when min(valid_available) >= target_per_env, continuing the loop when any env has only fallbacks and retries remain.
There was a problem hiding this comment.
Partial review.
It's looking better. Thanks for aligning the code paths between homogeneous and hetero geneous placement.
However, the clean up isn't complete. From what I can tell, the code has a bunch of branching for the case where we don't have per-env bounding boxes. This is left over from the homogeneous placement code-branch, which we have now removed. The problem with this is that it's making the code quite difficult to follow because there's lot of branching, even though that branching is not used in production (from what I can tell).
I would suggest that we complete the transition. We always extract per-env bounding boxes, even in the homogeneous case where such bounding boxes are the same across all environments, and we remove all code paths that switch on the non-presence of per-env bounding boxes.
I would also recommend that we abstract the handling of per-env bounding boxes into a class, and move it out of the object placer. Right now we have the same informatoin formatted 3 ways, with similar names, being passed around the code (also incorrectly called "override"). If we group these in a class with different ways of accessing the information, this will simplify matters.
| f"RigidObjectSet '{self.name}' has variant assignments for " | ||
| f"{len(self.variant_indices_by_env)} envs, got request for {num_envs}." | ||
| ) | ||
| return |
There was a problem hiding this comment.
suggestion to do if..else, rather than if...return.
There was a problem hiding this comment.
Addressed. Now it uses if else
| assert len(self.variant_indices_by_env) == num_envs, ( | ||
| f"RigidObjectSet '{self.name}' has variant assignments for " | ||
| f"{len(self.variant_indices_by_env)} envs, got request for {num_envs}." | ||
| ) |
There was a problem hiding this comment.
I feel that this function handle being called twice. If it's called twice regenerate the indices. On the second call it should maybe print a warning saying that it's regenerating the indices.
There was a problem hiding this comment.
For refills, we only want new placements, not new object variants. The warning at this time might be noisy and not informative
| prim_path: str | None = None, | ||
| scale: tuple[float, float, float] = (1.0, 1.0, 1.0), | ||
| random_choice: bool = False, | ||
| variant_indices_by_env: list[int] | None = None, |
There was a problem hiding this comment.
I think that this constructor argument is only used in a test? Suggestion to remove it and rely on the member function.
There was a problem hiding this comment.
Yeah, we remove this from the constructor argument
| spawn=sim_utils.MultiUsdFileCfg( | ||
| usd_path=self.object_usd_paths, | ||
| random_choice=self.random_choice, | ||
| random_choice=False, |
There was a problem hiding this comment.
Maybe add a small inline comment here that this is hardcoded false because the randomization is handled at the Arena level in this class.
There was a problem hiding this comment.
Addressed. inline comment added.
| Seeded assignments offset each set by its index so multiple sets do not | ||
| reuse the same random sequence. | ||
| """ | ||
| from isaaclab_arena.assets.object_set import RigidObjectSet |
There was a problem hiding this comment.
Do we need to make these local imports? Do we need to import this file in a context that Isaac Sim has not started in?
There was a problem hiding this comment.
Addressed. We do not need this to stay as a local import.
| @staticmethod | ||
| def _get_world_bbox_for_init( | ||
| obj: ObjectBase, | ||
| bbox_overrides: dict[ObjectBase, AxisAlignedBoundingBox] | None, |
There was a problem hiding this comment.
We're using the bounding boxes unconditionally now right?
Should we remove "overrides" in all the variable names and remove the option that the parameter is None?
This shoulds simplify some stuff.
There was a problem hiding this comment.
We have removed the overrides.
| if bbox_override is None: | ||
| return obj.get_world_bounding_box() |
There was a problem hiding this comment.
Is this still a valid option?
There was a problem hiding this comment.
Addressed. We removed it.
| objects: list[ObjectBase], | ||
| anchor_objects: set[ObjectBase], | ||
| generator: torch.Generator | None = None, | ||
| child_bboxes: dict[ObjectBase, AxisAlignedBoundingBox] | None = None, |
There was a problem hiding this comment.
I think that we never use the child_bboxes=None branch.
This is making the code a lot more complex because we're maintaining a bunch of code paths that are no longer used.
There was a problem hiding this comment.
Addressed. We removed the unused child_bboxes=None branch
| positions[obj] = initial_pose.position_xyz | ||
| elif any(isinstance(r, On) for r in obj.get_relations()): | ||
| positions[obj] = self._compute_on_guided_position(obj, anchor_objects, anchor_bbox, generator) | ||
| bbox_override = child_bboxes.get(obj) if child_bboxes else None |
There was a problem hiding this comment.
see comment above, I think that the branches related to Non specified bounding boxes are no longer used.
| @staticmethod | ||
| def _get_world_bbox_for_init( | ||
| obj: ObjectBase, | ||
| bbox_overrides: dict[ObjectBase, AxisAlignedBoundingBox] | None, |
There was a problem hiding this comment.
The code branches related to bbox_overrides=None are no longer used.
This is making the code more complex because we're always checking for conditions that are no longer valid.
There was a problem hiding this comment.
Addressed. We removed the optional bbox_overrides=None
3 participants
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.