-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Adds locomanipulation data generation via. disjoint navigation #3259
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adds locomanipulation data generation via. disjoint navigation #3259
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made a first pass. Will go more in depth again later.
scripts/imitation_learning/disjoint_navigation/display_dataset.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR needs to run the lint check as it is missing necessary docstrings
| 1. Record a static manipulation trajectory of "picking up" and "dropping off" an object. | ||
| In this phase, the robot base is stationary. This is done by human teleoperation. | ||
|
|
||
| 2. Augment the static manipulation trajectory using mimic data generation pipeline. This will |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| 2. Augment the static manipulation trajectory using mimic data generation pipeline. This will | |
| 2. Augment the in-place manipulation trajectory using mimic data generation pipeline. This will |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@michaellin6 can you please confirm? Is the data collected using a floating base config or fixed base config?
| --output_file_name=dataset_generated_disjoint_nav.hdf5 | ||
| If you are using a different trajectory, you will need to change some parameters. Notably, you will need to set |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| If you are using a different trajectory, you will need to change some parameters. Notably, you will need to set | |
| If you are using a different trajectory, you will need to change some parameters. Notably, you will need to set |
| If you are using a different trajectory, you will need to change some parameters. Notably, you will need to set | ||
|
|
||
| - --lift_step - The step index where the robot has finished grasping the object and is ready to lift it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In mimic, they use the object height change for this purpose. Can we do the same here? This is quite manual.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea. Since we haven't tested this yet, I would prefer to add this post-merge. We can expose the ability to do this manually or automatically.
| @@ -0,0 +1,25 @@ | |||
| # Disjoint Navigation | |||
|
|
|||
| This folder contains code for running the disjoint navigation data generation script. This assumes that you have collected a static manipulation dataset. | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| This folder contains code for running the disjoint navigation data generation script. This assumes that you have collected a static manipulation dataset. | |
| This folder contains code for running the disjoint navigation data generation script. This assumes that you have collected a static manipulation dataset. |
| num_hand_joints=14, | ||
| show_ik_warnings=False, | ||
| variable_input_tasks=[ | ||
| FrameTask( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume this needs to be updated with @michaellin6's latest update.
| end-to-end locomanipulation trajectories by combining the static manipulation sequences with | ||
| path planning. | ||
|
|
||
| Step 1 - Static manipulation teleoperation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here you can link the teleop_imitation.rst sections that have more detailed instructions on this
| .. code:: bash | ||
| ./isaaclab.sh -p \ | ||
| scripts/imitation_learning/disjoint_navigation/replay.py \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it may be confusing for a user that the script is called replay. Can we name it something else? Like generate_navigation?
|
|
||
| ```bash | ||
| ./isaaclab.sh -p \ | ||
| scripts/imitation_learning/disjoint_navigation/replay.py \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above. Replay seems misleading.
| import isaaclab.utils.math as math_utils | ||
|
|
||
|
|
||
| def transform_to_matrix(transform: torch.Tensor): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is already tools in Isaac Lab for this. check out unmake_pose in source/isaaclab/isaaclab/utils/math.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function uses unmake_pose under the hood. It's a helper function to reduce repeated / copied code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to vote against the shim wrapper that introduces another naming-convention in doing common math.
if it just saves few lines just use unmake_pose, don't make a wrapper
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, we should just use unmake_pose directly in the code to avoid another abstraction layer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have reduced these functions as much as possible and use make_pose/unmake_pose where possible
| return pose_matrix | ||
|
|
||
|
|
||
| def transform_from_matrix(matrix: torch.Tensor): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is already tools in Isaac Lab for this. check out make_pose in source/isaaclab/isaaclab/utils/math.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function uses make_pose under the hood.
| return torch.cat([pos, quat], dim=-1) | ||
|
|
||
|
|
||
| def transform_inv(transform: torch.Tensor): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is already tools in Isaac Lab for this. check out pose_inv in source/isaaclab/isaaclab/utils/math.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @michaellin6 . This function actually uses pose_inv under the hood. I started by using the math functions directly. This quickly became cumbersome and there was a lot of repeated code. These transform_ functions were added organically to remove code redundancy.
| return transform_from_matrix(matrix) | ||
|
|
||
|
|
||
| def transform_mul(transform_a, transform_b): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for all these math functions, please check that they are already available in IL
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to above. These methods use existing utilities under the hood. They are helper functions added to remove a bit of duplicate code inside the pipeline, and standardize on a [translation,quaternion] format for pose to reduce bookkeeping.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It maybe acceptable to have a one or two shim wrapper for cumbersome utility, but do we need these many? and most of them seem to only save 1 or 2 lines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we have simplified the math utility functions. Still need some of them as the navigation code works with pose vectors [translation, quaternion], rather than homogeneous transforms.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, this common file is kind of large and seems like a big mix of many components. Can we split it up a bit? Some seem to be utilities, and some seem to be abstract classes which are different.
|
|
||
|
|
||
| class G1DisjointNavScenario(DisjointNavScenario): | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are we defining a new environment in script? shoudn't that be apart of isaaclab_task?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this has been moved to isaaclab_mimic, as the env is really just for data generation purpose, we found that to be more appropriate.
|
|
||
| from isaaclab.app import AppLauncher | ||
|
|
||
| # Launch Isaac Lab |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is there a replay.py and there is also a tools/replay_demos.py?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
script has been renamed and refactored. Now it is called generate_navigation.py and we have improved the organization of the script.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this probably should no go into github, maybe nucleus
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is in nucleus. Its outdated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR. It does looks like an interesting environment and a lot of cool utilities.
The main concern is how the files and utility are introduced diverge quite a lot from the rest of isaaclab components. It almost feel like a new repository build on top of isaac lab rather than something that sits nicely in isaaclab and extend features. I was not prepared to see so enviornment, mdps, and policy replay all defined under scripts. Other changes to the core seems to have some duplicates with other PR, and it is hard to judge what changes it make. If this PR depends on other PRs, we might need to merge other PR first then spent more time reviewing this.
Thanks for the PR again,
but for above reason it doesn't quite make sense to merge it in main at current stage.
I am not sure how much work this PR was done with in loop with other folks in Mimic Team. Please work with them closely to ensure this MR is an extension improves on existing functionity and feature, rather than whole new pipeline. If it is whole new pipeline we need better code quality and discussion to make sure it is done right.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR will need a lot of work, please review the repo structure (https://isaac-sim.github.io/IsaacLab/main/source/overview/developer-guide/repo_structure.html) and contribution guidelines (https://isaac-sim.github.io/IsaacLab/main/source/refs/contributing.html#) to get an understanding of where files should belong and the coding style guides we follow.
Many of the scripts are lacking comments and docstrings, making it very difficult to understand. Would be great to do a pass through everything to make sure we have all the components outlined in the contribution guidelines covered.
docs/index.rst
Outdated
| source/overview/augmented_imitation | ||
| source/overview/showroom | ||
| source/overview/simple_agents | ||
| source/overview/locomanipulation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we move this under the imitation learning section - https://isaac-sim.github.io/IsaacLab/main/source/overview/imitation-learning/index.html? it seems more relevant to teleoperation and imitation learning
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is more of a placeholder. We will move all documentation in this PR into teleop_imitation.rst
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moved docs to appropriate location
| import isaaclab.utils.math as math_utils | ||
|
|
||
|
|
||
| def transform_to_matrix(transform: torch.Tensor): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, we should just use unmake_pose directly in the code to avoid another abstraction layer
| """Launch Isaac Sim Simulator first.""" | ||
|
|
||
|
|
||
| import enum |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is the purpose of this script? it doesn't seem like a script we can run directly. the Scripts folder should hold scripts we can run directly from python. this seems to belong more into the mimic extension or its own extension
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this was refactored into multiple components and split up into different files for path planning, navigation base classes and and scene utilities
| @@ -0,0 +1,665 @@ | |||
| # Copyright (c) 2022-2025, The Isaac Lab Project Developers (https://github.com/isaac-sim/IsaacLab/blob/main/CONTRIBUTORS.md). | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should pick one license for this, if it's for mimic, we can use Apach
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Picked Apache V2
48ac9f7 to
da6be51
Compare
da6be51 to
9d6b321
Compare
|
Hi @kellyguo11 @ooctipus, Thank you for taking the time to review this PR. I have force-pushed an update that now places these changes on top of #3150. I hope this makes the diff cleaner. For context: the changes that this PR introduces are currently all contained in scripts/imitation_learning/disjoint_navigation/. We discussed with the mimic team that this data generation pipeline would be a separate script from the existing mimic pipeline, reusing the existing teleoperation and mimic data generation code to create input datasets for our pipeline. I'm happy to address any restructuring needs to better align with Isaac Lab's format. Here are two organizational approaches for your consideration: Current structure: scripts/imitation_learning/disjoint_navigation/ Option 1: Integrated with Isaac Lab Tasks StructureProposed reorganization: Environment definitionsource/isaaclab_tasks/isaaclab_tasks/manager_based/locomanipulation/ Data generation script and utilitiesscripts/imitation_learning/disjoint_nav/ Option 2: Integrated with Isaac Lab Mimic StructureProposed reorganization: Data generation scriptscripts/imitation_learning/isaaclab_mimic/disjoint_nav/ Helper functions and utilitiessource/isaaclab_mimic/isaaclab_mimic/disjoint_nav/ Environment definitionsource/isaaclab_tasks/isaaclab_tasks/manager_based/locomanipulation/ Both options include plans to add comprehensive documentation, type hints, and adherence to Isaac Lab code style guidelines. Looking forward to your thoughts. |
42cc2a0 to
6d85bf3
Compare
scripts/imitation_learning/disjoint_navigation/generate_navigation.py
Outdated
Show resolved
Hide resolved
scripts/imitation_learning/disjoint_navigation/generate_navigation.py
Outdated
Show resolved
Hide resolved
scripts/imitation_learning/disjoint_navigation/generate_navigation.py
Outdated
Show resolved
Hide resolved
scripts/imitation_learning/disjoint_navigation/generate_navigation.py
Outdated
Show resolved
Hide resolved
| @@ -0,0 +1,8 @@ | |||
| # Copyright (c) 2024-2025, The Isaac Lab Project Developers (https://github.com/isaac-sim/IsaacLab/blob/main/CONTRIBUTORS.md). | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in the proposal, I believe the environment was placed under isaaclab_tasks. is there a reason why this is now under isaaclab_mimic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We took this cue from the existing isaaclab_mimic codebase, where the "mimic-specific" environments are defined in isaaclab_mimic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, should we group this together with isaaclab_mimic/envs or is it preferred to keep it separate?
source/isaaclab_mimic/isaaclab_mimic/navigation_path_planner/disjoint_nav/disjoint_nav_data.py
Show resolved
Hide resolved
|
can you please run also seeing some test failures in CI currently: |
7fd270a to
70b3e90
Compare
Fixed the unit tests in the upstream PR, and rebased this PR. Also, checked that formatting is passing. |
70b3e90 to
0a7198c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good to me. From the refactoring I see that:
- We are now using the task from
locomanipulation_g1_envfrom isaaclab_tasks - We've moved the environments and config files used for navigation data generation into isaaclab_mimic which is more appropriate as that is where other mimic data generation envrionments and config files are.
- Reduced the code in scripts/ to really only scripts
- Improved the code documentation and readability.
LGTM. If other reviewers can also take a look, it would be great.
0a7198c to
0aa36fa
Compare
2920d88 to
81830fb
Compare
- Add data generation scripts to add navigation data to manipulation datasets - Implement DisjointNavEnv base class and G1-specific environment - Add path planning utilities with occupancy mapping and trajectory generation - Include visualization tools for trajectory analysis and debugging - Update documentation for imitation learning and locomanipulation workflows Co-authored-by: John Welsh <jwelsh@nvidia.com> Co-authored-by: Michael Lin <michalin@nvidia.com> Co-authored-by: Huihua Zhao <huihuaz@nvidia.com>
fb59c25 to
1a5cca1
Compare
Description
This PR adds locomanipulation data generation via disjoint navigation. It allows the user to replay static manipulation data recorded with the G1 through an end to end pick-up -> navigate -> drop-off pipeline.
The code for this PR is contained in the folder
scripts/isaaclab/imitation_learning/disjoint_navigation, and does not modify existing IsaacLab extensions.The code is as follows:
replay.py- Main entry script for loading the static-manipulation dataset and generating locomanipulation datasetcommon.py- Common classes and abstract base classes for the data generation pipeline. This code may be re-used by different scenarios.occupancy_map.py- Helper class and methods for working with occupancy maps.path_utils.py- Helper class used by path controller for following a pathdisplay_dataset.py-- Utility script for visualizing generated trajectories with matplotlibg1_29dof.py- The example scenario using G1 + WBC with random forklifts and boxes. This may be copied modified by end users to support their own scenarios.visualization.py- Code to visualize occupancy map in USD stageType of change
Screenshots
Please attach before and after screenshots of the change if applicable.
Checklist
pre-commitchecks with./isaaclab.sh --formatconfig/extension.tomlfileCONTRIBUTORS.mdor my name already exists there