Fix bin_packing.py and Refactor with Wrapper APIs#5839
Conversation
github-actions
Bot
added
bug
![isaaclab-review-bot[bot]](https://avatars.githubusercontent.com/u/157846462?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Summary
Thank you for fixing these demo scripts! The changes address API compatibility issues with the underlying Warp library and RSL-RL configuration handling.
Analysis
Overall Assessment: ✅ The changes look correct and are necessary to maintain demo script functionality.
1. quadcopter.py
- Change: Wrapping
get_masses()result withwp.to_torch()conversion - Verdict: ✅ Correct fix for API change where
get_masses()now returns a Warp array
2. h1_locomotion.py
- Change: Adding
handle_deprecated_rsl_rl_cfg()to handle RSL-RL config compatibility - Verdict: ✅ Good practice for maintaining backward compatibility with RSL-RL library updates
3. bin_packing.py
- Changes:
num_objects→num_bodies(API rename)object_pos_w→body_link_pos_w.torch- Added
reshape_data_to_view_3d()helper for Warp conversion - Multiple torch-to-warp conversions for
set_transforms,set_velocities,set_masses,set_disable_gravities
- Observations:
- The helper function pattern is clean and improves readability
- The
.contiguous()calls are necessary for Warp interop
Minor Suggestions (non-blocking)
-
Consider type hints in the new
reshape_data_to_view_3d()function - the function signature has type annotations, which is great! -
Docstring clarity: The docstring mentions "body-major view order" - this is clear to those familiar with the codebase, but a brief example of the shape transformation could help future readers.
Checklist Reminder
The PR checklist shows several unchecked items. Please verify:
- Pre-commit checks run (
./isaaclab.sh --format) - No new warnings generated
- Changelog updated if required
Verdict: The technical changes are sound. Once the checklist items are verified, this looks good to merge. 👍
Update (ab06fd7)
Reviewed incremental changes from d974c6b6 → ab06fd77:
bin_packing.py
- Change: Fixed
groceries_mask_helpertensor shape to matchroot_viewordering (body_0/env_0..env_N pattern) - Before:
torch.arange(...) % num_objectswithview(num_envs, -1) - After:
torch.arange(num_objects).view(num_objects, 1)with proper broadcasting - Verdict: ✅ Correct fix - aligns mask construction with the actual data layout
quadcopter.py
- Change: Replaced
wp.to_torch(robot.root_view.get_masses())withrobot.data.body_mass.torch - Also: Removed unused
import warp as wp - Verdict: ✅ Cleaner approach - uses the existing
.torchproperty accessor instead of manual conversion, and removes now-unused import
Summary: Good incremental improvements! The bin_packing fix properly aligns the mask tensor dimensions with the root_view ordering, and the quadcopter change is a cleaner API usage pattern. Both changes are correct. 👍
Update (e4b6b73)
Reviewed incremental changes from ab06fd77 → e4b6b734:
bin_packing.py
- Removed redundant
.contiguous()calls on already-sliced Warp arrays inset_transformsandset_velocities - Added explicit
.to(torch.uint8)cast before Warp conversion inset_disable_gravities
Verdict: ✅ Minor cleanup - the explicit dtype cast ensures proper type compatibility with the Warp API. LGTM. 👍
Update (8250949)
Reviewed incremental changes from e4b6b734 → 8250949a:
bin_packing.py — Major API Migration
The script now migrates away from the root_view API entirely:
reshape_data_to_view→ Newreshape_data_to_view_3d()helper wrapping Warp conversion. ✅root_view.set_transforms/set_velocities→write_body_link_pose_to_sim_index/write_body_com_velocity_to_sim_index. ✅root_view.set_masses→set_masses_indexwith shape(num_instances, num_objects). ✅root_view.set_disable_gravities→ Removed entirely. ✅object_pos_w→body_link_pos_w.torch. ✅- Out-of-bounds reset now reads current state before patching stray objects back. ✅
Previous Concerns Status
All previous inline issues (.contiguous() on Warp arrays, uint8 casting for set_disable_gravities) are now moot — those root_view code paths have been completely replaced with new higher-level APIs.
New Issues
None identified. The Warp interop in reshape_data_to_view_3d correctly calls .contiguous() on the torch tensor before wp.from_torch() (not on a Warp array). The new API usage is consistent with the RigidObjectCollection interface.
Verdict: LGTM 👍
Update (ca616ab): No substantive changes to bin_packing.py since last review (8250949a). The new HEAD incorporates a base-branch merge (Cartpole preset consolidation) but the PR's own file is byte-identical. Previous assessment stands: LGTM 👍
Greptile SummaryThis PR updates three demo scripts to adapt to a breaking API change in Isaac Lab where PhysX view methods now accept and return Warp arrays instead of PyTorch tensors. The H1 locomotion script also adds a compatibility shim for a newer version of rsl-rl.
Confidence Score: 3/5The bin_packing demo will crash on every reset cycle due to two incorrect warp interop calls; the quadcopter and H1 locomotion changes are functionally correct. The bin_packing.py changes introduce two bugs that trigger on the first physics reset: calling .contiguous() on a warp array slice (no such method exists) and feeding a torch.bool tensor into wp.from_torch with dtype=wp.uint8 (dtype mismatch). Both are on the hot path inside reset_object_collections, which is called every 250 steps and on every out-of-bounds object. The quadcopter mass-sum fix and the rsl-rl compatibility shim in h1_locomotion are straightforward and appear correct. scripts/demos/bin_packing.py — the warp interop changes around set_transforms/set_velocities and set_disable_gravities need the two fixes before this script will run. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[torch.Tensor] -->|wp.from_torch| B[wp.array]
B -->|slice view| C[wp.array view]
C -->|".contiguous() CRASH"| D[AttributeError]
C -->|pass directly| E[root_view.set_transforms OK]
F[torch.bool tensor] -->|"wp.from_torch uint8 ERROR"| G[dtype mismatch]
F -->|".to uint8 first"| H[wp.from_torch dtype=wp.uint8]
H --> I[root_view.set_disable_gravities OK]
J[get_masses returns wp.array] -->|wp.to_torch| K[torch.Tensor 2D]
K -->|index 0 and sum| L[scalar robot_mass]
Reviews (1): Last reviewed commit: "Fix bin_packing.py" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| active_spawn_poses = groceries.reshape_data_to_view(active_spawn_poses) | ||
| cached_spawn_poses = groceries.reshape_data_to_view(cached_spawn_poses) | ||
| spawn_w = groceries.reshape_data_to_view(default_state_w).clone() | ||
| active_spawn_poses = reshape_data_to_view_3d(groceries, active_spawn_poses, 7, device) |
There was a problem hiding this comment.
reshape_data_to_view_3d - is this an API that would still make sense as part of RigidObjectCollection? the previous reshape_data_to_view seems to be part of the collection class. +@AntoineRichard
There was a problem hiding this comment.
I agree it should work
| random_masses = torch.rand(groceries.num_instances * num_objects, device=device) * 0.2 + 0.2 | ||
| groceries.root_view.set_masses(random_masses.cpu(), view_indices.cpu()) | ||
| groceries.root_view.set_disable_gravities((~groceries_mask).cpu(), indices=view_indices.cpu()) | ||
| random_masses_wp = wp.from_torch(random_masses.cpu().contiguous(), dtype=wp.float32) |
There was a problem hiding this comment.
are all the .contiguous() calls necessary? do we know if they impact performance?
There was a problem hiding this comment.
They should not be needed anymore
| view_states_wp = wp.from_torch(view_states.contiguous(), dtype=wp.float32) | ||
| view_ids_wp = wp.from_torch(view_ids.contiguous().to(dtype=torch.int32), dtype=wp.int32) | ||
|
|
||
| rigid_object_collection.root_view.set_transforms(view_states_wp[:, :7], indices=view_ids_wp) |
There was a problem hiding this comment.
if we switch from using the root_view directly to using APIs from RigidObjectCollection to set transforms like write_body_link_pose_to_sim_index(), would we be able to avoid a lot of the torch/warp interop?
| def reshape_data_to_view_3d( | ||
| rigid_object_collection: RigidObjectCollection, data: torch.Tensor, data_dim: int, device: str | ||
| ) -> torch.Tensor: | ||
| """Reshape env-major ``(num_envs, num_bodies, data_dim)`` data to body-major view order.""" | ||
|
|
||
| data_wp = wp.from_torch(data.contiguous(), dtype=wp.float32) | ||
| return wp.to_torch(rigid_object_collection.reshape_data_to_view_3d(data_wp, data_dim, device=device)) |
There was a problem hiding this comment.
This should not be needed
There was a problem hiding this comment.
reshape_data_to_view_3d requires data to be wp.array, which is actually torch.Tensor at the call locations.
| view_states[view_ids, :7] = torch.concat((positions, orientations), dim=-1) | ||
| view_states[view_ids, 7:] = new_velocities | ||
|
|
||
| rigid_object_collection.root_view.set_transforms(view_states[:, :7], indices=view_ids) | ||
| rigid_object_collection.root_view.set_velocities(view_states[:, 7:], indices=view_ids) | ||
| view_states_wp = wp.from_torch(view_states.contiguous(), dtype=wp.float32) | ||
| view_ids_wp = wp.from_torch(view_ids.contiguous().to(dtype=torch.int32), dtype=wp.int32) |
There was a problem hiding this comment.
Do not rely on states, this is outdated
| active_spawn_poses = groceries.reshape_data_to_view(active_spawn_poses) | ||
| cached_spawn_poses = groceries.reshape_data_to_view(cached_spawn_poses) | ||
| spawn_w = groceries.reshape_data_to_view(default_state_w).clone() | ||
| active_spawn_poses = reshape_data_to_view_3d(groceries, active_spawn_poses, 7, device) |
There was a problem hiding this comment.
I agree it should work
| random_masses = torch.rand(groceries.num_instances * num_objects, device=device) * 0.2 + 0.2 | ||
| groceries.root_view.set_masses(random_masses.cpu(), view_indices.cpu()) | ||
| groceries.root_view.set_disable_gravities((~groceries_mask).cpu(), indices=view_indices.cpu()) | ||
| random_masses_wp = wp.from_torch(random_masses.cpu().contiguous(), dtype=wp.float32) |
There was a problem hiding this comment.
They should not be needed anymore
| random_masses = torch.rand(groceries.num_instances * num_objects, device=device) * 0.2 + 0.2 | ||
| groceries.root_view.set_masses(random_masses.cpu(), view_indices.cpu()) | ||
| groceries.root_view.set_disable_gravities((~groceries_mask).cpu(), indices=view_indices.cpu()) | ||
| random_masses_wp = wp.from_torch(random_masses.cpu().contiguous(), dtype=wp.float32) |
There was a problem hiding this comment.
You shouldn't need to convert to warp
There was a problem hiding this comment.
Yes, I found set_masses_index() instead, which takes in torch.Tensor.
| view_indices_cpu_wp = wp.from_torch(view_indices.cpu().contiguous().to(torch.int32), dtype=wp.int32) | ||
| groceries.root_view.set_masses(random_masses_wp, view_indices_cpu_wp) |
There was a problem hiding this comment.
We have a built in method to set the mass
There was a problem hiding this comment.
Yes, I found set_masses_index() instead. However, I did not find a built-in method for set_disable_gravities. Is there one?
There was a problem hiding this comment.
No there is none, we don't provide wrappers for everything.
| xy = groceries.reshape_data_to_view(groceries.data.object_pos_w - scene.env_origins.unsqueeze(1))[:, :2] | ||
| body_pos_b = groceries.data.body_link_pos_w.torch - scene.env_origins.unsqueeze(1) | ||
| xy = reshape_data_to_view_3d(groceries, body_pos_b, 3, device)[:, :2] |
There was a problem hiding this comment.
Consider using the built on method
🤖 Update Review (121478a)Reviewed new commit Changes in
|
|
Update (c7665b5): Reviewed incremental changes since Summary of Changes
AssessmentThese changes properly migrate the demo scripts to use the updated RigidObjectCollection and ArticulationView APIs. The refactoring looks correct and maintains the intended functionality while adapting to the new data access patterns. |
YizeWang
force-pushed
the
yizew/fix-demo-scripts
branch
from
c7665b5 to
8250949
Compare
There was a problem hiding this comment.
Re-review After Recent Updates
Thanks for the updates addressing reviewer feedback! The recent commits show good progress:
✅ Improvements Made
set_masses_index()API - Now using the wrapper API instead ofroot_view.set_masses()- Removed
set_disable_gravities()- Simplified the reset logic - Wrapper APIs for state writes - Using
write_body_link_pose_to_sim_index()andwrite_body_com_velocity_to_sim_index()instead of direct root_view manipulation
🔍 Open Discussion Points
Based on @AntoineRichard's feedback, there's still discussion around:
-
Warp conversions - The
reshape_data_to_view_3dhelper still useswp.from_torch()/wp.to_torch(). @YizeWang mentioned this is required becausereshape_data_to_view_3dexpectswp.array. Could there be a torch-native path through the API? -
.contiguous()calls - The helper still calls.contiguous()on the torch tensor before warp conversion. Is this strictly necessary, or can it be avoided if the underlying API handles non-contiguous tensors?
These seem like API design questions worth clarifying with the maintainers to ensure the demo showcases best practices.
📝 Minor Observation
The PR description mentions fixes to quadcopter.py and h1_locomotion.py, but the current diff only shows changes to bin_packing.py. Were those changes moved to a separate PR, or is the description outdated?
Overall the direction looks good - just want to ensure the warp conversion question is resolved before merge.
Update (ca616ab): PR has been rebased to a single commit focusing on bin_packing.py. PR title now correctly reflects this as "Fix bin_packing.py". My earlier observation about missing quadcopter.py/h1_locomotion.py changes is now moot - the scope is clearer. The inline comments on h1_locomotion.py from earlier reviews are no longer applicable since that file is not part of this PR.
The warp conversion discussion points remain open but are API design questions for maintainers, not blockers. The code correctly uses the wrapper APIs as intended. ✅
Update (f6dc1d2): All previous open discussion points are now fully resolved. This commit removes Warp entirely from bin_packing.py:
- ✅ Warp conversions removed —
reshape_data_to_view_3dhelper deleted,import warp as wpremoved. All reshape logic is now pure PyTorch (.view()). - ✅
.contiguous()concern moot — no Warp interop means no need for contiguous torch tensors before conversion. - ✅ Tensor layout simplified — data stays in env-major format
(num_envs, num_objects, ...)throughout, with.view(-1, D)for flat indexing where needed.
No new issues found. The torch-native approach is cleaner and eliminates the Warp dependency entirely. LGTM 👍
Update (0c26464): Minor refinements, no new issues.
- ✅ Docstring fix —
states→view_statesinreset_object_collectionsdocstring to match the actual parameter name. - ✅
groceries_mask_helperrefactored — Now usesarange(num_objects * num_envs) % num_objectswith explicit.view(num_envs, -1)instead of implicit broadcasting. Functionally equivalent, slightly more explicit about the per-env structure.
No bugs or regressions introduced. LGTM 👍
Update (5ccd1b2): Minor code cleanup, one small nit.
- ✅ Inlined
out_bound_mask— The intermediate variable was merged into thetorch.nonzero()call. Slightly more compact, functionally identical. - ✅ Variable rename —
current_state_w→states_w. Cleaner naming. - 📝 Misplaced comment — The
# Increment countercomment ended up on thereset_object_collectionsline instead of on thecount += 1line. Minor nit, not a blocker.
No bugs or regressions. Still LGTM 👍
Update (d0a0c11): Major refactor separating poses and velocities into distinct tensors. Good design improvement, but introduces a typo bug:
- 🔴 Bug: Line 202
write_body_com_velocity_to_sim_index(body_velocities=vel)usesvelinstead ofvels(the parameter name). This will raiseNameErrorat runtime. - ✅ Previous misplaced comment nit — resolved (comment is now correctly placed above
count += 1). - ✅ Docstrings — properly updated to reflect new
poses/velssignature.
The split from a single (N, 13) state tensor to separate (num_envs, num_bodies, 7) poses + (num_envs, num_bodies, 6) vels is cleaner and type-safer. Just needs the typo fix (vel → vels) to work.
Update (197e2dd): All previous concerns resolved. No new issues.
- ✅
vel→velstypo fixed — The NameError bug from the previous review is resolved. - ✅ Naming refactored —
view_statessplit into separateposesandvelsparameters with proper docstrings reflecting env-major shapes(num_envs, num_bodies, 7)/(num_envs, num_bodies, 6). - ✅ Comments cleaned up — Inline comments and docstrings are accurate and consistent.
The code is clean, the wrapper API usage is correct, and the refactoring properly avoids the old env-major → body-major conversion overhead. LGTM 👍
YizeWang
force-pushed
the
yizew/fix-demo-scripts
branch
from
8250949 to
ca616ab
Compare
| # Vary the mass and gravity settings so cached objects stay parked. | ||
| random_masses = torch.rand(groceries.num_instances * num_objects, device=device) * 0.2 + 0.2 | ||
| groceries.root_view.set_masses(random_masses.cpu(), view_indices.cpu()) | ||
| groceries.root_view.set_disable_gravities((~groceries_mask).cpu(), indices=view_indices.cpu()) |
There was a problem hiding this comment.
set_disable_gravities does not have a wrapper API yet. This line can be removed without an end-to-end behavioral impacts because the position and velocity will be reset in the out-of-bounds process. I argue we should avoid this low-level data operation to make the extension to other backends cleanly.
There was a problem hiding this comment.
It's ok to keep it this way
| out_bound = torch.nonzero((~((xy >= bounds_xy[0]) & (xy <= bounds_xy[1])).all(dim=-1)).view(-1)).flatten() | ||
| if out_bound.numel(): | ||
| # Teleport stray objects back into the active stack to keep the bin tidy. | ||
| reset_object_collections(scene, "groceries", spawn_w, out_bound) |
There was a problem hiding this comment.
It could be an issue in the original implementation - passing spawn_w into reset_object_collections, because spawn_w is only updated at every other 250 frame, not frame-by-frame. It used to reset a wrong object!
| rigid_object_collection.root_view.set_transforms(view_states[:, :7], indices=view_ids) | ||
| rigid_object_collection.root_view.set_velocities(view_states[:, 7:], indices=view_ids) | ||
| rigid_object_collection.write_body_link_pose_to_sim_index(body_poses=poses) | ||
| rigid_object_collection.write_body_com_velocity_to_sim_index(body_velocities=vel) |
There was a problem hiding this comment.
🔴 Bug (d0a0c11): Typo — vel should be vels (the parameter name). This will raise NameError at runtime.
| rigid_object_collection.write_body_com_velocity_to_sim_index(body_velocities=vel) | |
| rigid_object_collection.write_body_com_velocity_to_sim_index(body_velocities=vels) |
| view_ids: torch.Tensor, | ||
| noise: bool = False, | ||
| ) -> None: | ||
| """Apply states to a subset of a collection, with optional noise. |
There was a problem hiding this comment.
According to @AntoineRichard's suggestion, we should not deprecate states because they are not contiguous anymore in the latest IsaacLab. We want to instruct users to modify positions and velocities separately instead of combining them as states.
| rigid_object_collection.root_view.set_transforms(view_states[:, :7], indices=view_ids) | ||
| rigid_object_collection.root_view.set_velocities(view_states[:, 7:], indices=view_ids) | ||
| rigid_object_collection.write_body_link_pose_to_sim_index(body_poses=poses) | ||
| rigid_object_collection.write_body_com_velocity_to_sim_index(body_velocities=vel) |
There was a problem hiding this comment.
🔴 Typo bug: vel should be vels (the function parameter). This will raise NameError at runtime.
YizeWang
changed the title
AntoineRichard
left a comment
AntoineRichard
left a comment
There was a problem hiding this comment.
LGTM thanks for the changes. Still would be good to track the issue with reshape_data_to_view only supporting warp.
Signed-off-by: Yize Wang <yizew@nvidia.com>
Signed-off-by: Yize Wang <yizew@nvidia.com>
Signed-off-by: Yize Wang <yizew@nvidia.com>
Signed-off-by: Yize Wang <yizew@nvidia.com>
Signed-off-by: Yize Wang <yizew@nvidia.com>
Signed-off-by: Yize Wang <yizew@nvidia.com>
Signed-off-by: Yize Wang <yizew@nvidia.com>
Signed-off-by: Yize Wang <yizew@nvidia.com>
Signed-off-by: Yize Wang <yizew@nvidia.com>
Signed-off-by: Yize Wang <yizew@nvidia.com>
YizeWang
force-pushed
the
yizew/fix-demo-scripts
branch
from
45274bd to
197e2dd
Compare
3 participants
Description
This PR updates the
scripts/demos/bin_packing.pydemo to use the currentRigidObjectCollectionbody pose and velocity APIs instead of manipulating flat PhysX root-view tensors directly.The demo now keeps body poses and body velocities in separate env-major buffers, avoiding the deprecated combined state pattern. It also switches mass updates to the collection-level
set_masses_index()API and usesbody_link_pos_w,body_link_pose_w, andbody_com_vel_wdata accessors for out-of-bounds recovery.No new dependencies are required.
Fixes
bin_packing.py.Type of change
Screenshots
Testing
./isaaclab.sh -p scripts/demos/bin_packing.py --num_envs 32./isaaclab.sh -fChecklist
pre-commitchecks with./isaaclab.sh -fconfig/extension.tomlfileCONTRIBUTORS.mdor my name already exists there