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.

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

[Proposal] Inconsistent Grid Cloning #287

Closed
2 of 3 tasks
shafeef901 opened this issue Mar 14, 2024 · 4 comments · Fixed by #300
Closed
2 of 3 tasks

[Proposal] Inconsistent Grid Cloning #287

shafeef901 opened this issue Mar 14, 2024 · 4 comments · Fixed by #300
Labels
bug Something isn't working enhancement New feature or request

Comments

@shafeef901
Copy link
Contributor

shafeef901 commented Mar 14, 2024

Proposal

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. Is there any particular reasoning/need for it?

Motivation

While initializing a new entity, I'd like to save some values from the environments during its initialization. However, during the initialization of an entity, the grid locations are based on the env_origins generated by grid_cloner.py but later overridden by the one in terrain_importer.py. This feature can be beneficial in avoiding recomputation of values that do not change in an environment, hence speeding up the training. In my case, these values were the poses of sampled points on a static object in the world frame. These values can be stored during initialization, and the distances from the robot base to these points could be updated as required.

Alternatives

I'm currently using a quadruped on a flat ground interacting with a static object. Hence, updating _compute_env_origins_grid() as follows would suffice to make the logic match:

def _compute_env_origins_grid(self, num_envs: int, env_spacing: float) -> torch.Tensor:
    """Compute the origins of the environments in a grid based on configured spacing."""
  
    num_rows = int(np.ceil(np.sqrt(num_envs)))  # Calculate number of rows based on total environments
    num_cols = int(np.ceil(num_envs / num_rows))  # Adjust number of columns based on rows
  
    env_origins = torch.zeros(num_envs, 3, device=self.device)
  
    for i in range(num_envs):
        row = i // num_cols
        col = i % num_cols
  
        # Calculate position with row-first approach
        x = -(row - (num_rows - 1) / 2) * env_spacing
        y = (col - (num_cols - 1) / 2) * env_spacing
  
        env_origins[i, 0] = x
        env_origins[i, 1] = y
        env_origins[i, 2] = 0.0
  
    return env_origins

Another possible solution would be to ensure that all entities are added to the scene only after the final grid configuration is computed, i.e., in this case, after _compute_env_origins_grid() is called. This would be a more permanent solution.

Checklist

  • I have checked that there is no similar issue in the repo (required)

Acceptance Criteria

  • Update logic of _compute_env_origins_grid() in terrain_importer.py
  • Add entities to the scene only after the final grid configuration is computed
@Mayankm96 Mayankm96 added bug Something isn't working enhancement New feature or request labels Mar 20, 2024
@Mayankm96
Copy link
Contributor

Hi @shafeef901 ,

There is no reason for this inconsistency. We wanted to avoid having a for loop. Would be happy to have a fix that makes it consistent without looping over all the environments. Maybe the indexing order in the meshgrid function does the trick?

@shafeef901
Copy link
Contributor Author

shafeef901 commented Mar 20, 2024

Thank you for your response. As suggested, changing the indexing order to its default 'ij' and a slight change in the logic (negation in the row dimension) does the job as below:

def _compute_env_origins_grid(self, num_envs: int, env_spacing: float) -> torch.Tensor:
    """Compute the origins of the environments in a grid based on configured spacing"""
    # create tensor based on number of environments
    env_origins = torch.zeros(num_envs, 3, device=self.device)
    # create a grid of origins
    num_rows = int(np.ceil(np.sqrt(num_envs))) 
    num_cols = int(np.ceil(num_envs / num_rows))  
    ii, jj = torch.meshgrid(torch.arange(num_rows, device=self.device), torch.arange(num_cols, device=self.device), indexing="ij")
    env_origins[:, 0] = -(ii.flatten()[:num_envs] - (num_rows - 1) / 2) * env_spacing
    env_origins[:, 1] = (jj.flatten()[:num_envs] - (num_cols - 1) / 2) * env_spacing
    env_origins[:, 2] = 0.0
    return env_origins

This logic also looks identical to the previously proposed loop-based approach above.

@Mayankm96
Copy link
Contributor

That's great. We should merge this to be consistent with Isaac Sim (until they also move to meshgrid logic). Could you please send an MR? Or should we handle this? :)

@shafeef901
Copy link
Contributor Author

shafeef901 commented Mar 20, 2024

I have sent an MR. Kindly let me know if everything looks good :)

Thanks!

Mayankm96 added a commit that referenced this issue Mar 24, 2024
# Description
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 

## Type of change

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

## Checklist

- [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 issue Mar 24, 2024
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 issue Mar 24, 2024
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 issue Mar 24, 2024
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 issue Mar 24, 2024
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 issue Aug 8, 2024
…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 issue Nov 21, 2024
…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
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants