Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a new spatial event functor to sample planner placement positions from a 2D grid, and updates the event-functors documentation to include newly available functors (including the new sampler).
Changes:
- Add
planner_grid_cell_samplerfunctor for grid-based object placement sampling. - Update event functor docs to list additional physics/visual/spatial/asset functors, including the new sampler.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
embodichain/lab/gym/envs/managers/randomization/spatial.py |
Introduces planner_grid_cell_sampler functor for sampling grid cells and placing rigid objects. |
docs/source/overview/gym/event_functors.md |
Documents additional event functors and adds an entry for planner_grid_cell_sampler. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| obj_positions[obj_id][env_id] = torch.tensor( | ||
| [x, y, z], dtype=torch.float32, device=env.device | ||
| ) |
There was a problem hiding this comment.
obj_positions[obj_id] is shaped (len(env_ids), 3), but it’s indexed with env_id (the global env index). If env_ids isn’t 0..len(env_ids)-1 (e.g., [2, 5]), this will be out-of-bounds or write the wrong row. Index by the loop-local row (e.g., enumerate env_ids) or build an env_id→row mapping.
| elif not isinstance(env_ids, torch.Tensor): | ||
| env_ids = torch.tensor(env_ids, device=env.device) | ||
|
|
||
| self.reset(env_ids) |
There was a problem hiding this comment.
The docstring says cells “will not be resampled until the grid is reset”, but __call__ unconditionally calls self.reset(env_ids) here, clearing occupancy on every invocation. If the intent is persistent no-replacement across resets, remove this call and let the EventManager/caller trigger reset() explicitly.
| self.reset(env_ids) |
| if len(available_cells[0]) == 0: | ||
| logger.log_warning( | ||
| f"No available cells in grid for environment {env_id_int}. All cells occupied." | ||
| ) | ||
| break | ||
|
|
There was a problem hiding this comment.
If the grid has no available cells, the code breaks out of the per-object loop, leaving later objects in that env without a sampled position. Because obj_positions is initialized to zeros, those unplaced objects will later get a zero position applied (teleporting to origin). Track placement success per object/env and skip pose updates for unplaced entries (or preserve current pose).
| self._grid_state[env_id_int] = torch.zeros( | ||
| rows, cols, device=env.device, dtype=torch.uint8 | ||
| ) | ||
| self._grid_cell_sizes[env_id_int] = (cell_width, cell_height) |
There was a problem hiding this comment.
self._grid_cell_sizes is written (self._grid_cell_sizes[env_id_int] = ...) but never read anywhere in this functor. If it isn’t needed, remove it; if it is intended for downstream logic, add the corresponding usage/exposure so the extra state is justified.
| self._grid_cell_sizes[env_id_int] = (cell_width, cell_height) |
| pose = rigid_object.get_local_pose()[env_ids] | ||
| pose[:, 0:3] = obj_positions[obj_id] | ||
|
|
There was a problem hiding this comment.
This assignment applies obj_positions[obj_id] to all env_ids for every object, even if some environments didn’t successfully sample a cell for that object (e.g., grid full and loop broke early). That can overwrite poses with default zeros. Consider masking the pose update per-env based on which placements succeeded.
Description
Add a spatial functor to perform spatial planner placement sampling.
Type of change
Checklist
black .command to format the code base.