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
Greptile SummaryThis PR adds heterogeneous object placement support — each simulation environment can now spawn a different variant of an object (via
Confidence Score: 4/5Safe to merge for the heterogeneous placement feature; known init-pose limitations for env-indexed pools before the first reset event are acknowledged in code comments. The core placement pipeline, pool refill logic, and partial-reset fix are all well-covered by the new test suite. Two minor issues were found: pooled_object_placer.py (_store_env_matched_results total_valid accounting) and object_placer.py (place_ranked_per_env / apply_positions_to_objects inconsistency). Important Files Changed
Sequence DiagramsequenceDiagram
participant AEB as ArenaEnvBuilder
participant RSI as RelationSolverInterface
participant POP as PooledObjectPlacer
participant OP as ObjectPlacer
participant BBH as BoundingBoxHelpers
participant RS as RelationSolver
AEB->>RSI: solve_and_apply_relation_placement()
RSI->>POP: PooledObjectPlacer(objects, num_envs)
POP->>OP: _solve_env_ranked_layouts(batch_size)
OP->>BBH: assign_variants_for_envs(objects, num_envs)
BBH->>BBH: RigidObjectSet.assign_variants()
OP->>BBH: build_per_env_bounding_boxes(objects, num_envs)
BBH-->>OP: PerEnvBoundingBoxes (num_envs x 3 tensors)
OP->>RS: solve(objects, initial_positions, env_bboxes)
RS-->>OP: solved positions per candidate
OP->>OP: _rank_candidates() per env
OP-->>POP: ranked_results_per_env
POP->>POP: _store_env_matched_results()
note over POP: On reset event
POP->>POP: sample_for_envs(reset_env_ids)
POP-->>RSI: dict[env_id to PlacementResult]
RSI->>RSI: write_root_pose_to_sim per env
Reviews (20): Last reviewed commit: "add return shape" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
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.
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
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.
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
📝 Update (d6631af)
Commits reviewed: 29f4076 → d6631af (49 commits)
Summary
Major update includes a relation placement solver API refactor (#707), Lightwheel asset retry handling (#731), and extensive heterogeneous placement refinements addressing accumulated review feedback.
🔄 Key Changes
1. Relation Solver Interface Refactor (relation_solver_interface.py — NEW)
- Extracted placement orchestration from
ArenaEnvBuilderinto dedicated module - New
solve_and_apply_relation_placement()as the single public entry point - Helper functions:
_apply_dynamic_spawn_pose(),_apply_static_initial_poses() - Cleaner separation:
ArenaEnvBuilder._solve_relations()now just calls the interface
2. Lightwheel Asset Retry Handling (lightwheel_utils.py — NEW)
acquire_lightwheel_asset()wrapper with configurable retry logic- Handles
TimeoutErrorwith automatic retries and delay - Temporary timeout override during acquisition, restores original after
- Applied to Microwave, CoffeeMachine, Broccoli, SweetPotato, Jug, BeerBottle in
object_library.py
3. Object.relations Constructor Parameter
Object.__init__()now acceptsrelationsparameter- Enables relation setup at construction time (vs. post-construction
add_relation()) - Test coverage added in
test_heterogeneous_placement.py
4. PooledObjectPlacer Enhancements
sample_for_envs(env_ids)— consumes layouts only for specific absolute env IDshad_fallbacksproperty — tracks whether any pool accepted best-loss fallbacks- Improved partial reset handling — no longer requires full env rounds for reusable layouts
5. Placement Event Refinements (placement_events.py)
- Env-indexed vs reusable pool routing in
solve_and_place_objects() - Warning when writing best-loss fallback placements
- Validation: pool
num_envsmust match scene env origins count
📁 File-by-File Highlights
| File | Changes |
|---|---|
relation_solver_interface.py |
NEW — 180 lines, placement orchestration extracted from builder |
lightwheel_utils.py |
NEW — Asset fetching with retry logic |
object.py |
Added relations parameter, stores as self.relations = list(relations) |
object_library.py |
6 objects now use acquire_lightwheel_asset() wrapper |
object_set.py |
Refined docstrings, member_usd_paths rename from object_usd_paths |
arena_env_builder.py |
Simplified to delegate to solve_and_apply_relation_placement() |
pooled_object_placer.py |
sample_for_envs(), had_fallbacks, cleaner pool management |
placement_events.py |
Partial reset optimization, env-indexed routing |
gr1_table_multi_object_no_collision_environment.py |
Updated to use office_table_background for tabletop reference |
📊 Test Additions
| File | Lines Added | Coverage |
|---|---|---|
test_heterogeneous_placement.py |
+200 | Pool strategies, fallback behavior, env-indexed sampling |
test_relation_solver_interface.py |
NEW ~140 | Interface helpers, missing positions handling |
test_lightwheel.py |
NEW ~67 | Retry logic, timeout handling |
test_placement_events.py |
+108 | Partial resets, env-indexed pools, fallback warnings |
test_object_set.py |
+122 | Variant assignment, seeding, regeneration |
test_no_collision_loss.py |
+26 | On-pair no-overlap skip |
test_object_placer_init.py |
+27 | Env-specific bbox initialization |
test_validate_placement.py |
+5 | Env-bbox parameter passing |
✅ Previous Feedback Addressed
| Issue | Status |
|---|---|
| Solver/builder coupling | ✅ Extracted into relation_solver_interface.py |
| Lightwheel timeout resilience | ✅ Retry wrapper with configurable attempts/delay |
| Partial reset performance | ✅ Reusable pools consume exactly len(env_ids) layouts |
| Fallback transparency | ✅ had_fallbacks flag + warning messages |
⚠️ Minor Observations (Non-blocking)
- Lightwheel retry defaults — Default 3 attempts with 1s delay; consider exposing via config for production tuning.
- Test fixture monkey-patching — The
_patch_bounding_box_helpers_for_test_doublesfixture is clever but complex; document its purpose prominently.
📊 Final Assessment
| Category | Status |
|---|---|
| Architecture | ✅ Cleaner separation with new interface module |
| Robustness | ✅ Lightwheel retry handling, pool fallbacks |
| Test Coverage | ✅ Excellent — 600+ new test lines |
| Code Quality | ✅ Consistent style, good documentation |
| Backward Compat | ✅ All existing APIs preserved |
Verdict: ✅ Ready for merge — The refactored relation solver interface and Lightwheel retry handling are solid improvements. Test coverage continues to be comprehensive.
Reviewed commits: 29f4076...d6631af
📝 Update (483b0ef)
Commits reviewed: d6631af → 483b0efc
Summary
This update introduces a new CompositeTaskBase class for unordered multi-task compositions and refactors SequentialTaskBase to inherit from it, improving code reuse while maintaining ordering semantics.
🆕 New Features
1. CompositeTaskBase (composite_task_base.py — NEW)
- Base class for composite tasks where subtask completion order does not matter
- All subtasks can be completed in any order; overall success when all are done
- Core shared logic extracted:
_evaluate_subtask_successes(),_add_suffix_configclass_transform(),_remove_configclass_transform() desired_subtask_success_statenow supportsNoneentries as "don't care" positions
2. SequentialTaskBase Refactored
- Now inherits from
CompositeTaskBaseinstead ofTaskBase - Adds state-machine ordering enforcement:
_current_subtask_idxtracks active subtask per env - Subtask N+1 cannot be evaluated until subtask N has succeeded
- Renamed internal variable:
_subtask_success_state→_subtask_ever_succeeded(clearer semantics)
3. New Test Coverage
| File | Lines | Description |
|---|---|---|
test_composite_task_base.py |
~272 | Unit tests for CompositeTaskBase, configclass transforms |
test_composite_open_door.py |
~362 | Integration tests: dual-microwave composite door task |
📁 Key File Changes
| File | Changes |
|---|---|
composite_task_base.py |
NEW — 400 lines, unordered composite task base class |
sequential_task_base.py |
Refactored to inherit from CompositeTaskBase; ~240 lines removed |
test_composite_task_base.py |
NEW — Configclass transform tests, desired_state with None |
test_composite_open_door.py |
NEW — Multi-microwave composite task tests |
test_sequential_task_base.py |
Expanded: ordering tests, latching, reset behavior |
test_sequential_open_door.py |
Simplified inline task class removed |
✅ Design Highlights
- Clean inheritance hierarchy:
TaskBase→CompositeTaskBase→SequentialTaskBase - Shared success evaluation:
_evaluate_subtask_successes()is used by both composite and sequential - Flexible desired state:
[True, None, False]means "subtask 0 must be True, subtask 1 don't care, subtask 2 must be False" - Semantic rename:
_subtask_ever_succeededclearly indicates latching behavior - Static helper methods:
_add_suffix_configclass_transform,_remove_configclass_transformnow correctly prefixed with underscore
📊 Test Additions
| Test Function | Coverage |
|---|---|
test_composite_open_door_microwave |
Forward order completion |
test_reverse_order_composite_open_door_microwave |
Order independence |
test_composite_open_door_microwave_multiple_envs |
Multi-env composite |
test_sequential_success_advances_in_order |
State machine ordering |
test_sequential_success_latches |
Once succeeded, stays succeeded |
test_sequential_desired_subtask_success_state_with_none |
None as don't-care |
test_sequential_reset_clears_state_and_index |
Reset behavior |
⚠️ Minor Observations (Non-blocking)
- Composite vs Sequential naming — The distinction is clear in code but could benefit from a brief comment in the module docstring explaining the hierarchy.
desired_subtask_success_statevalidation — Currently allows[None, None, None]which trivially matches; may want to warn if all are None.
📊 Assessment
| Category | Status |
|---|---|
| Architecture | ✅ Clean inheritance, good code reuse |
| Backward Compat | ✅ SequentialTaskBase API unchanged |
| Test Coverage | ✅ Comprehensive — 634 new test lines |
| Code Quality | ✅ Good semantic naming, clear docstrings |
Verdict: ✅ Ready for merge — The CompositeTaskBase extraction is a clean refactor that enables unordered multi-task compositions while preserving existing sequential task behavior.
Reviewed commits: d6631af...483b0ef
📝 Update (4ede437)
Commits reviewed: 483b0efc → 4ede4375 (3 commits)
Summary
Minor refinements addressing review feedback: comment cleanup, return type documentation, and import organization.
📁 Changes
| File | Change |
|---|---|
relation_solver_interface.py |
Moved RigidObject import to local scope (avoids circular import risk) |
bounding_box_helpers.py |
Added return shape documentation in docstrings |
| Various test files | Minor assertion adjustments for env-indexed pool tests |
🔄 Commits
| Commit | Summary |
|---|---|
3cc95375 |
Address comments — import organization |
4ede4375 |
Add return shape documentation to bbox helpers |
✅ Changes Are Minimal & Safe
- Import scope change —
RigidObjectimport moved to method level inrelation_solver_interface.pyto avoid potential circular dependencies - Docstring improvements — Return shapes now documented in
bounding_box_helpers.pyfor clarity - No functional changes — All behavior remains the same
📊 Assessment
| Category | Status |
|---|---|
| Risk | ✅ Trivial — documentation and import organization |
| Behavior | ✅ Unchanged |
| Code Quality | ✅ Cleaner imports, better documentation |
Verdict: ✅ LGTM — Ready for merge
Reviewed commits: 483b0ef...4ede437
📝 Incremental Review Update (95334a2)
Commits reviewed: 4ede4375 → 95334a2d
Summary of Changes Since Last Review
This update includes 52 additional commits (~2 weeks of iteration) addressing reviewer feedback and refining the heterogeneous placement implementation.
Key Improvements
| Category | Changes |
|---|---|
| Solver Unification | Homogeneous and heterogeneous modes now share unified placement path through ObjectPlacer |
| Relation Registration | New @register_object_relation and @register_task decorators for registry-based discovery |
| Metrics Refactor | MetricTermCfg support and compute_metrics() moved into IsaacLabArenaManagerBasedRLEnvCfg |
| Pool Strategies | Cleaner separation between env-indexed (heterogeneous) and reusable (homogeneous) layout pools |
| No-Collision Fix | _compute_no_overlap_loss() now skips On-linked pairs to prevent solver oscillation |
| Test Robustness | NUM_ENVS bumped from 2→10 in object set termination tests for reliable variant coverage |
Notable Architecture Refinements
-
Solver State (
relation_solver_state.py)- New
get_bbox()accessor centralizes env-bbox lookups - Handles fallback to
obj.get_bounding_box()when env_bboxes omitted
- New
-
Placement Events (
placement_events.py)sample_for_envs()support for partial resets with env-indexed layouts- Warning output when writing best-loss fallback placements
-
Task Library (
task_library.py)- Lazy import pattern avoids USD/SimulationApp import-order issues
- Mirrors existing
object_library.pypattern
-
Test Coverage (
test_heterogeneous_placement.py)- Monkeypatch fixture for
HeterogeneousDummyObjecttest doubles - Multi-set variant tests (different variant counts across object sets)
- Pool fallback and refill edge-case coverage
- Monkeypatch fixture for
Removed/Deprecated
result_per_envparameter removed fromObjectPlacer.place()— all envs now get distinct layoutsobject_on_destinationdirect calls in tests replaced with reading task's success termination term
✅ Assessment
This iteration demonstrates thorough response to review feedback with improved code organization, unified placement paths, and comprehensive edge-case handling. The PR is maturing toward merge-readiness.
| Check | Status |
|---|---|
| Review feedback addressed | ✅ Multiple rounds incorporated |
| Architecture consistency | ✅ Unified homo/hetero paths |
| Test coverage | ✅ Expanded pool & termination tests |
| Documentation | ✅ Docstrings updated |
zhx06
force-pushed
the
zxiao/heter-support
branch
2 times, most recently
from
09557d4 to
7969e98
Compare
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.
zhx06
force-pushed
the
zxiao/heter-support
branch
2 times, most recently
from
d6631af to
483b0ef
Compare
alexmillane
left a comment
alexmillane
left a comment
There was a problem hiding this comment.
Thanks for addressing all the comments. This is looking really good.
I have made a couple of comments about the PerEnvBoundingBoxes. I'm still a bit confused about the format of the various outputs. So I'd suggest expanding the comments there (the implementation is good now. thanks for putting it into a separate class).
I've approved so merge when you've addressed the above :)
zhx06
force-pushed
the
zxiao/heter-support
branch
from
a4f00fa to
95334a2
Compare
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.