Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fixes grid origins in TerrainImporter to match Isaac Sim cloner #300

Merged
merged 14 commits into from
Mar 24, 2024
Merged

Fixes grid origins in TerrainImporter to match Isaac Sim cloner #300

merged 14 commits into from
Mar 24, 2024
+129 −6

Conversation

shafeef901
Copy link
Contributor

@shafeef901 shafeef901 commented Mar 20, 2024 •
edited
Loading

Description

The logic for grid cloning in grid_cloner.py (func: get_clone_transforms()) and in terrain_importer.py (func: _compute_env_origins_grid()) are different, consequently they give inconsistent values. This PR fixes it by updating the logic of _compute_env_origins_grid() to make it consistent with IsaacSim.

Fixes #287

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Checklist

  • I have run the pre-commit checks with ./orbit.sh --format
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • I have run all the tests with ./orbit.sh --test and they pass
  • I have updated the changelog and the corresponding version in the extension's config/extension.toml file
  • I have added my name to the CONTRIBUTORS.md or my name already exists there

@shafeef901
Grid Cloning in terrain_importer consistent with Isaac Sim (Issue #287)
ac8bfb4 
Signed-off-by: Shafeef Omar <shafeef901@gmail.com>
@shafeef901
Update CONTRIBUTORS.md
120ec7d 
Signed-off-by: Shafeef Omar <shafeef901@gmail.com>
@Mayankm96 Mayankm96 changed the title Fixes inconsistent grid cloning Fixes grid origins in TerrainImporter to match Isaac Sim cloner Mar 20, 2024
@Mayankm96
Copy link
Contributor

Mayankm96 commented Mar 20, 2024 •
edited
Loading

Can you please also update the changelog and version in extension.toml of the affected extensions?

Would it be possible to include a unit test as well to make sure if Isaac Sim changes behavior, we are up to speed with that. Thanks a lot!

@Mayankm96 Mayankm96 added bug Something isn't working enhancement New feature or request labels Mar 20, 2024
@Mayankm96 Mayankm96 self-requested a review March 20, 2024 19:43
shafeef901 and others added 7 commits March 21, 2024 10:39
@shafeef901
Update extension.toml
da83191 
Signed-off-by: Shafeef Omar <shafeef901@gmail.com>
@shafeef901
Update CHANGELOG.rst
12b19f4 
Signed-off-by: Shafeef Omar <shafeef901@gmail.com>
@shafeef901
Correct rounding of num_rows, num_cols w.r.t. grid cloner
cefa5e2 
Signed-off-by: Shafeef Omar <shafeef901@gmail.com>
@shafeef901
Adds unit test for env_origins match
f1c78f4 
Adds unit test that checks if the env_origins generated by the grid cloner of IsaacSim and that generated by TerrainImporter match. 

Signed-off-by: Shafeef Omar <shafeef901@gmail.com>
@shafeef901
Update CHANGELOG.rst
4b05231 
Signed-off-by: Shafeef Omar <shafeef901@gmail.com>
@shafeef901
Pulled from upstream and merged with forked branch with resolved conf…
e1ccf0a 
…lict in changelog
@shafeef901
Formatted code
25cfdf3
@shafeef901
Copy link
Contributor Author

Hi @Mayankm96 ,
I have added a unit test that compares the env_origins from both sources and then runs the sim for a couple of time steps.
Let me know if it looks good to be merged :)

@Mayankm96
Copy link
Contributor

Looks good. Thanks a lot! :)

@Mayankm96
Merge branch 'main' into main
5c8f65c 
Signed-off-by: Mayank Mittal <12863862+Mayankm96@users.noreply.github.com>
@Mayankm96
Copy link
Contributor

Ah @shafeef901 can you please also update the extension.toml file with the version update? Thanks a lot for the test and the fix!

@shafeef901
Copy link
Contributor Author

Done!

@Mayankm96
Apply suggestions from code review
8a3bc44 
Signed-off-by: Mayank Mittal <12863862+Mayankm96@users.noreply.github.com>
@Mayankm96 Mayankm96 merged commit f4f9358 into isaac-sim:main Mar 24, 2024
Mayankm96 added a commit that referenced this pull request Mar 24, 2024
@shafeef901 @Mayankm96
Fixes grid origins in TerrainImporter to match Isaac Sim cloner (#300)
c05d8fd 
The logic for grid cloning in Isaac Sim GridCloner (func:
`get_clone_transforms()`) and in TerrainImporter.py (func:
`_compute_env_origins_grid()`) are different. Consequently, they give
inconsistent values.

This PR fixes the TerrainImporter by updating the logic of
`_compute_env_origins_grid()` to make it consistent with IsaacSim.

Fixes #287

- Bug fix (non-breaking change which fixes an issue)

- [x] I have run the [`pre-commit` checks](https://pre-commit.com/) with
`./orbit.sh --format`
- [x] I have made corresponding changes to the documentation
- [x] My changes generate no new warnings
- [x] I have added tests that prove my fix is effective or that my
feature works
- [x] I have run all the tests with `./orbit.sh --test` and they pass
- [x] I have updated the changelog and the corresponding version in the
extension's `config/extension.toml` file
- [x] I have added my name to the `CONTRIBUTORS.md` or my name already
exists there

---------

Signed-off-by: Shafeef Omar <shafeef901@gmail.com>
Co-authored-by: Mayank Mittal <12863862+Mayankm96@users.noreply.github.com>
Mayankm96 added a commit that referenced this pull request Mar 24, 2024
@shafeef901 @Mayankm96
Fixes grid origins in TerrainImporter to match Isaac Sim cloner (#300)
4de816d 
The logic for grid cloning in Isaac Sim GridCloner (func:
`get_clone_transforms()`) and in TerrainImporter.py (func:
`_compute_env_origins_grid()`) are different. Consequently, they give
inconsistent values.

This PR fixes the TerrainImporter by updating the logic of
`_compute_env_origins_grid()` to make it consistent with IsaacSim.

Fixes #287

- Bug fix (non-breaking change which fixes an issue)

- [x] I have run the [`pre-commit` checks](https://pre-commit.com/) with
`./orbit.sh --format`
- [x] I have made corresponding changes to the documentation
- [x] My changes generate no new warnings
- [x] I have added tests that prove my fix is effective or that my
feature works
- [x] I have run all the tests with `./orbit.sh --test` and they pass
- [x] I have updated the changelog and the corresponding version in the
extension's `config/extension.toml` file
- [x] I have added my name to the `CONTRIBUTORS.md` or my name already
exists there

---------

Signed-off-by: Shafeef Omar <shafeef901@gmail.com>
Co-authored-by: Mayank Mittal <12863862+Mayankm96@users.noreply.github.com>
@Mayankm96
Copy link
Contributor

Thanks a lot for the MR. Merged with some small fixes to the test.

Mainly, instead of assertTrue(torch.allclose), you can simply do torch.testing.all_close. Also since you used the sim context through context manager, you don't need to write setUp and tearDown anymore :)

Mayankm96 added a commit that referenced this pull request Mar 24, 2024
@shafeef901 @Mayankm96
Fixes grid origins in TerrainImporter to match Isaac Sim cloner (#300)
3e7a470 
The logic for grid cloning in Isaac Sim GridCloner (func:
`get_clone_transforms()`) and in TerrainImporter.py (func:
`_compute_env_origins_grid()`) are different. Consequently, they give
inconsistent values.

This PR fixes the TerrainImporter by updating the logic of
`_compute_env_origins_grid()` to make it consistent with IsaacSim.

Fixes #287

- Bug fix (non-breaking change which fixes an issue)

- [x] I have run the [`pre-commit` checks](https://pre-commit.com/) with
`./orbit.sh --format`
- [x] I have made corresponding changes to the documentation
- [x] My changes generate no new warnings
- [x] I have added tests that prove my fix is effective or that my
feature works
- [x] I have run all the tests with `./orbit.sh --test` and they pass
- [x] I have updated the changelog and the corresponding version in the
extension's `config/extension.toml` file
- [x] I have added my name to the `CONTRIBUTORS.md` or my name already
exists there

---------

Signed-off-by: Shafeef Omar <shafeef901@gmail.com>
Co-authored-by: Mayank Mittal <12863862+Mayankm96@users.noreply.github.com>
Mayankm96 added a commit that referenced this pull request Mar 24, 2024
@shafeef901 @Mayankm96
Fixes grid origins in TerrainImporter to match Isaac Sim cloner (#300)
cfcabba 
The logic for grid cloning in Isaac Sim GridCloner (func:
`get_clone_transforms()`) and in TerrainImporter.py (func:
`_compute_env_origins_grid()`) are different. Consequently, they give
inconsistent values.

This PR fixes the TerrainImporter by updating the logic of
`_compute_env_origins_grid()` to make it consistent with IsaacSim.

Fixes #287

- Bug fix (non-breaking change which fixes an issue)

- [x] I have run the [`pre-commit` checks](https://pre-commit.com/) with
`./orbit.sh --format`
- [x] I have made corresponding changes to the documentation
- [x] My changes generate no new warnings
- [x] I have added tests that prove my fix is effective or that my
feature works
- [x] I have run all the tests with `./orbit.sh --test` and they pass
- [x] I have updated the changelog and the corresponding version in the
extension's `config/extension.toml` file
- [x] I have added my name to the `CONTRIBUTORS.md` or my name already
exists there

---------

Signed-off-by: Shafeef Omar <shafeef901@gmail.com>
Co-authored-by: Mayank Mittal <12863862+Mayankm96@users.noreply.github.com>
fatimaanes pushed a commit to fatimaanes/omniperf that referenced this pull request Aug 8, 2024
@shafeef901 @Mayankm96
Fixes grid origins in TerrainImporter to match Isaac Sim cloner (isaa…
7431f5b 
…c-sim#300)

The logic for grid cloning in Isaac Sim GridCloner (func:
`get_clone_transforms()`) and in TerrainImporter.py (func:
`_compute_env_origins_grid()`) are different. Consequently, they give
inconsistent values.

This PR fixes the TerrainImporter by updating the logic of
`_compute_env_origins_grid()` to make it consistent with IsaacSim.

Fixes isaac-sim#287

- Bug fix (non-breaking change which fixes an issue)

- [x] I have run the [`pre-commit` checks](https://pre-commit.com/) with
`./orbit.sh --format`
- [x] I have made corresponding changes to the documentation
- [x] My changes generate no new warnings
- [x] I have added tests that prove my fix is effective or that my
feature works
- [x] I have run all the tests with `./orbit.sh --test` and they pass
- [x] I have updated the changelog and the corresponding version in the
extension's `config/extension.toml` file
- [x] I have added my name to the `CONTRIBUTORS.md` or my name already
exists there

---------

Signed-off-by: Shafeef Omar <shafeef901@gmail.com>
Co-authored-by: Mayank Mittal <12863862+Mayankm96@users.noreply.github.com>
iamdrfly pushed a commit to iamdrfly/IsaacLab that referenced this pull request Nov 21, 2024
@shafeef901 @Mayankm96
Fixes grid origins in TerrainImporter to match Isaac Sim cloner (isaa…
29f24ed 
…c-sim#300)

The logic for grid cloning in Isaac Sim GridCloner (func:
`get_clone_transforms()`) and in TerrainImporter.py (func:
`_compute_env_origins_grid()`) are different. Consequently, they give
inconsistent values.

This PR fixes the TerrainImporter by updating the logic of
`_compute_env_origins_grid()` to make it consistent with IsaacSim.

Fixes isaac-sim#287

- Bug fix (non-breaking change which fixes an issue)

- [x] I have run the [`pre-commit` checks](https://pre-commit.com/) with
`./orbit.sh --format`
- [x] I have made corresponding changes to the documentation
- [x] My changes generate no new warnings
- [x] I have added tests that prove my fix is effective or that my
feature works
- [x] I have run all the tests with `./orbit.sh --test` and they pass
- [x] I have updated the changelog and the corresponding version in the
extension's `config/extension.toml` file
- [x] I have added my name to the `CONTRIBUTORS.md` or my name already
exists there

---------

Signed-off-by: Shafeef Omar <shafeef901@gmail.com>
Co-authored-by: Mayank Mittal <12863862+Mayankm96@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mayankm96 Mayankm96 Mayankm96 approved these changes

Assignees
No one assigned
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Proposal] Inconsistent Grid Cloning
2 participants
@shafeef901 @Mayankm96