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

Allows setting USD variants when loading prim from USD file #402

Merged
merged 13 commits into from
May 13, 2024

Conversation

lorenwel
Copy link
Contributor

@lorenwel lorenwel commented May 8, 2024

Description

This PR introduces a variants attribute in the UsdFileCfg which can be used to set different variants when loading an asset from a USD file.

  • New function set_usd_variants which applies variant sets to a prim
  • New, optional variants attribute in the UsdFileCfg to specify the attributes to be set
  • Add variant setting in the _spawn_from_usd_file function

Fixes #401

Type of change

  • New feature (non-breaking change which adds functionality)

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

Comment on lines 72 to 78
variants: object | dict[str, str] | None = None
"""Variants to apply to the USD file. Defaults to None.

Can either be a configclass object, in which case each attribute is used as a variant name and value,
or a dictionary where the keys are the variant names and the values are the variant values.
If None, then no variants are applied.
"""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you elaborate here with an example of what this config looks like? Also probably helps to have a unit test for it to ensure we are checking with USD API updates in the future?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it need to be a configclass or that is just out of convenience here?

Copy link
Contributor Author

@lorenwel lorenwel May 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you elaborate here with an example of what this config looks like?
Also probably helps to have a unit test for it to ensure we are checking with USD API updates in the future?

Added an example and a unit-test

Does it need to be a configclass or that is just out of convenience here?

Needs to be a configclass, because of the dict conversion. Otherwise we would have to manually filter all the __blabla__ members.


# Convert do dict if we have a configclass object.
if variants is not isinstance(variants, dict):
variants = variants.__dict__
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using this method might be better since it checks no dunder functions (like del) are captured

Suggested change
variants = variants.__dict__
variants = variants.to_dict()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, @configclass takes care of that as well even with __dict__.
image
But I can of course change it if you have strong feelings about this.


def set_usd_variants(prim_path: str, variants: object | dict[str, str], stage: Usd.Stage | None = None) -> None:
"""
Sets the variant selections for the specified variant sets on a USD prim.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would help a lot here to have more documentation on variants and what that means. You can refer to this function in the USDFileCfg configclass object to avoid duplication in docstrings for variants.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a link to the official USD documentation. Let me know if you think more is necessary.
Imo, people will mostly only use this function if they already know what Variants are, so I wouldn't want to write a lengthy explanation here.

@lorenwel lorenwel requested a review from Mayankm96 May 10, 2024 16:11
@lorenwel lorenwel force-pushed the feature/set_usd_variants branch from 4212f76 to 7e45eff Compare May 10, 2024 16:12
Ruthrash pushed a commit to Ruthrash/Orbit that referenced this pull request May 10, 2024
# Description

For the cluster deployment, the latest docker version (25.xx) is not yet
compatible with the latest apptainer version. This PR adds a check
before building the singularity image and adds a warning to the docs.
Also the NOHTTPS warning has been resolved.

Fixes isaac-sim#371 

## Type of change

- Bug fix (non-breaking change which fixes an issue)
- This change requires a documentation update

## 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
- [ ] 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
- [x] I have added my name to the `CONTRIBUTORS.md` or my name already
exists there

---------

Signed-off-by: Pascal Roth <57946385+pascal-roth@users.noreply.github.com>
Co-authored-by: Mayank Mittal <12863862+Mayankm96@users.noreply.github.com>
@Mayankm96 Mayankm96 changed the title Allow setting USD variants on loading from file Allows setting USD variants when loading prim from USD file May 13, 2024
@Mayankm96 Mayankm96 merged commit 88cf9cd into isaac-sim:main May 13, 2024
isaac-racine pushed a commit to isaac-racine/orbit that referenced this pull request Jun 5, 2024
…m#402)

# Description

This PR introduces a `variants` attribute in the `UsdFileCfg` which can
be used to set different variants when loading an asset from a USD file.

- New function `set_usd_variants` which applies variant sets to a prim
- New, optional `variants` attribute in the `UsdFileCfg` to specify the
attributes to be set
- Add variant setting in the `_spawn_from_usd_file` function

Fixes isaac-sim#401

## Type of change

- New feature (non-breaking change which adds functionality)

## 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
fatimaanes pushed a commit to fatimaanes/omniperf that referenced this pull request Aug 8, 2024
# Description

For the cluster deployment, the latest docker version (25.xx) is not yet
compatible with the latest apptainer version. This PR adds a check
before building the singularity image and adds a warning to the docs.
Also the NOHTTPS warning has been resolved.

Fixes isaac-sim#371 

## Type of change

- Bug fix (non-breaking change which fixes an issue)
- This change requires a documentation update

## 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
- [ ] 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
- [x] I have added my name to the `CONTRIBUTORS.md` or my name already
exists there

---------

Signed-off-by: Pascal Roth <57946385+pascal-roth@users.noreply.github.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
…m#402)

# Description

This PR introduces a `variants` attribute in the `UsdFileCfg` which can
be used to set different variants when loading an asset from a USD file.

- New function `set_usd_variants` which applies variant sets to a prim
- New, optional `variants` attribute in the `UsdFileCfg` to specify the
attributes to be set
- Add variant setting in the `_spawn_from_usd_file` function

Fixes isaac-sim#401

## Type of change

- New feature (non-breaking change which adds functionality)

## 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
iamdrfly pushed a commit to iamdrfly/IsaacLab that referenced this pull request Nov 21, 2024
# Description

For the cluster deployment, the latest docker version (25.xx) is not yet
compatible with the latest apptainer version. This PR adds a check
before building the singularity image and adds a warning to the docs.
Also the NOHTTPS warning has been resolved.

Fixes isaac-sim#371 

## Type of change

- Bug fix (non-breaking change which fixes an issue)
- This change requires a documentation update

## 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
- [ ] 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
- [x] I have added my name to the `CONTRIBUTORS.md` or my name already
exists there

---------

Signed-off-by: Pascal Roth <57946385+pascal-roth@users.noreply.github.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
…m#402)

# Description

This PR introduces a `variants` attribute in the `UsdFileCfg` which can
be used to set different variants when loading an asset from a USD file.

- New function `set_usd_variants` which applies variant sets to a prim
- New, optional `variants` attribute in the `UsdFileCfg` to specify the
attributes to be set
- Add variant setting in the `_spawn_from_usd_file` function

Fixes isaac-sim#401

## Type of change

- New feature (non-breaking change which adds functionality)

## 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Proposal] Allow setting USD variants
2 participants