Fix croodinate pickment - #514
Conversation
Greptile SummaryThe PR updates coordinated-pickment assets and gripper parameters, versions the antipodal cache, and expands antipodal ray origins to recover missing contact pairs.
Confidence Score: 4/5The PR should not merge until optional tutorial assets are resolved lazily so unrelated object selections do not fail during import. The module-level preset construction unconditionally enters the download path for two new assets before command-line selection, while the remaining accepted feedback concerns explicit public API declarations. Files Needing Attention: scripts/tutorials/atomic_action/coordinated_pickment.py; embodichain/data/assets/obj_assets.py
|
| Filename | Overview |
|---|---|
| embodichain/data/assets/obj_assets.py | Adds two downloadable dataset classes, but leaves their public exports implicit rather than declaring all. |
| embodichain/toolkits/graspkit/pg_grasp/antipodal_generator.py | Versions antipodal cache filenames so results generated by the revised algorithm do not reuse stale cache entries. |
| embodichain/toolkits/graspkit/pg_grasp/antipodal_sampler.py | Adds a second ray-origin batch intended to recover contact pairs missed by the original raycast range. |
| scripts/tutorials/atomic_action/coordinated_pickment.py | Adds new presets and changes the default scene, but eagerly resolves both downloadable assets during every module import. |
| scripts/tutorials/atomic_action/tutorial_utils.py | Adjusts gripper sampling geometry and allows callers to provide an initial robot joint configuration. |
Prompt To Fix All With AI
### Issue 1
scripts/tutorials/atomic_action/coordinated_pickment.py:139
**Assets resolve during module import**
If either new asset is absent from the local cache and the download service is unavailable, constructing `OBJECT_PRESETS` invokes `get_data_path` before argument parsing and aborts the import. This prevents the tutorial from running even when the user selects an existing local object such as `pencil` that does not require either new asset.
### Issue 2
embodichain/data/assets/obj_assets.py:243
**Public asset exports remain implicit**
`PlasticTray` and `WaterBasin` are new public dataset classes, but the module does not declare them through `__all__` as required for public modules. Because the assets package imports this module with a wildcard, its exported API remains dependent on implementation-level names rather than an explicit declaration.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "update" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| ), | ||
| "water_basin": PickmentObjectPreset( | ||
| label="water_basin", | ||
| mesh_path=get_data_path("WaterBasin/water_basin.glb"), |
There was a problem hiding this comment.
If either new asset is absent from the local cache and the download service is unavailable, constructing OBJECT_PRESETS invokes get_data_path before argument parsing and aborts the import. This prevents the tutorial from running even when the user selects an existing local object such as pencil that does not require either new asset.
Prompt To Fix With AI
This is a comment left during a code review.
Path: scripts/tutorials/atomic_action/coordinated_pickment.py
Line: 139
Comment:
**Assets resolve during module import**
If either new asset is absent from the local cache and the download service is unavailable, constructing `OBJECT_PRESETS` invokes `get_data_path` before argument parsing and aborts the import. This prevents the tutorial from running even when the user selects an existing local object such as `pencil` that does not require either new asset.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
| path = EMBODICHAIN_DEFAULT_DATA_ROOT if data_root is None else data_root | ||
|
|
||
| super().__init__(prefix, data_descriptor, path) | ||
|
|
There was a problem hiding this comment.
PlasticTray and WaterBasin are new public dataset classes, but the module does not declare them through __all__ as required for public modules. Because the assets package imports this module with a wildcard, its exported API remains dependent on implementation-level names rather than an explicit declaration.
Context Used: CLAUDE.md (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: embodichain/data/assets/obj_assets.py
Line: 243
Comment:
**Public asset exports remain implicit**
`PlasticTray` and `WaterBasin` are new public dataset classes, but the module does not declare them through `__all__` as required for public modules. Because the assets package imports this module with a wildcard, its exported API remains dependent on implementation-level names rather than an explicit declaration.
**Context Used:** CLAUDE.md ([source](https://github.com/dexforce/embodichain/blob/main/CLAUDE.md))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Pull request overview
This PR targets the coordinated pickment tutorial and the antipodal grasp pipeline by adjusting tutorial gripper parameters, adding new downloadable object assets, and changing antipodal sampling/caching behavior to address missing contact-pair annotations.
Changes:
- Update tutorial gripper constants and allow overriding UR5+gripper initial joint positions (
init_qpos) in tutorial helpers. - Extend the coordinated pickment tutorial with new object presets (PlasticTray/WaterBasin) and tweak scene/options parameters.
- Adjust antipodal raycasting behavior and invalidate old antipodal caches via a versioned cache key; register new object asset datasets.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/tutorials/atomic_action/tutorial_utils.py | Adds init_qpos plumbing and tweaks grasp/collision sampling parameters used by tutorials. |
| scripts/tutorials/atomic_action/coordinated_pickment.py | Updates the demo scene/object presets and coordinated pickment options. |
| embodichain/toolkits/graspkit/pg_grasp/antipodal_sampler.py | Changes raycast sampling strategy to address missing antipodal contact pairs. |
| embodichain/toolkits/graspkit/pg_grasp/antipodal_generator.py | Versions the cache filename to invalidate stale cached annotations after sampler changes. |
| embodichain/data/assets/obj_assets.py | Adds dataset descriptors for PlasticTray and WaterBasin assets. |
Suppressed comments (1)
scripts/tutorials/atomic_action/coordinated_pickment.py:150
- Calling get_data_path() while building OBJECT_PRESETS runs dataset resolution/download at module import time (even when a different --object is selected). Prefer storing the relative asset path here and letting resolve_cached_data_path() handle resolution when the object is created.
mesh_path=get_data_path("PlasticTray/plastic_tray.glb"),
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| ), | ||
| "water_basin": PickmentObjectPreset( | ||
| label="water_basin", | ||
| mesh_path=get_data_path("WaterBasin/water_basin.glb"), |
| label=preset.label, | ||
| n_sample=args.n_sample, | ||
| # n_sample = 1000, | ||
| force_reannotate=args.force_reannotate, |
| sim: SimulationManager, | ||
| init_pos: Sequence[float] = (0.0, 0.0, 0.0), | ||
| init_qpos: Sequence[float] | None = None, | ||
| ) -> Robot: |
| qpos = ( | ||
| [0.0, -1.57, 1.57, -1.57, -1.57, 0.0, 0.0, 0.0] | ||
| if init_qpos is None | ||
| else list(init_qpos) | ||
| ) |
| ray_origin_2 = sample_points - 2.0 * self.cfg.max_length * ray_direc | ||
| all_ray_origin = torch.cat([ray_origin, ray_origin_2], dim=0) | ||
| all_ray_direc = torch.cat([ray_direc, ray_direc], dim=0) | ||
| all_surface_origin = torch.cat([sample_points, sample_points], dim=0) | ||
| return self._get_raycast_result( |
Description
scripts/tutorials/atomic_action/coordinated_pickment.pyType of change
Checklist
black .command to format the code base.